* [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached
@ 2023-05-26 8:53 Chen-Yu Tsai
2023-05-29 7:55 ` AngeloGioacchino Del Regno
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Chen-Yu Tsai @ 2023-05-26 8:53 UTC (permalink / raw)
To: Yong Wu, Joerg Roedel, Will Deacon, Robin Murphy,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: Chen-Yu Tsai, iommu, linux-mediatek, linux-arm-kernel, linux-kernel
If an IOMMU domain was never attached, it lacks any linkage to the
actual IOMMU hardware. Attempting to do flush_iotlb_all() on it will
result in a NULL pointer dereference. This seems to happen after the
recent IOMMU core rework in v6.4-rc1.
Unable to handle kernel read from unreadable memory at virtual address 0000000000000018
Call trace:
mtk_iommu_flush_iotlb_all+0x20/0x80
iommu_create_device_direct_mappings.part.0+0x13c/0x230
iommu_setup_default_domain+0x29c/0x4d0
iommu_probe_device+0x12c/0x190
of_iommu_configure+0x140/0x208
of_dma_configure_id+0x19c/0x3c0
platform_dma_configure+0x38/0x88
really_probe+0x78/0x2c0
Check if the "bank" field has been filled in before actually attempting
the IOTLB flush to avoid it. The IOTLB is also flushed when the device
comes out of runtime suspend, so it should have a clean initial state.
Fixes: 08500c43d4f7 ("iommu/mediatek: Adjust the structure")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
I think this is a valid fix, but I'm not very familiar with the hardware
or the design of the driver. The ARM SMMU drivers seem to do this as well.
drivers/iommu/mtk_iommu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index aecc7d154f28..e93906d6e112 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -781,7 +781,8 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
{
struct mtk_iommu_domain *dom = to_mtk_domain(domain);
- mtk_iommu_tlb_flush_all(dom->bank->parent_data);
+ if (dom->bank)
+ mtk_iommu_tlb_flush_all(dom->bank->parent_data);
}
static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
--
2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached
2023-05-26 8:53 [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached Chen-Yu Tsai
@ 2023-05-29 7:55 ` AngeloGioacchino Del Regno
2023-05-29 8:32 ` Yong Wu (吴勇)
2023-06-01 9:50 ` Joerg Roedel
2 siblings, 0 replies; 4+ messages in thread
From: AngeloGioacchino Del Regno @ 2023-05-29 7:55 UTC (permalink / raw)
To: Chen-Yu Tsai, Yong Wu, Joerg Roedel, Will Deacon, Robin Murphy,
Matthias Brugger
Cc: iommu, linux-mediatek, linux-arm-kernel, linux-kernel
Il 26/05/23 10:53, Chen-Yu Tsai ha scritto:
> If an IOMMU domain was never attached, it lacks any linkage to the
> actual IOMMU hardware. Attempting to do flush_iotlb_all() on it will
> result in a NULL pointer dereference. This seems to happen after the
> recent IOMMU core rework in v6.4-rc1.
>
> Unable to handle kernel read from unreadable memory at virtual address 0000000000000018
> Call trace:
> mtk_iommu_flush_iotlb_all+0x20/0x80
> iommu_create_device_direct_mappings.part.0+0x13c/0x230
> iommu_setup_default_domain+0x29c/0x4d0
> iommu_probe_device+0x12c/0x190
> of_iommu_configure+0x140/0x208
> of_dma_configure_id+0x19c/0x3c0
> platform_dma_configure+0x38/0x88
> really_probe+0x78/0x2c0
>
> Check if the "bank" field has been filled in before actually attempting
> the IOTLB flush to avoid it. The IOTLB is also flushed when the device
> comes out of runtime suspend, so it should have a clean initial state.
>
> Fixes: 08500c43d4f7 ("iommu/mediatek: Adjust the structure")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
Not only ARM SMMU does this, others are doing the same, some in a different
form (walking a list)... So I agree with this being a valid fix.
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>
> I think this is a valid fix, but I'm not very familiar with the hardware
> or the design of the driver. The ARM SMMU drivers seem to do this as well.
>
> drivers/iommu/mtk_iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index aecc7d154f28..e93906d6e112 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -781,7 +781,8 @@ static void mtk_iommu_flush_iotlb_all(struct iommu_domain *domain)
> {
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>
> - mtk_iommu_tlb_flush_all(dom->bank->parent_data);
> + if (dom->bank)
> + mtk_iommu_tlb_flush_all(dom->bank->parent_data);
> }
>
> static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached
2023-05-26 8:53 [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached Chen-Yu Tsai
2023-05-29 7:55 ` AngeloGioacchino Del Regno
@ 2023-05-29 8:32 ` Yong Wu (吴勇)
2023-06-01 9:50 ` Joerg Roedel
2 siblings, 0 replies; 4+ messages in thread
From: Yong Wu (吴勇) @ 2023-05-29 8:32 UTC (permalink / raw)
To: robin.murphy, wenst, joro, will, angelogioacchino.delregno, matthias.bgg
Cc: linux-arm-kernel, linux-kernel, linux-mediatek, iommu
On Fri, 2023-05-26 at 16:53 +0800, Chen-Yu Tsai wrote:
>
> If an IOMMU domain was never attached, it lacks any linkage to the
> actual IOMMU hardware. Attempting to do flush_iotlb_all() on it will
> result in a NULL pointer dereference. This seems to happen after the
> recent IOMMU core rework in v6.4-rc1.
>
> Unable to handle kernel read from unreadable memory at virtual
> address 0000000000000018
> Call trace:
> mtk_iommu_flush_iotlb_all+0x20/0x80
> iommu_create_device_direct_mappings.part.0+0x13c/0x230
> iommu_setup_default_domain+0x29c/0x4d0
> iommu_probe_device+0x12c/0x190
> of_iommu_configure+0x140/0x208
> of_dma_configure_id+0x19c/0x3c0
> platform_dma_configure+0x38/0x88
> really_probe+0x78/0x2c0
>
> Check if the "bank" field has been filled in before actually
> attempting
> the IOTLB flush to avoid it. The IOTLB is also flushed when the
> device
> comes out of runtime suspend, so it should have a clean initial
> state.
>
> Fixes: 08500c43d4f7 ("iommu/mediatek: Adjust the structure")
The interface "iommu_setup_default_domain" doesn't exist in v6.4-rc1.
This is a fixes for linux-next. And the Fixes tag should be:
152431e4fe7f ("iommu: Do iommu_group_create_direct_mappings() before
attach")
then:
Reviewed-by: Yong Wu <yong.wu@mediatek.com>
Thanks very much.
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>
> I think this is a valid fix, but I'm not very familiar with the
> hardware
> or the design of the driver. The ARM SMMU drivers seem to do this as
> well.
>
> drivers/iommu/mtk_iommu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index aecc7d154f28..e93906d6e112 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -781,7 +781,8 @@ static void mtk_iommu_flush_iotlb_all(struct
> iommu_domain *domain)
> {
> struct mtk_iommu_domain *dom = to_mtk_domain(domain);
>
> - mtk_iommu_tlb_flush_all(dom->bank->parent_data);
> + if (dom->bank)
> + mtk_iommu_tlb_flush_all(dom->bank->parent_data);
> }
>
> static void mtk_iommu_iotlb_sync(struct iommu_domain *domain,
> --
> 2.41.0.rc0.172.g3f132b7071-goog
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached
2023-05-26 8:53 [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached Chen-Yu Tsai
2023-05-29 7:55 ` AngeloGioacchino Del Regno
2023-05-29 8:32 ` Yong Wu (吴勇)
@ 2023-06-01 9:50 ` Joerg Roedel
2 siblings, 0 replies; 4+ messages in thread
From: Joerg Roedel @ 2023-06-01 9:50 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Yong Wu, Will Deacon, Robin Murphy, Matthias Brugger,
AngeloGioacchino Del Regno, iommu, linux-mediatek,
linux-arm-kernel, linux-kernel
On Fri, May 26, 2023 at 04:53:59PM +0800, Chen-Yu Tsai wrote:
> Fixes: 08500c43d4f7 ("iommu/mediatek: Adjust the structure")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
Applied for 6.4, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-01 9:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 8:53 [PATCH RFC] iommu/mediatek: Flush IOTLB completely only if domain has been attached Chen-Yu Tsai
2023-05-29 7:55 ` AngeloGioacchino Del Regno
2023-05-29 8:32 ` Yong Wu (吴勇)
2023-06-01 9:50 ` Joerg Roedel
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).