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

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

base-commit: be84e7fe58b51f6b6dd907a038f0ef998a1e281e
prerequisite-patch-id: 795f6e9425cc6a953166b530ae68df466a7a3c2b
-- 
2.39.5

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