Re: [PATCH] x86/bugs: Adjust SRSO mitigation to new features

[Sean Christopherson]

On Tue, Nov 05, 2024, Borislav Petkov wrote:
> On Mon, Nov 04, 2024 at 04:57:20PM -0800, Sean Christopherson wrote:
> > scripts/get_maintainer.pl :-)
> 
> That's what I used but I pruned the list.
> 
> Why, did I miss anyone?

All of the actual maintainers.  AFAIK, Paolo doesn't subscribe to kvm@.

> > But why do this in KVM?  E.g. why not set-and-forget in init_amd_zen4()?
> 
> Because there's no need to impose an unnecessary - albeit small - perf impact
> on users who don't do virt.
> 
> I'm currently gravitating towards the MSR toggling thing, i.e., only when the
> VMs number goes 0=>1 but I'm not sure. If udev rules *always* load kvm.ko then
> yes, the toggling thing sounds better. I.e., set it only when really needed.
> 
> > Shouldn't these be two separate patches?  AFAICT, while the two are related,
> > there are no strict dependencies between SRSO_USER_KERNEL_NO and
> > SRSO_MSR_FIX.
> 
> Meh, I can split them if you really want me to.

I do.

> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 9df3e1e5ae81..03f29912a638 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -608,6 +608,9 @@ static void svm_disable_virtualization_cpu(void)
> > >  	kvm_cpu_svm_disable();
> > >  
> > >  	amd_pmu_disable_virt();
> > > +
> > > +	if (cpu_feature_enabled(X86_FEATURE_SRSO_MSR_FIX))
> > > +		msr_clear_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
> > 
> > I don't like assuming the state of hardware.  E.g. if MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT
> > was already set, then KVM shouldn't clear it.
> 
> Right, I don't see that happening tho. If we have to sync the toggling of this
> bit between different places, we'll have to do some dance but so far its only
> user is KVM.
> 
> > KVM's usual method of restoring host MSRs is to snapshot the MSR into
> > "struct kvm_host_values" on module load, and then restore from there as
> > needed.  But that assumes all CPUs have the same value, which might not be
> > the case here?
> 
> Yes, the default value is 0 out of reset and it should be set on each logical
> CPU whenever we run VMs on it. I'd love to make it part of the VMRUN microcode
> but... :-)
> 
> > All that said, I'd still prefer that MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT is set
> > during boot, unless there's a good reason not to do so.
> 
> Yeah, unnecessary penalty on machines not running virt.

What does the bit actually do?  I can't find any useful documentation, and the
changelog is equally useless.