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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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".

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.

>> --- 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.)

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.

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