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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/11/2024 4:50 pm, 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.

Yeah...  It turns out that also breaks PVH Boot in Gitlab, for reasons
we still don't understand.

I'd still like to clean it up, but it wants to be detached from the
mechanics of changing the data-structures.

>
>>         /* 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?

Clobbering this prevents the loop constructs from working.

Safety is now based on the .released field, which is better IMO.

~Andrew