linux-kernel - Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dc98a109-7121-36b7-0854-f899b09692a4@linux.intel.com>
Date:   Wed, 8 Jul 2020 10:53:12 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Robin Murphy <robin.murphy@....com>,
        Cornelia Huck <cohuck@...hat.com>,
        Kevin Tian <kevin.tian@...el.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Liu Yi L <yi.l.liu@...el.com>,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH v2 1/2] iommu: iommu_aux_at(de)tach_device() extension

Hi Alex,

Thanks a lot for your comments. Please check my reply inline.

On 7/8/20 5:04 AM, Alex Williamson wrote:
> On Tue,  7 Jul 2020 09:39:56 +0800
> Lu Baolu<baolu.lu@...ux.intel.com>  wrote:
> 
>> The hardware assistant vfio mediated device is a use case of iommu
>> aux-domain. The interactions between vfio/mdev and iommu during mdev
>> creation and passthr are:
>>
>> - Create a group for mdev with iommu_group_alloc();
>> - Add the device to the group with
>>          group = iommu_group_alloc();
>>          if (IS_ERR(group))
>>                  return PTR_ERR(group);
>>
>>          ret = iommu_group_add_device(group, &mdev->dev);
>>          if (!ret)
>>                  dev_info(&mdev->dev, "MDEV: group_id = %d\n",
>>                           iommu_group_id(group));
>> - Allocate an aux-domain
>>          iommu_domain_alloc()
>> - Attach the aux-domain to the physical device from which the mdev is
>>    created.
>>          iommu_aux_attach_device()
>>
>> In the whole process, an iommu group was allocated for the mdev and an
>> iommu domain was attached to the group, but the group->domain leaves
>> NULL. As the result, iommu_get_domain_for_dev() doesn't work anymore.
>>
>> The iommu_get_domain_for_dev() is a necessary interface for device
>> drivers that want to support aux-domain. For example,
>>
>>          struct iommu_domain *domain;
>>          struct device *dev = mdev_dev(mdev);
>>          unsigned long pasid;
>>
>>          domain = iommu_get_domain_for_dev(dev);
>>          if (!domain)
>>                  return -ENODEV;
>>
>>          pasid = iommu_aux_get_pasid(domain, dev->parent);
> How did we know this was an aux domain? ie. How did we know we could
> use it with iommu_aux_get_pasid()?

Yes. It's a bit confusing if iommu_get_domain_for_dev() is reused here
for aux-domain.

> 
> Why did we assume the parent device is the iommu device for the aux
> domain?  Should that level of detail be already known by the aux domain?
> 
> Nits - The iomu device of an mdev device is found via
> mdev_get_iommu_device(dev), it should not be assumed to be the parent.
> The parent of an mdev device is found via mdev_parent_dev(mdev).

My bad. The driver should use mdev_get_iommu_device() instead.

> 
> The leaps in logic here make me wonder if we should instead be exposing
> more of an aux domain API rather than blurring the differences between
> these domains.  Thanks,

How about add below API?

/**
  * iommu_aux_get_domain_for_dev - get aux domain for a device
  * @dev: the accessory device
  *
  * The caller should pass a valid @dev to iommu_aux_attach_device() before
  * calling this api. Return an attached aux-domain, or NULL otherwise.
  */
struct iommu_domain *iommu_aux_get_domain_for_dev(struct device *dev)
{
         struct iommu_domain *domain = NULL;
         struct iommu_group *group;

         group = iommu_group_get(dev);
         if (!group)
                 return NULL;

         if (group->aux_domain_attached)
                 domain = group->domain;

         iommu_group_put(group);

         return domain;
}
EXPORT_SYMBOL_GPL(iommu_aux_get_domain_for_dev);

Best regards,
baolu