lkml.org 
[lkml]   [2020]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()
    From
    Date
    Hi Alex,

    On 8/1/20 2:14 AM, Alex Williamson wrote:
    > On Fri, 31 Jul 2020 14:30:03 +0800
    > Lu Baolu <baolu.lu@linux.intel.com> wrote:
    >
    >> Hi Alex,
    >>
    >> On 2020/7/30 4:25, Alex Williamson wrote:
    >>> On Tue, 14 Jul 2020 13:57:02 +0800
    >>> Lu Baolu<baolu.lu@linux.intel.com> wrote:
    >>>
    >>>> The device driver needs an API to get its aux-domain. A typical usage
    >>>> scenario is:
    >>>>
    >>>> unsigned long pasid;
    >>>> struct iommu_domain *domain;
    >>>> struct device *dev = mdev_dev(mdev);
    >>>> struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
    >>>>
    >>>> domain = iommu_aux_get_domain_for_dev(dev);
    >>>> if (!domain)
    >>>> return -ENODEV;
    >>>>
    >>>> pasid = iommu_aux_get_pasid(domain, iommu_device);
    >>>> if (pasid <= 0)
    >>>> return -EINVAL;
    >>>>
    >>>> /* Program the device context */
    >>>> ....
    >>>>
    >>>> This adds an API for such use case.
    >>>>
    >>>> Suggested-by: Alex Williamson<alex.williamson@redhat.com>
    >>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
    >>>> ---
    >>>> drivers/iommu/iommu.c | 18 ++++++++++++++++++
    >>>> include/linux/iommu.h | 7 +++++++
    >>>> 2 files changed, 25 insertions(+)
    >>>>
    >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
    >>>> index cad5a19ebf22..434bf42b6b9b 100644
    >>>> --- a/drivers/iommu/iommu.c
    >>>> +++ b/drivers/iommu/iommu.c
    >>>> @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct iommu_domain *domain,
    >>>> }
    >>>> EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
    >>>>
    >>>> +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;
    >>> Why wouldn't the aux domain flag be on the domain itself rather than
    >>> the group? Then if we wanted sanity checking in patch 1/ we'd only
    >>> need to test the flag on the object we're provided.
    >>
    >> Agreed. Given that a group may contain both non-aux and aux devices,
    >> adding such flag in iommu_group doesn't make sense.
    >>
    >>>
    >>> If we had such a flag, we could create an iommu_domain_is_aux()
    >>> function and then simply use iommu_get_domain_for_dev() and test that
    >>> it's an aux domain in the example use case. It seems like that would
    >>> resolve the jump from a domain to an aux-domain just as well as adding
    >>> this separate iommu_aux_get_domain_for_dev() interface. The is_aux
    >>> test might also be useful in other cases too.
    >>
    >> Let's rehearsal our use case.
    >>
    >> unsigned long pasid;
    >> struct iommu_domain *domain;
    >> struct device *dev = mdev_dev(mdev);
    >> struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
    >>
    >> [1] domain = iommu_get_domain_for_dev(dev);
    >> if (!domain)
    >> return -ENODEV;
    >>
    >> [2] pasid = iommu_aux_get_pasid(domain, iommu_device);
    >> if (pasid <= 0)
    >> return -EINVAL;
    >>
    >> /* Program the device context */
    >> ....
    >>
    >> The reason why I add this iommu_aux_get_domain_for_dev() is that we need
    >> to make sure the domain got at [1] is valid to be used at [2].
    >>
    >> https://lore.kernel.org/linux-iommu/20200707150408.474d81f1@x1.home/
    >
    > Yep, I thought that was a bit of a leap in logic.
    >
    >> When calling into iommu_aux_get_pasid(), the iommu driver should make
    >> sure that @domain is a valid aux-domain for @iommu_device. Hence, for
    >> our use case, it seems that there's no need for a is_aux_domain() api.
    >>
    >> Anyway, I'm not against adding a new is_aux_domain() api if there's a
    >> need elsewhere.
    >
    > I think it could work either way, we could have an
    > iommu_get_aux_domain_for_dev() which returns NULL if the domain is not
    > an aux domain, or we could use iommu_get_domain_for_dev() and the
    > caller could test the domain with iommu_is_aux_domain() if they need to
    > confirm if it's an aux domain. The former could even be written using
    > the latter, a wrapper of iommu_get_domain_for_dev() that checks aux
    > property before returning.

    Okay. So iommu_get_domain_for_dev() and iommu_is_aux_domain() are what
    we wanted. The iommu_get_domain_for_dev() could be a simple wrapper of
    them.

    Thanks a lot for the guide. I will implement a new version according to
    the feedbacks.

    Best regards,
    baolu

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