Re: [PATCH v4 13/20] KVM:VMX: Emulate read and write to CET MSRs

[Yang, Weijiang]

On 7/26/2023 4:06 PM, Chao Gao wrote:
> On Thu, Jul 20, 2023 at 11:03:45PM -0400, Yang Weijiang wrote:
> > Add VMX specific emulation for CET MSR read and write.
> > IBT feature is only available on Intel platforms now and the
> > virtualization interface to the control fields is vensor
> > specific, so split this part from the common code.
> >
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 40 ++++++++++++++++++++++++++++++++++++++++
> > arch/x86/kvm/x86.c     |  7 -------
> > 2 files changed, 40 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index c8d9870cfecb..b29817ec6f2e 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2093,6 +2093,21 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > 		else
> > 			msr_info->data = vmx->pt_desc.guest.addr_a[index / 2];
> > 		break;
> > +	case MSR_IA32_U_CET:
> > +	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> > +		return kvm_get_msr_common(vcpu, msr_info);
> kvm_get_msr_common() is called for the "default" case. so this can be dropped.

yes, I can drop these in vmx_get_msr(), just wanted to make it clearer, 
Thanks!

> > +	case MSR_IA32_S_CET:
> > +	case MSR_KVM_GUEST_SSP:
> > +	case MSR_IA32_INT_SSP_TAB:
> > +		if (kvm_get_msr_common(vcpu, msr_info))
> > +			return 1;
> > +		if (msr_info->index == MSR_KVM_GUEST_SSP)
> > +			msr_info->data = vmcs_readl(GUEST_SSP);
> > +		else if (msr_info->index == MSR_IA32_S_CET)
> > +			msr_info->data = vmcs_readl(GUEST_S_CET);
> > +		else if (msr_info->index == MSR_IA32_INT_SSP_TAB)
> > +			msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE);
> > +		break;
> > 	case MSR_IA32_DEBUGCTLMSR:
> > 		msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
> > 		break;
> > @@ -2402,6 +2417,31 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > 		else
> > 			vmx->pt_desc.guest.addr_a[index / 2] = data;
> > 		break;
> > +#define VMX_CET_CONTROL_MASK		(~GENMASK_ULL(9, 6))
> bits9-6 are reserved for both intel and amd. Shouldn't this check be
> done in the common code?

My thinking is, on AMD platform, bit 63:2 is anyway reserved since it 
doesn't support IBT,

so the checks in common code for AMD is enough, when the execution flow 
comes here,

it should be vmx, and need this additional check.

> > +#define CET_LEG_BITMAP_BASE(data)	((data) >> 12)
> > +#define CET_EXCLUSIVE_BITS		(CET_SUPPRESS | CET_WAIT_ENDBR)
> > +	case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> > +		return kvm_set_msr_common(vcpu, msr_info);
> this hunk can be dropped as well.

In patch 16, these lines still need to be added back for PL{0,1,2}_SSP, 
so would like keep it

here.

> > +		break;
> > +	case MSR_IA32_U_CET:
> > +	case MSR_IA32_S_CET:
> > +	case MSR_KVM_GUEST_SSP:
> > +	case MSR_IA32_INT_SSP_TAB:
> > +		if ((msr_index == MSR_IA32_U_CET ||
> > +		     msr_index == MSR_IA32_S_CET) &&
> > +		    ((data & ~VMX_CET_CONTROL_MASK) ||
> > +		     !IS_ALIGNED(CET_LEG_BITMAP_BASE(data), 4) ||
> > +		     (data & CET_EXCLUSIVE_BITS) == CET_EXCLUSIVE_BITS))
> > +			return 1;
> how about
>
> 	case MSR_IA32_U_CET:
> 	case MSR_IA32_S_CET:
> 		if ((data & ~VMX_CET_CONTROL_MASK) || ...
> 			...
>
> 	case MSR_KVM_GUEST_SSP:
> 	case MSR_IA32_INT_SSP_TAB:

Do you mean to use "fallthrough"?

If not, for  MSR_IA32_S_CET, we need vmcs_write() to handle it,

this is vmx specific code.