lkml.org 
[lkml]   [2018]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC PATCH v2 08/10] vfio/type1: Add domain at(de)taching group helpers
From
Date
Hi,

On 09/19/2018 07:26 AM, Tian, Kevin wrote:
>> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com]
>> Sent: Tuesday, September 18, 2018 11:52 PM
>>
>> On 15/09/2018 03:36, Tian, Kevin wrote:
>>>> 4) Userspace opens another mdev.
>>>> -> iommu.c calls domain->ops->attach_dev(domain2, dev)
>>>
>>> another mdev in same VFIO container or different? I assume the
>>> latter since you mentioned a new domain2.
>>
>> I was thinking a different VFIO container actually. I used domain2 to
>> try to make the example clearer
>>
>>>> 1)? When the container is closed, VFIO calls
>>>> iommu_detach_device(domain2, parent_dev)
>>>> -> iommu.c calls default_domain->ops->attach_dev(default_domain,
>> dev)
>>>> Given that auxiliary domains are attached, the IOMMU driver could
>> deduce
>>>> that this actually means "detach an auxiliary domain". But which one?
>>>
>>> I didn't get this one. There is no need to stick to 1) behavior for
>>> 4), i.e. below is expected:
>>>         domain2->ops->detach_dev(domain2, dev)
>>
>> But in order to get that, the IOMMU core needs to know that domain2 is
>> auxiliary. Otherwise, detach_dev is never called when a default_domain
>> is present for the parent dev.
>>
>> I guess one solution is to add an "auxiliary" attribute to iommu_domain,
>> so __iommu_detach_group would do something like:
>
> this doesn't work. same domain can be also attached to another physical
> device as non-aux domain (e.g. passthrough) at the same time (vfio-pci
> device and vfio-mdev device in same container), then default domain
> tweak is required in that case. "aux" takes effect only per-device, not
> per-domain.

If we have below APIs for aux domain (the API names are just for
discussion purpose, subject to change):

iommu_querry_aux_domain_capability(dev)
iommu_enable_aux_domain(dev)
iommu_disable_aux_domain(dev)
iommu_check_aux_domain_status(dev)

then, we could do this like below:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ab3d7d3b1583..3bfb652c78e8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1469,12 +1469,31 @@ static int iommu_group_do_detach_device(struct
device *dev, void *data)
return 0;
}

+static int iommu_group_check_aux_domain(struct device *dev, void *data)
+{
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+ if (ops && ops->check_auxd)
+ return !ops->check_auxd(dev);
+
+ return -EINVAL;
+}
+
+/*
+ * Check whether devices in @group have aux domain enabled.
+ */
+static int iommu_group_aux_domain_enabled(struct iommu_group *group)
+{
+ return __iommu_group_for_each_dev(group, NULL,
+ iommu_group_check_aux_domain);
+}
+
static void __iommu_detach_group(struct iommu_domain *domain,
struct iommu_group *group)
{
int ret;

- if (!group->default_domain) {
+ if (!group->default_domain ||
iommu_group_aux_domain_enabled(group)) {
__iommu_group_for_each_dev(group, domain,
iommu_group_do_detach_device);
group->domain = NULL;
Best regards,
Lu Baolu

>
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 7113fe398b70..2b3e9b91aee7 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1786,10 +1786,11 @@ static void __iommu_detach_group(struct
>> iommu_domain *domain,
>> {
>> int ret;
>>
>> - if (!group->default_domain) {
>> + if (!group->default_domain || domain->auxiliary) {
>> __iommu_group_for_each_dev(group, domain,
>> iommu_group_do_detach_device);
>> - group->domain = NULL;
>> + if (!domain->auxiliary)
>> + group->domain = NULL;
>> return;
>> }
>>
>> Not sure who would set this "auxiliary" attribute... Maybe the IOMMU
>> driver, when attaching the domain to a device that has auxiliary mode
>> enabled?
>>
>>> why cannot ARM implement a detach_dev for aux_domain too? My
>>> feeling is that default domain twist is only for switch between 1/2/3
>>> in concept.
>>
>> If the core actually calls it, we can implement detach_dev :) The
>> problem is that the core never calls detach_dev when default_domain is
>> present (affects any IOMMU driver that relies on default_domain,
>> including AMD), and even in case 4) the default_domain is present for
>> the parent device
>
> Then can we change that core logic so detach_dev is invoked in all
> cases? yes there will be some changes in vendor drivers, but I expect
> this change trivial (especially considering the gain in IOMMU API
> simplicity side as described below).
>
>>
>>>> So the proposed interface doesn't seem to work as is. If we want to use
>>>> iommu_attach/detach_device for auxiliary domains, the existing
>> behavior
>>>> of iommu.c, and IOMMU drivers that rely on it, have to change. Any
>>>> change I can think of right now seems more daunting than introducing a
>>>> different path for auxiliary domains, like iommu_attach_aux_domain for
>>>> example.
>>>>
>>>
>>> introducing *aux* specific API will cause different VFIO code path to
>>> handle RID-based and PASID-based mdev, since RID-based still needs
>>> to use normal attach_domain that way.
>>
>> The PASID-based mdev still requires a special case to retrieve the
>> allocated PASID and program it in the parent device, so VFIO will need
>> to know the difference between the two
>>
>
> that retrieve/program is down by parent driver, instead of VFIO.
>
> Thanks
> Kevin
>

\
 
 \ /
  Last update: 2018-09-19 04:12    [W:0.537 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site