Re: [PATCH v6 17/44] x86/boot: convert microcode loading to consume struct boot_info

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/10/2024 4:49 pm, Daniel P. Smith wrote:
> On 10/17/24 21:14, Andrew Cooper wrote:
>> On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
>>> diff --git a/xen/arch/x86/cpu/microcode/core.c
>>> b/xen/arch/x86/cpu/microcode/core.c
>>> index 8564e4d2c94c..22fea80bc97e 100644
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -178,16 +177,16 @@ static void __init microcode_scan_module(
>>>       /*
>>>        * Try all modules and see whichever could be the microcode blob.
>>>        */
>>> -    for ( i = 1 /* Ignore dom0 kernel */; i < mbi->mods_count; i++ )
>>> +    for ( i = 1 /* Ignore dom0 kernel */; i < bi->nr_modules; i++ )
>>>       {
>>>           if ( !test_bit(i, module_map) )
>>>               continue;
>>>   
>>
>> Somewhere in this series, it would be nice to purge the these "module 0
>> is dom0" special cases.  I'm not sure where best in the series to do it,
>> and it may not be here.
>>
>> Later, in "x86/boot: remove module_map usage from microcode loading" you
>> convert this test_bit() into a type != UNKNOWN check, but the pattern is
>> used elsewhere too.
>>
>> How about a for_each_unknown_module(bi, bm) helper?  (which at this
>> point can even use module_map in scope).
>
> So I do have the first_boot_module_index() iterator which I am using
> to effectively open-code your suggested for_each_unknown_module() in
> one or two places. I can introduce that helper when I first do the
> open coding,
> though I would like to make it a little more generic. I would prefer
> to do it as for_each_bootmodule(bi, bm, type). There is a
> scenario/enhancement that I would like to get to that may require
> doing an iteration on a type other than BOOTMOD_UNKNOWN.
>
> Would you be okay with leaving the module_map usage at this point and
> changing over to the for_each_bootmodule() when it is dropped? As I
> see it, otherwise I would have to make the helper initially work with
> module_map only to turn around and drop it when module_map goes away.
> At least to me, seems like unnecessary churn unless you see a way
> without causing the churn in the helper.

I'll leave it to your judgement about when is best to introduce the
helper.  You know the series better than I do.

~Andrew