Re: [PATCH 2/5] x86/ucode: Fold early_update_cache() into microcode_init_cache()

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> It is not valid to retain a bootstrap_map() across returning back to
>>> __start_xen(), but various pointers get stashed across calls.
>> It's error prone, yes, but "not valid" isn't really true imo: As long as
>> nothing calls bootstrap_map(NULL) all mappings will remain as they are.
> 
> And how is this code supposed to know whether it's stashed pointer is
> any good or not?
> 
> This is precisely "not valid" means.  It doesn't mean "it definitely
> doesn't work", but very much does mean "can't rely on it working".

Hmm, not my understanding of "not valid".

> c/s dc380df12ac which introduced microcode_init_cache() demonstrates the
> problem by having to call scan module a second time to refresh the
> cached pointer in ucode_blob.data.
> 
> The code we have today really is buggy, and is having to go out of its
> way to not trip over.

Right - the code is fragile. And it's okay calling it so.

>>> --- 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.
> 
> Right now, the early boostrap map limit is 1G-16M, so this really does
> fail with a large enough initrd to scan.  There is a corner case where
> the second pass can succeed where the first one failed, but this is also
> fixed in the general case when thread 1 loads microcode (and updates the
> BSP too.)

As said - with the removal justified in the description (e.g. by an
abridged version of the above), I'm okay.

> I'm also not convinced that early bootstrap mapping is safe if the
> bootloader places Xen in [16M, 1G) but if that goes wrong, it will go
> wrong before we get to microcode loading.

Hmm, yes, this could be a concern on systems with very little memory
below 4G.

Jan

> Overall, I think that bootstrap_map() itself should warn (and ideally
> provide a backtrace) if it fails to map, because one way or another it's
> an issue wanting fixing.  Individual callers can't really do anything
> useful.
> 
> ~Andrew