Re: [PATCH 01/20] KVM: Treat the device list as an rculist

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 27, 2024, Oliver Upton wrote:
> A subsequent change to KVM/arm64 will necessitate walking the device
> list outside of the kvm->lock. Prepare by converting to an rculist.
> Note that this has zero effect the destruction path, as every reader
> should be protected by a valid reference on the KVM struct.

This should leave the destruction path alone then.  Simply using list_del_rcu()
doesn't convert this to an rculist, you need to actually synchronize against RCU
for this to have any meaning/protection.  The above alludes to this, and the
comment in kvm_destroy_devices() helps a little, but the code itself is actively
misleading.

I don't know how I feel about using list_add_rcu() in combination with list_del(),
but I like it a lot better than using list_del_rcu() in a way that is blatantly
wrong.

So if we don't have a better option, I would much rather do only the list_add_rcu(),
and add a comment _there_ explaining why KVM inserts with RCU protection, but
frees using regular ol' list_del().

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  virt/kvm/kvm_main.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index c681149c382a..a1423f8064b0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1326,10 +1326,12 @@ static void kvm_destroy_devices(struct kvm *kvm)
>  	/*
>  	 * We do not need to take the kvm->lock here, because nobody else
>  	 * has a reference to the struct kvm at this point and therefore
> -	 * cannot access the devices list anyhow.
> +	 * cannot access the devices list anyhow. Furthermore, there are no RCU
> +	 * readers after the last reference is dropped, so there is no need to
> +	 * defer destruction until a grace period has elapsed.
>  	 */
>  	list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) {
> -		list_del(&dev->vm_node);
> +		list_del_rcu(&dev->vm_node);
>  		dev->ops->destroy(dev);
>  	}
>  }
> @@ -4781,7 +4783,7 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
>  		kfree(dev);
>  		return ret;
>  	}
> -	list_add(&dev->vm_node, &kvm->devices);
> +	list_add_rcu(&dev->vm_node, &kvm->devices);
>  	mutex_unlock(&kvm->lock);
>  
>  	if (ops->init)
> -- 
> 2.44.0.rc1.240.g4c46232300-goog
>