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

[<dan.j.williams@intel.com>]

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@intel.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@intel.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.