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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Jan 2023 10:23:16 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Yamahata, Isaku" <isaku.yamahata@...el.com>
CC:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Shahar, Sagi" <sagis@...gle.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
        "dmatlack@...gle.com" <dmatlack@...gle.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v11 021/113] KVM: TDX: Refuse to unplug the last cpu on
 the package

On Thu, 2023-01-12 at 08:31 -0800, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> 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@...el.com>
> ---
> 

[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]