Re: [PATCH v4 04/44] x86/boot: move mmap info to boot info

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   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