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

[Adrian Hunter <adrian.hunter@intel.com>]

On 26/11/24 04:20, Chao Gao wrote:
> 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.

Arguably, making the addition of new XFAM bits more visible
is a good thing.

> Note kvm_caps.supported_xss/xcr0 need to be updated for normal VMs anyway.

The way the code is at the moment, that seems too fragile.
At the moment there are direct changes to XFAM bits in
tdx_get_supported_xfam() that are not reflected in supported_xss
and so have to be added to tdx_restore_host_xsave_state() as well.
That is, with the current code, changes to tdx_get_supported_xfam()
can break tdx_restore_host_xsave_state().

The new approach:
	reflects what the TDX Module does
	reflects what the TDX Module base spec says
	makes it harder to break tdx_restore_host_xsave_state()

> 
>>
>> 	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);
>>