On Mon, Jan 16, 2023 at 2:47 PM Jan Beulich <jbeul...@suse.com> 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
