From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753314AbdJUJCd (ORCPT ); Sat, 21 Oct 2017 05:02:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55348 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752824AbdJUJCb (ORCPT ); Sat, 21 Oct 2017 05:02:31 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CF7CA883B9 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=eric.auger@redhat.com Subject: Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions to free the caches To: Christoffer Dall References: <1506518920-18571-1-git-send-email-eric.auger@redhat.com> <1506518920-18571-8-git-send-email-eric.auger@redhat.com> <20171013133516.GJ8927@cbox> 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 From: Auger Eric Message-ID: <48a4cb2e-81cd-4026-5f30-0a0a77d506f8@redhat.com> Date: Sat, 21 Oct 2017 11:02:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171013133516.GJ8927@cbox> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Sat, 21 Oct 2017 09:02:31 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Christoffer, On 13/10/2017 15:35, Christoffer Dall wrote: > 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() First my apologies as most of your comments have been left out of the v3-v4 respin by oversight. Some comments below. > > 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? Handling deletion of device and collection separately is valid I think as MAPC (vgic_its_cmd_handle_mapc) and MAPD(vgic_its_cmd_handle_mapd) commands do that separately. However, ..., a collection can be referred by an ITE and I should reset the ite->collection = NULL for all ITEs referencing a deleted ITE. vgic_its_free_collection do that. By the way, vgic_its_unmap_device() is same as vgic_its_free_device() so I can remove vgic_its_free_device. > >> +} >> + >> +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 ? damned. > >> +} >> + >> + >> 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. I think you agreed on my previous statement: https://www.spinics.net/lists/kvm-arm/msg27198.html I understand the sequence is: 1) vm_ioctl_create_device |_ ops->create |_ vgic_create_its INIT_LIST_HEAD(&its->device_list); INIT_LIST_HEAD(&its->collection_list); list_add(&dev->vm_node, &kvm->devices); kvm_destroy_devices list_for_each_entry_safe(dev, tmp, &kvm->devices, vm_node) { ops->destroy |_ vgic_its_destroy so vgic_its_destroy is called on an its device that was added to the kvm->devices list. If so the list was created. Then we have vgic_mmio_write_its_baser() which is new caller introduced in subsequent patch. for vgic_mmio_write_its_baser() to be called, vgic_register_its_iodev must have been called. This latter is called on set_attr=vgic_its_set_attr set_attr can be called only if the fd is created. This happens in kvm_ioctl_create_device after ops->create() has been successful, ie meaning the lists are created. What do I miss? What is the case you identified where the device_list is not initialized? Thanks Eric > > 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 >