Re: [PATCH v5 09/19] KVM:x86: Make guest supervisor states as non-XSAVE managed

[Yang, Weijiang]

On 8/3/2023 7:15 PM, Chao Gao wrote:
> On Thu, Aug 03, 2023 at 12:27:22AM -0400, Yang Weijiang wrote:
> > +void save_cet_supervisor_ssp(struct kvm_vcpu *vcpu)
> > +{
> > +	if (unlikely(guest_can_use(vcpu, X86_FEATURE_SHSTK))) {
> > +		rdmsrl(MSR_IA32_PL0_SSP, vcpu->arch.cet_s_ssp[0]);
> > +		rdmsrl(MSR_IA32_PL1_SSP, vcpu->arch.cet_s_ssp[1]);
> > +		rdmsrl(MSR_IA32_PL2_SSP, vcpu->arch.cet_s_ssp[2]);
> > +		/*
> > +		 * Omit reset to host PL{1,2}_SSP because Linux will never use
> > +		 * these MSRs.
> > +		 */
> > +		wrmsrl(MSR_IA32_PL0_SSP, 0);
> This wrmsrl() can be dropped because host doesn't support SSS yet.
Frankly speaking, I want to remove this line of code. But that would mess up the MSR
on host side, i.e., from host perspective, the MSRs could be filled with garbage data,
and looks awful. Anyway, I can remove it.
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(save_cet_supervisor_ssp);
> > +
> > +void reload_cet_supervisor_ssp(struct kvm_vcpu *vcpu)
> > +{
> > +	if (unlikely(guest_can_use(vcpu, X86_FEATURE_SHSTK))) {
> ditto
Below is to reload guest supervisor SSPs instead of resetting host ones.
> > +		wrmsrl(MSR_IA32_PL0_SSP, vcpu->arch.cet_s_ssp[0]);
> > +		wrmsrl(MSR_IA32_PL1_SSP, vcpu->arch.cet_s_ssp[1]);
> > +		wrmsrl(MSR_IA32_PL2_SSP, vcpu->arch.cet_s_ssp[2]);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(reload_cet_supervisor_ssp);
> > +
> > int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> > {
> > 	struct kvm_queued_exception *ex = &vcpu->arch.exception;
> > @@ -12133,6 +12158,7 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> >
> > 	vcpu->arch.cr3 = 0;
> > 	kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
> > +	memset(vcpu->arch.cet_s_ssp, 0, sizeof(vcpu->arch.cet_s_ssp));
> >
> > 	/*
> > 	 * CR0.CD/NW are set on RESET, preserved on INIT.  Note, some versions
> > @@ -12313,6 +12339,7 @@ void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
> > 		pmu->need_cleanup = true;
> > 		kvm_make_request(KVM_REQ_PMU, vcpu);
> > 	}
> > +
> remove the stray newline.
OK.
> > 	static_call(kvm_x86_sched_in)(vcpu, cpu);
> > }
> >
> > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> > index 6e6292915f8c..c69fc027f5ec 100644
> > --- a/arch/x86/kvm/x86.h
> > +++ b/arch/x86/kvm/x86.h
> > @@ -501,6 +501,9 @@ static inline void kvm_machine_check(void)
> >
> > void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu);
> > void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu);
> > +void save_cet_supervisor_ssp(struct kvm_vcpu *vcpu);
> > +void reload_cet_supervisor_ssp(struct kvm_vcpu *vcpu);
> nit: please add kvm_ prefix to the function names because they are exposed to
> other modules. "cet" in the names is a little redundant. I slightly prefer
> kvm_save/load_guest_supervisor_ssp()
Sure, actually I wanted to add the prefix, but at a second thought, the functions with
kvm_ are mostly generic functions in KVM, but here are the CET specific functions.
> Overall, this patch looks good to me. Hence,
>
> Reviewed-by: Chao Gao <chao.gao@xxxxxxxxx>
Thanks a lot for the review!
> > +
> > int kvm_spec_ctrl_test_value(u64 value);
> > bool __kvm_is_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4);
> > int kvm_handle_memory_failure(struct kvm_vcpu *vcpu, int r,
> > --
> > 2.27.0