Transition the memory map info to be held in struct boot_info.

No functional change intended.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
 xen/arch/x86/include/asm/bootinfo.h |  5 +++++
 xen/arch/x86/setup.c                | 12 +++++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index d2ca077d2356..e785ed1c5982 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -8,11 +8,16 @@
 #ifndef __XEN_X86_BOOTINFO_H__
 #define __XEN_X86_BOOTINFO_H__
 
+#include <xen/types.h>
+
 struct boot_info {
     unsigned int nr_mods;
 
     const char *boot_loader_name;
     const char *cmdline;
+
+    paddr_t mmap_addr;
+    uint32_t mmap_length;
 };
 
 #endif
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index a945fa10555f..c6b45ced00ae 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -297,6 +297,12 @@ static void __init multiboot_to_bootinfo(multiboot_info_t *mbi)
     else
         info.cmdline = "";
 
+    if ( mbi->flags & MBI_MEMMAP )
+    {
+        info.mmap_addr = mbi->mmap_addr;
+        info.mmap_length = mbi->mmap_length;
+    }
+
     boot_info = &info;
 }
 
@@ -1200,13 +1206,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
     {
         memmap_type = "Xen-e820";
     }
-    else if ( mbi->flags & MBI_MEMMAP )
+    else if ( boot_info->mmap_addr )
     {
         memmap_type = "Multiboot-e820";
-        while ( bytes < mbi->mmap_length &&
+        while ( bytes < boot_info->mmap_length &&
                 e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) )
         {
-            memory_map_t *map = __va(mbi->mmap_addr + bytes);
+            memory_map_t *map = __va(boot_info->mmap_addr + bytes);
 
             /*
              * This is a gross workaround for a BIOS bug. Some bootloaders do
-- 
2.30.2

On 30.08.2024 23:46, Daniel P. Smith wrote:
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -8,11 +8,16 @@
>  #ifndef __XEN_X86_BOOTINFO_H__
>  #define __XEN_X86_BOOTINFO_H__
>  
> +#include <xen/types.h>
> +
>  struct boot_info {
>      unsigned int nr_mods;
>  
>      const char *boot_loader_name;
>      const char *cmdline;
> +
> +    paddr_t mmap_addr;
> +    uint32_t mmap_length;

Why would this need to be a fixed-width type, unless we went in the direction
of what Alejandro mentioned in reply to patch 1? IOW at least we want to be
consistent with which kind of types are used here.

Jan

On 9/4/24 02:26, Jan Beulich wrote:
> On 30.08.2024 23:46, Daniel P. Smith wrote:
>> --- a/xen/arch/x86/include/asm/bootinfo.h
>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>> @@ -8,11 +8,16 @@
>>   #ifndef __XEN_X86_BOOTINFO_H__
>>   #define __XEN_X86_BOOTINFO_H__
>>   
>> +#include <xen/types.h>
>> +
>>   struct boot_info {
>>       unsigned int nr_mods;
>>   
>>       const char *boot_loader_name;
>>       const char *cmdline;
>> +
>> +    paddr_t mmap_addr;
>> +    uint32_t mmap_length;
> 
> Why would this need to be a fixed-width type, unless we went in the direction
> of what Alejandro mentioned in reply to patch 1? IOW at least we want to be
> consistent with which kind of types are used here.

At of right now, I would prefer not to go down the path of a boot 
protocol, but I am willing to have the discussion if a majority pushes 
for it. Unless that happens, I will switch this to size_t.

v/r,
dps

On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
> Transition the memory map info to be held in struct boot_info.
>
> No functional change intended.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
>  xen/arch/x86/include/asm/bootinfo.h |  5 +++++
>  xen/arch/x86/setup.c                | 12 +++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
> index d2ca077d2356..e785ed1c5982 100644
> --- a/xen/arch/x86/include/asm/bootinfo.h
> +++ b/xen/arch/x86/include/asm/bootinfo.h
> @@ -8,11 +8,16 @@
>  #ifndef __XEN_X86_BOOTINFO_H__
>  #define __XEN_X86_BOOTINFO_H__
>  
> +#include <xen/types.h>
> +
>  struct boot_info {
>      unsigned int nr_mods;
>  
>      const char *boot_loader_name;
>      const char *cmdline;
> +
> +    paddr_t mmap_addr;
> +    uint32_t mmap_length;

memmap please.

> @@ -1200,13 +1206,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>      {
>          memmap_type = "Xen-e820";
>      }
> -    else if ( mbi->flags & MBI_MEMMAP )
> +    else if ( boot_info->mmap_addr )
>      {
>          memmap_type = "Multiboot-e820";
> -        while ( bytes < mbi->mmap_length &&
> +        while ( bytes < boot_info->mmap_length &&
>                  e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) )
>          {
> -            memory_map_t *map = __va(mbi->mmap_addr + bytes);
> +            memory_map_t *map = __va(boot_info->mmap_addr + bytes);
>  
>              /*
>               * This is a gross workaround for a BIOS bug. Some bootloaders do

This is some very gnarly logic.  pvh_init() plays with e820_raw behind
the scenes and doesn't set MBI_MEMMAP.

Perhaps for later cleanup too, this logic wants folding into the new
multiboot_fill_boot_info() and leave __start_xen().

~Andrew

On 9/3/24 19:18, Andrew Cooper wrote:
> On 30/08/2024 10:46 pm, Daniel P. Smith wrote:
>> Transition the memory map info to be held in struct boot_info.
>>
>> No functional change intended.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>>   xen/arch/x86/include/asm/bootinfo.h |  5 +++++
>>   xen/arch/x86/setup.c                | 12 +++++++++---
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
>> index d2ca077d2356..e785ed1c5982 100644
>> --- a/xen/arch/x86/include/asm/bootinfo.h
>> +++ b/xen/arch/x86/include/asm/bootinfo.h
>> @@ -8,11 +8,16 @@
>>   #ifndef __XEN_X86_BOOTINFO_H__
>>   #define __XEN_X86_BOOTINFO_H__
>>   
>> +#include <xen/types.h>
>> +
>>   struct boot_info {
>>       unsigned int nr_mods;
>>   
>>       const char *boot_loader_name;
>>       const char *cmdline;
>> +
>> +    paddr_t mmap_addr;
>> +    uint32_t mmap_length;
> 
> memmap please.

Ack.

>> @@ -1200,13 +1206,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>>       {
>>           memmap_type = "Xen-e820";
>>       }
>> -    else if ( mbi->flags & MBI_MEMMAP )
>> +    else if ( boot_info->mmap_addr )
>>       {
>>           memmap_type = "Multiboot-e820";
>> -        while ( bytes < mbi->mmap_length &&
>> +        while ( bytes < boot_info->mmap_length &&
>>                   e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) )
>>           {
>> -            memory_map_t *map = __va(mbi->mmap_addr + bytes);
>> +            memory_map_t *map = __va(boot_info->mmap_addr + bytes);
>>   
>>               /*
>>                * This is a gross workaround for a BIOS bug. Some bootloaders do
> 
> This is some very gnarly logic.  pvh_init() plays with e820_raw behind
> the scenes and doesn't set MBI_MEMMAP.
> 
> Perhaps for later cleanup too, this logic wants folding into the new
> multiboot_fill_boot_info() and leave __start_xen().

I can add another patch that focuses on moving this to 
multiboot_fill_boot_info(). If there is also a transition to 
pvh_fill_boot_info(), then the question I have is, should this be better
served as a separate function similar to my proposal with the command 
line parsing?

v/r,
dps