Re: [PATCH 09/12] x86/boot: add start and size fields to struct boot_module

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

On 11/7/24 15:47, Jason Andryuk wrote:
> On 2024-11-02 13:25, Daniel P. Smith wrote:
> > Introduce the start and size fields to struct boot_module and
> > assigns their value during boot_info construction. All uses of 
> > module_t to get
> > the address and size of a module are replaced with start and size.
> >
> > The EFI entry point is a special case, as the EFI file loading boot 
> > service may
> > load a file beyond the 4G barrier. As a result, to make the address 
> > fit in the
> > 32bit integer used by the MB1 module_t structure, the frame number is 
> > stored in
> > mod_start and size in mod_end. Until the EFI entry point is 
> > enlightened to work
> > with boot_info and boot_module, multiboot_fill_boot_info will handle the
> > alternate values in mod_start and mod_end when EFI is detected.
> >
> > A result of the switch to start/size removes all uses of the mod field in
> > struct boot_modules, along with the uses of bootstra_map() and 
> > release_module()
> > functions. With all usage gone, they all are dropped here.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > --
> > Changes since v7:
> > - add the start/size change to bootstrap_map_bm()
> > - convert all BM start/size when introduced, consolidates:
> >      x86/boot: populate boot module for xen entry
> >      x86/boot: transition relocation calculations to struct boot_module
> > - consolidates all the removal commits
> >
> > Changes since v6:
> > - put the efi conversion for mod_start and mod_end back along with check
> > - dropped unnecessary cast
> > - updated the population of start and size fields to take into account 
> > efi
> > Changes since v5:
> > - switched EFI population of mod_start/mod_end to addresses
> >
> > a#    edit 336ac1fc0019 x86/boot: introduce boot domain
> > ---
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index d9785acf89b6..18b93d6a272a 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -313,13 +313,29 @@ static struct boot_info *__init 
> > multiboot_fill_boot_info(
> >        */
> >       for ( i = 0; i < MAX_NR_BOOTMODS && i < bi->nr_modules; i++ )
> >       {
> > -        bi->mods[i].mod = &mods[i];
> > -
> >           bi->mods[i].cmdline_pa = mods[i].string;
> > +
> > +        if ( !efi_enabled(EFI_LOADER) )
> > +        {
> > +            /*
> > +             * The EFI loader gives us modules which are already 
> > frame/size.
> > +             * Switch back to address/size.
> > +             */
> This comment...
>
> > +            bi->mods[i].start = mods[i].mod_start;
> > +            bi->mods[i].size = mods[i].mod_end - mods[i].mod_start;
> > +        }
> > +        else
> > +        {
> > +            /*
> > +             * PVH and BIOS loaders give us modules which are start/end.
> > +             * Switch to address/size.
> > +             */
> And this comment are reversed?  But I would just use positive logic:
>
> if ( efi_enabled(EFI_LOADER) )
>     /* EFI case */
> else
>     /* non-EFI */
Ack.

> > +            bi->mods[i].start = pfn_to_paddr(mods[i].mod_start);
> > +            bi->mods[i].size = mods[i].mod_end;
> > +        }
> >       }
> >       /* Variable 'i' should be one entry past the last module. */
> > -    bi->mods[i].mod = &mods[bi->nr_modules];
> >       bi->mods[i].type = BOOTMOD_XEN;
> >       return bi;
> > @@ -335,8 +351,8 @@ unsigned long __init 
> > initial_images_nrpages(nodeid_t node)
> >       for ( nr = i = 0; i < bi->nr_modules; ++i )
> >       {
> > -        unsigned long start = bi->mods[i].mod->mod_start;
> > -        unsigned long end   = start + PFN_UP(bi->mods[i].mod->mod_end);
> > +        unsigned long start = bi->mods[i].start;
> This should be paddr_to_pfn(bi->mods[i].start)?
Yes, and honestly probably a bad names for the vars as start/end tends 
to refer to addresses while pages usually have mfn/pfn in their name. 
The question is the churn from variable name not desired.
> > +        unsigned long end   = start + PFN_UP(bi->mods[i].size);
> >           if ( end > node_start && node_end > start )
> >               nr += min(node_end, end) - max(node_start, start);
> > @@ -1745,13 +1733,11 @@ void asmlinkage __init noreturn __start_xen(void)
> >       for ( i = 0; i < bi->nr_modules; ++i )
> >       {
> > -        const struct boot_module *bm = &bi->mods[i];
> > +        unsigned long s = bi->mods[i].start, l = bi->mods[i].size;
> > -        set_pdx_range(bm->mod->mod_start,
> > -                      bm->mod->mod_start + PFN_UP(bm->mod->mod_end));
> > -        map_pages_to_xen((unsigned long)mfn_to_virt(bm->mod->mod_start),
> > -                         _mfn(bm->mod->mod_start),
> > -                         PFN_UP(bm->mod->mod_end), PAGE_HYPERVISOR);
> > +        set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s) + PFN_UP(l));
> This is fine today since s (.start) is checked for page alignment.  The 
> other option would be `paddr_to_pfn(s + l) + 1`, but I'm not sure that 
> is an improvement.
Out of curiosity, why are you thinking that module start would never be 
paged aligned?
> You don't have to change anything.  Just noting something I noticed.
Ack.

v/r,
dps