Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2019 00:18, Eslam Elnikety wrote:
> On 10.12.19 10:37, Jan Beulich wrote:
>> On 09.12.2019 09:41, Eslam Elnikety wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -2113,7 +2113,7 @@ logic applies:
>>>      active by default.
>>>   
>>>   ### ucode (x86)
>>> -> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
>>> +> `= List of [ <integer> | scan=<bool> | builtin=<bool>, nmi=<bool> ]`
>>
>> Despite my other question regarding the usefulness of this as a
>> whole a few comments.
>>
>> Do "scan" and "builtin" really need to exclude each other? I.e.
>> don't you mean , instead of | ?
> The useful case here would be specifying ucode=scan,builtin which would 
> translate to fallback onto the builtin microcode if a scan fails. In 
> fact, this is already the case given the implementation in v1. It will 
> be better to clarify this semantic by allowing scan,builtin.
> 
> On that note, I really see the "<integer>" and "scan=<bool>" to be 
> equal. Following the logic earlier we should probably also allow 
> ucode=<integer>,builtin. This translates to fallback to builtin if there 
> are no modules at index <integer>.

Almost - if the builtin one is newer than the separate blob, then
either of the cmdline options you name should still cause the
builtin one to be loaded. IOW you want to honor both options, not
prefer the earlier over a later one.

>>> +   ---help---
>>> +     Include the CPU microcode update in the Xen image itself. With this
>>> +     support, Xen can update the CPU microcode upon boot using the builtin
>>> +     microcode, with no need for an additional microcode boot modules.
>>> +
>>> +     If unsure, say N.
>>> +
>>> +config BUILTIN_UCODE_DIR
>>> +   string
>>> +   default "/lib/firmware"
>>> +   depends on BUILTIN_UCODE
>>
>> ... are two separate options needed at all? Can't this latter one
>> being the empty string just imply the feature to be disabled?
> 
> I can go either way here. To me, two options is clearer.

It's the other way around here, but I'd accept being outvoted.

>>> +    ucode_blob.data = xmalloc_bytes(ucode_blob.size);
>>> +    if ( !ucode_blob.data )
>>> +        return;
>>> +
>>> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>>> +        memcpy(ucode_blob.data, __builtin_amd_ucode_start, 
>>> ucode_blob.size);
>>> +    else
>>> +        memcpy(ucode_blob.data, __builtin_intel_ucode_start, 
>>> ucode_blob.size);
>>
>> ... why the copying? Can't you simply point ucode_blob.data at
>> __builtin_{amd,intel}_ucode_start?
> 
> I am all onboard. See my earlier response to Andrew. I used the same 
> logic that already exists for scan (which assumes that ucode_blob.data 
> is allocated and should be freed when all CPUs are updated).

The scan case may be different in that it may not lend itself
to re-using the blob in its original location. If that's not
the reason for the present behavior, then I think we would
want to do away with the unnecessary copying there as well.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel