Received-SPF: pass (google.com: domain of linux-kernel+bounces-97953-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 10 Mar 2024 00:28:55 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Isaku Yamahata <isaku.yamahata@intel.com>
CC: <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<isaku.yamahata@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>,
	<erdemaktas@google.com>, Sean Christopherson <seanjc@google.com>, Sagi Shahar
	<sagis@google.com>, Kai Huang <kai.huang@intel.com>, <chen.bo@intel.com>,
	<hang.yuan@intel.com>, <tina.zhang@intel.com>,
	<isaku.yamahata@linux.intel.com>
Subject: Re: [PATCH v19 080/130] KVM: TDX: restore host xsave state when exit
 from the guest TD
Message-ID: <ZeyOR/YaubFwyiOC@chenyu5-mobl2>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <2894ed10014279f4b8caab582e3b7e7061b5dad3.1708933498.git.isaku.yamahata@intel.com>
 <Zel7kFS31SSVsSaJ@chenyu5-mobl2>
 <20240308205838.GA713729@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240308205838.GA713729@ls.amr.corp.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Cdemv5TeaI9/iPkzY4gUwy4xAbsEANDDhlyGaU80Ekq3mGSR2tJ8nZCq/NyR?=
 =?us-ascii?Q?DYHy/+d85u9MxKJde0U7lIU7b27RAfEwY3AuNr1EfVh8VZ1QTA/oYPFeUKnq?=
 =?us-ascii?Q?+3Q09j3SyfNEC4e/0j8MLsepXf6QJpew9RxtSJG2WIIg8kb12y09gNz6+oF6?=
 =?us-ascii?Q?WufJxVmu4Iijef/F346et5JUX8W37tt0MEK/V7JPx03ZomNLB9F3nSQu9vCw?=
 =?us-ascii?Q?EUBCjrgHH2UOpFks5WmSp7myWt2ZnP3Vl2x25EsFdivc9S+Td6lm5wy1fEDw?=
 =?us-ascii?Q?+3/a5PolCSNzQb+LDW1z4rMqTJDdYtg4mBfVvVsT4GfsGbRRmD8ND8Ro53Hw?=
 =?us-ascii?Q?ssPLVWg0atcZIrhLQDrGAZTB9gkHGM6pE2wrVQq56VVajZQXhMjXixJjIHh1?=
 =?us-ascii?Q?1nfLOUC1tXqJ+9Z4KyfgD9DnIL0KTcAszr8zHLatMgPN0vrU5fzOrv8oVNHc?=
 =?us-ascii?Q?03uA6Qh0O5Badq28ejfJGQ185DH84d2+POp2s0bMtTALDhOXhDt8M8roG/dt?=
 =?us-ascii?Q?TkHrKDnaUffWQi+M+aYiW9ghvjwhb7nUalMT4RxDMqcpWzUBOrfDKADutSD1?=
 =?us-ascii?Q?gUv0MADhNxh9x/ooXrdAqgekvlwKk3dnhYKTvxeP7JXPOjxVaoH01LC/1GHe?=
 =?us-ascii?Q?7vmk7EGnLpAnu3bm7IxYk7b5z4FKwHKgrakp9mOLqM2mDZtuFOdsZKkexpUq?=
 =?us-ascii?Q?H3spEOfXy7VMAGRxusy3hsNISYmOMxqaFhuVr4fVJXyfIJpcmjlG2WLZtBdT?=
 =?us-ascii?Q?R/laEsuPLBb/61ynCUxt+taoLmYzIwVoE0iPQVv2WZpeAJnIPj1/awdZ3O53?=
 =?us-ascii?Q?4pYOYRP1naXvfG3ic1N2uV51Hj4J1nPzNJ2cIAXMvwcYyPmQqMMxg3LD94Hq?=
 =?us-ascii?Q?O8brUN+i6X+AoVgQEhIJMyQxxixkoblkLuTmfkUTDNcbQ7Yv3S65nDd2hdnj?=
 =?us-ascii?Q?ZufxU+iDk+lZtJEf7C/eRL/32kIT2kTXC39kw2/EnwzNUmr81BMskEGv7vs5?=
 =?us-ascii?Q?dud+6rQJh8bhnjcITnOuhSuryL202ju6hduTYda/KVfWu/lDaALxlzFapoVK?=
 =?us-ascii?Q?jV0hDbqxrOdpFLFixij6AvcT4wcIxs27GOxbm23LzehZ6EfkdkLbMMM9+LoP?=
 =?us-ascii?Q?JVSwuKX9lpkRL01/m0Cfn8J/Rdk1F+pmhhoZfJN1zT95F8pCWAzzISTJv/Hw?=
 =?us-ascii?Q?kuaMvs4fg8eYKXvZwXdstmtDfeNmniE6i1gF5cqGTegk+bBLwcBSlQAz61sl?=
 =?us-ascii?Q?eczq6N/uwS1+iHBbm2P+60tx6uXUaFuhWCi9z+T2Xd8f3Fy6zqlmspUJvCJW?=
 =?us-ascii?Q?1jGlWW16uMSRUs2cLttHk1KfPGztumlznEqCjcqP1EOsvAVhZUgalYhX/Om8?=
 =?us-ascii?Q?fbHUsrjGiWXUUCUApfQgs7hnHk9+EFmiwGSVEbtX/hd7sgfRLU9BMRN0krpQ?=
 =?us-ascii?Q?SxPryHrBlhOAgvFYkRqgz1EWZmcgKyn4RyYUaNt1iPz9VkqcpziqgMZOXqvW?=
 =?us-ascii?Q?9W2W2i5spW4Cajy5JQYZIlBDuz91RILH2kHpH8njc2ldJLktOl3jk8fw6ETi?=
 =?us-ascii?Q?mXE86q3L7+MBfAlht4TgzVVN1mnuZji/m29WnyOc?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fff7818f-5bab-4169-be32-08dc40560c9c
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6020.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2024 16:29:10.4030
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: tmfkeRF3OwRYNiq6y1eEeG4lIuVp7lQuZMldqTPXyuw8tYiVIbRpR/Zw6uHmUWnrMAe7RtwgmX6ggftyAMSjhQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6569
X-OriginatorOrg: intel.com

On 2024-03-08 at 12:58:38 -0800, Isaku Yamahata wrote:
> On Thu, Mar 07, 2024 at 04:32:16PM +0800,
> Chen Yu <yu.c.chen@intel.com> wrote:
> 
> > On 2024-02-26 at 00:26:22 -0800, isaku.yamahata@intel.com wrote:
> > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > 
> > > On exiting from the guest TD, xsave state is clobbered.  Restore xsave
> > > state on TD exit.
> > > 
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > ---
> > > v19:
> > > - Add EXPORT_SYMBOL_GPL(host_xcr0)
> > > 
> > > v15 -> v16:
> > > - Added CET flag mask
> > > 
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > > ---
> > >  arch/x86/kvm/vmx/tdx.c | 19 +++++++++++++++++++
> > >  arch/x86/kvm/x86.c     |  1 +
> > >  2 files changed, 20 insertions(+)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index 9616b1aab6ce..199226c6cf55 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -2,6 +2,7 @@
> > >  #include <linux/cpu.h>
> > >  #include <linux/mmu_context.h>
> > >  
> > > +#include <asm/fpu/xcr.h>
> > >  #include <asm/tdx.h>
> > >  
> > >  #include "capabilities.h"
> > > @@ -534,6 +535,23 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > >  	 */
> > >  }
> > >  
> > > +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) &&
> > > +	    host_xcr0 != (kvm_tdx->xfam & kvm_caps.supported_xcr0))
> > > +		xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
> > > +	if (static_cpu_has(X86_FEATURE_XSAVES) &&
> > > +	    /* PT can be exposed to TD guest regardless of KVM's XSS support */
> > > +	    host_xss != (kvm_tdx->xfam &
> > > +			 (kvm_caps.supported_xss | XFEATURE_MASK_PT | TDX_TD_XFAM_CET)))
> > > +		wrmsrl(MSR_IA32_XSS, host_xss);
> > > +	if (static_cpu_has(X86_FEATURE_PKU) &&
> > > +	    (kvm_tdx->xfam & XFEATURE_MASK_PKRU))
> > > +		write_pkru(vcpu->arch.host_pkru);
> > > +}
> > 
> > Maybe one minor question regarding the pkru restore. In the non-TDX version
> > kvm_load_host_xsave_state(), it first tries to read the current setting
> > vcpu->arch.pkru = rdpkru(); if this setting does not equal to host_pkru,
> > it trigger the write_pkru on host. Does it mean we can also leverage that mechanism
> > in TDX to avoid 1 pkru write(I guess pkru write is costly than a read pkru)?
> 
> Yes, that's the intention.  When we set the PKRU feature for the guest, TDX
> module unconditionally initialize pkru.

I see, thanks for the information. Please correct me if I'm wrong, and I'm not sure
if wrpkru instruction would trigger the TD exit. The TDX module spec[1] mentioned PKS
(protected key for supervisor pages), but does not metion PKU for user pages. PKS
is controlled by MSR IA32_PKRS. The TDX module will passthrough the MSR IA32_PKRS
write in TD, because TDX module clears the PKS bitmap in VMCS:
https://github.com/intel/tdx-module/blob/tdx_1.5/src/common/helpers/helpers.c#L1723
so neither write to MSR IA32_PKRS nor wrpkru triggers TD exit.

However, after a second thought, I found that after commit 72a6c08c44e4, the current
code should not be a problem, because write_pkru() would first read the current pkru
settings and decide whether to update to the pkru register.

> Do you have use case that wrpkru()
> (without rdpkru()) is better?

I don't have use case yet. But with/without rdpkru() in tdx_restore_host_xsave_state(),
there is no much difference because write_pkru() has taken care of it if I understand
correctly.

thanks,
Chenyu