| Message ID | 20200113175020.32730-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Remove 16M total-size restriction | expand |
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
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
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
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
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")
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(-)