pvh_init() sets up the mbi pointer, but leaves mbi_p at 0.  This isn't
compatbile with multiboot_fill_boot_info() starting from the physical address,
in order to remove the use of the mbi pointer.

Fixes: 038826b61e85 ("x86/boot: move x86 boot module counting into a new boot_info struct")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>

This is a testiment to how tangled the boot code really is.
---
 xen/arch/x86/setup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 6746ed8cced6..bfede5064e8c 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1048,6 +1048,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     {
         ASSERT(mbi_p == 0);
         pvh_init(&mbi, &mod);
+        mbi_p = __pa(mbi);
     }
     else
     {

base-commit: e9f227685e7204cb2293576ee5b745b828cb3cd7
-- 
2.39.5

On Sat, Oct 19, 2024 at 07:20:54PM +0100, Andrew Cooper wrote:
> pvh_init() sets up the mbi pointer, but leaves mbi_p at 0.  This isn't
> compatbile with multiboot_fill_boot_info() starting from the physical address,
> in order to remove the use of the mbi pointer.
> 
> Fixes: 038826b61e85 ("x86/boot: move x86 boot module counting into a new boot_info struct")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> This is a testiment to how tangled the boot code really is.

Did it causes crash in some boot configuration? If so, did some test
tripped on this (from what I see, not a gitlab one)?

> ---
>  xen/arch/x86/setup.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6746ed8cced6..bfede5064e8c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1048,6 +1048,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>      {
>          ASSERT(mbi_p == 0);
>          pvh_init(&mbi, &mod);
> +        mbi_p = __pa(mbi);
>      }
>      else
>      {
> 
> base-commit: e9f227685e7204cb2293576ee5b745b828cb3cd7
> -- 
> 2.39.5
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

On 21/10/2024 2:54 pm, Marek Marczykowski-Górecki wrote:
> On Sat, Oct 19, 2024 at 07:20:54PM +0100, Andrew Cooper wrote:
>> pvh_init() sets up the mbi pointer, but leaves mbi_p at 0.  This isn't
>> compatbile with multiboot_fill_boot_info() starting from the physical address,
>> in order to remove the use of the mbi pointer.
>>
>> Fixes: 038826b61e85 ("x86/boot: move x86 boot module counting into a new boot_info struct")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>> This is a testiment to how tangled the boot code really is.
> Did it causes crash in some boot configuration? If so, did some test
> tripped on this (from what I see, not a gitlab one)?

OSSTest found it.

E1t2D0w-0005rF-45@osstest.test-lab.xenproject.org

I'm writing a PVSHIM test for Gitlab CI right now, seeing as OSSTest is
going away imminently.

https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1505384944

~Andrew

On 10/19/24 14:20, Andrew Cooper wrote:
> pvh_init() sets up the mbi pointer, but leaves mbi_p at 0.  This isn't
> compatbile with multiboot_fill_boot_info() starting from the physical address,
> in order to remove the use of the mbi pointer.
> 
> Fixes: 038826b61e85 ("x86/boot: move x86 boot module counting into a new boot_info struct")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> This is a testiment to how tangled the boot code really is.
> ---
>   xen/arch/x86/setup.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6746ed8cced6..bfede5064e8c 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1048,6 +1048,7 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>       {
>           ASSERT(mbi_p == 0);
>           pvh_init(&mbi, &mod);
> +        mbi_p = __pa(mbi);
>       }
>       else
>       {
> 
> base-commit: e9f227685e7204cb2293576ee5b745b828cb3cd7

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

On 19/10/2024 7:23 pm, Daniel P. Smith wrote:
> On 10/19/24 14:20, Andrew Cooper wrote:
>> pvh_init() sets up the mbi pointer, but leaves mbi_p at 0.  This isn't
>> compatbile with multiboot_fill_boot_info() starting from the physical
>> address,
>> in order to remove the use of the mbi pointer.
>>
>> Fixes: 038826b61e85 ("x86/boot: move x86 boot module counting into a
>> new boot_info struct")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
>>
>> This is a testiment to how tangled the boot code really is.
>> ---
>>   xen/arch/x86/setup.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 6746ed8cced6..bfede5064e8c 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1048,6 +1048,7 @@ void asmlinkage __init noreturn
>> __start_xen(unsigned long mbi_p)
>>       {
>>           ASSERT(mbi_p == 0);
>>           pvh_init(&mbi, &mod);
>> +        mbi_p = __pa(mbi);
>>       }
>>       else
>>       {
>>
>> base-commit: e9f227685e7204cb2293576ee5b745b828cb3cd7
>
> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>

Thanks.  I'll get this committed right away so OSSTest can start working
on untangling itself.

~Andrew