Re: [RESEND PATCH v8 01/11] iommu: Add DMA ownership management interfaces
On 15/06/2022 11:57, Robin Murphy wrote:
> On 2022-06-15 10:53, Steven Price wrote:
>> On 18/04/2022 01:49, Lu Baolu wrote:
>>> Multiple devices may be placed in the same IOMMU group because they
>>> cannot be isolated from each other. These devices must either be
>>> entirely under kernel control or userspace control, never a mixture.
>>>
>>> This adds dma ownership management in iommu core and exposes several
>>> interfaces for the device drivers and the device userspace assignment
>>> framework (i.e. VFIO), so that any conflict between user and kernel
>>> controlled dma could be detected at the beginning.
>>>
>>> The device driver oriented interfaces are,
>>>
>>> int iommu_device_use_default_domain(struct device *dev);
>>> void iommu_device_unuse_default_domain(struct device *dev);
>>>
>>> By calling iommu_device_use_default_domain(), the device driver tells
>>> the iommu layer that the device dma is handled through the kernel DMA
>>> APIs. The iommu layer will manage the IOVA and use the default domain
>>> for DMA address translation.
>>>
>>> The device user-space assignment framework oriented interfaces are,
>>>
>>> int iommu_group_claim_dma_owner(struct iommu_group *group,
>>> void *owner);
>>> void iommu_group_release_dma_owner(struct iommu_group *group);
>>> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>>>
>>> The device userspace assignment must be disallowed if the DMA owner
>>> claiming interface returns failure.
>>>
>>> Signed-off-by: Jason Gunthorpe
>>> Signed-off-by: Kevin Tian
>>> Signed-off-by: Lu Baolu
>>> Reviewed-by: Robin Murphy
>>
>> I'm seeing a regression that I've bisected to this commit on a Firefly
>> RK3288 board. The display driver fails to probe properly because
>> __iommu_attach_group() returns -EBUSY. This causes long hangs and splats
>> as the display flips timeout.
>>
>> The call stack to __iommu_attach_group() is:
>>
>> __iommu_attach_group from iommu_attach_device+0x64/0xb4
>> iommu_attach_device from rockchip_drm_dma_attach_device+0x20/0x50
>> rockchip_drm_dma_attach_device from vop_crtc_atomic_enable+0x10c/0xa64
>> vop_crtc_atomic_enable from
>> drm_atomic_helper_commit_modeset_enables+0xa8/0x290
>> drm_atomic_helper_commit_modeset_enables from
>> drm_atomic_helper_commit_tail_rpm+0x44/0x8c
>> drm_atomic_helper_commit_tail_rpm from commit_tail+0x9c/0x180
>> commit_tail from drm_atomic_helper_commit+0x164/0x18c
>> drm_atomic_helper_commit from drm_atomic_commit+0xac/0xe4
>> drm_atomic_commit from drm_client_modeset_commit_atomic+0x23c/0x284
>> drm_client_modeset_commit_atomic from
>> drm_client_modeset_commit_locked+0x60/0x1c8
>> drm_client_modeset_commit_locked from
>> drm_client_modeset_commit+0x24/0x40
>> drm_client_modeset_commit from drm_fb_helper_set_par+0xb8/0xf8
>> drm_fb_helper_set_par from drm_fb_helper_hotplug_event.part.0+0xa8/0xc0
>> drm_fb_helper_hotplug_event.part.0 from output_poll_execute+0xb8/0x224
>>
>>> @@ -2109,7 +2115,7 @@ static int __iommu_attach_group(struct
>>> iommu_domain *domain,
>>> {
>>> int ret;
>>> - if (group->default_domain && group->domain !=
>>> group->default_domain)
>>> + if (group->domain && group->domain != group->default_domain)
>>> return -EBUSY;
>>> ret = __iommu_group_for_each_dev(group, domain,
>>
>> Reverting this 'fixes' the problem for me. The follow up 0286300e6045
>> ("iommu: iommu_group_claim_dma_owner() must always assign a domain")
>> doesn't help.
>>
>> Adding some debug printks I can see that domain is a valid pointer, but
>> both default_domain and blocking_domain are NULL.
>>
>> I'm using the DTB from the kernel tree (rk3288-firefly.dtb).
>>
>> Any ideas?
>
> Hmm, TBH I'm not sure how that worked previously... it'll be complaining
> because the ARM DMA domain is still attached, but even when the attach
> goes ahead and replaces the ARM domain with the driver's new one, it's
> not using the special arm_iommu_detach_device() interface anywhere so
> the device would still be left with the wrong DMA ops :/
>
> I guess the most pragmatic option is probably to give rockchip-drm a
> similar bodge to exynos and tegra, to explicitly remove the ARM domain
> before attaching its own.
A bodge like below indeed 'fixes' the problem:
---8<---
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 67d38f53d3e5..cbc6a5121296 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -23,6 +23,14 @@
#include
#include
+#if defined(CONFIG_ARM_DMA_USE_IOMMU)
+#include
+#else
+#define arm_iommu_detach_device(...) ({ })
+#define arm_iommu_release_mapping(...) ({ })
+#define to_dma_iommu_mapping(dev) NULL
+#endif
+
#include "rockchip_drm_drv.h"
#include "rockchip_drm_fb.h"
#include "rockchip_drm_gem.h"
@@ -49,6 +57,14 @@ int rockchip_drm_dma_attach_device(struct drm_device
*drm_dev,
if (
Re: [RESEND PATCH v8 01/11] iommu: Add DMA ownership management interfaces
On 2022-06-15 10:53, Steven Price wrote:
On 18/04/2022 01:49, Lu Baolu wrote:
Multiple devices may be placed in the same IOMMU group because they
cannot be isolated from each other. These devices must either be
entirely under kernel control or userspace control, never a mixture.
This adds dma ownership management in iommu core and exposes several
interfaces for the device drivers and the device userspace assignment
framework (i.e. VFIO), so that any conflict between user and kernel
controlled dma could be detected at the beginning.
The device driver oriented interfaces are,
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
By calling iommu_device_use_default_domain(), the device driver tells
the iommu layer that the device dma is handled through the kernel DMA
APIs. The iommu layer will manage the IOVA and use the default domain
for DMA address translation.
The device user-space assignment framework oriented interfaces are,
int iommu_group_claim_dma_owner(struct iommu_group *group,
void *owner);
void iommu_group_release_dma_owner(struct iommu_group *group);
bool iommu_group_dma_owner_claimed(struct iommu_group *group);
The device userspace assignment must be disallowed if the DMA owner
claiming interface returns failure.
Signed-off-by: Jason Gunthorpe
Signed-off-by: Kevin Tian
Signed-off-by: Lu Baolu
Reviewed-by: Robin Murphy
I'm seeing a regression that I've bisected to this commit on a Firefly
RK3288 board. The display driver fails to probe properly because
__iommu_attach_group() returns -EBUSY. This causes long hangs and splats
as the display flips timeout.
The call stack to __iommu_attach_group() is:
__iommu_attach_group from iommu_attach_device+0x64/0xb4
iommu_attach_device from rockchip_drm_dma_attach_device+0x20/0x50
rockchip_drm_dma_attach_device from vop_crtc_atomic_enable+0x10c/0xa64
vop_crtc_atomic_enable from
drm_atomic_helper_commit_modeset_enables+0xa8/0x290
drm_atomic_helper_commit_modeset_enables from
drm_atomic_helper_commit_tail_rpm+0x44/0x8c
drm_atomic_helper_commit_tail_rpm from commit_tail+0x9c/0x180
commit_tail from drm_atomic_helper_commit+0x164/0x18c
drm_atomic_helper_commit from drm_atomic_commit+0xac/0xe4
drm_atomic_commit from drm_client_modeset_commit_atomic+0x23c/0x284
drm_client_modeset_commit_atomic from
drm_client_modeset_commit_locked+0x60/0x1c8
drm_client_modeset_commit_locked from drm_client_modeset_commit+0x24/0x40
drm_client_modeset_commit from drm_fb_helper_set_par+0xb8/0xf8
drm_fb_helper_set_par from drm_fb_helper_hotplug_event.part.0+0xa8/0xc0
drm_fb_helper_hotplug_event.part.0 from output_poll_execute+0xb8/0x224
@@ -2109,7 +2115,7 @@ static int __iommu_attach_group(struct iommu_domain
*domain,
{
int ret;
- if (group->default_domain && group->domain != group->default_domain)
+ if (group->domain && group->domain != group->default_domain)
return -EBUSY;
ret = __iommu_group_for_each_dev(group, domain,
Reverting this 'fixes' the problem for me. The follow up 0286300e6045
("iommu: iommu_group_claim_dma_owner() must always assign a domain")
doesn't help.
Adding some debug printks I can see that domain is a valid pointer, but
both default_domain and blocking_domain are NULL.
I'm using the DTB from the kernel tree (rk3288-firefly.dtb).
Any ideas?
Hmm, TBH I'm not sure how that worked previously... it'll be complaining
because the ARM DMA domain is still attached, but even when the attach
goes ahead and replaces the ARM domain with the driver's new one, it's
not using the special arm_iommu_detach_device() interface anywhere so
the device would still be left with the wrong DMA ops :/
I guess the most pragmatic option is probably to give rockchip-drm a
similar bodge to exynos and tegra, to explicitly remove the ARM domain
before attaching its own.
Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RESEND PATCH v8 01/11] iommu: Add DMA ownership management interfaces
On 18/04/2022 01:49, Lu Baolu wrote:
> Multiple devices may be placed in the same IOMMU group because they
> cannot be isolated from each other. These devices must either be
> entirely under kernel control or userspace control, never a mixture.
>
> This adds dma ownership management in iommu core and exposes several
> interfaces for the device drivers and the device userspace assignment
> framework (i.e. VFIO), so that any conflict between user and kernel
> controlled dma could be detected at the beginning.
>
> The device driver oriented interfaces are,
>
> int iommu_device_use_default_domain(struct device *dev);
> void iommu_device_unuse_default_domain(struct device *dev);
>
> By calling iommu_device_use_default_domain(), the device driver tells
> the iommu layer that the device dma is handled through the kernel DMA
> APIs. The iommu layer will manage the IOVA and use the default domain
> for DMA address translation.
>
> The device user-space assignment framework oriented interfaces are,
>
> int iommu_group_claim_dma_owner(struct iommu_group *group,
> void *owner);
> void iommu_group_release_dma_owner(struct iommu_group *group);
> bool iommu_group_dma_owner_claimed(struct iommu_group *group);
>
> The device userspace assignment must be disallowed if the DMA owner
> claiming interface returns failure.
>
> Signed-off-by: Jason Gunthorpe
> Signed-off-by: Kevin Tian
> Signed-off-by: Lu Baolu
> Reviewed-by: Robin Murphy
I'm seeing a regression that I've bisected to this commit on a Firefly
RK3288 board. The display driver fails to probe properly because
__iommu_attach_group() returns -EBUSY. This causes long hangs and splats
as the display flips timeout.
The call stack to __iommu_attach_group() is:
__iommu_attach_group from iommu_attach_device+0x64/0xb4
iommu_attach_device from rockchip_drm_dma_attach_device+0x20/0x50
rockchip_drm_dma_attach_device from vop_crtc_atomic_enable+0x10c/0xa64
vop_crtc_atomic_enable from drm_atomic_helper_commit_modeset_enables+0xa8/0x290
drm_atomic_helper_commit_modeset_enables from
drm_atomic_helper_commit_tail_rpm+0x44/0x8c
drm_atomic_helper_commit_tail_rpm from commit_tail+0x9c/0x180
commit_tail from drm_atomic_helper_commit+0x164/0x18c
drm_atomic_helper_commit from drm_atomic_commit+0xac/0xe4
drm_atomic_commit from drm_client_modeset_commit_atomic+0x23c/0x284
drm_client_modeset_commit_atomic from
drm_client_modeset_commit_locked+0x60/0x1c8
drm_client_modeset_commit_locked from drm_client_modeset_commit+0x24/0x40
drm_client_modeset_commit from drm_fb_helper_set_par+0xb8/0xf8
drm_fb_helper_set_par from drm_fb_helper_hotplug_event.part.0+0xa8/0xc0
drm_fb_helper_hotplug_event.part.0 from output_poll_execute+0xb8/0x224
> @@ -2109,7 +2115,7 @@ static int __iommu_attach_group(struct iommu_domain
> *domain,
> {
> int ret;
>
> - if (group->default_domain && group->domain != group->default_domain)
> + if (group->domain && group->domain != group->default_domain)
> return -EBUSY;
>
> ret = __iommu_group_for_each_dev(group, domain,
Reverting this 'fixes' the problem for me. The follow up 0286300e6045
("iommu: iommu_group_claim_dma_owner() must always assign a domain")
doesn't help.
Adding some debug printks I can see that domain is a valid pointer, but
both default_domain and blocking_domain are NULL.
I'm using the DTB from the kernel tree (rk3288-firefly.dtb).
Any ideas?
Thanks,
Steve
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RESEND PATCH v8 01/11] iommu: Add DMA ownership management interfaces
Multiple devices may be placed in the same IOMMU group because they
cannot be isolated from each other. These devices must either be
entirely under kernel control or userspace control, never a mixture.
This adds dma ownership management in iommu core and exposes several
interfaces for the device drivers and the device userspace assignment
framework (i.e. VFIO), so that any conflict between user and kernel
controlled dma could be detected at the beginning.
The device driver oriented interfaces are,
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
By calling iommu_device_use_default_domain(), the device driver tells
the iommu layer that the device dma is handled through the kernel DMA
APIs. The iommu layer will manage the IOVA and use the default domain
for DMA address translation.
The device user-space assignment framework oriented interfaces are,
int iommu_group_claim_dma_owner(struct iommu_group *group,
void *owner);
void iommu_group_release_dma_owner(struct iommu_group *group);
bool iommu_group_dma_owner_claimed(struct iommu_group *group);
The device userspace assignment must be disallowed if the DMA owner
claiming interface returns failure.
Signed-off-by: Jason Gunthorpe
Signed-off-by: Kevin Tian
Signed-off-by: Lu Baolu
Reviewed-by: Robin Murphy
---
include/linux/iommu.h | 31 +
drivers/iommu/iommu.c | 153 +-
2 files changed, 181 insertions(+), 3 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9208eca4b0d1..77972ef978b5 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -675,6 +675,13 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+int iommu_device_use_default_domain(struct device *dev);
+void iommu_device_unuse_default_domain(struct device *dev);
+
+int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
+void iommu_group_release_dma_owner(struct iommu_group *group);
+bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1031,6 +1038,30 @@ static inline struct iommu_fwspec
*dev_iommu_fwspec_get(struct device *dev)
{
return NULL;
}
+
+static inline int iommu_device_use_default_domain(struct device *dev)
+{
+ return 0;
+}
+
+static inline void iommu_device_unuse_default_domain(struct device *dev)
+{
+}
+
+static inline int
+iommu_group_claim_dma_owner(struct iommu_group *group, void *owner)
+{
+ return -ENODEV;
+}
+
+static inline void iommu_group_release_dma_owner(struct iommu_group *group)
+{
+}
+
+static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
+{
+ return false;
+}
#endif /* CONFIG_IOMMU_API */
/**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..eba8e8ccf19d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,6 +48,8 @@ struct iommu_group {
struct iommu_domain *default_domain;
struct iommu_domain *domain;
struct list_head entry;
+ unsigned int owner_cnt;
+ void *owner;
};
struct group_device {
@@ -294,7 +296,11 @@ int iommu_probe_device(struct device *dev)
mutex_lock(&group->mutex);
iommu_alloc_default_domain(group, dev);
- if (group->default_domain) {
+ /*
+* If device joined an existing group which has been claimed, don't
+* attach the default domain.
+*/
+ if (group->default_domain && !group->owner) {
ret = __iommu_attach_device(group->default_domain, dev);
if (ret) {
mutex_unlock(&group->mutex);
@@ -2109,7 +2115,7 @@ static int __iommu_attach_group(struct iommu_domain
*domain,
{
int ret;
- if (group->default_domain && group->domain != group->default_domain)
+ if (group->domain && group->domain != group->default_domain)
return -EBUSY;
ret = __iommu_group_for_each_dev(group, domain,
@@ -2146,7 +2152,11 @@ static void __iommu_detach_group(struct iommu_domain
*domain,
{
int ret;
- if (!group->default_domain) {
+ /*
+* If the group has been claimed already, do not re-attach the default
+* domain.
+*/
+ if (!group->default_domain || group->owner) {
__iommu_group_for_each_dev(group, domain,
iommu_group_do_detach_device);
group->domain = NULL;
@@ -3095,3 +3105,140 @@ static ssize_t iommu_group_store_type(struct
iommu_group *group,
return ret;
}
+
+/**
+ * iommu_device_use_default_domain() - Device driver wants to handle device
+ * DMA through the kernel DMA AP
