| Message ID | 20241023144448.731688-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: Yet more PVH module handling fixes | expand |
On 10/23/24 10:44, Andrew Cooper wrote: > The use of physical addresses in __start_xen() has proved to be fertile soure > of bugs. > > The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c variable > even), then re-loads it immediately before calling __start_xen(). For this, > we can just drop the function parameter and read multiboot_ptr in the one > place it's used. > > The EFI path also passes this parameter into __start_xen(). Have the EFI path > set up multiboot_ptr too, and move the explanation of phyiscal-mode pointers. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > CC: Daniel P. Smith <dpsmith@apertussolutions.com> For EFI: Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com> For remainder: Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
On 23/10/2024 3:58 pm, Daniel P. Smith wrote: > On 10/23/24 10:44, Andrew Cooper wrote: >> The use of physical addresses in __start_xen() has proved to be >> fertile soure >> of bugs. >> >> The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c >> variable >> even), then re-loads it immediately before calling __start_xen(). >> For this, >> we can just drop the function parameter and read multiboot_ptr in the >> one >> place it's used. >> >> The EFI path also passes this parameter into __start_xen(). Have the >> EFI path >> set up multiboot_ptr too, and move the explanation of phyiscal-mode >> pointers. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> >> CC: Daniel P. Smith <dpsmith@apertussolutions.com> > > For EFI: > Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > For remainder: > Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com> Thanks. That's usually just R-by then. In Xen, we take A-by to mean "sure, whatever", and R-by to mean "I've looked at this in detail and think it's good". R-by implies A-by, and either are fine from a maintainership point of view. ~Andrew
diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 04bb62ae8680..26b9d1c2df9a 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -77,8 +77,6 @@ ENTRY(__high_start) tailcall start_secondary .L_bsp: - /* Pass off the Multiboot info structure to C land (if applicable). */ - mov multiboot_ptr(%rip),%edi tailcall __start_xen .section .data.page_aligned, "aw", @progbits diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index 94f34433640f..3b26f0b0f500 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -248,6 +248,12 @@ static void __init noreturn efi_arch_post_exit_boot(void) efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start); memcpy((void *)trampoline_phys, trampoline_start, cfg.size); + /* + * We're in physical mode right now (i.e. identity map), so a regular + * pointer is also a phyiscal address to the rest of Xen. + */ + multiboot_ptr = (unsigned long)&mbi; + /* Set system registers and transfer control. */ asm volatile("pushq $0\n\tpopfq"); rdmsrl(MSR_EFER, efer); @@ -279,8 +285,7 @@ static void __init noreturn efi_arch_post_exit_boot(void) [cr4] "+&r" (cr4) : [cr3] "r" (idle_pg_table), [cs] "i" (__HYPERVISOR_CS), - [ds] "r" (__HYPERVISOR_DS), - "D" (&mbi) + [ds] "r" (__HYPERVISOR_DS) : "memory" ); unreachable(); } diff --git a/xen/arch/x86/include/asm/setup.h b/xen/arch/x86/include/asm/setup.h index 3d189521189d..811855e57478 100644 --- a/xen/arch/x86/include/asm/setup.h +++ b/xen/arch/x86/include/asm/setup.h @@ -14,6 +14,7 @@ extern unsigned long xenheap_initial_phys_start; extern uint64_t boot_tsc_stamp; extern void *stack_start; +extern unsigned int multiboot_ptr; void early_cpu_init(bool verbose); void early_time_init(void); diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c5b37bd2112e..c9129c21787b 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -157,8 +157,8 @@ char asmlinkage __section(".init.bss.stack_aligned") __aligned(STACK_SIZE) /* Used by the BSP/AP paths to find the higher half stack mapping to use. */ void *stack_start = cpu0_stack + STACK_SIZE - sizeof(struct cpu_info); -/* Used by the boot asm to stash the relocated multiboot info pointer. */ -unsigned int asmlinkage __initdata multiboot_ptr; +/* Used by the boot asm and EFI to stash the multiboot_info paddr. */ +unsigned int __initdata multiboot_ptr; struct cpuinfo_x86 __read_mostly boot_cpu_data = { 0, 0, 0, 0, -1 }; @@ -1014,7 +1014,7 @@ static struct domain *__init create_dom0(const module_t *image, /* How much of the directmap is prebuilt at compile time. */ #define PREBUILT_MAP_LIMIT (1 << L2_PAGETABLE_SHIFT) -void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) +void asmlinkage __init noreturn __start_xen(void) { const char *memmap_type = NULL; char *kextra; @@ -1059,7 +1059,6 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) if ( pvh_boot ) { - ASSERT(mbi_p == 0); pvh_init(&mbi, &mod); /* * mbi and mod are regular pointers to .initdata. These remain valid @@ -1068,7 +1067,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) } else { - mbi = __va(mbi_p); + mbi = __va(multiboot_ptr); mod = __va(mbi->mods_addr); /* @@ -1078,11 +1077,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) * For EFI, these are directmap pointers into the Xen image. They do * not remain valid across move_xen(). EFI boot only functions * because a non-zero xen_phys_start inhibits move_xen(). - * - * Don't be fooled by efi_arch_post_exit_boot() passing "D" (&mbi). - * This is a EFI physical-mode (i.e. identity map) pointer. */ - ASSERT(mbi_p < MB(1) || xen_phys_start); + ASSERT(multiboot_ptr < MB(1) || xen_phys_start); } bi = multiboot_fill_boot_info(mbi, mod);
The use of physical addresses in __start_xen() has proved to be fertile soure of bugs. The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c variable even), then re-loads it immediately before calling __start_xen(). For this, we can just drop the function parameter and read multiboot_ptr in the one place it's used. The EFI path also passes this parameter into __start_xen(). Have the EFI path set up multiboot_ptr too, and move the explanation of phyiscal-mode pointers. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> CC: Daniel P. Smith <dpsmith@apertussolutions.com> https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1509072662 --- xen/arch/x86/boot/x86_64.S | 2 -- xen/arch/x86/efi/efi-boot.h | 9 +++++++-- xen/arch/x86/include/asm/setup.h | 1 + xen/arch/x86/setup.c | 14 +++++--------- 4 files changed, 13 insertions(+), 13 deletions(-) base-commit: be84e7fe58b51f6b6dd907a038f0ef998a1e281e prerequisite-patch-id: 795f6e9425cc6a953166b530ae68df466a7a3c2b