svn commit: r327950 - in head/sys/powerpc: aim include powerpc ps3
Konstantin Belousov
kostikbel at gmail.com
Mon Jan 15 11:18:22 UTC 2018
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)
More information about the svn-src-head
mailing list