RE: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar

Kaplan, David
2025-05-05T16:42:28Z

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Monday, May 5, 2025 11:30 AM
> To: Kaplan, David <David.Kaplan@amd.com>
> Cc: Borislav Petkov <bp@alien8.de>; Yosry Ahmed <yosry.ahmed@linux.dev>;
> Patrick Bellasi <derkling@google.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Josh Poimboeuf <jpoimboe@redhat.com>; Pawan Gupta
> <pawan.kumar.gupta@linux.intel.com>; x86@kernel.org; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org; Patrick Bellasi <derkling@matbug.net>; Brendan
> Jackman <jackmanb@google.com>; Michael Larabel
> <Michael@michaellarabel.com>
> Subject: Re: x86/bugs: KVM: Add support for SRSO_MSR_FIX, back for moar
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Mon, May 05, 2025, David Kaplan wrote:
> > > On Thu, May 01, 2025 at 09:56:44AM -0700, Sean Christopherson wrote:
> > > > Heh, I considered that, and even tried it this morning because I
> > > > thought it wouldn't be as tricky as I first thought, but turns
> > > > out, yeah, it's tricky. The complication is that KVM needs to
> > > > ensure
> > > BP_SPEC_REDUCE=1 on all CPUs before any VM is created.
> > > >
> > > > I thought it wouldn't be _that_ tricky once I realized the 1=>0
> > > > case doesn't require ordering, e.g. running host code while other
> > > > CPUs have
> > > > BP_SPEC_REDUCE=1 is totally fine, KVM just needs to ensure no
> > > > guest code is
> > > executed with BP_SPEC_REDUCE=0.
> > > > But guarding against all the possible edge cases is comically difficult.
> > > >
> > > > For giggles, I did get it working, but it's a rather absurd amount
> > > > of complexity
> > >
> > > Thanks for taking the time to explain - that's, well, funky. :-\
> > >
> > > Btw, in talking about this, David had this other idea which sounds
> > > interesting:
> > >
> > > How about we do a per-CPU var which holds down whether
> > > BP_SPEC_REDUCE is enabled on the CPU?
> > >
> > > It'll toggle the MSR bit before VMRUN on the CPU when num VMs goes
> > > 0=>1. This way you avoid the IPIs and you set the bit on time.
> >
> > Almost. My thought was that kvm_run could do something like:
> >
> > If (!this_cpu_read(bp_spec_reduce_is_set)) {
> > wrmsrl to set BP_SEC_REDUCE
> > this_cpu_write(bp_spec_reduce_is_set, 1) }
> >
> > That ensures the bit is set for your core before VMRUN. And as noted
> > below, you can clear the bit when the count drops to 0 but that one is
> > safe from race conditions.
>
> /facepalm
>
> I keep inverting the scenario in my head. I'm so used to KVM needing to ensure it
> doesn't run with guest state that I keep forgetting that running with
> BP_SPEC_REDUCE=1 is fine, just a bit slower.
>
> With that in mind, the best blend of simplicity and performance is likely to hook
> svm_prepare_switch_to_guest() and svm_prepare_host_switch().
> switch_to_guest() is called when KVM is about to do VMRUN, and host_switch() is
> called when the vCPU is put, i.e. when the task is scheduled out or when KVM_RUN
> exits to userspace.
>
> The existing svm->guest_state_loaded guard avoids toggling the bit when KVM
> handles a VM-Exit and re-enters the guest. The kernel may run a non-trivial
> amount of code with BP_SPEC_REDUCE, e.g. if #NPF triggers swap-in, an IRQ
> arrives while handling the exit, etc., but that's all fine from a security perspective.
>
> IIUC, per Boris[*] an IBPB is needed when toggling BP_SPEC_REDUCE on-
> demand:
>
> : You want to IBPB before clearing the MSR as otherwise host kernel will be
> : running with the mistrained gunk from the guest.
>
> [*]
> https://lore.kernel.org/all/20250217160728.GFZ7NewJHpMaWdiX2M@fat_crate.loc
> al
>
> Assuming that's the case...
>
> Compile-tested only. If this looks/sounds sane, I'll test the mechanics and write a
> changelog.

I'm having trouble following the patch...where do you clear the MSR bit?

I thought a per-cpu "cache" of the MSR bit might be good to avoid having to issue slow RDMSRs, if these paths are 'hot'. I don't know if that's the case or not.

Also keep in mind this is a shared (per-core) MSR bit. You can't just clear it when you exit one guest because the sibling thread might be running another guest.

So I think you still want the global count of VMs. You clear the MSR bit when that count drops to 0. But you can set the bit on your core before you enter guest mode if needed.

There is still a race condition I suppose where the count drops to 0, and you go to send the IPI to clear the MSR bit, but in the meantime somebody else starts a new VM. However that probably works naturally because in order to process the IPI you exit guest mode and on the next VMRUN, you'd check and see your MSR bit isn't set and go and set it.

--David Kaplan

>
> ---
> arch/x86/include/asm/msr-index.h | 2 +-
> arch/x86/kvm/svm/svm.c | 26 +++++++++++++++++++-------
> arch/x86/kvm/x86.h | 1 +
> arch/x86/lib/msr.c | 2 --
> 4 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index e6134ef2263d..0cc9267b872e 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -725,7 +725,7 @@
>
> /* Zen4 */
> #define MSR_ZEN4_BP_CFG 0xc001102e
> -#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT 4
> +#define MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE BIT(4)
> #define MSR_ZEN4_BP_CFG_SHARED_BTB_FIX_BIT 5
>
> /* Fam 19h MSRs */
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index
> cc1c721ba067..2d87ec216811 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -607,9 +607,6 @@ static void svm_disable_virtualization_cpu(void)
> kvm_cpu_svm_disable();
>
> amd_pmu_disable_virt();
> -
> - if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
> - msr_clear_bit(MSR_ZEN4_BP_CFG,
> MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
> }
>
> static int svm_enable_virtualization_cpu(void)
> @@ -687,9 +684,6 @@ static int svm_enable_virtualization_cpu(void)
> rdmsr(MSR_TSC_AUX, sev_es_host_save_area(sd)->tsc_aux, msr_hi);
> }
>
> - if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
> - msr_set_bit(MSR_ZEN4_BP_CFG,
> MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
> -
> return 0;
> }
>
> @@ -1550,12 +1544,25 @@ static void svm_prepare_switch_to_guest(struct
> kvm_vcpu *vcpu)
> (!boot_cpu_has(X86_FEATURE_V_TSC_AUX) || !sev_es_guest(vcpu-
> >kvm)))
> kvm_set_user_return_msr(tsc_aux_uret_slot, svm->tsc_aux, -1ull);
>
> + if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE))
> + wrmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg |
> + MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE);
> +
> svm->guest_state_loaded = true;
> }
>
> static void svm_prepare_host_switch(struct kvm_vcpu *vcpu) {
> - to_svm(vcpu)->guest_state_loaded = false;
> + struct vcpu_svm *svm = to_svm(vcpu);
> +
> + if (!svm->guest_state_loaded)
> + return;
> +
> + if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) {
> + indirect_branch_prediction_barrier();
> + rdmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg);
> + }
> + svm->guest_state_loaded = false;
> }
>
> static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu) @@ -5364,6 +5371,11
> @@ static __init int svm_hardware_setup(void)
>
> init_msrpm_offsets();
>
> + if (cpu_feature_enabled(X86_FEATURE_SRSO_BP_SPEC_REDUCE)) {
> + rdmsrl(MSR_ZEN4_BP_CFG, kvm_host.zen4_bp_cfg);
> + WARN_ON(kvm_host.zen4_bp_cfg &
> MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE);
> + }
> +
> kvm_caps.supported_xcr0 &= ~(XFEATURE_MASK_BNDREGS |
> XFEATURE_MASK_BNDCSR);
>
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index
> 88a9475899c8..629eae9e4f59 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -51,6 +51,7 @@ struct kvm_host_values {
> u64 xcr0;
> u64 xss;
> u64 arch_capabilities;
> + u64 zen4_bp_cfg;
> };
>
> void kvm_spurious_fault(void);
> diff --git a/arch/x86/lib/msr.c b/arch/x86/lib/msr.c index
> 5a18ecc04a6c..4bf4fad5b148 100644
> --- a/arch/x86/lib/msr.c
> +++ b/arch/x86/lib/msr.c
> @@ -103,7 +103,6 @@ int msr_set_bit(u32 msr, u8 bit) {
> return __flip_bit(msr, bit, true); } -EXPORT_SYMBOL_GPL(msr_set_bit);
>
> /**
> * msr_clear_bit - Clear @bit in a MSR @msr.
> @@ -119,7 +118,6 @@ int msr_clear_bit(u32 msr, u8 bit) {
> return __flip_bit(msr, bit, false); } -EXPORT_SYMBOL_GPL(msr_clear_bit);
>
> #ifdef CONFIG_TRACEPOINTS
> void do_trace_write_msr(unsigned int msr, u64 val, int failed)
>
> base-commit: 45eb29140e68ffe8e93a5471006858a018480a45
> --