Re: [PATCH 4/7] KVM: TDX: restore host xsave state when exit from the guest TD

[Chao Gao <chao.gao@intel.com>]

On Mon, Nov 25, 2024 at 01:10:37PM +0200, Adrian Hunter wrote:
>On 22/11/24 07:49, Chao Gao wrote:
>>> +static void tdx_restore_host_xsave_state(struct kvm_vcpu *vcpu)
>>> +{
>>> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
>>> +
>>> +	if (static_cpu_has(X86_FEATURE_XSAVE) &&
>>> +	    kvm_host.xcr0 != (kvm_tdx->xfam & kvm_caps.supported_xcr0))
>>> +		xsetbv(XCR_XFEATURE_ENABLED_MASK, kvm_host.xcr0);
>>> +	if (static_cpu_has(X86_FEATURE_XSAVES) &&
>>> +	    /* PT can be exposed to TD guest regardless of KVM's XSS support */
>>> +	    kvm_host.xss != (kvm_tdx->xfam &
>>> +			 (kvm_caps.supported_xss | XFEATURE_MASK_PT |
>>> +			  XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)))
>> 
>> Should we drop CET/PT from this series? I think they are worth a new
>> patch/series.
>
>This is not really about CET/PT
>
>What is happening here is that we are calculating the current
>MSR_IA32_XSS value based on the TDX Module spec which says the
>TDX Module sets MSR_IA32_XSS to the XSS bits from XFAM.  The
>TDX Module does that literally, from TDX Module source code:
>
>	#define XCR0_SUPERVISOR_BIT_MASK            0x0001FD00
>and
>	ia32_wrmsr(IA32_XSS_MSR_ADDR, xfam & XCR0_SUPERVISOR_BIT_MASK);
>
>For KVM, rather than:
>
>			kvm_tdx->xfam &
>			 (kvm_caps.supported_xss | XFEATURE_MASK_PT |
>			  XFEATURE_MASK_CET_USER | XFEATURE_MASK_CET_KERNEL)
>
>it would be more direct to define the bits and enforce them
>via tdx_get_supported_xfam() e.g.
>
>/* 
> * Before returning from TDH.VP.ENTER, the TDX Module assigns:
> *   XCR0 to the TD’s user-mode feature bits of XFAM (bits 7:0, 9)
> *   IA32_XSS to the TD's supervisor-mode feature bits of XFAM (bits 8, 16:10)
> */
>#define TDX_XFAM_XCR0_MASK	(GENMASK(7, 0) | BIT(9))
>#define TDX_XFAM_XSS_MASK	(GENMASK(16, 10) | BIT(8))
>#define TDX_XFAM_MASK		(TDX_XFAM_XCR0_MASK | TDX_XFAM_XSS_MASK)
>
>static u64 tdx_get_supported_xfam(const struct tdx_sys_info_td_conf *td_conf)
>{
>	u64 val = kvm_caps.supported_xcr0 | kvm_caps.supported_xss;
>
>	/* Ensure features are in the masks */
>	val &= TDX_XFAM_MASK;

Before exposing a feature to TD VMs, both the TDX module and KVM must support
it. In other words, kvm_tdx->xfam & kvm_caps.supported_xss should yield the
same result as kvm_tdx->xfam & TDX_XFAM_XSS_MASK. So, to me, the current
approach and your new proposal are functionally identical.

I prefer checking against kvm_caps.supported_xss because we don't need to
update TDX_XFAM_XSS/XCR0_MASK when new user/supervisor xstate bits are added.
Note kvm_caps.supported_xss/xcr0 need to be updated for normal VMs anyway.

>
>	if ((val & td_conf->xfam_fixed1) != td_conf->xfam_fixed1)
>		return 0;
>
>	val &= td_conf->xfam_fixed0;
>
>	return val;
>}
>
>and then:
>
>	if (static_cpu_has(X86_FEATURE_XSAVE) &&
>	    kvm_host.xcr0 != (kvm_tdx->xfam & TDX_XFAM_XCR0_MASK))
>		xsetbv(XCR_XFEATURE_ENABLED_MASK, kvm_host.xcr0);
>	if (static_cpu_has(X86_FEATURE_XSAVES) &&
>	    kvm_host.xss != (kvm_tdx->xfam & TDX_XFAM_XSS_MASK))
>		wrmsrl(MSR_IA32_XSS, kvm_host.xss);
>