Re: [PATCH V2 1/1] KVM: TDX: Add sub-ioctl KVM_TDX_TERMINATE_VM

[Adrian Hunter <adrian.hunter@intel.com>]

On 19/04/25 04:12, Sean Christopherson wrote:
> On Thu, Apr 17, 2025, Adrian Hunter wrote:
>> From: Sean Christopherson <seanjc@google.com>
>>
>> Add sub-ioctl KVM_TDX_TERMINATE_VM to release the HKID prior to shutdown,
>> which enables more efficient reclaim of private memory.
>>
>> Private memory is removed from MMU/TDP when guest_memfds are closed. If
>> the HKID has not been released, the TDX VM is still in RUNNABLE state,
>> so pages must be removed using "Dynamic Page Removal" procedure (refer
>> TDX Module Base spec) which involves a number of steps:
>> 	Block further address translation
>> 	Exit each VCPU
>> 	Clear Secure EPT entry
>> 	Flush/write-back/invalidate relevant caches
>>
>> However, when the HKID is released, the TDX VM moves to TD_TEARDOWN state
>> where all TDX VM pages are effectively unmapped, so pages can be reclaimed
>> directly.
>>
>> Reclaiming TD Pages in TD_TEARDOWN State was seen to decrease the total
>> reclaim time.  For example:
>>
>> 	VCPUs	Size (GB)	Before (secs)	After (secs)
>> 	 4	 18		  72		 24
>> 	32	107		 517		134
>> 	64	400		5539		467
>>
>> [Adrian: wrote commit message, added KVM_TDX_TERMINATE_VM documentation,
>>  and moved cpus_read_lock() inside kvm->lock for consistency as reported
>>  by lockdep]
> 
> /facepalm
> 
> I over-thought this.  We've had an long-standing battle with kvm_lock vs.
> cpus_read_lock(), but this is kvm->lock, not kvm_lock.  /sigh
> 
>> +static int tdx_terminate_vm(struct kvm *kvm)
>> +{
>> +	int r = 0;
>> +
>> +	guard(mutex)(&kvm->lock);
> 
> With kvm->lock taken outside cpus_read_lock(), just handle KVM_TDX_TERMINATE_VM
> in the switch statement, i.e. let tdx_vm_ioctl() deal with kvm->lock.

Ok, also cpus_read_lock() can go back where it was in __tdx_release_hkid().

But also in __tdx_release_hkid(), there is

	if (KVM_BUG_ON(refcount_read(&kvm->users_count) && !terminate, kvm))
		return;

However, __tdx_td_init() calls tdx_mmu_release_hkid() on the
error path so that is not correct.

> 
>> +	cpus_read_lock();
>> +
>> +	if (!kvm_trylock_all_vcpus(kvm)) {
>> +		r = -EBUSY;
>> +		goto out;
>> +	}
>> +
>> +	kvm_vm_dead(kvm);
>> +	kvm_unlock_all_vcpus(kvm);
>> +
>> +	__tdx_release_hkid(kvm, true);
>> +out:
>> +	cpus_read_unlock();
>> +	return r;
>> +}
>> +
>>  int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>>  {
>>  	struct kvm_tdx_cmd tdx_cmd;
>> @@ -2805,6 +2827,9 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>>  	if (tdx_cmd.hw_error)
>>  		return -EINVAL;
>>  
>> +	if (tdx_cmd.id == KVM_TDX_TERMINATE_VM)
>> +		return tdx_terminate_vm(kvm);
>> +
>>  	mutex_lock(&kvm->lock);
>>  
>>  	switch (tdx_cmd.id) {
>> -- 
>> 2.43.0
>>