Re: [PATCH v9 2/6] x86/boot: introduce module release

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

On 11/15/24 11:50, Jason Andryuk wrote:
> On 2024-11-15 08:12, Daniel P. Smith wrote:
> > A precarious approach was used to release the pages used to hold a 
> > boot module.
> > The precariousness stemmed from the fact that in the case of PV dom0, the
> > initrd module pages may be either mapped or copied into the dom0 
> > address space.
> > In the former case, the PV dom0 construction code will set the size of 
> > the
> > module to zero, relying on discard_initial_images() to skip any 
> > modules with a
> > size of zero. In the latter case, the pages are freed by the PV dom0
> > construction code. This freeing of pages is done so that in either 
> > case, the
> > initrd variable can be reused for tracking the initrd location in dom0 
> > memory
> > through the remaining dom0 construction code.
> >
> > To encapsulate the logical action of releasing a boot module, the 
> > function
> > release_boot_module() is introduced along with the `released` flag 
> > added to
> > boot module. The boot module flag `released` allows the tracking of 
> > when a boot
> > module has been released by release_boot_module().
> >
> > As part of adopting release_boot_module() the function 
> > discard_initial_images()
> > is renamed to free_boot_modules(), a name that better reflects the 
> > functions
> > actions.
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes since v8:
> > - completely reworked the commit
> >    - switch backed to a releasing all but pv initrd approach
> >    - renamed discard_initial_images to free_boot_modules
> > ---
> >   xen/arch/x86/hvm/dom0_build.c       |  2 +-
> >   xen/arch/x86/include/asm/bootinfo.h |  2 ++
> >   xen/arch/x86/include/asm/setup.h    |  4 +++-
> >   xen/arch/x86/pv/dom0_build.c        | 27 +++++++++++++--------------
> >   xen/arch/x86/setup.c                | 27 +++++++++++++++------------
> >   5 files changed, 34 insertions(+), 28 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/ 
> > dom0_build.c
> > index d1bdf1b14601..d1410e1a02b0 100644
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -755,7 +755,7 @@ static int __init pvh_load_kernel(
> >       }
> >       /* Free temporary buffers. */
> > -    discard_initial_images();
> > +    free_boot_modules();
> This...
>
> >       if ( cmdline != NULL )
> >       {
> > diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> > index 6be3d7745fab..2580162f3df4 100644
> > --- a/xen/arch/x86/pv/dom0_build.c
> > +++ b/xen/arch/x86/pv/dom0_build.c
> > @@ -875,7 +874,7 @@ static int __init dom0_construct(struct boot_info 
> > *bi, struct domain *d)
> >       }
> >       /* Free temporary buffers. */
> > -    discard_initial_images();
> > +    free_boot_modules();
> ...and this.  I think Andrew requested/suggested moving to a single 
> free_boot_modules call:
>      They're both right at the end of construction, so it would
>      make far more sense for __start_xen() to do this after
>      create_dom0().   That also avoids needing to export the function.
I wanted to do this and had it written this way. Then I started testing 
it and the pvhshim test failed due to not enough ram to build the domU 
inside pvshim. I started splitting this commit to see where it broke the 
test case, and for an unknown reason, replacing these two calls with a 
single call in __start_xen() just after create_dom0() is the cause. 
Instead of trying to tear apart the construction logic to determine why, 
I backed this part of the change out for the time being.
> >       /* Set up start info area. */
> >       si = (start_info_t *)vstartinfo_start;
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 495e90a7e132..0bda1326a485 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > +void __init free_boot_modules(void)
> >   {
> >       struct boot_info *bi = &xen_boot_info;
> >       unsigned int i;
> >       for ( i = 0; i < bi->nr_modules; ++i )
> >       {
> > -        uint64_t start = pfn_to_paddr(bi->mods[i].mod->mod_start);
> > -        uint64_t size  = bi->mods[i].mod->mod_end;
> > -
> > -        /*
> > -         * Sometimes the initrd is mapped, rather than copied, into 
> > dom0.
> > -         * Size being 0 is how we're instructed to leave the module 
> > alone.
> > -         */
> > -        if ( size == 0 )
> > +        if ( bi->mods[i].released )
> >               continue;
> > -        init_domheap_pages(start, start + PAGE_ALIGN(size));
> > +        release_boot_module(&bi->mods[i]);
> >       }
> > -
> > -    bi->nr_modules = 0;
> IIUC, zero-ing here was a safety feature to ensure boot modules could 
> not be used after this point.  Should it be retained?
The released flag displaced the need for this, but I realized it would 
make it stronger if in bootstrap_map_bm() we add a check that the 
released flag is not set before mapping. I think this is a stronger 
approach without loosing information like the number of boot modules 
were passed.
v/r,
dps