[3/4] x86/boot: Create the l2_xenmap[] mappings dynamically
diff mbox series

Message ID 20200113175020.32730-5-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86: Remove 16M total-size restriction
Related show

Commit Message

Andrew Cooper Jan. 13, 2020, 5:50 p.m. UTC
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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

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.
---
 xen/arch/x86/boot/head.S    | 14 ++++++++++++++
 xen/arch/x86/boot/x86_64.S  | 23 ++++++++---------------
 xen/arch/x86/efi/efi-boot.h | 14 ++++++++++++++
 xen/arch/x86/xen.lds.S      |  3 +++
 4 files changed, 39 insertions(+), 15 deletions(-)

Comments

Jan Beulich Jan. 14, 2020, 4:45 p.m. UTC | #1
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?

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

> --- 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        */

?

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

Jan
Andrew Cooper Jan. 14, 2020, 7:31 p.m. UTC | #2
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
Jan Beulich Jan. 15, 2020, 9:23 a.m. UTC | #3
On 14.01.2020 20:31, Andrew Cooper wrote:
> On 14/01/2020 16:45, Jan Beulich wrote:
>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>> --- 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?

Fine with me; I assume you mean this to go on a separate line?

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

Thanks for the explanation. What I'm still unclear about is this use
of "arbitrary", though. Looking at it again I guess I'm also
struggling to understand what "This" at the beginning of the sentence
refers to.

Jan
Andrew Cooper Jan. 16, 2020, 7:41 p.m. UTC | #4
On 15/01/2020 09:23, Jan Beulich wrote:
> On 14.01.2020 20:31, Andrew Cooper wrote:
>> On 14/01/2020 16:45, Jan Beulich wrote:
>>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>>> --- 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?
> Fine with me; I assume you mean this to go on a separate line?

Yes.  i.e. exactly as the patch presented, but with an extra " ^" in the
comment.

>
>>>> --- 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.
> Thanks for the explanation. What I'm still unclear about is this use
> of "arbitrary", though. Looking at it again I guess I'm also
> struggling to understand what "This" at the beginning of the sentence
> refers to.

I'll attempt to rewrite the explanation from scratch and see if that
comes out clearer.

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c5acbf56ae..94bed4a2d3 100644
--- 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        */
+        mov     $sym_offs(_end - 1), %edx
+        shr     $L2_PAGETABLE_SHIFT, %ecx   /* %ecx = First slot to write */
+        shr     $L2_PAGETABLE_SHIFT, %edx   /* %edx = Final slot to write */
+
+1:      mov     %eax, sym_offs(l2_xenmap)(%esi, %ecx, 8)
+        add     $1, %ecx
+        add     $1 << L2_PAGETABLE_SHIFT, %eax
+
+        cmp     %edx, %ecx
+        jbe     1b
+
         /*
          * Map Xen into the directmap (needed for early-boot pagetable
          * handling/walking), and identity map Xen into bootmap (needed for
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index aabf561b23..e63bece460 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -43,6 +43,14 @@  multiboot_ptr:
 GLOBAL(stack_start)
         .quad   cpu0_stack + STACK_SIZE - CPUINFO_sizeof
 
+        .section .bss.page_aligned, "aw", @nobits
+        .align PAGE_SIZE, 0
+
+/* L2 mapping the Xen text/data/bss region.  Uses 1x 4k page. */
+GLOBAL(l2_xenmap)
+        .fill L2_PAGETABLE_ENTRIES, 8, 0
+        .size l2_xenmap, . - l2_xenmap
+
         .section .data.page_aligned, "aw", @progbits
         .align PAGE_SIZE, 0
 /*
@@ -80,21 +88,6 @@  GLOBAL(l2_directmap)
         .fill 4 * L2_PAGETABLE_ENTRIES - 1, 8, 0
         .size l2_directmap, . - l2_directmap
 
-/*
- * L2 mapping the 1GB Xen text/data/bss region.  At boot it maps 16MB from
- * __image_base__, and is modified when Xen relocates itself.  Uses 1x 4k
- * page.
- */
-GLOBAL(l2_xenmap)
-        .quad 0
-        idx = 1
-        .rept 7
-        .quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + (PAGE_HYPERVISOR_RWX | _PAGE_PSE)
-        idx = idx + 1
-        .endr
-        .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
-        .size l2_xenmap, . - l2_xenmap
-
 /* L2 mapping the fixmap.  Uses 1x 4k page. */
 l2_fixmap:
         idx = 0
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 50d1499867..e750db6f5c 100644
--- 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[].
+     */
+    for ( i =  l2_table_offset((UINTN)_start   - xen_phys_start);
+          i <= l2_table_offset((UINTN)_end - 1 - xen_phys_start); ++i )
+        l2_xenmap[i] =
+            l2e_from_paddr(xen_phys_start + (i << L2_PAGETABLE_SHIFT),
+                           PAGE_HYPERVISOR_RWX | _PAGE_PSE);
+
     /* Check that there is at least 4G of mapping space in l2_*map[] */
     BUILD_BUG_ON((sizeof(l2_bootmap)   / L2_PAGETABLE_ENTRIES) < 4);
     BUILD_BUG_ON((sizeof(l2_directmap) / L2_PAGETABLE_ENTRIES) < 4);
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 7f82f64078..7c351b9df3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -359,6 +359,9 @@  ASSERT(__image_base__ > XEN_VIRT_START |
 ASSERT(kexec_reloc_size - kexec_reloc <= PAGE_SIZE, "kexec_reloc is too large")
 #endif
 
+/* The Multiboot setup paths depend on this to simplify superpage PTE creation. */
+ASSERT(IS_ALIGNED(_start,            MB(2)), "_start misaligned")
+
 ASSERT(IS_ALIGNED(__2M_text_end,     SECTION_ALIGN), "__2M_text_end misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_start, SECTION_ALIGN), "__2M_rodata_start misaligned")
 ASSERT(IS_ALIGNED(__2M_rodata_end,   SECTION_ALIGN), "__2M_rodata_end misaligned")