| Message ID | 20240830214730.1621-5-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
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 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/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
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
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
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(-)