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

[Sean Christopherson <seanjc@google.com>]

On Mon, May 05, 2025, David Kaplan wrote:
> > > 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?

Gah, the rdmsrl() in svm_prepare_host_switch() should be a wrmsrl().

> 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.

Oh, you were only proposing deferring setting BP_SPEC_REDUCE.  I like it.  That
avoids setting BP_SPEC_REDUCE on CPUs that aren't running VMs, e.g. housekeeping
CPUs, and makes it a heck of a lot easier to elide the lock on 1<=>N transitions.

I posted v2 using that approach (when lore catches up):

https://lore.kernel.org/all/20250505180300.973137-1-seanjc@google.com

> 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.

Heh, well then never mind.  I'm not touching SMT coordination. 

> >  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);

As above, this is supposed to be wrmsrl().

> > +       }
> > +       svm->guest_state_loaded = false;
> >  }