| Message ID | 20230111142329.4379-1-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] x86/ucode/AMD: apply the patch early on every logical thread | expand |
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. 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. Jan
On Mon, Jan 16, 2023 at 2:47 PM Jan Beulich <jbeulich@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
On 23.01.2023 15:32, Sergey Dyasli wrote: > On Mon, Jan 16, 2023 at 2:47 PM Jan Beulich <jbeulich@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? 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
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..626aeb4d08 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -6,7 +6,8 @@ extern bool opt_ucode_allow_same; enum microcode_match_result { - OLD_UCODE, /* signature matched, but revision id is older or equal */ + OLD_UCODE, /* signature matched, but revision id is older */ + SAME_UCODE, /* signature matched, but revision id is the same */ NEW_UCODE, /* signature matched, but revision id is newer */ MIS_UCODE, /* signature mismatched */ };
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. 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). Intel's side of things is modified for consistency but provides no functional change. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v1 --> v2: - Expanded the commit message with the levelling section - Adjusted comment for OLD_UCODE 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 | 3 ++- 3 files changed, 22 insertions(+), 6 deletions(-)