lkml.org 
[lkml]   [2020]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v13 4/8] iommu/vt-d: Add bind guest PASID support
From
Date
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

\
 
 \ /
  Last update: 2020-05-16 08:03    [W:0.066 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site