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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.01.2020 17:16, Jan Beulich wrote:
> On 06.01.2020 16:54, Andrew Cooper wrote:
>> --- 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."

And if this change is to be made, won't it mean the code in setup.c
commented with "Make boot page tables match non-EFI boot" can then
go away at the same time?

Jan

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