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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/11/2024 5:18 pm, Jason Andryuk wrote:
> 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:
>>>>       /* 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.

Metadata about which module is which is potentially still interesting.

Either way, clobbering nr_modules was to make discard_initial_images()
idempotent, and the released flag is a better way of doing this now.

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

This is a long-standing issue.

The only way to fail is prior to the directmap being set up and the sum
of bootstrap_map_*()'s since the last unmap() exceeding ~1G.

Some callers check, others don't.  Really there wants to be uniform
handling (probably a backtrace from the innermost layer), and callers
able to handle NULL.

But, e.g. with microcode handling, only being able to map the first 2M
or 4M of a 2G initrd would still be useful.  Then again, if we can't map
the initrd, then later parts of boot are still going to go wrong.


Either way - this patch is an improvement.  Other improvements can come
later.

~Andrew