Re: [PATCH v6 11/44] x86/boot: add start and size fields to struct boot_module

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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