[PATCH 0/5] iommu: apple-dart: Support locked DARTs

[PATCH 0/5] iommu: apple-dart: Support locked DARTs

[Alyssa Rosenzweig]

Some DARTs on Apple SoCs are "locked" at boot time, meaning we cannot
write most of their configuration registers. Currently, we refuse to
probe locked DARTs and do not include any in the upstream device trees.
However, the DARTs used for the display controller are locked, so we
need to handle locked DARTs as a prerequisite for a KMS driver.

This series teaches the iommu/apple-dart driver about locked DARTs,
preparing for a future upstream display driver. It has been extensively
tested downstream with our work-in-progress display driver.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
Alyssa Rosenzweig (5):
      iommu/dart: Track if the DART is locked
      iommu/dart: Skip reset for locked DARTs
      iommu/dart: Set DMA domain for locked DARTs
      iommu/dart: Reject identity domain for locked DARTs
      iommu/dart: Assert !locked when configuring

 drivers/iommu/apple-dart.c | 47 +++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250210-locked-dart-d7ece05f19a8

Best regards,
-- 
Alyssa Rosenzweig <alyssa@rosenzweig.io>

[PATCH 1/5] iommu/dart: Track if the DART is locked

[Alyssa Rosenzweig]

Some DARTs are locked at boot-time. That means they are already
configured and we cannot change their configuration, which requires
special handling. Locked DARTs are identified in the configuration
register. Check this bit when probing and save the result so we can
handle accordingly.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/iommu/apple-dart.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 95ba3caeb40177901086076416874b9905a1f40a..460cf96bc8001e051916f356d5d175b35e36c608 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -197,6 +197,7 @@ struct apple_dart_hw {
  * @lock: lock for hardware operations involving this dart
  * @pgsize: pagesize supported by this DART
  * @supports_bypass: indicates if this DART supports bypass mode
+ * @locked: indicates if this DART is locked
  * @sid2group: maps stream ids to iommu_groups
  * @iommu: iommu core device
  */
@@ -217,6 +218,7 @@ struct apple_dart {
 	u32 pgsize;
 	u32 num_streams;
 	u32 supports_bypass : 1;
+	u32 locked : 1;
 
 	struct iommu_group *sid2group[DART_MAX_STREAMS];
 	struct iommu_device iommu;
@@ -1076,6 +1078,11 @@ static irqreturn_t apple_dart_t8110_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static bool apple_dart_is_locked(struct apple_dart *dart)
+{
+	return !!(readl(dart->regs + dart->hw->lock) & dart->hw->lock_bit);
+}
+
 static int apple_dart_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -1143,6 +1150,7 @@ static int apple_dart_probe(struct platform_device *pdev)
 		goto err_clk_disable;
 	}
 
+	dart->locked = apple_dart_is_locked(dart);
 	ret = apple_dart_hw_reset(dart);
 	if (ret)
 		goto err_clk_disable;
@@ -1165,9 +1173,9 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 	dev_info(
 		&pdev->dev,
-		"DART [pagesize %x, %d streams, bypass support: %d, bypass forced: %d] initialized\n",
+		"DART [pagesize %x, %d streams, bypass support: %d, bypass forced: %d, locked: %d] initialized\n",
 		dart->pgsize, dart->num_streams, dart->supports_bypass,
-		dart->pgsize > PAGE_SIZE);
+		dart->pgsize > PAGE_SIZE, dart->locked);
 	return 0;
 
 err_sysfs_remove:

-- 
2.48.1

[PATCH 2/5] iommu/dart: Skip reset for locked DARTs

[Alyssa Rosenzweig]

Locked DARTs cannot be reconfigured, therefore the reset/restore
procedure can't work and should not be needed. Skip it, allowing locked
DARTs to probe.

Co-developed-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/iommu/apple-dart.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 460cf96bc8001e051916f356d5d175b35e36c608..9c74a95eb7e819e94ab2fb47ed0d411a1eba8bf7 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -452,17 +452,9 @@ apple_dart_t8110_hw_invalidate_tlb(struct apple_dart_stream_map *stream_map)
 
 static int apple_dart_hw_reset(struct apple_dart *dart)
 {
-	u32 config;
 	struct apple_dart_stream_map stream_map;
 	int i;
 
-	config = readl(dart->regs + dart->hw->lock);
-	if (config & dart->hw->lock_bit) {
-		dev_err(dart->dev, "DART is locked down until reboot: %08x\n",
-			config);
-		return -EINVAL;
-	}
-
 	stream_map.dart = dart;
 	bitmap_zero(stream_map.sidmap, DART_MAX_STREAMS);
 	bitmap_set(stream_map.sidmap, 0, dart->num_streams);
@@ -1151,9 +1143,11 @@ static int apple_dart_probe(struct platform_device *pdev)
 	}
 
 	dart->locked = apple_dart_is_locked(dart);
-	ret = apple_dart_hw_reset(dart);
-	if (ret)
-		goto err_clk_disable;
+	if (!dart->locked) {
+		ret = apple_dart_hw_reset(dart);
+		if (ret)
+			goto err_clk_disable;
+	}
 
 	ret = request_irq(dart->irq, dart->hw->irq_handler, IRQF_SHARED,
 			  "apple-dart fault handler", dart);
@@ -1192,7 +1186,9 @@ static void apple_dart_remove(struct platform_device *pdev)
 {
 	struct apple_dart *dart = platform_get_drvdata(pdev);
 
-	apple_dart_hw_reset(dart);
+	if (!dart->locked)
+		apple_dart_hw_reset(dart);
+
 	free_irq(dart->irq, dart);
 
 	iommu_device_unregister(&dart->iommu);
@@ -1325,6 +1321,10 @@ static __maybe_unused int apple_dart_resume(struct device *dev)
 	unsigned int sid, idx;
 	int ret;
 
+	/* Locked DARTs can't be restored, and they should not need it */
+	if (dart->locked)
+		return 0;
+
 	ret = apple_dart_hw_reset(dart);
 	if (ret) {
 		dev_err(dev, "Failed to reset DART on resume\n");

-- 
2.48.1

[PATCH 3/5] iommu/dart: Set DMA domain for locked DARTs

[Alyssa Rosenzweig]

This is required.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/iommu/apple-dart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 9c74a95eb7e819e94ab2fb47ed0d411a1eba8bf7..9c6f780dc7220096ed6bba692fa1a4bd859b0d61 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -941,6 +941,8 @@ static int apple_dart_def_domain_type(struct device *dev)
 		return IOMMU_DOMAIN_IDENTITY;
 	if (!cfg->stream_maps[0].dart->supports_bypass)
 		return IOMMU_DOMAIN_DMA;
+	if (cfg->stream_maps[0].dart->locked)
+		return IOMMU_DOMAIN_DMA;
 
 	return 0;
 }

-- 
2.48.1

[PATCH 4/5] iommu/dart: Reject identity domain for locked DARTs

[Alyssa Rosenzweig]

This cannot work.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Signed-off-by: Janne Grunau <j@jannau.net>
---
 drivers/iommu/apple-dart.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 9c6f780dc7220096ed6bba692fa1a4bd859b0d61..29b627b38e8c37afd2b6a72865f43d24b633834a 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -681,6 +681,9 @@ static int apple_dart_attach_dev_identity(struct iommu_domain *domain,
 	if (!cfg->stream_maps[0].dart->supports_bypass)
 		return -EINVAL;
 
+	if (cfg->stream_maps[0].dart->locked)
+		return -EINVAL;
+
 	for_each_stream_map(i, cfg, stream_map)
 		apple_dart_hw_enable_bypass(stream_map);
 	return 0;

-- 
2.48.1

[PATCH 5/5] iommu/dart: Assert !locked when configuring

[Alyssa Rosenzweig]

Configuration is only possible and needed for non-locked DARTs and will
fail for locked DARTs. We cannot try -- assert that we do not.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 drivers/iommu/apple-dart.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 29b627b38e8c37afd2b6a72865f43d24b633834a..87eb87bb2f5158d000a2c2fc801b722a2262c941 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -309,6 +309,7 @@ apple_dart_hw_enable_translation(struct apple_dart_stream_map *stream_map)
 	struct apple_dart *dart = stream_map->dart;
 	int sid;
 
+	WARN_ON(stream_map->dart->locked);
 	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
 		writel(dart->hw->tcr_enabled, dart->regs + DART_TCR(dart, sid));
 }
@@ -318,6 +319,7 @@ static void apple_dart_hw_disable_dma(struct apple_dart_stream_map *stream_map)
 	struct apple_dart *dart = stream_map->dart;
 	int sid;
 
+	WARN_ON(stream_map->dart->locked);
 	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
 		writel(dart->hw->tcr_disabled, dart->regs + DART_TCR(dart, sid));
 }
@@ -328,7 +330,7 @@ apple_dart_hw_enable_bypass(struct apple_dart_stream_map *stream_map)
 	struct apple_dart *dart = stream_map->dart;
 	int sid;
 
-	WARN_ON(!stream_map->dart->supports_bypass);
+	WARN_ON(stream_map->dart->locked || !stream_map->dart->supports_bypass);
 	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
 		writel(dart->hw->tcr_bypass,
 		       dart->regs + DART_TCR(dart, sid));
@@ -340,6 +342,7 @@ static void apple_dart_hw_set_ttbr(struct apple_dart_stream_map *stream_map,
 	struct apple_dart *dart = stream_map->dart;
 	int sid;
 
+	WARN_ON(stream_map->dart->locked);
 	WARN_ON(paddr & ((1 << dart->hw->ttbr_shift) - 1));
 	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
 		writel(dart->hw->ttbr_valid |
@@ -353,6 +356,7 @@ static void apple_dart_hw_clear_ttbr(struct apple_dart_stream_map *stream_map,
 	struct apple_dart *dart = stream_map->dart;
 	int sid;
 
+	WARN_ON(stream_map->dart->locked);
 	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
 		writel(0, dart->regs + DART_TTBR(dart, sid, idx));
 }

-- 
2.48.1

Re: [PATCH 0/5] iommu: apple-dart: Support locked DARTs

[Neal Gompa]

On Mon, Feb 10, 2025 at 2:44 PM Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> Some DARTs on Apple SoCs are "locked" at boot time, meaning we cannot
> write most of their configuration registers. Currently, we refuse to
> probe locked DARTs and do not include any in the upstream device trees.
> However, the DARTs used for the display controller are locked, so we
> need to handle locked DARTs as a prerequisite for a KMS driver.
>
> This series teaches the iommu/apple-dart driver about locked DARTs,
> preparing for a future upstream display driver. It has been extensively
> tested downstream with our work-in-progress display driver.
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
> Alyssa Rosenzweig (5):
>       iommu/dart: Track if the DART is locked
>       iommu/dart: Skip reset for locked DARTs
>       iommu/dart: Set DMA domain for locked DARTs
>       iommu/dart: Reject identity domain for locked DARTs
>       iommu/dart: Assert !locked when configuring
>
>  drivers/iommu/apple-dart.c | 47 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250210-locked-dart-d7ece05f19a8
>
> Best regards,
> --
> Alyssa Rosenzweig <alyssa@rosenzweig.io>
>
>

Series LGTM, having looked over this many times as part of kernel
maintenance for Fedora Asahi Remix.

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!

Re: [PATCH 4/5] iommu/dart: Reject identity domain for locked DARTs

[Nick Chan]

Alyssa Rosenzweig 於 2025/2/11 凌晨3:39 寫道:
> This cannot work.
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Signed-off-by: Janne Grunau <j@jannau.net>
The submitter's sign-off must be the last sign-off. It appears that you are the author
of this patch, so drop Janne's sign-off.
[...]

Nick Chan

Re: [PATCH 4/5] iommu/dart: Reject identity domain for locked DARTs

[Alyssa Rosenzweig]

> > Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> > Signed-off-by: Janne Grunau <j@jannau.net>
> The submitter's sign-off must be the last sign-off. It appears that you are the author
> of this patch, so drop Janne's sign-off.

Thanks, fixed in v2.

Re: [PATCH 3/5] iommu/dart: Set DMA domain for locked DARTs

[Robin Murphy]

On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote:
> This is required.

Now that we can, I'd really rather do this properly and not offer an 
identity domain in the first place when it's not available.

Thanks,
Robin.

> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>   drivers/iommu/apple-dart.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 9c74a95eb7e819e94ab2fb47ed0d411a1eba8bf7..9c6f780dc7220096ed6bba692fa1a4bd859b0d61 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -941,6 +941,8 @@ static int apple_dart_def_domain_type(struct device *dev)
>   		return IOMMU_DOMAIN_IDENTITY;
>   	if (!cfg->stream_maps[0].dart->supports_bypass)
>   		return IOMMU_DOMAIN_DMA;
> +	if (cfg->stream_maps[0].dart->locked)
> +		return IOMMU_DOMAIN_DMA;
>   
>   	return 0;
>   }
> 

Re: [PATCH 2/5] iommu/dart: Skip reset for locked DARTs

[Robin Murphy]

On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote:
> Locked DARTs cannot be reconfigured, therefore the reset/restore
> procedure can't work and should not be needed. Skip it, allowing locked
> DARTs to probe.
> 
> Co-developed-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>   drivers/iommu/apple-dart.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 460cf96bc8001e051916f356d5d175b35e36c608..9c74a95eb7e819e94ab2fb47ed0d411a1eba8bf7 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -452,17 +452,9 @@ apple_dart_t8110_hw_invalidate_tlb(struct apple_dart_stream_map *stream_map)
>   
>   static int apple_dart_hw_reset(struct apple_dart *dart)
>   {
> -	u32 config;
>   	struct apple_dart_stream_map stream_map;
>   	int i;
>   
> -	config = readl(dart->regs + dart->hw->lock);
> -	if (config & dart->hw->lock_bit) {
> -		dev_err(dart->dev, "DART is locked down until reboot: %08x\n",
> -			config);
> -		return -EINVAL;
> -	}
> -
>   	stream_map.dart = dart;
>   	bitmap_zero(stream_map.sidmap, DART_MAX_STREAMS);
>   	bitmap_set(stream_map.sidmap, 0, dart->num_streams);
> @@ -1151,9 +1143,11 @@ static int apple_dart_probe(struct platform_device *pdev)
>   	}
>   
>   	dart->locked = apple_dart_is_locked(dart);
> -	ret = apple_dart_hw_reset(dart);
> -	if (ret)
> -		goto err_clk_disable;
> +	if (!dart->locked) {
> +		ret = apple_dart_hw_reset(dart);
> +		if (ret)
> +			goto err_clk_disable;
> +	}
>   
>   	ret = request_irq(dart->irq, dart->hw->irq_handler, IRQF_SHARED,
>   			  "apple-dart fault handler", dart);
> @@ -1192,7 +1186,9 @@ static void apple_dart_remove(struct platform_device *pdev)
>   {
>   	struct apple_dart *dart = platform_get_drvdata(pdev);
>   
> -	apple_dart_hw_reset(dart);
> +	if (!dart->locked)
> +		apple_dart_hw_reset(dart);

Not much point saving anything on suspend either, in that case.

Thanks,
Robin.

> +
>   	free_irq(dart->irq, dart);
>   
>   	iommu_device_unregister(&dart->iommu);
> @@ -1325,6 +1321,10 @@ static __maybe_unused int apple_dart_resume(struct device *dev)
>   	unsigned int sid, idx;
>   	int ret;
>   
> +	/* Locked DARTs can't be restored, and they should not need it */
> +	if (dart->locked)
> +		return 0;
> +
>   	ret = apple_dart_hw_reset(dart);
>   	if (ret) {
>   		dev_err(dev, "Failed to reset DART on resume\n");
> 

Re: [PATCH 3/5] iommu/dart: Set DMA domain for locked DARTs

[Alyssa Rosenzweig]

Hi Robin,

> > This is required.
> 
> Now that we can, I'd really rather do this properly and not offer an
> identity domain in the first place when it's not available.

I'm reading through iommu.c but I don't see a way to avoid offering an
identity domain without the DMA override here, just reading through the
logic of iommu_get_default_domain_type. Could you point me to what you
have in mind?

Thanks,
Alyssa

> > --- a/drivers/iommu/apple-dart.c
> > +++ b/drivers/iommu/apple-dart.c
> > @@ -941,6 +941,8 @@ static int apple_dart_def_domain_type(struct device *dev)
> >   		return IOMMU_DOMAIN_IDENTITY;
> >   	if (!cfg->stream_maps[0].dart->supports_bypass)
> >   		return IOMMU_DOMAIN_DMA;
> > +	if (cfg->stream_maps[0].dart->locked)
> > +		return IOMMU_DOMAIN_DMA;
> >   	return 0;
> >   }
> > 
> 

Re: [PATCH 5/5] iommu/dart: Assert !locked when configuring

[Robin Murphy]

On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote:
> Configuration is only possible and needed for non-locked DARTs and will
> fail for locked DARTs. We cannot try -- assert that we do not.

Except now we absolutely will - if a locked DART and its client device 
are advertised to Linux, instead of gracefully refusing to touch it, 
we'll now attach the client to a DMA domain, firing a barrage of 
multiple WARNs in the process, and give it DMA ops which still cannot 
work. I'm not really convinced this series on its own leaves us in a 
better position than we're already in now... :/

How hideous is the rest of what's required to actually make this usable?

Thanks,
Robin.

> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>   drivers/iommu/apple-dart.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 29b627b38e8c37afd2b6a72865f43d24b633834a..87eb87bb2f5158d000a2c2fc801b722a2262c941 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -309,6 +309,7 @@ apple_dart_hw_enable_translation(struct apple_dart_stream_map *stream_map)
>   	struct apple_dart *dart = stream_map->dart;
>   	int sid;
>   
> +	WARN_ON(stream_map->dart->locked);
>   	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
>   		writel(dart->hw->tcr_enabled, dart->regs + DART_TCR(dart, sid));
>   }
> @@ -318,6 +319,7 @@ static void apple_dart_hw_disable_dma(struct apple_dart_stream_map *stream_map)
>   	struct apple_dart *dart = stream_map->dart;
>   	int sid;
>   
> +	WARN_ON(stream_map->dart->locked);
>   	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
>   		writel(dart->hw->tcr_disabled, dart->regs + DART_TCR(dart, sid));
>   }
> @@ -328,7 +330,7 @@ apple_dart_hw_enable_bypass(struct apple_dart_stream_map *stream_map)
>   	struct apple_dart *dart = stream_map->dart;
>   	int sid;
>   
> -	WARN_ON(!stream_map->dart->supports_bypass);
> +	WARN_ON(stream_map->dart->locked || !stream_map->dart->supports_bypass);
>   	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
>   		writel(dart->hw->tcr_bypass,
>   		       dart->regs + DART_TCR(dart, sid));
> @@ -340,6 +342,7 @@ static void apple_dart_hw_set_ttbr(struct apple_dart_stream_map *stream_map,
>   	struct apple_dart *dart = stream_map->dart;
>   	int sid;
>   
> +	WARN_ON(stream_map->dart->locked);
>   	WARN_ON(paddr & ((1 << dart->hw->ttbr_shift) - 1));
>   	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
>   		writel(dart->hw->ttbr_valid |
> @@ -353,6 +356,7 @@ static void apple_dart_hw_clear_ttbr(struct apple_dart_stream_map *stream_map,
>   	struct apple_dart *dart = stream_map->dart;
>   	int sid;
>   
> +	WARN_ON(stream_map->dart->locked);
>   	for_each_set_bit(sid, stream_map->sidmap, dart->num_streams)
>   		writel(0, dart->regs + DART_TTBR(dart, sid, idx));
>   }
> 

Re: [PATCH 2/5] iommu/dart: Skip reset for locked DARTs

[Alyssa Rosenzweig]

> >   	struct apple_dart *dart = platform_get_drvdata(pdev);
> > -	apple_dart_hw_reset(dart);
> > +	if (!dart->locked)
> > +		apple_dart_hw_reset(dart);
> 
> Not much point saving anything on suspend either, in that case.

Quite right. Fixed in v2, thanks!

Re: [PATCH 5/5] iommu/dart: Assert !locked when configuring

[Alyssa Rosenzweig]

> > Configuration is only possible and needed for non-locked DARTs and will
> > fail for locked DARTs. We cannot try -- assert that we do not.
> 
> Except now we absolutely will - if a locked DART and its client device are
> advertised to Linux, instead of gracefully refusing to touch it, we'll now
> attach the client to a DMA domain, firing a barrage of multiple WARNs in the
> process, and give it DMA ops which still cannot work. I'm not really
> convinced this series on its own leaves us in a better position than we're
> already in now... :/

Fair point, thanks for raising that. "Fortunately" the upstream DTs
don't describe any locked DARTs yet.

> How hideous is the rest of what's required to actually make this usable?

It isn't... pretty, and it's going to be ugly no matter how we slice it.
Unfortunately the display controller DARTs really are locked so our
hands are tied here.

When I originally wrote these patches I had some hideous hack in the
shared page table code. I'm thrilled to see that Janne rewrote that code
to be local to apple-dart.c, at least:
https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a

I'll include that patch with v2, which makes the locked DARTs actually
usable, and restructure the series so we only probe after that commit is
there.

Re: [PATCH 5/5] iommu/dart: Assert !locked when configuring

[Janne Grunau]

On Tue, Feb 11, 2025 at 06:41:00PM +0000, Robin Murphy wrote:
> On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote:
> > Configuration is only possible and needed for non-locked DARTs and will
> > fail for locked DARTs. We cannot try -- assert that we do not.
> 
> Except now we absolutely will - if a locked DART and its client device 
> are advertised to Linux, instead of gracefully refusing to touch it, 
> we'll now attach the client to a DMA domain, firing a barrage of 
> multiple WARNs in the process, and give it DMA ops which still cannot 
> work. I'm not really convinced this series on its own leaves us in a 
> better position than we're already in now... :/
> 
> How hideous is the rest of what's required to actually make this usable?

The TTBR can not be changed but the preset first level table can
modified at will. The driver keeps a shadow first label table and syncs
that to the preset 1st level table on flush_tbl().
It gets more complicated by the fact that the iommu for the display
coprocessor is locked and mappings for its firmware and boot framebuffer
are preinstalled and have to be maintained or restored on
initialization.
This is handled via reserved memory with translation.

Downstream change to handle this is in
https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a
not including the change to parse / handle reserved memory with
translation in iommu core.

Janne

Re: [PATCH 3/5] iommu/dart: Set DMA domain for locked DARTs

[Robin Murphy]

On 2025-02-11 6:40 pm, Alyssa Rosenzweig wrote:
> Hi Robin,
> 
>>> This is required.
>>
>> Now that we can, I'd really rather do this properly and not offer an
>> identity domain in the first place when it's not available.
> 
> I'm reading through iommu.c but I don't see a way to avoid offering an
> identity domain without the DMA override here, just reading through the
> logic of iommu_get_default_domain_type. Could you point me to what you
> have in mind?

Since we finished cleaning up the domain allocation paths, it's finally 
safe to have per-instance ops, so you can mix and match some with an 
identity domain and some without, as s390 will be doing[1].

Cheers,
Robin.

[1] 
https://lore.kernel.org/linux-iommu/20250207205335.473946-5-mjrosato@linux.ibm.com/

> Thanks,
> Alyssa
> 
>>> --- a/drivers/iommu/apple-dart.c
>>> +++ b/drivers/iommu/apple-dart.c
>>> @@ -941,6 +941,8 @@ static int apple_dart_def_domain_type(struct device *dev)
>>>    		return IOMMU_DOMAIN_IDENTITY;
>>>    	if (!cfg->stream_maps[0].dart->supports_bypass)
>>>    		return IOMMU_DOMAIN_DMA;
>>> +	if (cfg->stream_maps[0].dart->locked)
>>> +		return IOMMU_DOMAIN_DMA;
>>>    	return 0;
>>>    }
>>>
>>

Re: [PATCH 5/5] iommu/dart: Assert !locked when configuring

[Robin Murphy]

On 2025-02-11 7:21 pm, Janne Grunau wrote:
> On Tue, Feb 11, 2025 at 06:41:00PM +0000, Robin Murphy wrote:
>> On 2025-02-10 7:39 pm, Alyssa Rosenzweig wrote:
>>> Configuration is only possible and needed for non-locked DARTs and will
>>> fail for locked DARTs. We cannot try -- assert that we do not.
>>
>> Except now we absolutely will - if a locked DART and its client device
>> are advertised to Linux, instead of gracefully refusing to touch it,
>> we'll now attach the client to a DMA domain, firing a barrage of
>> multiple WARNs in the process, and give it DMA ops which still cannot
>> work. I'm not really convinced this series on its own leaves us in a
>> better position than we're already in now... :/
>>
>> How hideous is the rest of what's required to actually make this usable?
> 
> The TTBR can not be changed but the preset first level table can
> modified at will. The driver keeps a shadow first label table and syncs
> that to the preset 1st level table on flush_tbl().
> It gets more complicated by the fact that the iommu for the display
> coprocessor is locked and mappings for its firmware and boot framebuffer
> are preinstalled and have to be maintained or restored on
> initialization.
> This is handled via reserved memory with translation.
> 
> Downstream change to handle this is in
> https://github.com/AsahiLinux/linux/commit/d90cc3590ea460e1c574b4b7c47fdafb2794af6a
> not including the change to parse / handle reserved memory with
> translation in iommu core.

Oh, if we handwave away the reserved region stuff for now, it doesn't 
seem *too* terrible, so definitely worth trying to land the bones of it 
along with this prep work, I reckon. From a quick look I think it might 
possibly be even cleaner as an io-pgtable quirk, to essentially skip 
allocating/freeing L1 and have some mechanism to fill in data->pgd with 
the remap afterwards (possible super cheeky version - also prepopulate 
cfg->apple_dart_cfg.ttbr and have alloc/free handle the 
remapping/unmapping themselves...). I'm not 100% sure off-hand, but 
since you avoid the DMA API and don't seem to have any other dependency 
on data->pgd having a linear map VA (other than the virt_to_phys() in 
the normal alloc path which you'd skip anyway), it feels like it ought 
to work out.

I guess to support multiple domains you do still end up having to 
save/restore the L1 contents at the driver level when attaching, so some 
kind of shadow table notion isn't entirely unavoidable... oh well, it's 
a thought, at least.

Thanks,
Robin.