Re: [PATCH v6 08/44] x86/boot: convert mod refs to boot_module mod

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

On 10/17/24 19:02, Jason Andryuk wrote:
> On 2024-10-17 13:02, Daniel P. Smith wrote:
> > To allow a slow conversion of x86 over to struct boot_module, start with
> > replacing all references to module_t mod, only in setup.c, to the mod 
> > element
> > of struct boot_module. These serves twofold, first to allow the 
> > incremental
> > transition from module_t fields to struct boot_module fields. The 
> > second is to
> > allow the conversion of function definitions from taking module_t 
> > parameters to
> > accepting struct boot_module as needed when a transitioned field will be
> > accessed.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes since v5:
> > - rewrote commit message
> > - coding style changes
> > - added comment for initial_images assignment
> > ---
> >   xen/arch/x86/setup.c | 64 +++++++++++++++++++++++++-------------------
> >   1 file changed, 37 insertions(+), 27 deletions(-)
> >
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 11e1027d72dd..1b56eaf26efe 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1460,9 +1465,9 @@ void asmlinkage __init noreturn 
> > __start_xen(unsigned long mbi_p)
> >                * move mod[0], we incorporate this as extra space at 
> > the start.
> >                */
> >               struct boot_module *bm = &bi->mods[j];
> > -            unsigned long size = PAGE_ALIGN(bm->headroom + 
> > mod[j].mod_end);
> > +            unsigned long size = PAGE_ALIGN(bm->headroom + 
> > bm->mod->mod_end);
> > -            if ( mod[j].reserved )
> > +            if ( bi->mods[j].mod->reserved )
> Use bm->mod->reserved here?
Yah, that would be better.

> >                   continue;
> >               /* Don't overlap with other modules (or Xen itself). */
> > @@ -1591,8 +1598,9 @@ void asmlinkage __init noreturn 
> > __start_xen(unsigned long mbi_p)
> >                   map_e = boot_e820.map[j].addr + boot_e820.map[j].size;
> >                   for ( j = 0; j < bi->nr_modules; ++j )
> >                   {
> > -                    uint64_t end = pfn_to_paddr(mod[j].mod_start) +
> > -                                   mod[j].mod_end;
> > +                    uint64_t end = pfn_to_paddr(
> > +                                   bi->mods[j].mod->mod_start) +
> pfn_to_paddr and mod_start can fit on one line
I will check but you have to also account for the '+' as generally it 
should stay with the LHS.
> > +                                   bi->mods[j].mod->mod_end;
> >                       if ( map_e < end )
> >                           map_e = end;
> > @@ -1666,11 +1674,12 @@ void asmlinkage __init noreturn 
> > __start_xen(unsigned long mbi_p)
> >       for ( i = 0; i < bi->nr_modules; ++i )
> >       {
> > -        set_pdx_range(mod[i].mod_start,
> > -                      mod[i].mod_start + PFN_UP(mod[i].mod_end));
> > -        map_pages_to_xen((unsigned long)mfn_to_virt(mod[i].mod_start),
> > -                         _mfn(mod[i].mod_start),
> > -                         PFN_UP(mod[i].mod_end), PAGE_HYPERVISOR);
> > +        set_pdx_range(bi->mods[i].mod->mod_start,
> > +                      bi->mods[i].mod->mod_start +
> > +                      PFN_UP(bi->mods[i].mod->mod_end));
> Indent PFN_UP since it's a continuation of the previous argument?
Ack.


v/r,
dps