On 16/05/2024 12:50 pm, Roger Pau Monné wrote:
> On Thu, May 16, 2024 at 12:31:03PM +0100, Andrew Cooper wrote:
>> When the revision in hardware is newer than anything Xen has to hand,
>> 'microcode_cache' isn't set up. Then, `xen-ucode` initiates the update
>> because it doesn't know whether the revisions across the system are symmetric
>> or not. This involves the patch getting all the way into the
>> apply_microcode() hooks before being found to be too old.
>>
>> This is all a giant mess and needs an overhaul, but in the short term simply
>> adjust the apply_microcode() to return -EEXIST.
>>
>> Also, unconditionally print the preexisting microcode revision on boot. It's
>> relevant information which is otherwise unavailable if Xen doesn't find new
>> microcode to use.
>>
>> Fixes: 648db37a155a ("x86/ucode: Distinguish "ucode already up to date"")
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Fouad Hilly <fouad.hi...@cloud.com>
>>
>> Sorry Fouad, but this collides with your `--force` series once again.
>> Hopefully it might make things fractionally easier.
>>
>> Background: For 06-55-04 (Skylake server, stepping 4 specifically), there's a
>> recent production firmware update which has a newer microcode revision than
>> exists in the Intel public microcode repository. It's causing a mess in our
>> automated testing, although it is finding good bugs...
>> ---
>> xen/arch/x86/cpu/microcode/amd.c | 7 +++++--
>> xen/arch/x86/cpu/microcode/core.c | 2 ++
>> xen/arch/x86/cpu/microcode/intel.c | 7 +++++--
>> 3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/amd.c
>> b/xen/arch/x86/cpu/microcode/amd.c
>> index 17e68697d5bf..f76a563c8b84 100644
>> --- a/xen/arch/x86/cpu/microcode/amd.c
>> +++ b/xen/arch/x86/cpu/microcode/amd.c
>> @@ -222,12 +222,15 @@ static int cf_check apply_microcode(const struct
>> microcode_patch *patch)
>> uint32_t rev, old_rev = sig->rev;
>> enum microcode_match_result result = microcode_fits(patch);
>>
>> + if ( result == MIS_UCODE )
>> + return -EINVAL;
>> +
>> /*
>> * Allow application of the same revision to pick up SMT-specific
>> changes
>> * even if the revision of the other SMT thread is already up-to-date.
>> */
>> - if ( result != NEW_UCODE && result != SAME_UCODE )
>> - return -EINVAL;
>> + if ( result == OLD_UCODE )
>> + return -EEXIST;
> Won't it be simpler to just add this check ahead of the existing one,
> so that you can leave the code as-is, iow:
>
> if ( result == OLD_UCODE )
> return -EEXIST;
>
> /*
> * Allow application of the same revision to pick up SMT-specific changes
> * even if the revision of the other SMT thread is already up-to-date.
> */
> if ( result != NEW_UCODE && result != SAME_UCODE )
> return -EINVAL;
>
> Thanks, Roger.
Not really, no. That still leaves this piece of logic which is
misleading IMO.
MIS_UCODE is the only -EINVAL worthy case.
Every other *_UCODE constant needs to be 0 or -EEXIST, depending on
allow-same/--force.
~Andrew
