RE: [PATCH v3 05/35] x86/bugs: Restructure taa mitigation

Kaplan, David
2025-02-11T17:17:20Z

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@kernel.org>
> Sent: Monday, February 10, 2025 4:50 PM
> To: Kaplan, David <David.Kaplan@amd.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>; Borislav Petkov <bp@alien8.de>; Peter
> Zijlstra <peterz@infradead.org>; Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com>; Ingo Molnar <mingo@redhat.com>; Dave
> Hansen <dave.hansen@linux.intel.com>; x86@kernel.org; H . Peter Anvin
> <hpa@zytor.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 05/35] x86/bugs: Restructure taa mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Jan 08, 2025 at 02:24:45PM -0600, David Kaplan wrote:
> > @@ -400,48 +402,71 @@ static void __init taa_select_mitigation(void)
> > return;
> > }
> >
> > - if (cpu_mitigations_off()) {
> > + if (cpu_mitigations_off())
> > taa_mitigation = TAA_MITIGATION_OFF;
> > - return;
> > - }
> >
> > /*
> > * TAA mitigation via VERW is turned off if both
> > * tsx_async_abort=off and mds=off are specified.
> > + *
> > + * MDS mitigation will be checked in taa_update_mitigation().
> > */
> > - if (taa_mitigation == TAA_MITIGATION_OFF &&
> > - mds_mitigation == MDS_MITIGATION_OFF)
> > + if (taa_mitigation == TAA_MITIGATION_OFF)
> > return;
>
> This check seems rather pointless, the only thing after this is the
> TAA_MITIGATION_AUTO check.

True, and it can be removed. But in patch 4 in the mds logic you did suggest having an explicit return to make it clear that none of the later conditions applied. I'm not sure I feel strongly either way, but I'd like to be consistent.

>
> >
> > - if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> > + /* Microcode will be checked in taa_update_mitigation(). */
> > + if (taa_mitigation == TAA_MITIGATION_AUTO)
> > taa_mitigation = TAA_MITIGATION_VERW;
> > - else
> > - taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
>
> In the previous patch, MDS checks for ucode in both select and update, which is
> overkill. That should probably be done only in
> mds_update_mitigation() to be consistent with how TAA does it here?

I assume you're referring to the X86_FEATURE_MD_CLEAR check. I'll fix this in the mds patch to be similar, good idea.

>
> >
> > - /*
> > - * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and
> MDS_NO=1.
> > - * A microcode update fixes this behavior to clear CPU buffers. It also
> > - * adds support for MSR_IA32_TSX_CTRL which is enumerated by the
> > - * ARCH_CAP_TSX_CTRL_MSR bit.
> > - *
> > - * On MDS_NO=1 CPUs if ARCH_CAP_TSX_CTRL_MSR is not set,
> microcode
> > - * update is required.
> > - */
> > - if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
> > - !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
> > - taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> > +}
>
> Extra whitespace here at the end of the function.

Ack

>
> > +static void __init taa_update_mitigation(void) {
> > + if (!boot_cpu_has_bug(X86_BUG_TAA) || cpu_mitigations_off())
> > + return;
> > +
> > + if (verw_mitigation_enabled())
> > + taa_mitigation = TAA_MITIGATION_VERW;
>
> This overwrites TAA_MITIGATION_TSX_DISABLED?
>
> I think reporting TSX disabled here is more accurate than reporting VERW, since
> the VERW is only done to mitigate the other vulns.

Agreed, will fix.

>
> > +static void __init taa_apply_mitigation(void) {
> > + if (taa_mitigation == TAA_MITIGATION_VERW ||
> > + taa_mitigation == TAA_MITIGATION_UCODE_NEEDED) {
> > + /*
> > + * TSX is enabled, select alternate mitigation for TAA which is
> > + * the same as MDS. Enable MDS static branch to clear CPU buffers.
> > + *
> > + * For guests that can't determine whether the correct microcode is
> > + * present on host, enable the mitigation for UCODE_NEEDED as well.
> > + */
> > + setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> > +
> > + if (taa_nosmt || cpu_mitigations_auto_nosmt())
> > + cpu_smt_disable(false);
> > + }
> >
> > - if (taa_nosmt || cpu_mitigations_auto_nosmt())
> > - cpu_smt_disable(false);
> > }
>
> Another extra whitespace here at the end of the function.

Ack

Thanks --David Kaplan