Re: [PATCH v2 07/25] KVM: VMX: Set intercept for FRED MSRs

[Xin Li]

On 6/12/2024 2:32 PM, Sean Christopherson wrote:
> On Fri, Apr 19, 2024, Chao Gao wrote:
> > On Wed, Feb 07, 2024 at 09:26:27AM -0800, Xin Li wrote:
> > > Add FRED MSRs to the valid passthrough MSR list and set FRED MSRs intercept
> > > based on FRED enumeration.
>
> This needs a *much* more verbose explanation.  It's pretty darn obvious _what_
> KVM is doing, but it's not at all clear _why_ KVM is passing through FRED MSRs.
> E.g. why is FRED_SSP0 not included in the set of passthrough MSRs?
>
> > > static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > {
> > > 	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > +	bool fred_enumerated;
> > >
> > > 	kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_FRED);
> > > +	fred_enumerated = guest_can_use(vcpu, X86_FEATURE_FRED);
> > >
> > > -	if (guest_can_use(vcpu, X86_FEATURE_FRED)) {
> > > +	if (fred_enumerated) {
> > > 		vm_entry_controls_setbit(vmx, VM_ENTRY_LOAD_IA32_FRED);
> > > 		secondary_vm_exit_controls_setbit(vmx,
> > > 						  SECONDARY_VM_EXIT_SAVE_IA32_FRED |
> > > @@ -7788,6 +7793,16 @@ static void vmx_vcpu_config_fred_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > 						    SECONDARY_VM_EXIT_SAVE_IA32_FRED |
> > > 						    SECONDARY_VM_EXIT_LOAD_IA32_FRED);
> > > 	}
> > > +
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP0, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP1, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP2, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_RSP3, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_STKLVLS, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP1, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP2, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_SSP3, MSR_TYPE_RW, !fred_enumerated);
> > > +	vmx_set_intercept_for_msr(vcpu, MSR_IA32_FRED_CONFIG, MSR_TYPE_RW, !fred_enumerated);
> >
> > Use a for-loop here? e.g.,
> > 	for (i = MSR_IA32_FRED_RSP0; i <= MSR_IA32_FRED_CONFIG; i++)
>
> Hmm, I'd prefer to keep the open coded version.  It's not pretty, but I don't
> expect this to have much, if any, maintenance cost.  And using a loop makes it
> harder to both understand _exactly_ what's happening, and to search for relevant
> code.  E.g. it's quite difficult to see that FRED_SSP0 is still intercepted (see
> my comment regarding the changelog).


I owe you an explanation; I have been thinking about figuring out a way
to include FRED SSP0 in the FRED KVM patch set...

MSR_IA32_FRED_SSP0 is an alias of the CET MSR_IA32_PL0_SSP and likely to
be used in the same way as FRED RSP0, i.e., host FRED SSP0 _should_ be
restored in arch_exit_to_user_mode_prepare().  However as of today Linux
has no plan to utilize kernel shadow stack thus no one cares host FRED
SSP0 (no?).  But lets say anyway it is host's responsibility to manage
host FRED SSP0, then KVM only needs to take care of guest FRED SSP0
(just like how KVM should handle guest FRED RSP0) even before the
supervisor shadow stack feature is advertised to guest.

Another question is should KVM handle userspace request to set/get FRED
SSP0?  IMO, it should be part of CET state management.

Your suggestion?

Thanks!
    Xin