Hi Oleksandr,
Apologies for the late answer.
On 11/08/2021 14:03, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko <[email protected]>
When a device is assigned/de-assigned it is required to properly set
IOMMU domain used to protect the device. This assignment was missing,
thus it was not possible to de-assign the device:
(XEN) Deassigning device 0000:03:00.0 from dom2
(XEN) smmu: 0000:03:00.0: not attached to domain 2
(XEN) d2: deassign (0000:03:00.0) failed (-3)
Fix this by assigning IOMMU domain on arm_smmu_assign_dev and reset it
to NULL on arm_smmu_deassign_dev.
I think this was introduced by commit 06d1f7a278dd "xen/arm: smmuv1:
Keep track of S2CR state". If so, please add:
Fixes: 06d1f7a278dd ("xen/arm: smmuv1: Keep track of S2CR state")
Looking at the commit message, the IOMMU domain used to be set/unset in
arm_smmu_{attach, detach}_dev() but Linux drop it because they now rely
in the core IOMMU framework to track the domain.
So I agree with the new position for...
Signed-off-by: Oleksandr Andrushchenko <[email protected]>
Signed-off-by: Oleksandr Tyshchenko <[email protected]>
---
xen/drivers/passthrough/arm/smmu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/drivers/passthrough/arm/smmu.c
b/xen/drivers/passthrough/arm/smmu.c
index c234ad9c7f1e..373d9d4d123a 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2768,6 +2768,7 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
arm_smmu_destroy_iommu_domain(domain);
} else {
atomic_inc(&domain->ref);
+ dev_iommu_domain(dev) = domain;
... this one. However...
}
out:
@@ -2794,7 +2795,10 @@ static int arm_smmu_deassign_dev(struct domain *d,
struct device *dev)
atomic_dec(&domain->ref);
if (domain->ref.counter == 0)
+ {
arm_smmu_destroy_iommu_domain(domain);
+ dev_iommu_domain(dev) = NULL;
+ }
... I think this one is incorrect because you would only unset
dev_iommu_domain() is the refcount drop to 0. You can have multiple
device in the same domain, so the ref.counter may not be 0.
So I think this needs to be moved outside of the if. Note that it is
also a good practice to remove any reference (e.g. set to NULL) before
freeing even if it doesn't much matter here.
Lastly, the file is using the Linux coding style. So { needs to be on
the same line as the if.
Cheers,
--
Julien Grall
