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

[Jan Beulich]

On 30.01.2023 12:55, Sergey Dyasli wrote:
> The original issue has been reported on AMD Bulldozer-based CPUs where
> ucode loading loses the LWP feature bit in order to gain the IBPB bit.
> LWP disabling is per-SMT/CMT core modification and needs to happen on
> each sibling thread despite the shared microcode engine. Otherwise,
> logical CPUs will end up with different cpuid capabilities.
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216211
> 
> Guests running under Xen happen to be not affected because of levelling
> logic for the feature masking/override MSRs which causes the LWP bit to
> fall out and hides the issue. The latest recommendation from AMD, after
> discussing this bug, is to load ucode on every logical CPU.
> 
> In Linux kernel this issue has been addressed by e7ad18d1169c
> ("x86/microcode/AMD: Apply the patch early on every logical thread").
> Follow the same approach in Xen.
> 
> Introduce SAME_UCODE match result and use it for early AMD ucode
> loading. Take this opportunity and move opt_ucode_allow_same out of
> compare_revisions() to the relevant callers and also modify the warning
> message based on it. Intel's side of things is modified for consistency
> but provides no functional change.
> 
> Late loading is still performed only on the first of SMT/CMT
> siblings and only if a newer ucode revision has been provided (unless
> allow_same option is specified).

Another sentence on the "why" would be helpful, or else it ends up
looking as if there was an issue left in place. (I guess latently it's
going to be left in place anyway, until such time where we re-evaluate
features after ucode application.)

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -612,13 +612,21 @@ static long cf_check microcode_update_helper(void *data)
>       * that ucode revision.
>       */
>      spin_lock(&microcode_mutex);
> -    if ( microcode_cache &&
> -         alternative_call(ucode_ops.compare_patch,
> -                          patch, microcode_cache) != NEW_UCODE )
> +    if ( microcode_cache )
>      {
> +        enum microcode_match_result result;
> +
> +        result = alternative_call(ucode_ops.compare_patch, patch,
> +                                                           microcode_cache);

Nit: Indentation (3rd arg wants to align with 1st one, not 2nd).

>          spin_unlock(&microcode_mutex);
> -        printk(XENLOG_WARNING "microcode: couldn't find any newer revision "
> -                              "in the provided blob!\n");
> +
> +        if ( result == NEW_UCODE ||
> +             (opt_ucode_allow_same && result == SAME_UCODE) )
> +            goto apply;
> +
> +        printk(XENLOG_WARNING "microcode: couldn't find any newer%s revision 
> "
> +                              "in the provided blob!\n", 
> opt_ucode_allow_same ?
> +                                                         " (or the same)" : 
> "");

Since you touch this entire printk() anyway, may I ask that you re-flow
it to meet our guidelines:

        printk(XENLOG_WARNING
               "microcode: couldn't find any newer%s revision in the provided 
blob!\n",
               opt_ucode_allow_same ? " (or the same)" : "");

?

> @@ -626,6 +634,7 @@ static long cf_check microcode_update_helper(void *data)
>      }
>      spin_unlock(&microcode_mutex);
>  
> +apply:

Nit: Style (labels indented by at least one blank; see ./CODING_STYLE).
Personally I'd prefer if you got away without another "goto" here, as
our rule of thumb suggests that "goto" generally is to be used only for
error handling or other situations where the alternative would be
significantly worse (excess indentation, code duplication, ...).

Jan