Date: Mon, 15 Jan 2018 19:06:03 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Nathan Whitehorn <nwhitehorn@freebsd.org> 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> In-Reply-To: <f6350c61-55d1-9bf7-c4b3-e10fb329a42a@freebsd.org> References: <20180114083036.GX1684@kib.kiev.ua> <ede06fc6-7c34-100c-8a7a-6346cd8cd363@freebsd.org> <20180114170502.GB1684@kib.kiev.ua> <184ba3ee-a9f7-01ed-bb02-1bcba9acc041@freebsd.org> <20180114175211.GD1684@kib.kiev.ua> <b2b1bf30-177b-af30-54ce-f484224bb2ad@freebsd.org> <f4b44b69-7b06-6b5a-c17c-31bd46ca1af0@freebsd.org> <e04bc7a6-fa77-9ca0-2aff-dc29c543c9a1@freebsd.org> <20180115111812.GF1684@kib.kiev.ua> <f6350c61-55d1-9bf7-c4b3-e10fb329a42a@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180115170603.GJ1684>