Re: [PATCH v5 07/10] KVM: arm/arm64: vgic-its: New helper functions to free the caches

[Christoffer Dall]

On Mon, Oct 23, 2017 at 04:08:26PM +0200, Eric Auger wrote:
> From: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
> 
> We create two new functions that free the device and
> collection lists. They are currently called by vgic_its_destroy()
> and other callers will be added in subsequent patches.
> 
> We also remove the check on its->device_list.next.
> Lists are initialized in vgic_create_its() and the device
> is added to the device list only if this latter succeeds.
> 
> vgic_its_destroy is the device destroy ops. This latter is called
> by kvm_destroy_devices() which loops on all created devices. So
> at this point the list is initialized.
> 
> Signed-off-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> 

Acked-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>

> ---
> 
> v4 -> v5:
> - add mutex_lock in vgic_its_free_collection_list
> - use list_for_each_entry_safe
> - reword commit message
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 51 ++++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 2f3c3a1..8098f91 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -910,6 +910,30 @@ static void vgic_its_free_device(struct kvm *kvm, struct its_device *device)
>  	kfree(device);
>  }
>  
> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct its_device *cur, *temp;
> +
> +	mutex_lock(&its->its_lock);
> +
> +	list_for_each_entry_safe(cur, temp, &its->device_list, dev_list)
> +		vgic_its_free_device(kvm, cur);
> +
> +	mutex_unlock(&its->its_lock);
> +}
> +
> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its *its)
> +{
> +	struct its_collection *cur, *temp;
> +
> +	mutex_lock(&its->its_lock);
> +
> +	list_for_each_entry_safe(cur, temp, &its->collection_list, coll_list)
> +		vgic_its_free_collection(its, cur->collection_id);
> +
> +	mutex_unlock(&its->its_lock);
> +}
> +
>  /* Must be called with its_lock mutex held */
>  static struct its_device *vgic_its_alloc_device(struct vgic_its *its,
>  						u32 device_id, gpa_t itt_addr,
> @@ -1627,32 +1651,9 @@ static void vgic_its_destroy(struct kvm_device *kvm_dev)
>  {
>  	struct kvm *kvm = kvm_dev->kvm;
>  	struct vgic_its *its = kvm_dev->private;
> -	struct list_head *cur, *temp;
> -
> -	/*
> -	 * We may end up here without the lists ever having been initialized.
> -	 * Check this and bail out early to avoid dereferencing a NULL pointer.
> -	 */
> -	if (!its->device_list.next)
> -		return;
> -
> -	mutex_lock(&its->its_lock);
> -	list_for_each_safe(cur, temp, &its->device_list) {
> -		struct its_device *dev;
> -
> -		dev = list_entry(cur, struct its_device, dev_list);
> -		vgic_its_free_device(kvm, dev);
> -	}
> -
> -	list_for_each_safe(cur, temp, &its->collection_list) {
> -		struct its_collection *coll;
> -
> -		coll = list_entry(cur, struct its_collection, coll_list);
> -		list_del(cur);
> -		kfree(coll);
> -	}
> -	mutex_unlock(&its->its_lock);
>  
> +	vgic_its_free_device_list(kvm, its);
> +	vgic_its_free_collection_list(kvm, its);
>  	kfree(its);
>  }
>  
> -- 
> 2.5.5
>