On 28.03.2023 17:07, Andrew Cooper wrote:
> On 28/03/2023 2:51 pm, Jan Beulich wrote:
>> On 27.03.2023 21:41, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -755,47 +755,51 @@ int microcode_update_one(void)
>>> return microcode_update_cpu(NULL);
>>> }
>>>
>>> -static int __init early_update_cache(const void *data, size_t len)
>>> +int __init microcode_init_cache(unsigned long *module_map,
>>> + const struct multiboot_info *mbi)
>>> {
>>> int rc = 0;
>>> struct microcode_patch *patch;
>>> + struct ucode_mod_blob blob = {};
>>>
>>> - if ( !data )
>>> - return -ENOMEM;
>> This is lost afaict. To be in sync with earlier code ) think you want to ...
>>
>>> + if ( ucode_scan )
>>> + /* Need to rescan the modules because they might have been
>>> relocated */
>>> + microcode_scan_module(module_map, mbi);
>>> +
>>> + if ( ucode_mod.mod_end )
>>> + {
>>> + blob.data = bootstrap_map(&ucode_mod);
>> ... check here instead of ...
>>
>>> + blob.size = ucode_mod.mod_end;
>>> + }
>>> + else if ( ucode_blob.size )
>>> + {
>>> + blob = ucode_blob;
>>> + }
>> (nit: unnecessary braces)
>>
>>> - patch = parse_blob(data, len);
>>> + if ( !blob.data )
>>> + return 0;
>> ... here, making the "return 0" the "else" to the earlier if/else-if.
>>
>> Alternatively, if you think the -ENOMEM isn't sensible, I'm happy to
>> consider respective justification for its removal.
>
> I'm a little on the fence here. Nothing checks the return value at all,
> and nothing printed a failure previously either.
So how about switching both microcode_init_cache() and, considering
the similar aspect in patch 3, early_microcode_init() to return void?
There's hardly any use the caller can make of the return value; if an
error message was to be logged, it likely would better be logged
closer to the source of the error.
Jan
