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. > > Sorry, this is the patch I meant to send. Do you plan to convert sf buf code on powerpc ? > -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) _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"