Re: [Xen-devel] [PATCH 3/4] x86/boot: Create the l2_xenmap[] mappings dynamically

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/01/2020 16:45, Jan Beulich wrote:
> On 13.01.2020 18:50, Andrew Cooper wrote:
>> The build-time construction of l2_xenmap[] imposes an arbitrary limit of 16M
>> total, which is a limit looking to be lifted.
>>
>> Move l2_xenmap[] into the bss, and adjust both the BIOS and EFI paths to fill
>> it in dynamically, based on the final linked size of Xen.  For current 
>> builds,
>> this reduces the number of .text/etc mappings from 7 to 4.
> Is the 4 named here applicable the same to xen.gz and xen.efi, despite
> the latter using large pages with distinct permissions while the former
> still doesn't?

TBH, I didn't check.  It will vary on the build anyway, and on
CONFIG_XEN_ALIGN_{DEFAULT,2M} (which is used to cause the shim xen.gz to
have 2M alignment.  I wonder how many people spotted this was a perf fix
for XSA-304...)

>> In principle, the non-EFI case could be made to work by having a post-link
>> script fill in a suitable number of _PAGE_PRESENT entries in l2_xenmap[].
>> This doesn't work for the EFI case, because pagetable relocation is instead
>> triggered on the ad-hoc relocation table, which would require the
>> _PAGE_PRESENT references to be in place before the link takes place.
> And to be honest, such a post-link script would seem rather ugly
> to have to me.

There are some post-link scripts which I'm intending to borrow from
Linux.  In particular, sorting the exception tables is a obvious thing
to do at build time rather than runtime.

A different one to consider is preparation of the IDT.  Mangling the
gates at build time would drop a moderate chunk of code, and allow for
earlier exception handling.

We have a lot of 32bit boot time code, which runs without any IDT. 
Adding even more 32bit code to construct an IDT is a waste of effort,
but if a build-time-prepared IDT were possible, the cost of one extra
lidt is a reasonable trade for a enough of a panic handler to at least
dump the registers, before continuing into a triple fault.

>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -668,6 +668,20 @@ trampoline_setup:
>>          add     %esi,sym_fs(__page_tables_start)-8(,%ecx,8)
>>  2:      loop    1b
>>  
>> +        /* Map Xen into the higher mappings using 2M superpages. */
>> +        lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), %eax
>> +        mov     $sym_offs(_start),   %ecx   /* %eax = PTE to write        */
> The comment is on the wrong line, isn't it? Perhaps
>
>         lea     _PAGE_PSE + PAGE_HYPERVISOR_RWX + sym_esi(_start), \
>                 %eax                /* %eax = PTE to write        */
>
> ?

That is why the comment had the register name, rather than trying to
claim that $sym_offs(_start) was the PTE to write.

I didn't really think splitting the lea like that across 2 lines was
better than this.

How about /* %eax = PTE to write ^      */ which will point properly at
%eax?

>
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -585,6 +585,20 @@ static void __init efi_arch_memory_setup(void)
>>      if ( !efi_enabled(EFI_LOADER) )
>>          return;
>>  
>> +    /*
>> +     * Map Xen into the higher mappings, using 2M superpages.
>> +     *
>> +     * NB: We are currently in physical mode, so a RIP-relative relocation
>> +     * against _start/_end gets their position as placed by the bootloader,
>> +     * not as expected in the final build.  This has arbitrary 2M alignment,
>> +     * so subtract xen_phys_start to get the appropriate slots in 
>> l2_xenmap[].
>> +     */
> It may just be a language issue, but I'm struggling with the
> "arbitrary" here. Is this in any way related to the
> --section-alignment=0x200000 option we pass to the linker (where
> the value isn't arbitrary at all)?

So this is the bug I spent ages trying to figure out console logging for.

The naive version of this loop (pre subtraction) ended up initialising
slots 173...177 which, when highlighted like that, is obviously why Xen
triple faulted when switching to the high mappings.

The point I'm trying to make is that l2_table_offset(_start) ends up
being junk because it is a rip-relative address and we're not running at
our linked address.  (It is in fact our physical position in memory's 2M
slot, modulo 512).

Subtracting xen_phys_start gets the number back into the same alias
which all the 32bit head.S code relies on, and gives us a sensible
sequence of slots starting from 1.

~Andrew

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