* [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device
@ 2021-08-11 13:03 Oleksandr Andrushchenko
2021-08-17 10:38 ` Rahul Singh
2021-08-17 17:21 ` Julien Grall
0 siblings, 2 replies; 4+ messages in thread
From: Oleksandr Andrushchenko @ 2021-08-11 13:03 UTC (permalink / raw)
To: sstabellini, julien, Volodymyr_Babchuk, xen-devel
Cc: Oleksandr Andrushchenko, Oleksandr Tyshchenko
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>
---
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;
}
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device
2021-08-11 13:03 [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device Oleksandr Andrushchenko
@ 2021-08-17 10:38 ` Rahul Singh
2021-08-17 17:21 ` Julien Grall
1 sibling, 0 replies; 4+ messages in thread
From: Rahul Singh @ 2021-08-17 10:38 UTC (permalink / raw)
To: Oleksandr Andrushchenko
Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk, xen-devel,
Oleksandr Andrushchenko, Oleksandr Tyshchenko
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device
2021-08-11 13:03 [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device Oleksandr Andrushchenko
2021-08-17 10:38 ` Rahul Singh
@ 2021-08-17 17:21 ` Julien Grall
2021-08-18 5:15 ` Oleksandr Andrushchenko
1 sibling, 1 reply; 4+ messages in thread
From: Julien Grall @ 2021-08-17 17:21 UTC (permalink / raw)
To: Oleksandr Andrushchenko, sstabellini, Volodymyr_Babchuk, xen-devel
Cc: Oleksandr Andrushchenko, Oleksandr Tyshchenko
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,
--
Julien Grall
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device
2021-08-17 17:21 ` Julien Grall
@ 2021-08-18 5:15 ` Oleksandr Andrushchenko
0 siblings, 0 replies; 4+ messages in thread
From: Oleksandr Andrushchenko @ 2021-08-18 5:15 UTC (permalink / raw)
To: Julien Grall, sstabellini, Volodymyr_Babchuk, xen-devel
Cc: Oleksandr Andrushchenko, Oleksandr Tyshchenko
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-18 5:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 13:03 [PATCH] xen/arm: smmu: Set/clear IOMMU domain for device Oleksandr Andrushchenko
2021-08-17 10:38 ` Rahul Singh
2021-08-17 17:21 ` Julien Grall
2021-08-18 5:15 ` Oleksandr Andrushchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).