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

[Jason Andryuk <jason.andryuk@amd.com>]

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@apertussolutions.com>
>>> ---
>>> 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