Re: [PATCH v2 3/7] KVM: x86/tdx: Do VMXON and TDX-Module initialization during subsys init

[dan.j.williams]

Sean Christopherson wrote:
> Now that VMXON can be done without bouncing through KVM, do TDX-Module
> initialization during subsys init (specifically before module_init() so
> that it runs before KVM when both are built-in).  Aside from the obvious
> benefits of separating core TDX code from KVM, this will allow tagging a
> pile of TDX functions and globals as being __init and __ro_after_init.
> 
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  Documentation/arch/x86/tdx.rst |  26 -----
>  arch/x86/include/asm/tdx.h     |   4 -
>  arch/x86/kvm/vmx/tdx.c         | 169 ++++++--------------------------
>  arch/x86/virt/vmx/tdx/tdx.c    | 170 ++++++++++++++++++---------------
>  arch/x86/virt/vmx/tdx/tdx.h    |   8 --
>  5 files changed, 124 insertions(+), 253 deletions(-)

yes!

> 
> @@ -3304,17 +3304,7 @@ int tdx_gmem_max_mapping_level(struct kvm *kvm, kvm_pfn_t pfn, bool is_private)
>  
>  static int tdx_online_cpu(unsigned int cpu)
>  {
> -	unsigned long flags;
> -	int r;
> -
> -	/* Sanity check CPU is already in post-VMXON */
> -	WARN_ON_ONCE(!(cr4_read_shadow() & X86_CR4_VMXE));
> -
> -	local_irq_save(flags);
> -	r = tdx_cpu_enable();
> -	local_irq_restore(flags);
> -
> -	return r;
> +	return 0;
>  }

Given this routine now has nothing to do...

> +	 * 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);

...the @startup param can be NULL. That also saves some grep pain no
more multiple implementations of a "tdx_online_cpu".

Along those lines, should tdx_offline_cpu() become
kvm_tdx_offline_cpu()?

[..]
>  /*
>   * Add a memory region as a TDX memory block.  The caller must make sure
> @@ -1156,67 +1194,50 @@ static int init_tdx_module(void)
>  	goto out_put_tdxmem;
>  }
>  
> -static int __tdx_enable(void)
> +static int tdx_enable(void)

Almost commented about this being able to be __init now, but then I see
you have a combo patch for that later.

With or without the additional tdx_{on,off}line_cpu fixups:

Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>