From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758198AbdJMNfP (ORCPT ); Fri, 13 Oct 2017 09:35:15 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:55052 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758063AbdJMNfM (ORCPT ); Fri, 13 Oct 2017 09:35:12 -0400 X-Google-Smtp-Source: AOwi7QCGURDdmw53bgWrla2VOqQPrkl5UI5U9tVnujap//ok2POb3GsKPnOfpLd+uEUkJRwkQVTbyA== Date: Fri, 13 Oct 2017 15:35:16 +0200 From: Christoffer Dall To: Eric Auger Cc: eric.auger.pro@gmail.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, marc.zyngier@arm.com, peter.maydell@linaro.org, andre.przywara@arm.com, wanghaibin.wang@huawei.com, wu.wubin@huawei.com Subject: Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches Message-ID: <20171013133516.GJ8927@cbox> References: <1506518920-18571-1-git-send-email-eric.auger@redhat.com> <1506518920-18571-8-git-send-email-eric.auger@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1506518920-18571-8-git-send-email-eric.auger@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 27, 2017 at 03:28:37PM +0200, Eric Auger wrote: > From: wanghaibin > > We create 2 new functions that frees the device and two free > collection lists. this is currently called by vgic_its_destroy() These are > and we will add other callers in subsequent patches. > > We also remove the check on its->device_list.next as it looks > unnecessary: Could you elude to why you're doing this in the first place in the next version of the commit message? Thanks. > > 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). I don't understand what this paragraph is trying to tell me beyond what some code already does irrelevant to this patch? > > 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 9e6b556..0df6d5f 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, &dev->itt_head, ite_list) > + its_free_ite(kvm, ite); > + list_del(&dev->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->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); > + } > + mutex_unlock(&its->its_lock); this changes semantics from locking across freeing both devices and collections to taking the locks separately. Is that valid? > +} > + > +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, &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); no mutex_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); > @@ -1634,46 +1673,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, &dev->itt_head, ite_list) > - its_free_ite(kvm, ite); > - list_del(&dev->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; I don't think this is valid. We can actually have a non-initialized list and without this check, list_for_each_entry_safe in vgic_its_free_device_list will crash the kernel. Note that an initialized empty list_head doesn't have head and tail pointing to NULL, but pointing to the list_head itself. > - > - 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 > Thanks, -Christoffer