From owner-svn-src-all@freebsd.org Mon Jan 15 17:06:13 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 631E4EBB68D; Mon, 15 Jan 2018 17:06:13 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 092E981D11; Mon, 15 Jan 2018 17:06:12 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id w0FH6343066472 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 15 Jan 2018 19:06:06 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua w0FH6343066472 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id w0FH63iT066471; Mon, 15 Jan 2018 19:06:03 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Mon, 15 Jan 2018 19:06:03 +0200 From: Konstantin Belousov To: Nathan Whitehorn Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3 Message-ID: <20180115170603.GJ1684@kib.kiev.ua> References: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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 17:06:13 -0000 On Mon, Jan 15, 2018 at 07:33:01AM -0800, Nathan Whitehorn wrote: > > > 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). Most likely this is an accidental feature of the linux code and not the specific decision by the freebsd emulation of it. > 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. I think I am fine with amd64 doing #define PMAP_HAS_DMAP 1 in machine/param.h. I do not insist on the name. Then ppc could define its version as a reference to the variable. I thought that might be you can create less clumsy model of propagating this to the MI VM level. > > > > >> 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)