Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/03/2020 12:33, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> ... and struct cpu_signature for good measure.
>>
>> No comment is passed on the suitability of the behaviour...
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/cpu/microcode/private.h | 46 
>> ++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-x86/microcode.h      |  5 ++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/private.h 
>> b/xen/arch/x86/cpu/microcode/private.h
>> index e64168a502..a2aec53047 100644
>> --- a/xen/arch/x86/cpu/microcode/private.h
>> +++ b/xen/arch/x86/cpu/microcode/private.h
>> @@ -14,14 +14,60 @@ enum microcode_match_result {
>>  struct microcode_patch; /* Opaque */
>>  
>>  struct microcode_ops {
>> +    /*
>> +     * Parse a microcode container.  Format is vendor-specific.
>> +     *
>> +     * Search within the container for the patch, suitable for the current
>> +     * CPU, which has the highest revision.  (Note: May be a patch which is
>> +     * older that what is running in the CPU.  This is a feature, to better
>> +     * cope with corner cases from buggy firmware.)
>> +     *
>> +     * If one is found, allocate and return a struct microcode_patch
>> +     * encapsulating the appropriate microcode patch.  Does not alias the
>> +     * original buffer.
>> +     *
>> +     * If one is not found, (nothing matches the current CPU), return NULL.
>> +     * Also may return ERR_PTR(-err), e.g. bad container, out of memory.
>> +     */
>>      struct microcode_patch *(*cpu_request_microcode)(const void *buf,
>>                                                       size_t size);
>> +
>> +    /* Obtain microcode-relevant details for the current CPU. */
>>      int (*collect_cpu_info)(struct cpu_signature *csig);
>> +
>> +    /*
>> +     * Attempt to load the provided patch into the CPU.  Returns -EIO if
>> +     * anything didn't go as expected.
>> +     */
>>      int (*apply_microcode)(const struct microcode_patch *patch);
> While at present -EIO may be the only error that may come back here, do
> we want to risk the comment going stale when another error return gets
> added? IOW - perhaps add "e.g." or some such?

Can do.

>
>> --- a/xen/include/asm-x86/microcode.h
>> +++ b/xen/include/asm-x86/microcode.h
>> @@ -7,8 +7,13 @@
>>  #include <public/xen.h>
>>  
>>  struct cpu_signature {
>> +    /* CPU signature (CPUID.1.EAX).  Only written on Intel. */
>>      unsigned int sig;
>> +
>> +    /* Platform Flags (only actually 1 bit).  Only applicable to Intel. */
>>      unsigned int pf;
> To me "only actually 1 bit" makes it an implication that this is the
> lowest bit (like in a bool represented in a 32-bit memory location).
> I didn't think this was the case though, so unless I'm wrong, could
> you clarify this a little?

There will be a single bit within the bottom 8 set (the 1 <<
MSR_PLATFORM_ID[52:50]), despite this field being called "Platform Flags".

~Andrew

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