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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-15 12:16, Daniel P. Smith wrote:
> 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.
Ah, ok.  Thanks for the info.

> > >       /* 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.
Andrew> Clobbering this prevents the loop constructs from working.

I thought the boot modules are unusable after free_boot_modules() is 
called, so I'm not clear on the utility of keeping the boot modules 
around and/or keeping the loop constructs working.  I wondered about, 
but didn't write, clearing the boot_module info in release_boot_module() 
to eliminate stale data hanging around.
Yes, a bootstrap_map_bm() check is a good idea.  Having said that, there 
is a lack of checking the return value of bootstrap_map_bm(), so would 
you panic?
Regards,
Jason