Received-SPF: pass (google.com: domain of linux-kernel+bounces-128998-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 3 Apr 2024 11:25:19 +0800
From: Chao Gao <chao.gao@intel.com>
To: <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>
Subject: Re: [PATCH v19 108/130] KVM: TDX: Handle TDX PV HLT hypercall
Message-ID: <ZgzMH3944ZaBx8B3@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <c083430632ba9e80abd09bccd5609fb3cd9d9c63.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <c083430632ba9e80abd09bccd5609fb3cd9d9c63.1708933498.git.isaku.yamahata@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?V9Rr0QFnPR182LGtbaAfhjXthQkboj8AbwOIOYWkAt/t45rMQ/jISXyc8X4N?=
 =?us-ascii?Q?DqmSdqnw7jzWrSqjLX9uMeYWeZHRQ01zWpfN4d7ULwX3VMQrA5mK58IJIyEh?=
 =?us-ascii?Q?04y3P3GAG471s55rOAnZebrMbpdDciN1x0ufdud99mYViNxh7Ko4ybJjJfMw?=
 =?us-ascii?Q?khSMCqveJJMBneubbr6jGQMtseG08sBpskhXu2+fpevYt6IAaJxN4CcC0dqw?=
 =?us-ascii?Q?Wt1AeMCgSUNDGFjLd3Hhp7EswCvdaa4lHW9PdUQYzjFqnbQf4ZMArA9lbosK?=
 =?us-ascii?Q?jrsMQQDHO3pF1mipiuRBC5uPhJDSD3eD2z8yznB9qWZ0341VSNyqQ89oHJ2t?=
 =?us-ascii?Q?+b8qA0vVtwm90rXzwObq9E6voAux4qA8wfqW5b7/4971uYZr2ckJIrPpgQX1?=
 =?us-ascii?Q?ZLqM000iI27OyR7kpJLIz23wftsySQ4drmUpAjlzOorq0Ean/bjEcnze8xQP?=
 =?us-ascii?Q?3VI5Kz5iEvZdox6NixML3PD3qo2XWIkO3gs7MVnhIXKrVrfsAQ8Ymj67lilu?=
 =?us-ascii?Q?P75k3iENlv2F4bm0NAHzccl9+S3rCNARwIEmCSQ6cAIBMVjN/tZVbgiySTBA?=
 =?us-ascii?Q?L+mP9HD1EP/o4qNdHbx/SGH4r+W/SUYpC/Pw2i/6fyrNelhfGBXXRmKT//IG?=
 =?us-ascii?Q?Knfbd4UqjY4gCRCR/zzM/dTqz5NtT0zegQ0mgr6nMIDLDP5q8zX3mrb/xwtx?=
 =?us-ascii?Q?gJKK4DBY3DYLL9DTOox7d/tqsWh+JPTD0PEnMpd4o+xoHk8vmLK5//k2UMym?=
 =?us-ascii?Q?/JBxE4JUkgK7W9E91NYhQiI9OqESMfu3t+LesdfcTDya1PdODtDPjvFMEZGh?=
 =?us-ascii?Q?U6is0589UdHlUnHrOsuCVb7p+WxgbN41QoLEyQh8xQljjJ1Xe7706xZ3J1NV?=
 =?us-ascii?Q?ZFLj3x9xd9fulVaBKXQVxrOrz3aGxT3LPh5mZpS0RJnX3+DPQd+jknth2Ap+?=
 =?us-ascii?Q?qj9U7TO5R/F5Lvovl+XkT1IbtK5i2R4Il9+ckPmz/+LL3wXk+YAMr001hk15?=
 =?us-ascii?Q?Z1G0kEZCne009KzaTWLpmXP8CgBeITX3o792M+si4wZu1hg66jLRFkQ+D8Ie?=
 =?us-ascii?Q?pskupaPJt29lVq2Fx1SAorKZgzUHv2OqBcaeTpc96snh+up72gW7sOLxjlz5?=
 =?us-ascii?Q?gwvhtK8z/oQ/BtTpCogzjUHhW9H32LjWs1YIsTySTsoKOCTyitfxiSRDwOWT?=
 =?us-ascii?Q?JZ8NnmJChYXmmRPbYmnYh9t8JgQh1jilBGxYffXdUTjJeJ5jmk/P3YsfVncv?=
 =?us-ascii?Q?PPLwVF7qUh8Qk6BQBWpx9PbndV5mCCBUJPKq4mRtrfoDhTE7ROpsM1jOHyLq?=
 =?us-ascii?Q?7h6PtVEtwf3/hvXbxlXW6ShBFN2uIWNsQBwh+VWE1SnKwF8SWf3g0hT4Zzqp?=
 =?us-ascii?Q?XdF6Xb21c8XlU/ihcuFHIOXbG37wFrb74KH0RQ/drudXxOgRQ9F8oksedU6J?=
 =?us-ascii?Q?s22lqM/XbG/UNe1Za7zKwuXmWOaWjsh96IYTPDDi8QMfqg+OjqQ752BfZ7Ms?=
 =?us-ascii?Q?gd/SAESujJ7tlVvC0MxNoDYsZfDrXXrmWmr7v0C2Uac5fKKa+CDgpOabKxmT?=
 =?us-ascii?Q?nTL4lOVZd3aOGJPTzdwHhJl2BOVi1QAk8Qu5i2EK?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 63d44092-ff1b-44eb-b59c-08dc538db58e
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2024 03:25:28.2676
 (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: Of/fu+CmJjZujtQJ58hXytddQaeboAS8aoZmAzPlw+Qb4G0NSaBHAu8WW3ECXBPIZNsDzBHfVwvyZdMSsUGtKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5929
X-OriginatorOrg: intel.com

On Mon, Feb 26, 2024 at 12:26:50AM -0800, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>Wire up TDX PV HLT hypercall to the KVM backend function.
>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>---
>v19:
>- move tdvps_state_non_arch_check() to this patch
>
>v18:
>- drop buggy_hlt_workaround and use TDH.VP.RD(TD_VCPU_STATE_DETAILS)
>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>---
> arch/x86/kvm/vmx/tdx.c | 26 +++++++++++++++++++++++++-
> arch/x86/kvm/vmx/tdx.h |  4 ++++
> 2 files changed, 29 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>index eb68d6c148b6..a2caf2ae838c 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -688,7 +688,18 @@ void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> 
> bool tdx_protected_apic_has_interrupt(struct kvm_vcpu *vcpu)
> {
>-	return pi_has_pending_interrupt(vcpu);
>+	bool ret = pi_has_pending_interrupt(vcpu);

Maybe
	bool has_pending_interrupt = pi_has_pending_interrupt(vcpu);

"ret" isn't a good name. or even call pi_has_pending_interrupt() directly in
the if statement below.

>+	union tdx_vcpu_state_details details;
>+	struct vcpu_tdx *tdx = to_tdx(vcpu);
>+
>+	if (ret || vcpu->arch.mp_state != KVM_MP_STATE_HALTED)
>+		return true;

Question: why mp_state matters here?

>+
>+	if (tdx->interrupt_disabled_hlt)
>+		return false;

Shouldn't we move this into vt_interrupt_allowed()? VMX calls the function to
check if interrupt is disabled. KVM can clear tdx->interrupt_disabled_hlt on
every TD-enter and set it only on TD-exit due to the guest making a
TDVMCALL(hlt) w/ interrupt disabled.

>+
>+	details.full = td_state_non_arch_read64(tdx, TD_VCPU_STATE_DETAILS_NON_ARCH);
>+	return !!details.vmxip;
> }
> 
> void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
>@@ -1130,6 +1141,17 @@ static int tdx_emulate_cpuid(struct kvm_vcpu *vcpu)
> 	return 1;
> }
> 
>+static int tdx_emulate_hlt(struct kvm_vcpu *vcpu)
>+{
>+	struct vcpu_tdx *tdx = to_tdx(vcpu);
>+
>+	/* See tdx_protected_apic_has_interrupt() to avoid heavy seamcall */
>+	tdx->interrupt_disabled_hlt = tdvmcall_a0_read(vcpu);
>+
>+	tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
>+	return kvm_emulate_halt_noskip(vcpu);
>+}
>+
> static int handle_tdvmcall(struct kvm_vcpu *vcpu)
> {
> 	if (tdvmcall_exit_type(vcpu))
>@@ -1138,6 +1160,8 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu)
> 	switch (tdvmcall_leaf(vcpu)) {
> 	case EXIT_REASON_CPUID:
> 		return tdx_emulate_cpuid(vcpu);
>+	case EXIT_REASON_HLT:
>+		return tdx_emulate_hlt(vcpu);
> 	default:
> 		break;
> 	}
>diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
>index 4399d474764f..11c74c34555f 100644
>--- a/arch/x86/kvm/vmx/tdx.h
>+++ b/arch/x86/kvm/vmx/tdx.h
>@@ -104,6 +104,8 @@ struct vcpu_tdx {
> 	bool host_state_need_restore;
> 	u64 msr_host_kernel_gs_base;
> 
>+	bool interrupt_disabled_hlt;
>+
> 	/*
> 	 * Dummy to make pmu_intel not corrupt memory.
> 	 * TODO: Support PMU for TDX.  Future work.
>@@ -166,6 +168,7 @@ static __always_inline void tdvps_vmcs_check(u32 field, u8 bits)
> }
> 
> static __always_inline void tdvps_management_check(u64 field, u8 bits) {}
>+static __always_inline void tdvps_state_non_arch_check(u64 field, u8 bits) {}
> 
> #define TDX_BUILD_TDVPS_ACCESSORS(bits, uclass, lclass)				\
> static __always_inline u##bits td_##lclass##_read##bits(struct vcpu_tdx *tdx,	\
>@@ -226,6 +229,7 @@ TDX_BUILD_TDVPS_ACCESSORS(32, VMCS, vmcs);
> TDX_BUILD_TDVPS_ACCESSORS(64, VMCS, vmcs);
> 
> TDX_BUILD_TDVPS_ACCESSORS(8, MANAGEMENT, management);
>+TDX_BUILD_TDVPS_ACCESSORS(64, STATE_NON_ARCH, state_non_arch);
> 
> static __always_inline u64 td_tdcs_exec_read64(struct kvm_tdx *kvm_tdx, u32 field)
> {
>-- 
>2.25.1
>
>