| Message ID | 20241006214956.24339-5-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Boot modules for Hyperlaunch | expand |
On 2024-10-06 17:49, 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> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
On 06.10.2024 23:49, Daniel P. Smith wrote: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -296,6 +296,12 @@ static struct boot_info __init *multiboot_fill_boot_info(unsigned long mbi_p) > else > bi->cmdline = ""; > > + if ( mbi->flags & MBI_MEMMAP ) > + { > + bi->memmap_addr = mbi->mmap_addr; > + bi->memmap_length = mbi->mmap_length; > + } > + > return bi; > } > > @@ -1185,13 +1191,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) > { > memmap_type = "Xen-e820"; > } > - else if ( mbi->flags & MBI_MEMMAP ) > + else if ( bi->memmap_addr ) I'd like to note that this isn't an exact transformation, as with the flag set the memory map could theoretically also like at address 0. As long as the legacy BIOS layout of low memory is as it is, that won't happen. I'm less certain going forward, for legacy-free hardware/firmware. Imo at the very least this needs mentioning as intentional in the description, for archeologists to later be able to tell whether this was an oversight. Or maybe it would be better to check ->memmap_length? That being zero clearly means there's effectively no map. Jan
On 10/9/24 11:13, Jan Beulich wrote: > On 06.10.2024 23:49, Daniel P. Smith wrote: >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -296,6 +296,12 @@ static struct boot_info __init *multiboot_fill_boot_info(unsigned long mbi_p) >> else >> bi->cmdline = ""; >> >> + if ( mbi->flags & MBI_MEMMAP ) >> + { >> + bi->memmap_addr = mbi->mmap_addr; >> + bi->memmap_length = mbi->mmap_length; >> + } >> + >> return bi; >> } >> >> @@ -1185,13 +1191,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) >> { >> memmap_type = "Xen-e820"; >> } >> - else if ( mbi->flags & MBI_MEMMAP ) >> + else if ( bi->memmap_addr ) > > I'd like to note that this isn't an exact transformation, as with the flag > set the memory map could theoretically also like at address 0. As long as > the legacy BIOS layout of low memory is as it is, that won't happen. I'm > less certain going forward, for legacy-free hardware/firmware. Imo at the > very least this needs mentioning as intentional in the description, for > archeologists to later be able to tell whether this was an oversight. > > Or maybe it would be better to check ->memmap_length? That being zero > clearly means there's effectively no map. I think checking memmap_length is a better approach because as you say, the only time it will be zero is if no map was provided. v/r, dps
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h index 327038465a44..87d311ac1399 100644 --- a/xen/arch/x86/include/asm/bootinfo.h +++ b/xen/arch/x86/include/asm/bootinfo.h @@ -8,6 +8,8 @@ #ifndef __XEN_X86_BOOTINFO_H__ #define __XEN_X86_BOOTINFO_H__ +#include <xen/types.h> + /* * Xen internal representation of information provided by the * bootloader/environment, or derived from the information. @@ -17,6 +19,9 @@ struct boot_info { const char *loader; const char *cmdline; + + paddr_t memmap_addr; + size_t memmap_length; }; #endif /* __XEN_X86_BOOTINFO_H__ */ diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 0921f296075f..f0482ca8cc55 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -296,6 +296,12 @@ static struct boot_info __init *multiboot_fill_boot_info(unsigned long mbi_p) else bi->cmdline = ""; + if ( mbi->flags & MBI_MEMMAP ) + { + bi->memmap_addr = mbi->mmap_addr; + bi->memmap_length = mbi->mmap_length; + } + return bi; } @@ -1185,13 +1191,13 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p) { memmap_type = "Xen-e820"; } - else if ( mbi->flags & MBI_MEMMAP ) + else if ( bi->memmap_addr ) { memmap_type = "Multiboot-e820"; - while ( bytes < mbi->mmap_length && + while ( bytes < bi->memmap_length && e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) ) { - memory_map_t *map = __va(mbi->mmap_addr + bytes); + memory_map_t *map = __va(bi->memmap_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(-)