| Message ID | 20200106155423.9508-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/boot: Remove mappings at 0 | expand |
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? > 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? > --- 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." Jan
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
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
On 07/01/2020 16:30, Jan Beulich wrote: >>> 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? When I've figured out why altering that causes the EFI boot to fail, yes - that was the plan... ~Andrew
On 07.01.2020 19:00, Andrew Cooper wrote: > On 07/01/2020 16:16, Jan Beulich wrote: >> On 06.01.2020 16:54, Andrew Cooper wrote: >>> 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. This is not meant as an excuse (I really should have done better back then), but you could have asked. > 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. I don't think I can wholeheartedly agree here: l?_bootmap[] are throw-away page tables (living in .init), and with the non-EFI and EFI boot paths being so different anyway, them using the available tables differently is not a big issue imo. This heavy difference of other aspects was also why back then I decided to be as defensive towards l2_bootmap[] size changes as possible in code which doesn't really need it to be multiple pages. As said - I'm going to try to not stand in the way of you re- arranging this, but - the new code should not break silently when (in particular) l2_bootmap[] changes - the description should be more explicit about the motivation of the change (which includes distinguishing between intentional mappings and ones simply appearing as a side effect, without getting in the way) Jan
On 08/01/2020 11:38, Jan Beulich wrote: >> 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. > I don't think I can wholeheartedly agree here: l?_bootmap[] are > throw-away page tables (living in .init), and with the non-EFI and > EFI boot paths being so different anyway, them using the available > tables differently is not a big issue imo. This heavy difference of > other aspects was also why back then I decided to be as defensive > towards l2_bootmap[] size changes as possible in code which doesn't > really need it to be multiple pages. From this description, it suggests that you haven't spotted the rather more subtle bug which will trip up anyone trying to develop in the future. This scheme is incompatible with trying to map a second object (e.g. the trampoline) into the bootmap, because depending on alignment, it may overlap with the PTEs which mapped Xen. There also typically isn't an l3_bootmap[0] => l2_bootmap[0] because of where xen.efi is loaded in memory. > > As said - I'm going to try to not stand in the way of you re- > arranging this, but > - the new code should not break silently when (in particular) > l2_bootmap[] changes What practical changes do you think could be done here? I can't spot any which would be helpful. A BUILD_BUG_ON() doesn't work. The most likely case for something going wrong here is an edit to x86_64.S and no equivalent edit to page.h, which a BUILD_BUG_ON() wouldn't spot. head.S similarly has no useful protections which could be added. ~Andrew
On 08.01.2020 17:15, Andrew Cooper wrote: > On 08/01/2020 11:38, Jan Beulich wrote: >> As said - I'm going to try to not stand in the way of you re- >> arranging this, but >> - the new code should not break silently when (in particular) >> l2_bootmap[] changes > > What practical changes do you think could be done here? I can't spot > any which would be helpful. > > A BUILD_BUG_ON() doesn't work. The most likely case for something going > wrong here is an edit to x86_64.S and no equivalent edit to page.h, > which a BUILD_BUG_ON() wouldn't spot. head.S similarly has no useful > protections which could be added. Well, the fundamental assumption is that the .S files and the C declaration of l?_bootmap[] are kept in sync. No BUILD_BUG_ON() can cover a mistake made there. But rather than using the literal 4 as you did, an ARRAY_SIZE() construct should be usable to either replace it, or amend it with a BUILD_BUG_ON(). Jan
On 08/01/2020 16:55, Jan Beulich wrote: > On 08.01.2020 17:15, Andrew Cooper wrote: >> On 08/01/2020 11:38, Jan Beulich wrote: >>> As said - I'm going to try to not stand in the way of you re- >>> arranging this, but >>> - the new code should not break silently when (in particular) >>> l2_bootmap[] changes >> What practical changes do you think could be done here? I can't spot >> any which would be helpful. >> >> A BUILD_BUG_ON() doesn't work. The most likely case for something going >> wrong here is an edit to x86_64.S and no equivalent edit to page.h, >> which a BUILD_BUG_ON() wouldn't spot. head.S similarly has no useful >> protections which could be added. > Well, the fundamental assumption is that the .S files and the > C declaration of l?_bootmap[] are kept in sync. No BUILD_BUG_ON() > can cover a mistake made there. But rather than using the literal > 4 as you did, an ARRAY_SIZE() construct should be usable to either > replace it, or amend it with a BUILD_BUG_ON(). You are aware that ARRAY_SIZE(l2_bootmap) is 2048 and ARRAY_SIZE(l3_bootmap) is 512, neither of which would be correct here? ~Andrew
On 08.01.2020 18:09, Andrew Cooper wrote: > On 08/01/2020 16:55, Jan Beulich wrote: >> On 08.01.2020 17:15, Andrew Cooper wrote: >>> On 08/01/2020 11:38, Jan Beulich wrote: >>>> As said - I'm going to try to not stand in the way of you re- >>>> arranging this, but >>>> - the new code should not break silently when (in particular) >>>> l2_bootmap[] changes >>> What practical changes do you think could be done here? I can't spot >>> any which would be helpful. >>> >>> A BUILD_BUG_ON() doesn't work. The most likely case for something going >>> wrong here is an edit to x86_64.S and no equivalent edit to page.h, >>> which a BUILD_BUG_ON() wouldn't spot. head.S similarly has no useful >>> protections which could be added. >> Well, the fundamental assumption is that the .S files and the >> C declaration of l?_bootmap[] are kept in sync. No BUILD_BUG_ON() >> can cover a mistake made there. But rather than using the literal >> 4 as you did, an ARRAY_SIZE() construct should be usable to either >> replace it, or amend it with a BUILD_BUG_ON(). > > You are aware that ARRAY_SIZE(l2_bootmap) is 2048 and > ARRAY_SIZE(l3_bootmap) is 512, neither of which would be correct here? Yes, I am (which is why I added "construct"). Dividing by L<n>_PAGETABLE_ENTRIES would be one option. In particular for l2_bootmap declaring at as [4][L2_PAGETABLE_ENTRIES] would be another. Jan
On 07/01/2020 16:30, Jan Beulich wrote: > 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? Nope. The comments were actually correct (and the code, remained correct). PAGE_HYPERVISOR and __PAGE_HYPERVISOR really do differ by NX as well, outside of asm code. I'm going to fix this because its too complicated to reason about. ~Andrew
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 7ee4511e26..f7d273ca36 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -668,7 +668,11 @@ trampoline_setup: add %esi,sym_fs(__page_tables_start)-8(,%ecx,8) 2: loop 1b - /* Initialize L2 boot-map/direct map page table entries (16MB). */ + /* + * Map Xen into the directmap (needed for early-boot pagetable + * handling/walking), and identity map Xen into bootmap (needed for + * the transition into long mode), using 2M superpages. + */ lea sym_esi(start),%ebx lea (1<<L2_PAGETABLE_SHIFT)*7+(PAGE_HYPERVISOR|_PAGE_PSE)(%ebx),%eax shr $(L2_PAGETABLE_SHIFT-3),%ebx @@ -678,7 +682,7 @@ trampoline_setup: sub $(1<<L2_PAGETABLE_SHIFT),%eax loop 1b - /* Initialize L3 boot-map page directory entry. */ + /* Initialize L3 boot-map page directory entries. */ lea __PAGE_HYPERVISOR+(L2_PAGETABLE_ENTRIES*8)*3+sym_esi(l2_bootmap),%eax mov $4,%ecx 1: mov %eax,sym_fs(l3_bootmap)-8(,%ecx,8) diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 676d616ff8..9c314e403a 100644 --- 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. + */ 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); } static void __init efi_arch_handle_module(struct file *file, const CHAR16 *name, diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 111edb5360..7f82f64078 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -381,3 +381,6 @@ ASSERT((trampoline_end - trampoline_start) < TRAMPOLINE_SPACE - MBI_SPACE_MIN, "not enough room for trampoline and mbi data") ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN, "wakeup stack too small") + +/* Plenty of boot code assumes that Xen isn't larger than 16M. */ +ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
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. 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. Finally, leave a linker assert covering the fact that plenty of code blindly assumes that Xen is less that 16M. This wants fixing in due course. 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> The MB path's dependency on Xen's identity mapping can be broken by having trampoline_boot_cpu_entry switch the alias of gdt_48 it uses. I took this approach first in an attempt to drop the bootmap entirely, but it is incompatible with the EFI path, and would also work against other plans to avoid using the trampoline during early boot. --- xen/arch/x86/boot/head.S | 8 ++++++-- xen/arch/x86/efi/efi-boot.h | 17 ++++++++++------- xen/arch/x86/xen.lds.S | 3 +++ 3 files changed, 19 insertions(+), 9 deletions(-)