[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/boot: Fix zap_low_mappings() to map less of the trampoline
On 12/03/2025 9:57 am, Roger Pau Monné wrote: > On Wed, Mar 12, 2025 at 09:31:37AM +0100, Jan Beulich wrote: >> On 11.03.2025 21:47, Andrew Cooper wrote: >>> On 06/01/2025 11:54 am, Jan Beulich wrote: >>>> On 06.01.2025 12:26, Andrew Cooper wrote: >>>>> Regular data access into the trampoline is via the directmap. >>>>> >>>>> As now discussed quite extensively in asm/trampoline.h, the trampoline is >>>>> arranged so that only the AP and S3 paths need an identity mapping, and >>>>> that >>>>> they fit within a single page. >>>>> >>>>> Right now, PFN_UP(trampoline_end - trampoline_start) is 2, causing more >>>>> than >>>>> expected of the trampoline to be mapped. Cut it down just the single >>>>> page it >>>>> ought to be. >>>>> >>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx> >>> Thanks. However, >>> >>>> on the basis that this improves things. However, ... >>>> >>>>> --- a/xen/arch/x86/x86_64/mm.c >>>>> +++ b/xen/arch/x86/x86_64/mm.c >>>>> @@ -718,14 +718,16 @@ void __init zap_low_mappings(void) >>>>> { >>>>> BUG_ON(num_online_cpus() != 1); >>>>> >>>>> - /* Remove aliased mapping of first 1:1 PML4 entry. */ >>>>> + /* Stop using l?_bootmap[] mappings. */ >>>>> l4e_write(&idle_pg_table[0], l4e_empty()); >>>>> flush_local(FLUSH_TLB_GLOBAL); >>>>> >>>>> - /* Replace with mapping of the boot trampoline only. */ >>>>> + /* >>>>> + * Insert an identity mapping of the AP/S3 part of the trampoline, >>>>> which >>>>> + * is arranged to fit in a single page. >>>>> + */ >>>>> map_pages_to_xen(trampoline_phys, maddr_to_mfn(trampoline_phys), >>>>> - PFN_UP(trampoline_end - trampoline_start), >>>>> - __PAGE_HYPERVISOR_RX); >>>>> + 1, __PAGE_HYPERVISOR_RX); >>>> ... literal numbers like this - however well they are commented - are >>>> potentially problematic to locate in case something changes significantly. >>>> The 1 here really would want connecting with the .equ establishing >>>> wakeup_stack. >>> how do you propose doing this? >>> >>> PFN_UP(wakeup_stack - trampoline_start) doesn't have the same obvious >>> connection, and it would involve partly undoing 7d73c6f196a5 which hid >>> the symbol recently. >>> >>> While 1 isn't ideal, it is next to a comment explaining what's going on, >>> and it's not going to go stale in a silent way... (It's also not liable >>> to go stale either.) >> If in >> >> .equ wakeup_stack, trampoline_start + PAGE_SIZE >> >> PAGE_SIZE was replaced by a new (in asm/trampoline.h) TRAMPOLINE_PERM_SIZE, >> you could use PFN_UP(TRAMPOLINE_PERM_SIZE) here to establish a connection. >> >> I have to admit I also don't really see why things going stale here would >> (a) be unlikely and (b) be guaranteed to not go silently. The size can't go to 0 or everything will break, and if it goes larger than 1 (which it almost certainly never will), then APs and/or S3 will break, and we've got both of these in CI. Furthermore, the actual thing which matters is: > /* Map the permanent trampoline page into l1_bootmap[]. */ > mov sym_esi(trampoline_phys), %ecx > lea __PAGE_HYPERVISOR_RX(%ecx), %edx /* %edx = PTE to write */ > shr $PAGE_SHIFT, %ecx /* %ecx = Slot to write */ > mov %edx, sym_offs(l1_bootmap)(%esi, %ecx, 8) which hardcodes 1 page, because there's almost certainly no chance this will ever change. >> We just don't >> know what we may need to add to the trampoline, sooner or later. > Maybe we could introduce trampoline_{ap?,runtime?}_{start,end} that > covers this more narrow section of the trampoline code? We already have one of those, and a linker assertion that it stays below 1k, so wakeup_stack is at least 3k. The complexity is that the wakeup_stack overlays some init-only logic in the placed trampoline. It's not something that exists concretely in the Xen image. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |