Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Jacob,

On 6/28/19 4:22 AM, Jacob Pan wrote:
>>> +		}
>>> +		refcount_set(&svm->refs, 0);
>>> +		ioasid_set_data(data->hpasid, svm);
>>> +		INIT_LIST_HEAD_RCU(&svm->devs);
>>> +		INIT_LIST_HEAD(&svm->list);
>>> +
>>> +		mmput(svm->mm);
>>> +	}
>>> +	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
>>> +	if (!sdev) {
>>> +		ret = -ENOMEM;
>>> +		goto out;
>> I think you need to clean up svm if its device list is empty here, as
>> you said above:
>>
> No, we come here only if the device list is not empty and the new
> device to bind is different than any existing device in the list. If we
> cannot allocate memory for the new device, should not free the existing
> SVM, right?
> 

I'm sorry, but the code doesn't show this. We come here even an svm data
structure was newly created with an empty device list. I post the code
below to ensure that we are reading a same piece of code.

         mutex_lock(&pasid_mutex);
         svm = ioasid_find(NULL, data->hpasid, NULL);
         if (IS_ERR(svm)) {
                 ret = PTR_ERR(svm);
                 goto out;
         }
         if (svm) {
                 /*
                  * If we found svm for the PASID, there must be at
                  * least one device bond, otherwise svm should be freed.
                  */
                 BUG_ON(list_empty(&svm->devs));

                 for_each_svm_dev() {
                         /* In case of multiple sub-devices of the same 
pdev assigned, we should
                          * allow multiple bind calls with the same 
PASID and pdev.
                          */
                         sdev->users++;
                         goto out;
                 }
         } else {
                 /* We come here when PASID has never been bond to a 
device. */
                 svm = kzalloc(sizeof(*svm), GFP_KERNEL);
                 if (!svm) {
                         ret = -ENOMEM;
                         goto out;
                 }
                 /* REVISIT: upper layer/VFIO can track host process 
that bind the PASID.
                  * ioasid_set = mm might be sufficient for vfio to 
check pasid VMM
                  * ownership.
                  */
                 svm->mm = get_task_mm(current);
                 svm->pasid = data->hpasid;
                 if (data->flags & IOMMU_SVA_GPASID_VAL) {
                         svm->gpasid = data->gpasid;
                         svm->flags &= SVM_FLAG_GUEST_PASID;
                 }
                 refcount_set(&svm->refs, 0);
                 ioasid_set_data(data->hpasid, svm);
                 INIT_LIST_HEAD_RCU(&svm->devs);
                 INIT_LIST_HEAD(&svm->list);

                 mmput(svm->mm);
         }
         sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
         if (!sdev) {
                 ret = -ENOMEM;
                 goto out;
         }
         sdev->dev = dev;
         sdev->users = 1;

Best regards,
Baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu