Re: [PATCH 04/12] x86/boot: introduce module release

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-11-02 13:25, 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 explicitly copied into the dom0
> address space. So to handle this situation, 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.
>
> A function is introduced to release a module when it is no longer needed that
> accepts a boolean parameter, free_mem, to indicate if the corresponding pages
> can be freed. To track that a module has been released, the boot module flag
> `released` is introduced.
>
> The previous release model was a free all at once except those of size zeros,
> which would handle any unused modules passed. The new model is one of, free
> consumed module after usage is complete, thus unconsumed modules do not have a
> consumer to free them.
Slightly confusing.  Maybe just "The new model is to free modules after 
they are consumed.  Thus unconsumed modules are not freed."
> To address this, the discard_uknown_boot_modules() is
"unknown"

> introduced and called after the last module identification occurs, initrd, to
> free the pages of any boot modules that are identified as not being released.
> After domain construction completes, all modules should be freed. A walk of the
> boot modules is added after domain construction to validate and notify if a
> module is found not to have been released.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
> Changes since v7:
> - This is a new approach as an alternative to the `consumed` flag.
> ---
>   xen/arch/x86/cpu/microcode/core.c   |  4 ++
>   xen/arch/x86/hvm/dom0_build.c       |  7 ++--
>   xen/arch/x86/include/asm/bootinfo.h |  2 +
>   xen/arch/x86/include/asm/setup.h    |  3 +-
>   xen/arch/x86/pv/dom0_build.c        | 20 ++--------
>   xen/arch/x86/setup.c                | 57 +++++++++++++++++++++++------
>   xen/xsm/xsm_core.c                  |  5 +++
>   7 files changed, 67 insertions(+), 31 deletions(-)
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index d061ece0541f..e6d2d25fd038 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -341,27 +341,55 @@ unsigned long __init initial_images_nrpages(nodeid_t node)
>       return nr;
>   }
>   
> -void __init discard_initial_images(void) /* a.k.a. Free boot modules */
> +void __init release_boot_module(struct boot_module *bm, bool free_mem)
> +{
> +    uint64_t start = pfn_to_paddr(bm->mod->mod_start);
> +    uint64_t size  = bm->mod->mod_end;
> +
> +    if ( bm->released )
> +    {
> +        printk(XENLOG_WARNING "Attempt second release boot module of type 
> %d\n",
> +               bm->type);
> +        return;
> +    }
> +
> +    if ( free_mem )
> +        init_domheap_pages(start, start + PAGE_ALIGN(size));
> +
> +    bm->released = true;
> +}
> +
> +void __init release_module(const module_t *m, bool free_mem)
>   {
>       struct boot_info *bi = &xen_boot_info;
>       unsigned int i;
>   
> -    for ( i = 0; i < bi->nr_modules; ++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;
> +        if ( bi->mods[i].mod == m )
> +            release_boot_module(&bi->mods[i], free_mem);
> +    }
> +}
>   
> -        /*
> -         * 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 )
> +static void __init discard_unknown_boot_modules(void)
> +{
> +    struct boot_info *bi = &xen_boot_info;
> +    unsigned int i, count = 0;
> +
> +    for_each_boot_module_by_type(i, bi, BOOTMOD_UNKNOWN)
for_each_boot_module_by_type ( i, bi, BOOTMOD_UNKNOWN )

To match style from 74af2d98276d ("x86/boot: eliminate module_map")

> +    {
> +        struct boot_module *bm = &bi->mods[i];
> +
> +        if ( bm == NULL || bm->released )
>               continue;
>   
> -        init_domheap_pages(start, start + PAGE_ALIGN(size));
> +        release_boot_module(bm, true);
> +        count++;
>       }
>   
> -    bi->nr_modules = 0;
> +    if ( count )
> +        printk(XENLOG_DEBUG "Releasing pages for uknown boot module %d\n",
"unknown".  Since the operation already happened, maybe "Released pages 
for %d unknown boot modules"?  I'm not sure of the value of that 
message.  It would be more informative to provide the module index and 
maybe a page count.  The index would at least point to which module is 
unused.
> +               count);
>   }
>   
>   static void __init init_idle_domain(void)
> @@ -2111,6 +2139,8 @@ void asmlinkage __init noreturn __start_xen(void)
>                      initrdidx);
>       }
>   
> +    discard_unknown_boot_modules();
> +
>       /*
>        * We're going to setup domain0 using the module(s) that we stashed safely
>        * above our heap. The second module, if present, is an initrd ramdisk.
> @@ -2122,6 +2152,11 @@ void asmlinkage __init noreturn __start_xen(void)
>       if ( !dom0 )
>           panic("Could not set up DOM0 guest OS\n");
>   
> +    /* Check and warn if any modules did not get released */
> +    for ( i = 0; i < bi->nr_modules; i++ )
> +        if ( !bi->mods[i].released )
> +            printk(XENLOG_ERR "Boot module %d not released, memory leaked", i);
> +
Why not release the memory here instead of leaking it?

I feel like the corner case of mapping the dom0 initrd is leading to 
this manual approach or releasing modules individually.  That logic then 
has to be spread around.  discard_initial_images() kept the logic 
centralized.  Maybe just replace the mod_end == 0 special case with a 
"don't release me" flag that is checked in discard_initial_images()?
Regards,
Jason