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 core modification and needs to happen on each
sibling SMT 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

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. Late loading is still performed only on the first of SMT
siblings and only if a newer ucode revision has been provided (unless
allow_same option is specified).

Intel's side of things is modified for consistency but provides no
functional change.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/cpu/microcode/amd.c     | 16 +++++++++++++---
 xen/arch/x86/cpu/microcode/intel.c   |  9 +++++++--
 xen/arch/x86/cpu/microcode/private.h |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 4b097187a0..96db10a2e0 100644
--- 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;
+    }
 
     return OLD_UCODE;
 }
@@ -220,8 +225,13 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     uint32_t rev, old_rev = sig->rev;
+    enum microcode_match_result result = microcode_fits(patch);
 
-    if ( microcode_fits(patch) != NEW_UCODE )
+    /*
+     * 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 ( check_final_patch_levels(sig) )
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index f7fec4b4ed..59a99eee4e 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -232,8 +232,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;
+    }
 
     /*
      * Treat pre-production as always applicable - anyone using pre-production
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index 73b095d5bf..c4c6729f56 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -7,6 +7,7 @@ extern bool opt_ucode_allow_same;
 
 enum microcode_match_result {
     OLD_UCODE, /* signature matched, but revision id is older or equal */
+    SAME_UCODE, /* signature matched, but revision id is the same */
     NEW_UCODE, /* signature matched, but revision id is newer */
     MIS_UCODE, /* signature mismatched */
 };
-- 
2.17.1

On 05/01/2023 1:20 pm, 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 core modification and needs to happen on each
> sibling SMT 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

Bulldozer is CMT, not SMT.  The relevant property is that both CMT's
share a single microcode sequencer.

Xen happens to not be impacted by that specific bug, because we level
the feature masking/override MSRs, and the LWP bit falls out.


It's also important to state that loading on every logical processor is
the recommendation from AMD, after discussing that bug.  Because it
contradicts the currently written advice in the BKDG/PPRs.

> diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
> index 73b095d5bf..c4c6729f56 100644
> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -7,6 +7,7 @@ extern bool opt_ucode_allow_same;
>  
>  enum microcode_match_result {
>      OLD_UCODE, /* signature matched, but revision id is older or equal */
> +    SAME_UCODE, /* signature matched, but revision id is the same */
>      NEW_UCODE, /* signature matched, but revision id is newer */
>      MIS_UCODE, /* signature mismatched */
>  };

I don't think this is a clever idea.  For one, OLD and SAME are now
ambiguous (at least as far as the comments go), and having the
difference between the two depend on allow_same is unexpected to say the
least.

I never really liked the enum to begin with, and I think the logic would
be cleaner without it.


We depend entirely on there being one ucode blob which is applicable
globally across the system, so MIS_UCODE can be expressed as returning
NULL from the initial searches.  Everything else can then be expressed
in a normal {mem,str}cmp() way (i.e. -1/0/+1).

~Andrew

On Thu, Jan 5, 2023 at 10:56 PM Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> > diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
> > index 73b095d5bf..c4c6729f56 100644
> > --- a/xen/arch/x86/cpu/microcode/private.h
> > +++ b/xen/arch/x86/cpu/microcode/private.h
> > @@ -7,6 +7,7 @@ extern bool opt_ucode_allow_same;
> >
> >  enum microcode_match_result {
> >      OLD_UCODE, /* signature matched, but revision id is older or equal */
> > +    SAME_UCODE, /* signature matched, but revision id is the same */
> >      NEW_UCODE, /* signature matched, but revision id is newer */
> >      MIS_UCODE, /* signature mismatched */
> >  };
>
> I don't think this is a clever idea.  For one, OLD and SAME are now
> ambiguous (at least as far as the comments go), and having the
> difference between the two depend on allow_same is unexpected to say the
> least.

Sorry I missed that "equal" comment which is easily removable. What I
don't follow is your concern about allow_same. It's already changing
if OLD/NEW is returned and my patch makes it SAME/NEW.

> I never really liked the enum to begin with, and I think the logic would
> be cleaner without it.
>
>
> We depend entirely on there being one ucode blob which is applicable
> globally across the system, so MIS_UCODE can be expressed as returning
> NULL from the initial searches.  Everything else can then be expressed
> in a normal {mem,str}cmp() way (i.e. -1/0/+1).

This idea sounds good but in practice there are vendor-specific functions
which return enum microcode_match_result and I don't see how it could be
easily replaced with NULL/-1/0/+1 without code changes. I also find the
enum values easier to read.

Sergey