Re: [PATCH v2] KVM: SVM: let alternatives handle the cases when RSB filling is required

[Amit Shah]

On (Mon) 08 Jul 2024 [11:59:45], Sean Christopherson wrote:
> On Mon, Jul 01, 2024, David Kaplan wrote:
> > > > >        /*
> > > > >         * AMD's AutoIBRS is equivalent to Intel's eIBRS - use the
> > > > > Intel feature
> > > > >         * flag and protect from vendor-specific bugs via the
> > > > > whitelist.
> > > > >         *
> > > > >         * Don't use AutoIBRS when SNP is enabled because it degrades
> > > > > host
> > > > >         * userspace indirect branch performance.
> > > > >         */
> > > > >        if ((x86_arch_cap_msr & ARCH_CAP_IBRS_ALL) ||
> > > > >            (cpu_has(c, X86_FEATURE_AUTOIBRS) &&
> > > > >             !cpu_feature_enabled(X86_FEATURE_SEV_SNP))) {
> > > > >                setup_force_cpu_cap(X86_FEATURE_IBRS_ENHANCED);
> > > > >                if (!cpu_matches(cpu_vuln_whitelist, NO_EIBRS_PBRSB)
> > > > > &&
> > > > >                    !(x86_arch_cap_msr & ARCH_CAP_PBRSB_NO))
> > > > >                        setup_force_cpu_bug(X86_BUG_EIBRS_PBRSB);
> > > > >        }
> > > >
> > > > Families 0FH through 12H don't have EIBRS or AutoIBRS, so there's no
> > > > cpu_vuln_whitelist[] lookup. Hence, no need to set the NO_EIBRS_PBRSB
> > > > bit, even if it is accurate.
> > >
> > > The commit that adds the RSB_VMEXIT_LITE feature flag does describe the
> > > bug in a good amount of detail:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i
> > > d=2b1299322016731d56807aa49254a5ea3080b6b3
> > >
> > > I've not seen any indication this is required for AMD CPUs.
> > >
> > > David, do you agree we don't need this?
> > >
> > 
> > It's not required, as AMD CPUs don't have the PBRSB issue with AutoIBRS.
> > Although I think Sean was talking about being extra paranoid
> 
> Ya.  I'm asking if there's a reason not to tack on X86_FEATURE_RSB_VMEXIT_LITE,
> beyond it effectively being dead code.  There's no runtime cost, and so assuming
> it doesn't get spuriously enabled, I don't see a downside.

Ah - I get it now.  You want to add this code for parity with
vmenter.S so that a future bug like this doesn't happen.

I disagree, though.  It's not really dead code - it does get patched
at runtime.  If a future AMD CPU has a bug that Intel doesn't, we'll
have to introduce a new ALTERNATIVE just for that condition - leading
to more complexity than is actually required.

Also - reviewers of code will get confused, wondering why this code
for AMD exists when the CPU vuln does not.

I get that we want to write defensive code, but this was a very
special condition that is unlikely to happen in this part of the code,
and also this was missed by the devs and the reviewers.

The good thing here is that missing this only leads to suboptimal
code, not a security bug.  So given all this, I vote for the
simplicity of code, rather than tacking on something.

Sound OK?


		Amit
--