Messages in this thread | | | Subject | Re: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support | From | Lu Baolu <> | Date | Sat, 16 May 2020 14:02:23 +0800 |
| |
Hi Jacob,
On 2020/5/14 23:57, Jacob Pan wrote: > Hi Christoph, > > Thanks a lot for the reviews, comments below. > > Jacob > > On Wed, 13 May 2020 22:59:30 -0700 > Christoph Hellwig<hch@infradead.org> wrote: > >>> + if (dev_is_pci(dev)) { >>> + /* VT-d supports devices with full 20 bit PASIDs >>> only */ >>> + if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >>> + return -EINVAL; >>> + } else { >>> + return -ENOTSUPP; >>> + } >> This looks strange. Why not: >> >> if (!dev_is_pci(dev)) { >> return -ENOTSUPP; >> >> /* VT-d supports devices with full 20 bit PASIDs only */ >> if (pci_max_pasids(to_pci_dev(dev)) != PASID_MAX) >> return -EINVAL; >> > That is better, will do. > >>> + for_each_svm_dev(sdev, svm, dev) { >>> + /* >>> + * For devices with aux domains, we should >>> allow multiple >>> + * bind calls with the same PASID and pdev. >>> + */ >>> + if (iommu_dev_feature_enabled(dev, >>> IOMMU_DEV_FEAT_AUX)) { >>> + sdev->users++; >>> + } else { >>> + dev_warn_ratelimited(dev, "Already >>> bound with PASID %u\n", >>> + svm->pasid); >>> + ret = -EBUSY; >>> + } >>> + goto out; >> Is this intentionally a for loop that jumps out of the loop after >> the first device? >> > The name is confusing, it is not a loop. I will change it to > find_svm_dev() and comments like this? > > /* > * Find the matching device in a given SVM. The bind code ensures that > * each device can only be added to the SVM list once. > */ > #define find_svm_dev(sdev, svm, d) \ > list_for_each_entry((sdev), &(svm)->devs, list) \ > if ((d) != (sdev)->dev) {} else >
The for_each_svm_dev() is not added by this series and is also used by other functions. How about changing it in a separated patch?
Best regards, baolu
| |