From owner-svn-src-all@freebsd.org Mon Jan 15 15:33:11 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 955BEEB4D93; Mon, 15 Jan 2018 15:33:11 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from d.mail.sonic.net (d.mail.sonic.net [64.142.111.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7DA4D7C681; Mon, 15 Jan 2018 15:33:11 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from comporellon.tachypleus.net (cpe-75-82-218-62.socal.res.rr.com [75.82.218.62]) (authenticated bits=0) by d.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id w0FFX2ri013178 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Mon, 15 Jan 2018 07:33:02 -0800 Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 To: Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <010d0153-8931-a3c2-db21-dfcbaf848fc0@freebsd.org> <20180114083036.GX1684@kib.kiev.ua> <20180114170502.GB1684@kib.kiev.ua> <184ba3ee-a9f7-01ed-bb02-1bcba9acc041@freebsd.org> <20180114175211.GD1684@kib.kiev.ua> <20180115111812.GF1684@kib.kiev.ua> From: Nathan Whitehorn Message-ID: Date: Mon, 15 Jan 2018 07:33:01 -0800 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180115111812.GF1684@kib.kiev.ua> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Sonic-CAuth: UmFuZG9tSVbZgFWzCsw8Gz2MIS6AXp9ZL+t2pUkf1bX1IuYlx8zGZcAHKRsLpiqkfUhKZ8kZSI3JyaWAEexzRcESL3crDaUtY3lXtsjKS5c= X-Sonic-ID: C;AFRpXwn65xGkh+BQ9eIpJw== M;zGnXXwn65xGkh+BQ9eIpJw== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Jan 2018 15:33:11 -0000 On 01/15/18 03:18, Konstantin Belousov wrote: > On Sun, Jan 14, 2018 at 03:46:38PM -0800, Nathan Whitehorn wrote: >> >> On 01/14/18 15:42, Nathan Whitehorn wrote: >>> >>> On 01/14/18 09:57, Nathan Whitehorn wrote: >>>> >>>> On 01/14/18 09:52, Konstantin Belousov wrote: >>>>> On Sun, Jan 14, 2018 at 09:30:53AM -0800, Nathan Whitehorn wrote: >>>>>> The immediate consequence of that is that no MI code that knows about >>>>>> direct maps can possibly take advantage of the direct map on this >>>>>> platform. Do we really want that to save some conditional logic that >>>>>> would get optimized out on amd64 and arm64 anyway? I really do not see >>>>>> the benefit here. >>>>> It is not clear what do you mean.š Are you saying that there is no >>>>> benefit >>>>> of providing the conditional logic, or that it is not benefit of >>>>> exclusing >>>>> powerpc ? >>>> Sorry, that was poorly stated. Let me try again: >>>> >>>> If we make a PPC_PHYS_TO_DMAP(), but there is an MI PHYS_TO_DMAP() >>>> API, consumer code in the MI parts of the kernel won't be able to >>>> benefit from the PPC direct map, which seems unfortunate. The cost >>>> from a code perspective of having an if (direct_map_available) seems >>>> low, since on systems where direct_map_available is defined to be 1, >>>> the compiler will optimize it to the same code as if gated by #ifdef. >>>> It might be more cumbersome to write the code, however. >>>> >>>>> I do not object against adding the conditional, but it should not be >>>>> too clumsy to use. >>>>> >>>> OK. Let me try to draft something in the next couple days and see how >>>> much of a pain it is in practice. >>>> -Nathan >>>> >>> How about the attached? It makes PHYS_TO_DMAP() return 0 if no mapping >>> exists. This is straightforward, does not introduce extra macros, and >>> can pretty easily replace SFBUF_OPTIONAL_DIRECT_MAP on the assumption >>> that PHYS_TO_DMAP() is cheap. I've modified the other MI-ish consumers >>> in the tree accordingly; compat/linuxkpi/common/src/linux_page.c >>> already does the right thing and needed no modifications. >>> -Nathan > I think that this is fine from the PoV of code complexity. > > We now require MI (but not amd64 and arm64 MD) code to check for > PHYS_TO_DMAP() return value, which is redundand for a*64. I am not sure > if this is good choice from the PoV of possible microoptimizations. > You promised something which is trivially detectable by compiler as > an excess code. Fair enough -- the logic was that a lot of code already checks for NULL pointers (the linux_page.c for instance required no changes to do the right thing). If we want it to be fully compiler-transparent, we could also add a flag, but that would add more code complexity. Do you have a preference? I would be happy to draft that too. > >> Sorry, this is the patch I meant to send. > Do you plan to convert sf buf code on powerpc ? Yes, once this is finalized. -Nathan > >> -Nathan >> Index: powerpc/include/vmparam.h >> =================================================================== >> --- powerpc/include/vmparam.h (revision 327952) >> +++ powerpc/include/vmparam.h (working copy) >> @@ -240,13 +240,12 @@ >> #define SFBUF_PHYS_DMAP(x) (x) >> >> /* >> - * We (usually) have a direct map of all physical memory. All >> - * uses of this macro must be gated by a check on hw_direct_map! >> - * The location of the direct map may not be 1:1 in future, so use >> - * of the macro is recommended; it may also grow an assert that hw_direct_map >> - * is set. >> + * We (usually) have a direct map of all physical memory, so provide >> + * a macro to use to get the kernel VA address for a given PA. Returns >> + * 0 if the direct map is unavailable. The location of the direct map >> + * may not be 1:1 in future, so use of the macro is recommended. >> */ >> -#define PHYS_TO_DMAP(x) x >> -#define DMAP_TO_PHYS(x) x >> +#define PHYS_TO_DMAP(x) (hw_direct_map ? (x) : 0) >> +#define DMAP_TO_PHYS(x) (hw_direct_map ? (x) : 0) >> >> #endif /* _MACHINE_VMPARAM_H_ */ >> Index: vm/vm_page.c >> =================================================================== >> --- vm/vm_page.c (revision 327952) >> +++ vm/vm_page.c (working copy) >> @@ -2937,7 +2937,8 @@ >> { >> >> #if defined(DIAGNOSTIC) && defined(PHYS_TO_DMAP) >> - if ((m->flags & PG_ZERO) != 0) { >> + if ((m->flags & PG_ZERO) != 0 && >> + PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m)) != 0) { >> uint64_t *p; >> int i; >> p = (uint64_t *)PHYS_TO_DMAP(VM_PAGE_TO_PHYS(m)); >> Index: dev/efidev/efirt.c >> =================================================================== >> --- dev/efidev/efirt.c (revision 327952) >> +++ dev/efidev/efirt.c (working copy) >> @@ -115,6 +115,11 @@ >> return (0); >> } >> efi_systbl = (struct efi_systbl *)PHYS_TO_DMAP(efi_systbl_phys); >> + if (efi_systbl == NULL) { >> + if (bootverbose) >> + printf("EFI systbl not mapped in kernel VA\n"); >> + return (0); >> + } >> if (efi_systbl->st_hdr.th_sig != EFI_SYSTBL_SIG) { >> efi_systbl = NULL; >> if (bootverbose)