| Message ID | 20210811130356.1143743-1-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: smmu: Set/clear IOMMU domain for device | expand |
Hi Oleksandr, > On 11 Aug 2021, at 2:03 pm, Oleksandr Andrushchenko <andr2000@gmail.com> wrote: > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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. > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Reviewed-by: Rahul Singh <rahul.singh@arm.com> Regards, Rahul
Hi Oleksandr, Apologies for the late answer. On 11/08/2021 14:03, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > 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 <oleksandr_andrushchenko@epam.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > 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,
Hi, Julien! On 17.08.21 20:21, Julien Grall wrote: > Hi Oleksandr, > > Apologies for the late answer. > > On 11/08/2021 14:03, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> 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") Will do > > 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 <oleksandr_andrushchenko@epam.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> 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. Yes, absolutely > 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, > Thanks, Oleksandr
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; } 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; + } spin_unlock(&xen_domain->lock);