Re: [Xen-devel] [PATCH 4/6] x86/boot: Clean up l?_bootmap[] construction

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 4/6] x86/boot: Clean up l?_bootmap[] construction
Date: Tue, 7 Jan 2020 17:16:46 +0100	[thread overview]
Message-ID: <3601fa20-ba6a-78d6-62a2-a204ee49021e@suse.com> (raw)
In-Reply-To: <20200106155423.9508-5-andrew.cooper3@citrix.com>

On 06.01.2020 16:54, Andrew Cooper wrote:
> The need for Xen to be identity mapped into the bootmap is not obvious, and
> differs between the MB and EFI boot paths.  Furthermore, the EFI side is
> further complicated by spraying non-identity aliases into the mix.

What (intentional) aliases are you talking about? The changes done here
don't remove any. Or do you mean the ones occurring as a side effect of
possibly using the same L2 in two L3 slots?

> Simplify the EFI bootmap construction code to make exactly one identity-map of
> Xen, which now matches the MB path.  Comment both pieces of logic, explaining
> what the mappings are needed for.

Is both boot map variants fully matching actually needed for anything?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -584,21 +584,24 @@ static void __init efi_arch_memory_setup(void)
>      if ( !efi_enabled(EFI_LOADER) )
>          return;
>  
> -    /* Initialise L2 identity-map and boot-map page table entries (16MB). */
> +    /*
> +     * Map Xen into the directmap (NX, needed for early-boot pagetable
> +     * handling/walking), and identity map Xen into bootmap (X, needed for the
> +     * transition from the EFI pagetables to Xen), using 2M superpages.
> +     */

How does NX vs X matter for the code below here? PAGE_HYPERVISOR and
__PAGE_HYPERVISOR, as used below, differ by just _PAGE_GLOBAL. Did
you mean to make further changes?

>      for ( i = 0; i < 8; ++i )
>      {
>          unsigned int slot = (xen_phys_start >> L2_PAGETABLE_SHIFT) + i;
>          paddr_t addr = slot << L2_PAGETABLE_SHIFT;
>  
>          l2_identmap[slot] = l2e_from_paddr(addr, PAGE_HYPERVISOR|_PAGE_PSE);
> -        slot &= L2_PAGETABLE_ENTRIES - 1;
>          l2_bootmap[slot] = l2e_from_paddr(addr, __PAGE_HYPERVISOR|_PAGE_PSE);
>      }
> -    /* Initialise L3 boot-map page directory entries. */
> -    l3_bootmap[l3_table_offset(xen_phys_start)] =
> -        l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
> -    l3_bootmap[l3_table_offset(xen_phys_start + (8 << L2_PAGETABLE_SHIFT) - 1)] =
> -        l3e_from_paddr((UINTN)l2_bootmap, __PAGE_HYPERVISOR);
> +
> +    /* Initialize L3 boot-map page directory entries. */
> +    for ( i = 0; i < 4; ++i )
> +        l3_bootmap[i] = l3e_from_paddr((UINTN)l2_bootmap + i * PAGE_SIZE,
> +                                       __PAGE_HYPERVISOR);

The idea behind the original code was to be immune to the number
of pages l2_bootmap[] covers, as long as it's at least one (which
it'll always be, I would say). The minimum requirement to any
change to this I have is that the build must break if the size
assumption here is violated. I.e. there may not be a literal 4 as
the upper loop bound here, or there would need to be a
BUILD_BUG_ON() right next to it. But I'd really prefer if the
code was left as is (perhaps with a comment added), unless you
can point out actual issues with it (which I can't see in the
description), or you can otherwise justify the change with better
than "the EFI side is further complicated by spraying non-identity
aliases into the mix."

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel