Received-SPF: pass (google.com: domain of linux-kernel+bounces-126546-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 1 Apr 2024 17:14:02 +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 103/130] KVM: TDX: Handle EXIT_REASON_OTHER_SMI with
 MSMI
Message-ID: <Zgp62iK3HQEvcDyQ@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <4a96a33c01b547f6e89ecf40224c80afa59c6aa4.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <4a96a33c01b547f6e89ecf40224c80afa59c6aa4.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?ujAg4lTmf9AB7bxOqmZ1E66gIzJj/73/xICBO9GU27ZU63lgoGrfPuiLAq51?=
 =?us-ascii?Q?P8bEEJbx1aXMy22OLARtzlEm7d2Qa6rHjcfoXC7K+caak2g/DaQ4FQf/UtxT?=
 =?us-ascii?Q?XGCGOxtD+WYNKNoFPN+q6crxiFFD57Tk0A4BJT0Mx6BZwKXlKMIpEZPLd+h5?=
 =?us-ascii?Q?5mwUKf3GqWKjEw3DS0/Vvlqw7we9GVYORrM5Eo2sSB3Z+VHV3rU8g6lYsS1U?=
 =?us-ascii?Q?DInfuYipWoVRJWREqh8eoX9hpD7+t94Jz4Q6+3rc6udq4+T4OLhZP/2cugQz?=
 =?us-ascii?Q?800GToD1F/NqhGVF0elSwO8E9erYDRT9QJkAV1m/H0Gg6TOa9ZBvtkQMSc9S?=
 =?us-ascii?Q?BcZnR5b8CldFpFj9BR5V+DYrO/AqxzpiFTb1Qea2rueu//QMwnyFKyFHTZUA?=
 =?us-ascii?Q?ngmK2/wVyLY2SzYMhbpifYCvUOKOmvYyIkQ69OSN7NxNFYAN72xB7XxcKxsq?=
 =?us-ascii?Q?51ijziiQoLKkfG/aBB6Tybwxq18Wq72KGbWXIYWbvDhvDUL1jmspG/sr+zmy?=
 =?us-ascii?Q?Ng1AtA8WuM7gmbUyqsBkPE08JaYo5/0Q5dwAeF5vTNcY6fssJOK23j2IoVXF?=
 =?us-ascii?Q?WXuHaFDfjHvX5cR8YnK8zKUNnY/kjNc+/VG1mK6C+OXgxCWwSasj02+R8rDf?=
 =?us-ascii?Q?ABI1/s7Q27YLlqx7ho1uVHu5yi32B9XGAw+IpRDr6eMcT4Po4Om/JEZdHHen?=
 =?us-ascii?Q?NWz2r1Hlj5Lyv7IHIsNWONKAhefXmmRH1NdE9NKgSjl3g9YTw7UufTq1BBWv?=
 =?us-ascii?Q?GtxgTWOC2hmhT0PRRan8jT8cJ0OemEzvUKjtdiv1jMw/DqXTNaQY0qKu+NR3?=
 =?us-ascii?Q?YkQFskTNims8Dnk6PVHzbeTfo15tb4veAQKuvZlgM/PwyHuamYfE3mNNeNHF?=
 =?us-ascii?Q?f2tj3SLkWGEvQ7r8hrm4/pEPJOojULtyJdrjBNq+s5A+Uqd32Mm2L8YvDA0s?=
 =?us-ascii?Q?QCkkbMjnE7b6O1pYWfZAaQNJynSG3LqPyh+c6vkKOFR34hvPCAx2wyKFOPJR?=
 =?us-ascii?Q?J79WajSDUJX/tdnqI6xZGJQmJcTIYuh6z4gsmTN4YSeu3QXEASGGgGzzhcKF?=
 =?us-ascii?Q?zfsePCaKhr+AbcRd1SasvpfLGXhLQ/duUEfYkZXftcr2PV7l37BRSTYMYsS7?=
 =?us-ascii?Q?fJJgTIQQt/s9tfsEXe0KfiqTVO9eEx6+3F7U9hykj6TsNBRR/MwkPnyokBLT?=
 =?us-ascii?Q?aysgc5+Jx1iL+gP8xoldwU3gXJvBmkdjVh1fQgG0sEEp15SRid6lMmzJoEQp?=
 =?us-ascii?Q?kuYB7Jd+cbcrniaYGvRGq5eTqSmhtAfkYeQBjMnM+Lpp0a8Tgi2R5EuZbky7?=
 =?us-ascii?Q?l9JYNaQ/+WP2YRy83wlP5dGmZ3k2xybZF9rbYMiZuxBuC2dpYWg4pVlc51u9?=
 =?us-ascii?Q?613omIqsoSN4yvpHLcQj2Q5X8VD1rqedOOp3RMeGOAEyjfM3/Ug2O/z0rUFC?=
 =?us-ascii?Q?U9JlMB1nHN+1/M5n/Gd9KFFk4RuG2RHpcNmDFbLf3cKs59vS/IjG7HAVbxYt?=
 =?us-ascii?Q?irJvUB7GZw1F/zwYRozTfVNofLFJuA++YBvNwFGPJnvjYiOXqtqWsjxzvNGI?=
 =?us-ascii?Q?NXGtwvfGVQWAHzwwBoXczVHnWtLHYeLMgJDer0uV?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e2257274-788b-456e-f932-08dc522c1816
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2024 09:14:11.9517
 (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: HaQdeHcYTFVdp/ERhyY33fHWnbUBebhOZPP+5VKRv7G98iHb5N0gt6p/ltKwNWA6VSnPZJMGexjHCpkJzzNo9Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB5140
X-OriginatorOrg: intel.com

On Mon, Feb 26, 2024 at 12:26:45AM -0800, isaku.yamahata@intel.com wrote:
>From: Isaku Yamahata <isaku.yamahata@intel.com>
>
>When BIOS eMCA MCE-SMI morphing is enabled, the #MC is morphed to MSMI
>(Machine Check System Management Interrupt).  Then the SMI causes TD exit
>with the read reason of EXIT_REASON_OTHER_SMI with MSMI bit set in the exit
>qualification to KVM instead of EXIT_REASON_EXCEPTION_NMI with MC
>exception.
>
>Handle EXIT_REASON_OTHER_SMI with MSMI bit set in the exit qualification as
>MCE(Machine Check Exception) happened during TD guest running.
>
>Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>---
> arch/x86/kvm/vmx/tdx.c      | 40 ++++++++++++++++++++++++++++++++++---
> arch/x86/kvm/vmx/tdx_arch.h |  2 ++
> 2 files changed, 39 insertions(+), 3 deletions(-)
>
>diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>index bdd74682b474..117c2315f087 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -916,6 +916,30 @@ void tdx_handle_exit_irqoff(struct kvm_vcpu *vcpu)
> 						     tdexit_intr_info(vcpu));
> 	else if (exit_reason == EXIT_REASON_EXCEPTION_NMI)
> 		vmx_handle_exception_irqoff(vcpu, tdexit_intr_info(vcpu));
>+	else if (unlikely(tdx->exit_reason.non_recoverable ||
>+		 tdx->exit_reason.error)) {

why not just:
	else if (tdx->exit_reason.basic == EXIT_REASON_OTHER_SMI) {


i.e., does EXIT_REASON_OTHER_SMI imply exit_reason.non_recoverable or
exit_reason.error?

>+		/*
>+		 * The only reason it gets EXIT_REASON_OTHER_SMI is there is an
>+		 * #MSMI(Machine Check System Management Interrupt) with
>+		 * exit_qualification bit 0 set in TD guest.
>+		 * The #MSMI is delivered right after SEAMCALL returns,
>+		 * and an #MC is delivered to host kernel after SMI handler
>+		 * returns.
>+		 *
>+		 * The #MC right after SEAMCALL is fixed up and skipped in #MC

Looks fixing up and skipping #MC on the first instruction after TD-exit is
missing in v19?

>+		 * handler because it's an #MC happens in TD guest we cannot
>+		 * handle it with host's context.
>+		 *
>+		 * Call KVM's machine check handler explicitly here.
>+		 */
>+		if (tdx->exit_reason.basic == EXIT_REASON_OTHER_SMI) {
>+			unsigned long exit_qual;
>+
>+			exit_qual = tdexit_exit_qual(vcpu);
>+			if (exit_qual & TD_EXIT_OTHER_SMI_IS_MSMI)

>+				kvm_machine_check();
>+		}
>+	}
> }
> 
> static int tdx_handle_exception(struct kvm_vcpu *vcpu)
>@@ -1381,6 +1405,11 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> 			      exit_reason.full, exit_reason.basic,
> 			      to_kvm_tdx(vcpu->kvm)->hkid,
> 			      set_hkid_to_hpa(0, to_kvm_tdx(vcpu->kvm)->hkid));
>+
>+		/*
>+		 * tdx_handle_exit_irqoff() handled EXIT_REASON_OTHER_SMI.  It
>+		 * must be handled before enabling preemption because it's #MC.
>+		 */

Then EXIT_REASON_OTHER_SMI is handled, why still go to unhandled_exit?

> 		goto unhandled_exit;
> 	}
> 
>@@ -1419,9 +1448,14 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
> 		return tdx_handle_ept_misconfig(vcpu);
> 	case EXIT_REASON_OTHER_SMI:
> 		/*
>-		 * If reach here, it's not a Machine Check System Management
>-		 * Interrupt(MSMI).  #SMI is delivered and handled right after
>-		 * SEAMRET, nothing needs to be done in KVM.
>+		 * Unlike VMX, all the SMI in SEAM non-root mode (i.e. when
>+		 * TD guest vcpu is running) will cause TD exit to TDX module,
>+		 * then SEAMRET to KVM. Once it exits to KVM, SMI is delivered
>+		 * and handled right away.
>+		 *
>+		 * - If it's an Machine Check System Management Interrupt
>+		 *   (MSMI), it's handled above due to non_recoverable bit set.
>+		 * - If it's not an MSMI, don't need to do anything here.

This corrects a comment added in patch 100. Maybe we can just merge patch 100 into 
this one?

> 		 */
> 		return 1;
> 	default:
>diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
>index efc3c61c14ab..87ef22e9cd49 100644
>--- a/arch/x86/kvm/vmx/tdx_arch.h
>+++ b/arch/x86/kvm/vmx/tdx_arch.h
>@@ -42,6 +42,8 @@
> #define TDH_VP_WR			43
> #define TDH_SYS_LP_SHUTDOWN		44
> 
>+#define TD_EXIT_OTHER_SMI_IS_MSMI	BIT(1)
>+
> /* TDX control structure (TDR/TDCS/TDVPS) field access codes */
> #define TDX_NON_ARCH			BIT_ULL(63)
> #define TDX_CLASS_SHIFT			56
>-- 
>2.25.1
>
>