Received-SPF: pass (google.com: domain of linux-kernel+bounces-109529-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 21 Mar 2024 09:06:46 +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>, Binbin Wu
	<binbin.wu@linux.intel.com>
Subject: Re: [PATCH v19 041/130] KVM: TDX: Refuse to unplug the last cpu on
 the package
Message-ID: <ZfuIJpHzp4sEjCi/@chao-email>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <15d4d08cf1fe16777f8f8d84ee940e41afbad406.1708933498.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <15d4d08cf1fe16777f8f8d84ee940e41afbad406.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?C3cP+J5ECp/LwcbUgInu6OgjXehekQR968utbSLjZwe4vzgdRFVfk8qThMLT?=
 =?us-ascii?Q?9Ow173I7AJjyVasnxH+iFskG4P8tB3pq95oR6sPqlneUtsG9oD8NRDtEr9u7?=
 =?us-ascii?Q?i7rOJz/PD5sLtYwg7ENHKRltggpuecoPBXlAm36bMiUOKeRKaWgfgi8y1uaa?=
 =?us-ascii?Q?uFFJEyD5ze30LYm6QrRpKLTAPvkwEksFkBn57sxHwpcmCRO/2qyE6IjSSXUX?=
 =?us-ascii?Q?s/LlOc+qNjVVLBqYRrAP7c/Sn/HmGn3ASPcjwfO7BszX9S3EZWFe5F1hYuoP?=
 =?us-ascii?Q?67P9xFZdKu8WeJ+Sf+v9/HGedKY/ht+kPr/kLWRoKT+JUXBliUWkKvO6RuJb?=
 =?us-ascii?Q?uoBmKpPeZObqtHbj7620lel5/LKkmseGbyHnNfZKonVtbeaPpHo0JSNzxq1N?=
 =?us-ascii?Q?UHYCAngqlti4EukweWBQiOBGG40X9xrJ8PnqKjyuH7r62bWws0a0wkry+MRH?=
 =?us-ascii?Q?aX+ItnL0Rkkj3laNj2G9xx1gnfis9abdwb03vzFI5iQPShhE+PUtG68Y9I38?=
 =?us-ascii?Q?DKVwFDofLbEsZJ5N14dOeaCoPuDcFfqBnXbtvZ3lGChyPOjxtwZOX8RA1wJj?=
 =?us-ascii?Q?spBbPnbrT9pHMcuyeH3OhvwVRx6GOMnGfjLI8JXuh00MC1n2LSaXnP932Ll/?=
 =?us-ascii?Q?7cVQXWM6h18Ce6Xmilb+PVNXIJVbK/6un3UDYpnmhgI8lYeFvvMwq7bh7x82?=
 =?us-ascii?Q?/uN9zhWiL0Cc70uEHQLk64xU+BQC4fJWoY1Nk3f8X8VJof40fyvHV4b43ZHR?=
 =?us-ascii?Q?4cZaePfXaau5IIz+Mxjm2NDkMex00ZNDPYrnLXS2q/aDYrv7xXLc6UM7lNmU?=
 =?us-ascii?Q?eTJ63120+5N67Qf1WBwtjJ7rhqPgV4ckxXNC04n/GwKJWSxUtGBPJ1h86IwX?=
 =?us-ascii?Q?ZoatyiE7nMg5Lr5jkavCH6rQ5+HlVg6pasgRIgzd83OTTK5tC3YLbAhxfi9s?=
 =?us-ascii?Q?uQohbnoRAZtPdvw5WEHQPQR2Z7ttNhBZZWtrhzA0e4BtlUxbieuDiub6XbXG?=
 =?us-ascii?Q?b1yteSadVyZNbfc8a7FqxBo2Sc8b8FBMck0zPwGedCltgzRW/wzQohSGwysh?=
 =?us-ascii?Q?8Te+nSPQVGGx6qc5h6fT+YeWr31YQ/TqljeqXuZrs6UoNAslW7iiqk/Ialb8?=
 =?us-ascii?Q?eop1rKDhQnwe8q5r1JWvzFq/OylPwV+fKt93Dl6f/LfVkT2rOHVdYzbp1hb4?=
 =?us-ascii?Q?7Idc8x7mLkfnZ64YSX9dGkvs6+W/CIX2AD6H8ir1QxLqqdrV8fIstSr0vnL0?=
 =?us-ascii?Q?wyQ/EOQqTRyaHjHsXHe/fwf76v6EEcNn9Pq11ahSOyiXfNBffazlQ+ve3Eh4?=
 =?us-ascii?Q?GDGJy7OfHAOREUxdFgvx1GdpkijEeQcflZ91/SK5AucsEo4qinsaeaztJpHs?=
 =?us-ascii?Q?1oRdvEdlX1Ac+PLEFurabkxU/Y56HqCt+P7dkL1BguDLS1RaRsGI8k1ligiJ?=
 =?us-ascii?Q?OPsuDspPAlJuYSl3ucPoa7K1X4lRHn8b+F+hwwpNRDy9B3L61Rze+fNs51uE?=
 =?us-ascii?Q?UzfaCvTgjLunWqSr9vAIEBD+f+pJLNYNEEvJ4i06Pz+IIk2ia8VufAjoLgck?=
 =?us-ascii?Q?lZTx3K2dgeflwAAf/0Sb5bAlNSfNbCKvdMBD74xg?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 950399c2-81ed-47cc-e0e5-08dc494334d9
X-MS-Exchange-CrossTenant-AuthSource: CH3PR11MB8660.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2024 01:06:58.2760
 (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: Z32i2lvNJXI1wtI4jRsK0rhO3CgT/uDUiG+zwXrEkF+QkXRkTgr2cBndwrbVHGmyT5+pNbV39J18DLBcgkC+bQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB8245
X-OriginatorOrg: intel.com

>diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
>index 437c6d5e802e..d69dd474775b 100644
>--- a/arch/x86/kvm/vmx/main.c
>+++ b/arch/x86/kvm/vmx/main.c
>@@ -110,6 +110,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
> 	.check_processor_compatibility = vmx_check_processor_compat,
> 
> 	.hardware_unsetup = vt_hardware_unsetup,
>+	.offline_cpu = tdx_offline_cpu,
> 
> 	/* TDX cpu enablement is done by tdx_hardware_setup(). */
> 	.hardware_enable = vmx_hardware_enable,
>diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>index b11f105db3cd..f2ee5abac14e 100644
>--- a/arch/x86/kvm/vmx/tdx.c
>+++ b/arch/x86/kvm/vmx/tdx.c
>@@ -97,6 +97,7 @@ int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>  */
> static DEFINE_MUTEX(tdx_lock);
> static struct mutex *tdx_mng_key_config_lock;
>+static atomic_t nr_configured_hkid;
> 
> static __always_inline hpa_t set_hkid_to_hpa(hpa_t pa, u16 hkid)
> {
>@@ -112,6 +113,7 @@ static inline void tdx_hkid_free(struct kvm_tdx *kvm_tdx)
> {
> 	tdx_guest_keyid_free(kvm_tdx->hkid);
> 	kvm_tdx->hkid = -1;
>+	atomic_dec(&nr_configured_hkid);

I may think it is better to extend IDA infrastructure e.g., add an API to check if
any ID is allocated for a given range. No strong opinion on this.

> }
> 
> static inline bool is_hkid_assigned(struct kvm_tdx *kvm_tdx)
>@@ -586,6 +588,7 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> 	if (ret < 0)
> 		return ret;
> 	kvm_tdx->hkid = ret;
>+	atomic_inc(&nr_configured_hkid);
> 
> 	va = __get_free_page(GFP_KERNEL_ACCOUNT);
> 	if (!va)
>@@ -1071,3 +1074,41 @@ void tdx_hardware_unsetup(void)
> 	kfree(tdx_info);
> 	kfree(tdx_mng_key_config_lock);
> }
>+
>+int tdx_offline_cpu(void)
>+{
>+	int curr_cpu = smp_processor_id();
>+	cpumask_var_t packages;
>+	int ret = 0;
>+	int i;
>+
>+	/* No TD is running.  Allow any cpu to be offline. */
>+	if (!atomic_read(&nr_configured_hkid))
>+		return 0;
>+
>+	/*
>+	 * In order to reclaim TDX HKID, (i.e. when deleting guest TD), need to
>+	 * call TDH.PHYMEM.PAGE.WBINVD on all packages to program all memory
>+	 * controller with pconfig.  If we have active TDX HKID, refuse to
>+	 * offline the last online cpu.
>+	 */
>+	if (!zalloc_cpumask_var(&packages, GFP_KERNEL))
>+		return -ENOMEM;
>+	for_each_online_cpu(i) {
>+		if (i != curr_cpu)
>+			cpumask_set_cpu(topology_physical_package_id(i), packages);
>+	}

Just check if any other CPU is in the same package of the one about to go
offline. This would obviate the need for the cpumask and allow us to break once
one cpu in the same package is found.

>+	/* Check if this cpu is the last online cpu of this package. */
>+	if (!cpumask_test_cpu(topology_physical_package_id(curr_cpu), packages))
>+		ret = -EBUSY;
>+	free_cpumask_var(packages);
>+	if (ret)
>+		/*
>+		 * Because it's hard for human operator to understand the
>+		 * reason, warn it.
>+		 */
>+#define MSG_ALLPKG_ONLINE \
>+	"TDX requires all packages to have an online CPU. Delete all TDs in order to offline all CPUs of a package.\n"
>+		pr_warn_ratelimited(MSG_ALLPKG_ONLINE);
>+	return ret;
>+}