[PATCH 0/5] iommu: sun50i: Add Allwinner H616 support

[PATCH 0/5] iommu: sun50i: Add Allwinner H616 support

[Andre Przywara]

The Allwinner H616 contains an IOMMU almost compatible to the one used
in the H6. The differing default reset value of the bypass register
makes the two technically incompatible, so use a new DT compatible
string to be on the safe side.
The required driver changes can be applied to both variants, so the driver
is ignorant of the differences between the two for now.

Change the driver to cope with the new variant in patch 1/5 and 2/5,
then apply the required devicetree and binding changes in the remaining
patches.

I could just verify that the driver probes and allocates the page table
from below 4 GB, but haven't tested the actual IOMMU operation, with a
device. I would be grateful if someone could test this in full swing.

Cheers,
Andre.

Andre Przywara (4):
  iommu: sun50i: allocate page tables from below 4 GiB
  dt-bindings: iommu: add new compatible strings
  iommu: sun50i: Add H616 compatible string
  arm64: dts: allwinner: h616: add IOMMU node

Jernej Skrabec (1):
  iommu: sun50i: clear bypass register

 .../bindings/iommu/allwinner,sun50i-h6-iommu.yaml        | 7 ++++++-
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi           | 9 +++++++++
 drivers/iommu/sun50i-iommu.c                             | 7 +++++--
 3 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.35.8

[PATCH 1/5] iommu: sun50i: clear bypass register

[Andre Przywara]

From: Jernej Skrabec <jernej.skrabec@gmail.com>

The Allwinner H6 IOMMU has a bypass register, which allows to circumvent
the page tables for each possible master. The reset value for this
register is 0, which disables the bypass.
The Allwinner H616 IOMMU resets this register to 0x7f, which activates
the bypass for all masters, which is not what we want.

Always clear this register to 0, to enforce the usage of page tables,
and make this driver compatible with the H616 in this respect.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/iommu/sun50i-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index c519b991749d7..dd3f07384624c 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -452,6 +452,7 @@ static int sun50i_iommu_enable(struct sun50i_iommu *iommu)
 		    IOMMU_TLB_PREFETCH_MASTER_ENABLE(3) |
 		    IOMMU_TLB_PREFETCH_MASTER_ENABLE(4) |
 		    IOMMU_TLB_PREFETCH_MASTER_ENABLE(5));
+	iommu_write(iommu, IOMMU_BYPASS_REG, 0);
 	iommu_write(iommu, IOMMU_INT_ENABLE_REG, IOMMU_INT_MASK);
 	iommu_write(iommu, IOMMU_DM_AUT_CTRL_REG(SUN50I_IOMMU_ACI_NONE),
 		    IOMMU_DM_AUT_CTRL_RD_UNAVAIL(SUN50I_IOMMU_ACI_NONE, 0) |
-- 
2.35.8

[PATCH 2/5] iommu: sun50i: allocate page tables from below 4 GiB

[Andre Przywara]

The Allwinner IOMMU is a strict 32-bit device, with the page table root
pointer as well as both level's page tables and also the target addresses
all required to be below 4GB.
The Allwinner H6 SoC only supports 32-bit worth of physical addresses
anyway, so this isn't a problem so far, but the H616 and later SoCs extend
the PA space beyond 32 bit to accommodate more DRAM.
To make sure we stay within the 32-bit PA range required by the IOMMU,
force the memory for the page tables to come from below 4GB. by using
allocations with the DMA32 flag.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/iommu/sun50i-iommu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index dd3f07384624c..c3244db5ac02f 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -682,7 +682,8 @@ sun50i_iommu_domain_alloc_paging(struct device *dev)
 	if (!sun50i_domain)
 		return NULL;
 
-	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL, get_order(DT_SIZE));
+	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL | GFP_DMA32,
+					      get_order(DT_SIZE));
 	if (!sun50i_domain->dt)
 		goto err_free_domain;
 
@@ -997,7 +998,7 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 
 	iommu->pt_pool = kmem_cache_create(dev_name(&pdev->dev),
 					   PT_SIZE, PT_SIZE,
-					   SLAB_HWCACHE_ALIGN,
+					   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA32,
 					   NULL);
 	if (!iommu->pt_pool)
 		return -ENOMEM;
-- 
2.35.8

[PATCH 3/5] dt-bindings: iommu: add new compatible strings

[Andre Przywara]

The Allwinner H616 and A523 contain IOMMU IP very similar to the H6, but
use a different reset value for the bypass register, which makes them
strictly speaking incompatible.

Add a new compatible string for the H616, and a version for the A523,
falling back to the H616.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../bindings/iommu/allwinner,sun50i-h6-iommu.yaml          | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml b/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
index e20016f120175..a8409db4a3e3d 100644
--- a/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
+++ b/Documentation/devicetree/bindings/iommu/allwinner,sun50i-h6-iommu.yaml
@@ -17,7 +17,12 @@ properties:
       The content of the cell is the master ID.
 
   compatible:
-    const: allwinner,sun50i-h6-iommu
+    oneOf:
+      - const: allwinner,sun50i-h6-iommu
+      - const: allwinner,sun50i-h616-iommu
+      - items:
+          - const: allwinner,sun55i-a523-iommu
+          - const: allwinner,sun50i-h616-iommu
 
   reg:
     maxItems: 1
-- 
2.35.8

[PATCH 4/5] iommu: sun50i: Add H616 compatible string

[Andre Przywara]

The IOMMU IP in the Allwinner H616 SoC is *almost* compatible to the H6,
but uses a different reset value for the bypass register, and adds some
more registers.

While a driver *can* be written to support both variants (which we in
fact do), the hardware itself is not fully compatible, so we require a
separate compatible string.

Add the new compatible string to the list, but without changing the
behaviour, since the driver already supports both variants.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/iommu/sun50i-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index c3244db5ac02f..478a4e8e94a89 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -1059,6 +1059,7 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 
 static const struct of_device_id sun50i_iommu_dt[] = {
 	{ .compatible = "allwinner,sun50i-h6-iommu", },
+	{ .compatible = "allwinner,sun50i-h616-iommu", },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, sun50i_iommu_dt);
-- 
2.35.8

[PATCH 5/5] arm64: dts: allwinner: h616: add IOMMU node

[Andre Przywara]

The Allwinner H616 contains a scatter-gather IOMMU connected to some
video related devices. It's almost compatible to the one used in the H6,
though with minor incompatibilities.

Add the DT node describing its resources, so that devices like the video
or display engine can connect to it.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index 9c1980e24cb21..44f04619a43ac 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -320,6 +320,15 @@ x32clk_fanout_pin: x32clk-fanout-pin {
 			};
 		};
 
+		iommu: iommu@30f0000 {
+			compatible = "allwinner,sun50i-h616-iommu";
+			reg = <0x030f0000 0x10000>;
+			interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_IOMMU>;
+			resets = <&ccu RST_BUS_IOMMU>;
+			#iommu-cells = <1>;
+		};
+
 		gic: interrupt-controller@3021000 {
 			compatible = "arm,gic-400";
 			reg = <0x03021000 0x1000>,
-- 
2.35.8

Re: [PATCH 2/5] iommu: sun50i: allocate page tables from below 4 GiB

[Robin Murphy]

On 2024-05-31 12:37 am, Andre Przywara wrote:
> The Allwinner IOMMU is a strict 32-bit device, with the page table root
> pointer as well as both level's page tables and also the target addresses
> all required to be below 4GB.
> The Allwinner H6 SoC only supports 32-bit worth of physical addresses
> anyway, so this isn't a problem so far, but the H616 and later SoCs extend
> the PA space beyond 32 bit to accommodate more DRAM.
> To make sure we stay within the 32-bit PA range required by the IOMMU,
> force the memory for the page tables to come from below 4GB. by using
> allocations with the DMA32 flag.

Uh-oh... what about the output addresses in sun50i_mk_pte()? Limiting 
its own accesses is OK, but if the IOMMU isn't capable of *mapping* any 
valid PA for its clients, we can't easily support that.

Thanks,
Robin.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   drivers/iommu/sun50i-iommu.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> index dd3f07384624c..c3244db5ac02f 100644
> --- a/drivers/iommu/sun50i-iommu.c
> +++ b/drivers/iommu/sun50i-iommu.c
> @@ -682,7 +682,8 @@ sun50i_iommu_domain_alloc_paging(struct device *dev)
>   	if (!sun50i_domain)
>   		return NULL;
>   
> -	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL, get_order(DT_SIZE));
> +	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL | GFP_DMA32,
> +					      get_order(DT_SIZE));
>   	if (!sun50i_domain->dt)
>   		goto err_free_domain;
>   
> @@ -997,7 +998,7 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
>   
>   	iommu->pt_pool = kmem_cache_create(dev_name(&pdev->dev),
>   					   PT_SIZE, PT_SIZE,
> -					   SLAB_HWCACHE_ALIGN,
> +					   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA32,
>   					   NULL);
>   	if (!iommu->pt_pool)
>   		return -ENOMEM;

Re: [PATCH 5/5] arm64: dts: allwinner: h616: add IOMMU node

[Robin Murphy]

On 2024-05-31 12:38 am, Andre Przywara wrote:
> The Allwinner H616 contains a scatter-gather IOMMU connected to some
> video related devices. It's almost compatible to the one used in the H6,
> though with minor incompatibilities.
> 
> Add the DT node describing its resources, so that devices like the video
> or display engine can connect to it.

Without also describing those connections, though, having this node 
enabled in the DT means the driver will just bind, block DMA, and 
prevent those devices from working. That's probably not what you want.

Thanks,
Robin.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>   arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> index 9c1980e24cb21..44f04619a43ac 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> @@ -320,6 +320,15 @@ x32clk_fanout_pin: x32clk-fanout-pin {
>   			};
>   		};
>   
> +		iommu: iommu@30f0000 {
> +			compatible = "allwinner,sun50i-h616-iommu";
> +			reg = <0x030f0000 0x10000>;
> +			interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_IOMMU>;
> +			resets = <&ccu RST_BUS_IOMMU>;
> +			#iommu-cells = <1>;
> +		};
> +
>   		gic: interrupt-controller@3021000 {
>   			compatible = "arm,gic-400";
>   			reg = <0x03021000 0x1000>,

Re: [PATCH 5/5] arm64: dts: allwinner: h616: add IOMMU node

[Andre Przywara]

On Fri, 31 May 2024 09:42:36 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

Hi Robin,

> On 2024-05-31 12:38 am, Andre Przywara wrote:
> > The Allwinner H616 contains a scatter-gather IOMMU connected to some
> > video related devices. It's almost compatible to the one used in the H6,
> > though with minor incompatibilities.
> > 
> > Add the DT node describing its resources, so that devices like the video
> > or display engine can connect to it.  
> 
> Without also describing those connections, though, having this node 
> enabled in the DT means the driver will just bind, block DMA, and 
> prevent those devices from working. That's probably not what you want.

The IOMMU manages the Display Engine (DE), the Deinterlacer (DI), the
video engine (VE) and the 2D acceleration engine (G2D).
None of those devices are supported for the H616 in mainline yet, but there
are patches out there for the DE and VE, at least. Especially the video
codecs benefit from scatter-gather, so with this patch they can make use
of it from day one.

I agree that the series on its own is not very useful, but there are quite
some H616 patches in flight out there, so this is an attempt to clean those
up, picking the low hanging fruits first ;-)

Cheers,
Andre

> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> > index 9c1980e24cb21..44f04619a43ac 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> > @@ -320,6 +320,15 @@ x32clk_fanout_pin: x32clk-fanout-pin {
> >   			};
> >   		};
> >   
> > +		iommu: iommu@30f0000 {
> > +			compatible = "allwinner,sun50i-h616-iommu";
> > +			reg = <0x030f0000 0x10000>;
> > +			interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&ccu CLK_BUS_IOMMU>;
> > +			resets = <&ccu RST_BUS_IOMMU>;
> > +			#iommu-cells = <1>;
> > +		};
> > +
> >   		gic: interrupt-controller@3021000 {
> >   			compatible = "arm,gic-400";
> >   			reg = <0x03021000 0x1000>,  
> 

Re: [PATCH 5/5] arm64: dts: allwinner: h616: add IOMMU node

[Robin Murphy]

On 2024-05-31 10:32 am, Andre Przywara wrote:
> On Fri, 31 May 2024 09:42:36 +0100
> Robin Murphy <robin.murphy@arm.com> wrote:
> 
> Hi Robin,
> 
>> On 2024-05-31 12:38 am, Andre Przywara wrote:
>>> The Allwinner H616 contains a scatter-gather IOMMU connected to some
>>> video related devices. It's almost compatible to the one used in the H6,
>>> though with minor incompatibilities.
>>>
>>> Add the DT node describing its resources, so that devices like the video
>>> or display engine can connect to it.
>>
>> Without also describing those connections, though, having this node
>> enabled in the DT means the driver will just bind, block DMA, and
>> prevent those devices from working. That's probably not what you want.
> 
> The IOMMU manages the Display Engine (DE), the Deinterlacer (DI), the
> video engine (VE) and the 2D acceleration engine (G2D).
> None of those devices are supported for the H616 in mainline yet, but there
> are patches out there for the DE and VE, at least. Especially the video
> codecs benefit from scatter-gather, so with this patch they can make use
> of it from day one.

Oh, I see - I assumed that stuff like display was more exciting and 
likely to be supported already, so read "can connect to it" in the 
present tense. If it's a future "can use it as soon as they are also 
supported", then there's no concern, and indeed this is arguably the 
ideal order of doing things.

Cheers,
Robin.

Re: [PATCH 2/5] iommu: sun50i: allocate page tables from below 4 GiB

[Andre Przywara]

On Fri, 31 May 2024 09:37:02 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

Hi Robin,

> On 2024-05-31 12:37 am, Andre Przywara wrote:
> > The Allwinner IOMMU is a strict 32-bit device, with the page table root
> > pointer as well as both level's page tables and also the target addresses
> > all required to be below 4GB.
> > The Allwinner H6 SoC only supports 32-bit worth of physical addresses
> > anyway, so this isn't a problem so far, but the H616 and later SoCs extend
> > the PA space beyond 32 bit to accommodate more DRAM.
> > To make sure we stay within the 32-bit PA range required by the IOMMU,
> > force the memory for the page tables to come from below 4GB. by using
> > allocations with the DMA32 flag.  
> 
> Uh-oh... what about the output addresses in sun50i_mk_pte()? Limiting 
> its own accesses is OK, but if the IOMMU isn't capable of *mapping* any 
> valid PA for its clients, we can't easily support that.

Right, that's indeed a problem. I was hoping that the DMA32 address limit
would somehow be enforced by the IOMMU master devices, so they would never
issue addresses above 4GB to the IOMMU in the first place.
Would this work if all those devices use a 32-bit DMA mask? Some of those
devices might have that limit anyways, but those video devices are not
my expertise, so I don't know much details.

IIUC, atm the incoming PA would be masked down to 32-bit, I guess we should have a
WARN_ONCE() there when this happens?
The 32-bit limit would only affect boards with exactly 4GB of DRAM (the
DRAM controller limit), and it only affects the last GB then, so using
DMA32 wouldn't be a terrible limitation, I think.

TBH, I picked this up from Jernej, so have to refer to him for further
details.

Cheers,
Andre

P.S. I agree that a 32-bit only IOMMU sounds somewhat stup^Wweird, but
that's what we have. Maybe we would use it just for the VE only then, where
it's really helpful to provide the illusion of large physically contiguous
buffers.

> Thanks,
> Robin.
> 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >   drivers/iommu/sun50i-iommu.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
> > index dd3f07384624c..c3244db5ac02f 100644
> > --- a/drivers/iommu/sun50i-iommu.c
> > +++ b/drivers/iommu/sun50i-iommu.c
> > @@ -682,7 +682,8 @@ sun50i_iommu_domain_alloc_paging(struct device *dev)
> >   	if (!sun50i_domain)
> >   		return NULL;
> >   
> > -	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL, get_order(DT_SIZE));
> > +	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL | GFP_DMA32,
> > +					      get_order(DT_SIZE));
> >   	if (!sun50i_domain->dt)
> >   		goto err_free_domain;
> >   
> > @@ -997,7 +998,7 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
> >   
> >   	iommu->pt_pool = kmem_cache_create(dev_name(&pdev->dev),
> >   					   PT_SIZE, PT_SIZE,
> > -					   SLAB_HWCACHE_ALIGN,
> > +					   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA32,
> >   					   NULL);
> >   	if (!iommu->pt_pool)
> >   		return -ENOMEM;  

Re: [PATCH 3/5] dt-bindings: iommu: add new compatible strings

[Krzysztof Kozlowski]

On 31/05/2024 01:37, Andre Przywara wrote:
> The Allwinner H616 and A523 contain IOMMU IP very similar to the H6, but
> use a different reset value for the bypass register, which makes them
> strictly speaking incompatible.
> 
> Add a new compatible string for the H616, and a version for the A523,
> falling back to the H616.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 5/5] arm64: dts: allwinner: h616: add IOMMU node

[Andre Przywara]

On Fri, 31 May 2024 10:45:41 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2024-05-31 10:32 am, Andre Przywara wrote:
> > On Fri, 31 May 2024 09:42:36 +0100
> > Robin Murphy <robin.murphy@arm.com> wrote:
> > 
> > Hi Robin,
> >   
> >> On 2024-05-31 12:38 am, Andre Przywara wrote:  
> >>> The Allwinner H616 contains a scatter-gather IOMMU connected to some
> >>> video related devices. It's almost compatible to the one used in the H6,
> >>> though with minor incompatibilities.
> >>>
> >>> Add the DT node describing its resources, so that devices like the video
> >>> or display engine can connect to it.  
> >>
> >> Without also describing those connections, though, having this node
> >> enabled in the DT means the driver will just bind, block DMA, and
> >> prevent those devices from working. That's probably not what you want.  
> > 
> > The IOMMU manages the Display Engine (DE), the Deinterlacer (DI), the
> > video engine (VE) and the 2D acceleration engine (G2D).
> > None of those devices are supported for the H616 in mainline yet, but there
> > are patches out there for the DE and VE, at least. Especially the video
> > codecs benefit from scatter-gather, so with this patch they can make use
> > of it from day one.  
> 
> Oh, I see - I assumed that stuff like display was more exciting and

Yeah, it arguably is, and Jernej had early patches already two years ago,
I think. However there are some changes in the IP that require some rework
in the DE driver for proper support, IIUC, so the upstreaming is somewhat
on hold.
Of course this didn't prevent Armbian from taking those early patches
anyway, so a lot of people are not even aware of the problem :-(

Cheers,
Andre

> likely to be supported already, so read "can connect to it" in the 
> present tense. If it's a future "can use it as soon as they are also 
> supported", then there's no concern, and indeed this is arguably the 
> ideal order of doing things.
> 
> Cheers,
> Robin.

Re: [PATCH 2/5] iommu: sun50i: allocate page tables from below 4 GiB

[Robin Murphy]

On 2024-05-31 11:02 am, Andre Przywara wrote:
> On Fri, 31 May 2024 09:37:02 +0100
> Robin Murphy <robin.murphy@arm.com> wrote:
> 
> Hi Robin,
> 
>> On 2024-05-31 12:37 am, Andre Przywara wrote:
>>> The Allwinner IOMMU is a strict 32-bit device, with the page table root
>>> pointer as well as both level's page tables and also the target addresses
>>> all required to be below 4GB.
>>> The Allwinner H6 SoC only supports 32-bit worth of physical addresses
>>> anyway, so this isn't a problem so far, but the H616 and later SoCs extend
>>> the PA space beyond 32 bit to accommodate more DRAM.
>>> To make sure we stay within the 32-bit PA range required by the IOMMU,
>>> force the memory for the page tables to come from below 4GB. by using
>>> allocations with the DMA32 flag.
>>
>> Uh-oh... what about the output addresses in sun50i_mk_pte()? Limiting
>> its own accesses is OK, but if the IOMMU isn't capable of *mapping* any
>> valid PA for its clients, we can't easily support that.
> 
> Right, that's indeed a problem. I was hoping that the DMA32 address limit
> would somehow be enforced by the IOMMU master devices, so they would never
> issue addresses above 4GB to the IOMMU in the first place.
> Would this work if all those devices use a 32-bit DMA mask? Some of those
> devices might have that limit anyways, but those video devices are not
> my expertise, so I don't know much details.

It's fine to have a 32-bit *input* to the IOMMU - that only affects the 
IOVA allocation, wherein iommu-dma already considers both the IOMMU 
domain geometry and the client device's DMA mask to ensure it gives back 
a usable DMA address. Indeed, plumbing 32-bit devices into a system with 
a >32-bit PA space is one of the common reasons to have an IOMMU, but it 
assumes that IOMMU translations are capable of targeting the entire 
larger PA range.

> IIUC, atm the incoming PA would be masked down to 32-bit, I guess we should have a
> WARN_ONCE() there when this happens?
> The 32-bit limit would only affect boards with exactly 4GB of DRAM (the
> DRAM controller limit), and it only affects the last GB then, so using
> DMA32 wouldn't be a terrible limitation, I think.

The problem is when a client driver does, say, a dma_map_single() of 
some kmalloced buffer which it doesn't control, and which already 
happens to be at a >32-bit PA; iommu-dma can't make that work for you. 
At best we'd hope the iommu_map() returns an error and terminally fails 
the whole DMA mapping operation, at worst the IOMMU driver silently 
truncates the PA, maps the wrong memory, and all hell breaks loose :(

Thanks,
Robin.

> TBH, I picked this up from Jernej, so have to refer to him for further
> details.
> 
> Cheers,
> Andre
> 
> P.S. I agree that a 32-bit only IOMMU sounds somewhat stup^Wweird, but
> that's what we have. Maybe we would use it just for the VE only then, where
> it's really helpful to provide the illusion of large physically contiguous
> buffers.
> 
>> Thanks,
>> Robin.
>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>    drivers/iommu/sun50i-iommu.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
>>> index dd3f07384624c..c3244db5ac02f 100644
>>> --- a/drivers/iommu/sun50i-iommu.c
>>> +++ b/drivers/iommu/sun50i-iommu.c
>>> @@ -682,7 +682,8 @@ sun50i_iommu_domain_alloc_paging(struct device *dev)
>>>    	if (!sun50i_domain)
>>>    		return NULL;
>>>    
>>> -	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL, get_order(DT_SIZE));
>>> +	sun50i_domain->dt = iommu_alloc_pages(GFP_KERNEL | GFP_DMA32,
>>> +					      get_order(DT_SIZE));
>>>    	if (!sun50i_domain->dt)
>>>    		goto err_free_domain;
>>>    
>>> @@ -997,7 +998,7 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
>>>    
>>>    	iommu->pt_pool = kmem_cache_create(dev_name(&pdev->dev),
>>>    					   PT_SIZE, PT_SIZE,
>>> -					   SLAB_HWCACHE_ALIGN,
>>> +					   SLAB_HWCACHE_ALIGN | SLAB_CACHE_DMA32,
>>>    					   NULL);
>>>    	if (!iommu->pt_pool)
>>>    		return -ENOMEM;
>