Re: [PATCH v4 08/11] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Christoffer Dall]

On Tue, Oct 17, 2017 at 09:10:06AM +0200, Eric Auger wrote:
> From: wanghaibin 
> 
> We create 2 new functions that frees the device and
> collection lists. this is currently called by vgic_its_destroy()
> and we will add other callers in subsequent patches.

See my previous comments about language issues in this paragraph.

> 
> We also remove the check on its->device_list.next as it looks
> unnecessary. Indeed, the device list always is initialized
> when vgic_its_destroy gets called: the kvm device is removed
> by kvm_destroy_devices() which loops on all the devices
> added to kvm->devices. kvm_ioctl_create_device() only adds
> the device to kvm_devices once the lists have been initialized
> (in vgic_create_its).
> 
> We also move vgic_its_free_device to prepare for new callers.
> 
> Signed-off-by: wanghaibin 
> Signed-off-by: Eric Auger 
> 
> ---
> [Eric] removed its->device_list.next which is not needed as
> pointed out by Wanghaibin. Reword the commit message
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 76 
> 
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 1c3e83f..f3f0026f 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite 
> *ite)
>   kfree(ite);
>  }
>  
> +static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> +{
> + struct its_ite *ite, *tmp;
> +
> + list_for_each_entry_safe(ite, tmp, >itt_head, ite_list)
> + its_free_ite(kvm, ite);
> + list_del(>dev_list);
> + kfree(dev);
> +}
> +
> +static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
> +{
> + struct list_head *cur, *temp;
> +
> + mutex_lock(>its_lock);
> + list_for_each_safe(cur, temp, >device_list) {
> + struct its_device *dev;
> +
> + dev = list_entry(cur, struct its_device, dev_list);
> + vgic_its_free_device(kvm, dev);
> + }
> + mutex_unlock(>its_lock);
> +}
> +
> +static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its 
> *its)
> +{
> + struct list_head *cur, *temp;
> +
> + list_for_each_safe(cur, temp, >collection_list) {
> + struct its_collection *coll;
> +
> + coll = list_entry(cur, struct its_collection, coll_list);
> + list_del(cur);
> + kfree(coll);
> + }
> + mutex_unlock(>its_lock);
> +}
> +
> +
>  static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
>  {
>   return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
> @@ -1644,46 +1683,13 @@ static int vgic_its_create(struct kvm_device *dev, 
> u32 type)
>   return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
>  }
>  
> -static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
> -{
> - struct its_ite *ite, *tmp;
> -
> - list_for_each_entry_safe(ite, tmp, >itt_head, ite_list)
> - its_free_ite(kvm, ite);
> - list_del(>dev_list);
> - kfree(dev);
> -}
> -
>  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;

Hmm, I feel like we managed to convince ourselves this was needed
before.

Andre, can you remember what your original rationale was here?

> -
> - mutex_lock(>its_lock);
> - list_for_each_safe(cur, temp, >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, >collection_list) {
> - struct its_collection *coll;
> -
> - coll = list_entry(cur, struct its_collection, coll_list);
> - list_del(cur);
> - kfree(coll);
> - }
> - mutex_unlock(>its_lock);
>  
> + vgic_its_free_device_list(kvm, its);
> + vgic_its_free_collection_list(kvm, its);
>   kfree(its);
>  }
>  
> -- 
> 2.5.5
> 

If we're really sure the original check was just a misunderstanding,
then this patch looks ok, given the fixes to the commit message.

Thanks,
-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 08/11] KVM: arm/arm64: vgic-its: new helper functions to free the caches

[Eric Auger]

From: wanghaibin 

We create 2 new functions that frees the device and
collection lists. this is currently called by vgic_its_destroy()
and we will add other callers in subsequent patches.

We also remove the check on its->device_list.next as it looks
unnecessary. Indeed, the device list always is initialized
when vgic_its_destroy gets called: the kvm device is removed
by kvm_destroy_devices() which loops on all the devices
added to kvm->devices. kvm_ioctl_create_device() only adds
the device to kvm_devices once the lists have been initialized
(in vgic_create_its).

We also move vgic_its_free_device to prepare for new callers.

Signed-off-by: wanghaibin 
Signed-off-by: Eric Auger 

---
[Eric] removed its->device_list.next which is not needed as
pointed out by Wanghaibin. Reword the commit message
---
 virt/kvm/arm/vgic/vgic-its.c | 76 
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 1c3e83f..f3f0026f 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -611,6 +611,45 @@ static void its_free_ite(struct kvm *kvm, struct its_ite 
*ite)
kfree(ite);
 }
 
+static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
+{
+   struct its_ite *ite, *tmp;
+
+   list_for_each_entry_safe(ite, tmp, >itt_head, ite_list)
+   its_free_ite(kvm, ite);
+   list_del(>dev_list);
+   kfree(dev);
+}
+
+static void vgic_its_free_device_list(struct kvm *kvm, struct vgic_its *its)
+{
+   struct list_head *cur, *temp;
+
+   mutex_lock(>its_lock);
+   list_for_each_safe(cur, temp, >device_list) {
+   struct its_device *dev;
+
+   dev = list_entry(cur, struct its_device, dev_list);
+   vgic_its_free_device(kvm, dev);
+   }
+   mutex_unlock(>its_lock);
+}
+
+static void vgic_its_free_collection_list(struct kvm *kvm, struct vgic_its 
*its)
+{
+   struct list_head *cur, *temp;
+
+   list_for_each_safe(cur, temp, >collection_list) {
+   struct its_collection *coll;
+
+   coll = list_entry(cur, struct its_collection, coll_list);
+   list_del(cur);
+   kfree(coll);
+   }
+   mutex_unlock(>its_lock);
+}
+
+
 static u64 its_cmd_mask_field(u64 *its_cmd, int word, int shift, int size)
 {
return (le64_to_cpu(its_cmd[word]) >> shift) & (BIT_ULL(size) - 1);
@@ -1644,46 +1683,13 @@ static int vgic_its_create(struct kvm_device *dev, u32 
type)
return vgic_its_set_abi(its, NR_ITS_ABIS - 1);
 }
 
-static void vgic_its_free_device(struct kvm *kvm, struct its_device *dev)
-{
-   struct its_ite *ite, *tmp;
-
-   list_for_each_entry_safe(ite, tmp, >itt_head, ite_list)
-   its_free_ite(kvm, ite);
-   list_del(>dev_list);
-   kfree(dev);
-}
-
 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_lock);
-   list_for_each_safe(cur, temp, >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, >collection_list) {
-   struct its_collection *coll;
-
-   coll = list_entry(cur, struct its_collection, coll_list);
-   list_del(cur);
-   kfree(coll);
-   }
-   mutex_unlock(>its_lock);
 
+   vgic_its_free_device_list(kvm, its);
+   vgic_its_free_collection_list(kvm, its);
kfree(its);
 }
 
-- 
2.5.5

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm