Message-ID: <48a4cb2e-81cd-4026-5f30-0a0a77d506f8@redhat.com>
Date:   Sat, 21 Oct 2017 11:02:27 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Christoffer Dall <cdall@...aro.org>
Cc:     eric.auger.pro@...il.com, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, marc.zyngier@....com,
        peter.maydell@...aro.org, andre.przywara@....com,
        wanghaibin.wang@...wei.com, wu.wubin@...wei.com
Subject: Re: [PATCH v2 07/10] KVM: arm/arm64: vgic-its: new helper functions
 to free the caches

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 <wanghaibin.wang@...wei.com>
>>
>> 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 <wanghaibin.wang@...wei.com>
>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>
>> ---
>> [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
> 

Powered by blists - more mailing lists