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

[Lu Baolu]

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