Re: [PATCH v2] x86/ucode/AMD: apply the patch early on every logical thread

[Sergey Dyasli <sergey.dyasli@xxxxxxxxx>]

On Mon, Jan 16, 2023 at 2:47 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 11.01.2023 15:23, Sergey Dyasli wrote:
> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -176,8 +176,13 @@ static enum microcode_match_result compare_revisions(
> >      if ( new_rev > old_rev )
> >          return NEW_UCODE;
> >
> > -    if ( opt_ucode_allow_same && new_rev == old_rev )
> > -        return NEW_UCODE;
> > +    if ( new_rev == old_rev )
> > +    {
> > +        if ( opt_ucode_allow_same )
> > +            return NEW_UCODE;
> > +        else
> > +            return SAME_UCODE;
> > +    }
>
> I find this misleading: "same" should not depend on the command line
> option.

The alternative diff I was considering is this:

--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -179,6 +179,9 @@ static enum microcode_match_result compare_revisions(
     if ( opt_ucode_allow_same && new_rev == old_rev )
         return NEW_UCODE;

+    if ( new_rev == old_rev )
+        return SAME_UCODE;
+
     return OLD_UCODE;
 }

Do you think the logic is clearer this way? Or should I simply remove
"else" from the first diff above?

> In fact the command line option should affect only the cases
> where ucode is actually to be loaded; it should not affect cases where
> the check is done merely to know whether the cache needs updating.
>
> With that e.g. microcode_update_helper() should then also be adjusted:
> It shouldn't say merely "newer" when "allow-same" is in effect.

I haven't tried late-loading an older ucode blob to see this
inconsistency, but you should be right. I'll test and adjust the
message.

Sergey