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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/01/2020 16:16, Jan Beulich wrote:
> 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?

This piece of logic took ages to reverse engineer, but yes - there are
aliases.

The logic previously read:

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);

which is suspicious and looks wrong, seeing as l2_bootmap[] and
l2_idetmap[] are both 4 pages long and used elsewhere as identity mappings.

This ends up working because of the l3 logic which may, in some
circumstances (the 16M of Xen crossing a 1G boundary), edit two entries
of l3_identmap[], rather than one.  In this case, there ends up being a
second (split) alias of Xen mapped at either end of 2G range which
covers Xen, as the same L2 is used in two L3e's

>> 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?

They don't actually fully match after this change.  Xen.efi doesn't map
the trampoline, and has only ever (AFAICT) booted because
zap_low_mappings() creates the trampoline mapping even if it was absent
previously.

The MB path needs the trampoline mapping because it unconditionally
bounces through there, even when no BIOS calls are needed.  This is
expected to change in the future with David's kexec plans.

As for why they should be matching, (or at least, used consistently when
used for the same purpose), my sanity trying to figure out how the EFI
side of things didn't explode on boot.

>
>> --- 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?

Hmm - good question.  I really did get the EFI build dying when using
code of the form:

l2_identmap[slot] = l2_bootmap[slot] =
    l2e_from_paddr(addr, __PAGE_HYPERVISOR | _PAGE_PSE);

I put that down to trying to use an NX mapping before EFER.NXE was set
up, but in light of your point, I suspect it was something else.

>
>>      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."

Given that what you describe here is totally undocumented, and AFAICT,
totally undescribed even in commit messages, it has cost me probably a
weeks worth of time to reverse to the point at which I was confident
that I knew all of what it was attempting to do.

The purpose of this was to make the handling of l?_bootmap[] as
consistent as possible between the various environments.  The pagetables
themselves are common, and should be used consistently.

~Andrew

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