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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2023 15:32, Sergey Dyasli wrote:
> 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?

Neither addresses my comment. I think the command line option check
needs to move out of this function, into ...

>> 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.

... some (but not all) of the callers.

Jan