Re: [PATCH v2 3/3] KVM: VMX: Initialize TDX during KVM module load

[Huang, Kai]

On 18/11/2024 2:22 pm, Chao Gao wrote:
> > +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);
>
> The comment above tdx_cpu_enable() is outdated because now it may be called
> from CPU hotplug rather than IPI function calls only.
>
> Can we relax the assertion lockdep_assert_irqs_disabled() in tdx_cpu_enable()?
> looks the requirement is just the enabling work won't be migrated and done to
> another CPU.

We can but I don't want to do it now.  We will need to revist both 
tdx_cpu_enable() and tdx_enable() when we move VMXON out of KVM anyway. 
I would like to focus on bringing KVM TDX support first and then to 
revisit them together at that timeframe.

> > +
> > +	return r;
> > +}
> > +
> > +static void __do_tdx_cleanup(void)
> > +{
> > +	/*
> > +	 * Once TDX module is initialized, it cannot be disabled and
> > +	 * re-initialized again w/o runtime update (which isn't
> > +	 * supported by kernel).  Only need to remove the cpuhp here.
> > +	 * The TDX host core code tracks TDX status and can handle
> > +	 * 'multiple enabling' scenario.
> > +	 */
> > +	WARN_ON_ONCE(!tdx_cpuhp_state);
> > +	cpuhp_remove_state_nocalls(tdx_cpuhp_state);
>
> ...
>
> > +	tdx_cpuhp_state = 0;
> > +}
> > +
> > +static int __init __do_tdx_bringup(void)
> > +{
> > +	int r;
> > +
> > +	/*
> > +	 * TDX-specific cpuhp callback to call tdx_cpu_enable() on all
> > +	 * online CPUs before calling tdx_enable(), and on any new
> > +	 * going-online CPU to make sure it is ready for TDX guest.
> > +	 */
> > +	r = cpuhp_setup_state_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > +					 "kvm/cpu/tdx:online",
> > +					 tdx_online_cpu, NULL);
> > +	if (r < 0)
> > +		return r;
> > +
> > +	tdx_cpuhp_state = r;
> > +
> > +	r = tdx_enable();
> > +	if (r)
> > +		__do_tdx_cleanup();
>
> this calls cpuhp_remove_state_nocalls(), which acquires cpu locks again,
> causing a potential deadlock IIUC.

Dam.. I'll fix. Thanks for catching.

> > +
> > +	return r;
> > +}
> > +
> > +static bool __init kvm_can_support_tdx(void)
>
> I think "static __init bool" is the preferred order. see
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html#function-prototypes

I think you are right, but IIUC we'd better to change all the existing 
'static <ret_type> __init' to 'static __init <ret_type>' in KVM code. 
I'd rather to keep the current way to make them aligned and we can 
change them at once if needed in the future.

> > +{
> > +	return cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM);
> > +}
> > +
> > +static int __init __tdx_bringup(void)
> > +{
> > +	int r;
> > +
> > +	/*
> > +	 * Enabling TDX requires enabling hardware virtualization first,
> > +	 * as making SEAMCALLs requires CPU being in post-VMXON state.
> > +	 */
> > +	r = kvm_enable_virtualization();
> > +	if (r)
> > +		return r;
> > +
> > +	cpus_read_lock();
> > +	r = __do_tdx_bringup();
> > +	cpus_read_unlock();
> > +
> > +	if (r)
> > +		goto tdx_bringup_err;
> > +
> > +	/*
> > +	 * Leave hardware virtualization enabled after TDX is enabled
> > +	 * successfully.  TDX CPU hotplug depends on this.
> > +	 */
>
> Shouldn't we make enable_tdx dependent on enable_virt_at_load? Otherwise, if
> someone sets enable_tdx=1 and enable_virt_at_load=0, they will get hardware
> virtualization enabled at load time while enable_virt_at_load still shows 0.
> This behavior is a bit confusing to me.
>
> I think a check against enable_virt_at_load in kvm_can_support_tdx() will work.
>
> The call of kvm_enable_virtualization() here effectively moves
> kvm_init_virtualization() out of kvm_init() when enable_tdx=1. I wonder if it
> makes more sense to refactor out kvm_init_virtualization() for non-TDX cases
> as well, i.e.,
>
>    vmx_init();
>    kvm_init_virtualization();
>    tdx_init();
>    kvm_init();
>
> I'm not sure if this was ever discussed. To me, this approach is better because
> TDX code needn't handle virtualization enabling stuff. It can simply check that
> enable_virt_at_load=1, assume virtualization is enabled and needn't disable
> virtualization on errors.

I think this was briefly discussed here:

https://lore.kernel.org/all/ZrrFgBmoywk7eZYC@xxxxxxxxxx/

The disadvantage of splitting out kvm_init_virtualization() is all other 
ARCHs (all non-TDX cases actually) will need to explicitly call 
kvm_init_virtualization() separately.

> A bonus is that on non-TDX-capable systems, hardware virtualization won't be
> toggled twice at KVM load time for no good reason.

I am fine with either way.

Sean, do you have any comments?