Hi,
On 09/19/2018 07:26 AM, Tian, Kevin wrote:
From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@xxxxxxx]
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
