On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
> This commit introduces the start and size fields to struct boot_module and
> assigns their value during boot_info construction.
>
> The EFI entry point special cased itself, and pre-converted mod_start and
> mod_end to mfn and size respectively. This required an additional test
> in the coversion loop to not convert mod_start and mod_end when the conversion
> was done for both the multiboot and PVH boot entry points. To simplify the
> logic populating the start and size fields, the EFI module population logic
> was coverted to using start and end addresses.
>
> Signed-off-by: Daniel P. Smith <[email protected]>
> ---
> Changes since v5:
> - switched EFI population of mod_start/mod_end to addresses
> ---
> xen/arch/x86/efi/efi-boot.h | 4 ++--
> xen/arch/x86/include/asm/bootinfo.h | 3 +++
> xen/arch/x86/setup.c | 7 ++++++-
> 3 files changed, 11 insertions(+), 3 deletions(-)
Despite being small already, I'd still prefer to split this into two
patches.
One changing the EFI path, and a separate one adding in the new (real)
start/size fields.
Sods law would says that if we don't, bisection is going to end up here,
except it doesn't need to...
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 94f34433640f..779f101c3de7 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -732,8 +732,8 @@ static void __init efi_arch_handle_module(const struct
> file *file,
> if ( options )
> place_string(&mb_modules[mbi.mods_count].string, options);
> place_string(&mb_modules[mbi.mods_count].string, local_name.s);
> - mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
> - mb_modules[mbi.mods_count].mod_end = file->size;
> + mb_modules[mbi.mods_count].mod_start = file->addr;
> + mb_modules[mbi.mods_count].mod_end = file->addr + file->size;
... because you can't drop this shift until you have a full-width start
field to use. file->addr might exceed 4G, and truncate with this patch
in place.
You're going to need to keep the old semantics here, fill in all 4
fields, and keep the EFI special case in the final hunk until you can
remove the ->mod_{start,end} narrow fields.
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index f87faa31a2cf..6ee352fc0cde 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -309,8 +309,13 @@ static struct boot_info *__init
> multiboot_fill_boot_info(unsigned long mbi_p)
> * reserved for Xen itself
> */
> for ( i = 0; i <= bi->nr_modules; i++ )
> + {
> bi->mods[i].mod = &mods[i];
>
> + bi->mods[i].start = (paddr_t)mods[i].mod_start;
This cast isn't needed, but don't we need a shift in here?
~Andrew
