From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 769F1C4332F for ; Tue, 22 Nov 2022 18:58:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C54BD6B0071; Tue, 22 Nov 2022 13:58:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BDC6E6B0073; Tue, 22 Nov 2022 13:58:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A56238E0001; Tue, 22 Nov 2022 13:58:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 8EC9C6B0071 for ; Tue, 22 Nov 2022 13:58:00 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 600F81C689B for ; Tue, 22 Nov 2022 18:58:00 +0000 (UTC) X-FDA: 80161987920.18.B8C25A9 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf22.hostedemail.com (Postfix) with ESMTP id 8BDE0C000C for ; Tue, 22 Nov 2022 18:57:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1669143479; x=1700679479; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=Dy8QPCf5a8i9eyvXCeVU8Af+cGBQOxVnsTSgurdjvVA=; b=i1MqTur9zmxV0q4Xd6732K5AJwC69LMDaNpjbgqGXe9YKTvfEDj04VVx 5nwhhuZxSopkJrjNwcFsEm7ARqVpxBaS4kjqBETgoKpd0//N2lUDD+GNh 3MI1uvGLrJScN4hSEQml7/pNcO1kkGVaF69/H/unZ0Ds2GZM+SdEBGpkm XJ+Pn0XtBOYBC0bqNOIn4TWrfZfdmAqPjleaa8PYiK9KaHUq3lq6siJAP 0nus7gHa4AXT/zisdcxuqnE9pGlY0wXokUHPxbw8cfiCQqhVtbFfYMeEP eighTfqZlzYCx209EXTCl706eykI9p/NF1UuWYZ9X8brvYjK/cwkOTDlA Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10539"; a="378148488" X-IronPort-AV: E=Sophos;i="5.96,184,1665471600"; d="scan'208";a="378148488" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Nov 2022 10:57:57 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10539"; a="766433655" X-IronPort-AV: E=Sophos;i="5.96,184,1665471600"; d="scan'208";a="766433655" Received: from coltsavx-mobl1.amr.corp.intel.com (HELO [10.255.0.114]) ([10.255.0.114]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Nov 2022 10:57:54 -0800 Message-ID: Date: Tue, 22 Nov 2022 10:57:52 -0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH v7 06/20] x86/virt/tdx: Shut down TDX module in case of error Content-Language: en-US To: Kai Huang , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: linux-mm@kvack.org, seanjc@google.com, pbonzini@redhat.com, dan.j.williams@intel.com, rafael.j.wysocki@intel.com, kirill.shutemov@linux.intel.com, ying.huang@intel.com, reinette.chatre@intel.com, len.brown@intel.com, tony.luck@intel.com, peterz@infradead.org, ak@linux.intel.com, isaku.yamahata@intel.com, chao.gao@intel.com, sathyanarayanan.kuppuswamy@linux.intel.com, bagasdotme@gmail.com, sagis@google.com, imammedo@redhat.com References: <48505089b645019a734d85c2c29f3c8ae2dbd6bd.1668988357.git.kai.huang@intel.com> From: Dave Hansen In-Reply-To: <48505089b645019a734d85c2c29f3c8ae2dbd6bd.1668988357.git.kai.huang@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=i1MqTur9; spf=pass (imf22.hostedemail.com: domain of dave.hansen@intel.com designates 134.134.136.100 as permitted sender) smtp.mailfrom=dave.hansen@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669143479; a=rsa-sha256; cv=none; b=RBwce7fByeeGtKsaZPsseVqUk82XupoCGgMFmONuFdJ6xB6C12D8avRzfkt1M7RoXyVpXw 9hls0PH0Xt8PTdX09/dCbEKz1BTlTvB0lKAafcCgmKvrNWDBYSRX6D57DHDb7i979xgfb2 UR4iC/dcPmuyjAAQv6EECHNlhc5WsLg= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669143479; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=fdoazxgS0bKYXzx0kqRvaXk9jXvA0ufwKCsDjW4lycw=; b=a3RkgllVYjzhiYs8GIn+SrhQOzBc4bBykR1x+4zX/PFrX1ZWTL2NBsLnncv7taw6N66BPx NXHZCFgRXihcBkGtWuz4nV9IZJl4QOqFKLiWqUJzmoG9VLNPBFhGwWfellIMiA/Yr6bKgj I+KOGCXcCFsxY0s5uB3DjRNb6e1UY98= X-Rspam-User: Authentication-Results: imf22.hostedemail.com; dkim=none ("invalid DKIM record") header.d=intel.com header.s=Intel header.b=i1MqTur9; spf=pass (imf22.hostedemail.com: domain of dave.hansen@intel.com designates 134.134.136.100 as permitted sender) smtp.mailfrom=dave.hansen@intel.com; dmarc=pass (policy=none) header.from=intel.com X-Stat-Signature: j9ifmc345yche7ng8wgsb1smcqabrg3k X-Rspamd-Queue-Id: 8BDE0C000C X-Rspamd-Server: rspam09 X-HE-Tag: 1669143479-753888 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 11/20/22 16:26, Kai Huang wrote: > TDX supports shutting down the TDX module at any time during its > lifetime. After the module is shut down, no further TDX module SEAMCALL > leaf functions can be made to the module on any logical cpu. > > Shut down the TDX module in case of any error during the initialization > process. It's pointless to leave the TDX module in some middle state. > > Shutting down the TDX module requires calling TDH.SYS.LP.SHUTDOWN on all > BIOS-enabled CPUs, and the SEMACALL can run concurrently on different > CPUs. Implement a mechanism to run SEAMCALL concurrently on all online > CPUs and use it to shut down the module. Later logical-cpu scope module > initialization will use it too. To me, this starts to veer way too far into internal implementation details. Issue the TDH.SYS.LP.SHUTDOWN SEAMCALL on all BIOS-enabled CPUs to shut down the TDX module. This is also the point where you should talk about the new infrastructure. Why do you need a new 'struct seamcall_something'? What makes it special? > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index b06c1a2bc9cb..5db1a05cb4bd 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -13,6 +13,8 @@ > #include > #include > #include > +#include > +#include > #include > #include > #include > @@ -124,15 +126,27 @@ bool platform_tdx_enabled(void) > return !!tdx_keyid_num; > } > > +/* > + * Data structure to make SEAMCALL on multiple CPUs concurrently. > + * @err is set to -EFAULT when SEAMCALL fails on any cpu. > + */ > +struct seamcall_ctx { > + u64 fn; > + u64 rcx; > + u64 rdx; > + u64 r8; > + u64 r9; > + atomic_t err; > +}; > + > /* > * Wrapper of __seamcall() to convert SEAMCALL leaf function error code > * to kernel error code. @seamcall_ret and @out contain the SEAMCALL > * leaf function return code and the additional output respectively if > * not NULL. > */ > -static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > - u64 *seamcall_ret, > - struct tdx_module_output *out) > +static int seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > + u64 *seamcall_ret, struct tdx_module_output *out) > { > u64 sret; > > @@ -166,6 +180,25 @@ static int __always_unused seamcall(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9, > } > } > > +static void seamcall_smp_call_function(void *data) > +{ > + struct seamcall_ctx *sc = data; > + int ret; > + > + ret = seamcall(sc->fn, sc->rcx, sc->rdx, sc->r8, sc->r9, NULL, NULL); > + if (ret) > + atomic_set(&sc->err, -EFAULT); > +} The atomic_t is kinda silly. I guess it's not *that* wasteful though. I think it would have actually been a lot more clear if instead of containing an errno it was a *count* of the number of encountered errors. An "atomic_set()" where everyone is overwriting each other is a bit counterintuitive. It's OK here, of course, but it still looks goofy. If this were: atomic_inc(&sc->nr_errors); it would be a lot more clear that *anyone* can increment and that it truly is shared. > +/* > + * Call the SEAMCALL on all online CPUs concurrently. Caller to check > + * @sc->err to determine whether any SEAMCALL failed on any cpu. > + */ > +static void seamcall_on_each_cpu(struct seamcall_ctx *sc) > +{ > + on_each_cpu(seamcall_smp_call_function, sc, true); > +} > + > /* > * Detect and initialize the TDX module. > * > @@ -181,7 +214,9 @@ static int init_tdx_module(void) > > static void shutdown_tdx_module(void) > { > - /* TODO: Shut down the TDX module */ > + struct seamcall_ctx sc = { .fn = TDH_SYS_LP_SHUTDOWN }; > + > + seamcall_on_each_cpu(&sc); > } The seamcall_on_each_cpu() function is silly as-is. Either collapse the functions or note in the changelog why this is not as silly as it looks. > static int __tdx_enable(void) > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 92a8de957dc7..215cc1065d78 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -12,6 +12,11 @@ > /* MSR to report KeyID partitioning between MKTME and TDX */ > #define MSR_IA32_MKTME_KEYID_PARTITIONING 0x00000087 > > +/* > + * TDX module SEAMCALL leaf functions > + */ > +#define TDH_SYS_LP_SHUTDOWN 44 > + > /* > * Do not put any hardware-defined TDX structure representations below > * this comment!