linux-kernel - Re: [RFC PATCH 3/4] KVM: x86/tdx: Do VMXON and TDX-Module initialization during tdx

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]
Message-ID: <68ee9bcf14fea_2f899100d2@dwillia2-mobl4.notmuch>
Date: Tue, 14 Oct 2025 11:51:59 -0700
From: <dan.j.williams@...el.com>
To: Chao Gao <chao.gao@...el.com>, Sean Christopherson <seanjc@...gle.com>
CC: Rick P Edgecombe <rick.p.edgecombe@...el.com>, "x86@...nel.org"
	<x86@...nel.org>, "kas@...nel.org" <kas@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"mingo@...hat.com" <mingo@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "bp@...en8.de" <bp@...en8.de>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, Kai Huang <kai.huang@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Dan J Williams
	<dan.j.williams@...el.com>, Adrian Hunter <adrian.hunter@...el.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "xin@...or.com" <xin@...or.com>
Subject: Re: [RFC PATCH 3/4] KVM: x86/tdx: Do VMXON and TDX-Module
 initialization during tdx_init()

Chao Gao wrote:
> On Mon, Oct 13, 2025 at 01:59:21PM -0700, Sean Christopherson wrote:
> >On Mon, Oct 13, 2025, Rick P Edgecombe wrote:
> >> On Fri, 2025-10-10 at 15:04 -0700, Sean Christopherson wrote:
> >> > @@ -3524,34 +3453,31 @@ static int __init __tdx_bringup(void)
> >> >  	if (td_conf->max_vcpus_per_td < num_present_cpus()) {
> >> >  		pr_err("Disable TDX: MAX_VCPU_PER_TD (%u) smaller than number of logical CPUs (%u).\n",
> >> >  				td_conf->max_vcpus_per_td, num_present_cpus());
> >> > -		goto get_sysinfo_err;
> >> > +		return -EINVAL;
> >> >  	}
> >> >  
> >> >  	if (misc_cg_set_capacity(MISC_CG_RES_TDX, tdx_get_nr_guest_keyids()))
> >> > -		goto get_sysinfo_err;
> >> > +		return -EINVAL;
> >> >  
> >> >  	/*
> >> > -	 * Leave hardware virtualization enabled after TDX is enabled
> >> > -	 * successfully.  TDX CPU hotplug depends on this.
> >> > +	 * TDX-specific cpuhp callback to disallow offlining the last CPU in a
> >> > +	 * packing while KVM is running one or more TDs.  Reclaiming HKIDs
> >> > +	 * requires doing PAGE.WBINVD on every package, i.e. offlining all CPUs
> >> > +	 * of a package would prevent reclaiming the HKID.
> >> >  	 */
> >> > +	r = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "kvm/cpu/tdx:online",
> >> > +			      tdx_online_cpu, tdx_offline_cpu);
> >> 
> >> Could pass NULL instead of tdx_online_cpu() and delete this version of
> >> tdx_online_cpu().
> >
> >Oh, nice, I didn't realize (or forgot) the startup call is optional.
> > 
> >> Also could remove the error handling too.
> >
> >No.  Partly on prinicple, but also because CPUHP_AP_ONLINE_DYN can fail if the
> >kernel runs out of dynamic entries (currently limited to 40).  The kernel WARNs
> >if it runs out of entries, but KVM should still do the right thing.
> >
> >> Also, can we name the two tdx_offline_cpu()'s differently? This one is all about
> >> keyid's being in use. tdx_hkid_offline_cpu()?
> >
> >Ya.  And change the description to "kvm/cpu/tdx:hkid_packages"?  Or something
> >like that.
> >
> 
> Is it a good idea to consolidate the two tdx_offline_cpu() functions, i.e.,
> integrate KVM's version into x86 core?

This looks good to me, some additional cleanup opportunities below:

> From 97165f9933f48d588f5390e2d543d9880c03532d Mon Sep 17 00:00:00 2001
> From: Chao Gao <chao.gao@...el.com>
> Date: Tue, 14 Oct 2025 01:00:06 -0700
> Subject: [PATCH] x86/virt/tdx: Consolidate TDX CPU hotplug handling
> 
> The core kernel registers a CPU hotplug callback to do VMX and TDX init
> and deinit while KVM registers a separate CPU offline callback to block
> offlining the last online CPU in a socket.
> 
> Splitting TDX-related CPU hotplug handling across two components is odd
> and adds unnecessary complexity.
> 
> Consolidate TDX-related CPU hotplug handling by integrating KVM's
> tdx_offline_cpu() to the one in the core kernel.
> 
> Also move nr_configured_hkid to the core kernel because tdx_offline_cpu()
> references it. Since HKID allocation and free are handled in the core
> kernel, it's more natural to track used HKIDs there.
> 
> Signed-off-by: Chao Gao <chao.gao@...el.com>
> ---
>  arch/x86/kvm/vmx/tdx.c      | 67 +------------------------------------
>  arch/x86/virt/vmx/tdx/tdx.c | 49 +++++++++++++++++++++++++--
>  2 files changed, 47 insertions(+), 69 deletions(-)
> 
[..]
> +	 */
> +#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);

Why the define?

> +	return -EBUSY;
> +
> +done:
> 	x86_virt_put_cpu(X86_FEATURE_VMX);
> 	return 0;
>  }
> @@ -1505,15 +1541,22 @@ EXPORT_SYMBOL_GPL(tdx_get_nr_guest_keyids);
>  
>  int tdx_guest_keyid_alloc(void)
>  {
> -	return ida_alloc_range(&tdx_guest_keyid_pool, tdx_guest_keyid_start,
> -			       tdx_guest_keyid_start + tdx_nr_guest_keyids - 1,
> -			       GFP_KERNEL);
> +	int ret;
> +
> +	ret = ida_alloc_range(&tdx_guest_keyid_pool, tdx_guest_keyid_start,
> +			      tdx_guest_keyid_start + tdx_nr_guest_keyids - 1,
> +			      GFP_KERNEL);
> +	if (ret >= 0)
> +		atomic_inc(&nr_configured_hkid);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(tdx_guest_keyid_alloc);
>  
>  void tdx_guest_keyid_free(unsigned int keyid)
>  {
> 	ida_free(&tdx_guest_keyid_pool, keyid);
> +	atomic_dec(&nr_configured_hkid);

So, ida has an ida_is_empty() helper. I believe you can just use that
in the offline helper and delete @nr_configured_hkid.