Re: [PATCH v11 021/113] KVM: TDX: Refuse to unplug the last cpu on the package

[Huang, Kai]

On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@xxxxxxxxx wrote:
> From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> 
> In order to reclaim TDX HKID, (i.e. when deleting guest TD), needs to call
> TDH.PHYMEM.PAGE.WBINVD on all packages.  If we have used TDX HKID, refuse
> to offline the last online cpu. Add arch callback for cpu offline.

I think it is worth to talk about suspend staff, i.e. why we only refuse to
offline the last cpu when there's active TD, but not choose to offline the last
cpu when TDX is enabled in KVM.  People may not be able to understand
immediately the reason behind this design.

Btw, I certainly don't want to speak for Sean, but it seems this was suggested
by Sean?  If so, add a 'Suggested-by' tag?

> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> ---
> 

[snip]

> +
> +int tdx_offline_cpu(void)
> +{
> +	int curr_cpu = smp_processor_id();
> +	cpumask_var_t packages;
> +	int ret = 0;
> +	int i;
> +
> +	if (!atomic_read(&nr_configured_hkid))
> +		return 0;

As mentioned above, I think it also worth to add some comment here.  When people
are trying to understand some code, I think mostly they are just going to look
at the code itself, but won't use 'git blame' to dig out the entire changelog to
understand some code.

> +
> +	/*
> +	 * To reclaim hkid, need to call TDH.PHYMEM.PAGE.WBINVD on all packages.
> +	 * If this is the last online cpu on the package, refuse offline.
> +	 */
> +	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);
> +	}
> +	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.
> +		 */
> +		pr_warn("TDX requires all packages to have an online CPU.  "
> +			"Delete all TDs in order to offline all CPUs of a package.\n");
> +	return ret;
> +}
> 

[snip]