iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Apple M1 DART IOMMU driver
@ 2021-03-20 15:19 Sven Peter via iommu
  2021-03-20 15:19 ` [PATCH 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter via iommu
                   ` (3 more replies)
  0 siblings, 4 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-20 15:19 UTC (permalink / raw)
  To: iommu
  Cc: Arnd Bergmann, devicetree, Will Deacon, Hector Martin,
	linux-kernel, Rob Herring, Marc Zyngier, Mohamed Mediouni,
	Stan Skowronek, Robin Murphy, linux-arm-kernel, Mark Kettenis

Hi,

After Hector's initial work [1] to bring up Linux on Apple's M1 it's time to
bring up more devices. Most peripherals connected to the SoC are behind a iommu
which Apple calls "Device Address Resolution Table", or DART for short [2].
Unfortunately, it only shares the name with PowerPC's DART.
Configuring this iommu is mandatory if these peripherals require DMA access.

This patchset implements initial support for this iommu. The hardware itself
uses a pagetable format that's very similar to the one already implement in
io-pgtable.c. There are some minor modifications, namely some details of the
PTE format and that there are always three pagetable levels, which I've
implement as a new format variant.

I have mainly tested this with the USB controller in device mode which is
compatible with Linux's dwc3 driver. Some custom PHY initialization (which is
not yet ready or fully understood) is required though to bring up the ports,
see e.g. my patches to our m1n1 bootloader [3,4]. If you want to test the same
setup you will probably need that branch for now and add the nodes from
the DT binding specification example to your device tree.

Even though each DART instances could support up to 16 devices usually only
a single device is actually connected. Different devices generally just use
an entirely separate DART instance with a seperate MMIO range, IRQ, etc.

I have just noticed today though that at least the USB DWC3 controller in host
mode uses *two* darts at the same time. I'm not sure yet which parts seem to
require which DART instance.

This means that we might need to support devices attached to two iommus
simultaneously and just create the same iova mappings. Currently this only
seems to be required for USB according to Apple's Device Tree.

I see two options for this and would like to get feedback before
I implement either one:

    1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
       to identify the DART and the second one to identify the master.
       The DART DT node would then also take two register ranges that would
       correspond to the two DARTs. Both instances use the same IRQ and the
       same clocks according to Apple's device tree and my experiments.
       This would keep a single device node and the DART driver would then
       simply map iovas in both DARTs if required.

    2) Keep #iommu-cells as-is but support
            iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
       instead.
       This would then require two devices nodes for the two DART instances and
       some housekeeping in the DART driver to support mapping iovas in both
       DARTs.
       I believe omap-iommu.c supports this setup but I will have to read
       more code to understand the details there and figure out how to implement
       this in a sane way.

I currently prefer the first option but I don't understand enough details of
the iommu system to actually make an informed decision.
I'm obviously also open to more options :-)


Best regards,


Sven

[1] https://lore.kernel.org/linux-arch/20210304213902.83903-1-marcan@marcan.st/
[2] https://developer.apple.com/library/archive/documentation/DeviceDrivers/Conceptual/IOKitFundamentals/DataMgmt/DataMgmt.html
[3] https://github.com/svenpeter42/m1n1/commit/1e2661abf5ea2c820297b3ff591235c408d19a34
[4] https://github.com/svenpeter42/m1n1/tree/usb-uartproxy-console-wip



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH 1/3] iommu: io-pgtable: add DART pagetable format
  2021-03-20 15:19 [PATCH 0/3] Apple M1 DART IOMMU driver Sven Peter via iommu
@ 2021-03-20 15:19 ` Sven Peter via iommu
  2021-03-24 16:37   ` Robin Murphy
  2021-03-20 15:20 ` [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings Sven Peter via iommu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-20 15:19 UTC (permalink / raw)
  To: iommu
  Cc: Arnd Bergmann, devicetree, Will Deacon, Hector Martin,
	linux-kernel, Rob Herring, Sven Peter, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, linux-arm-kernel,
	Mark Kettenis

Apple's DART iommu uses a pagetable format that's very similar to the ones
already implemented by io-pgtable.c.
Add a new format variant to support the required differences.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 drivers/iommu/Kconfig          | 13 +++++++
 drivers/iommu/io-pgtable-arm.c | 70 ++++++++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c     |  3 ++
 include/linux/io-pgtable.h     |  6 +++
 4 files changed, 92 insertions(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 192ef8f61310..3c95c8524abe 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -39,6 +39,19 @@ config IOMMU_IO_PGTABLE_LPAE
 	  sizes at both stage-1 and stage-2, as well as address spaces
 	  up to 48-bits in size.

+config IOMMU_IO_PGTABLE_APPLE_DART
+	bool "Apple DART Descriptor Format"
+	select IOMMU_IO_PGTABLE
+	select IOMMU_IO_PGTABLE_LPAE
+	depends on ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64)
+	help
+	  Enable support for the Apple DART iommu pagetable format.
+	  This format is a variant of the ARMv7/v8 Long Descriptor
+	  Format specific to Apple's iommu found in their SoCs.
+
+	  Say Y here if you have a Apple SoC like the M1 which
+	  contains DART iommus.
+
 config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 	bool "LPAE selftests"
 	depends on IOMMU_IO_PGTABLE_LPAE
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 87def58e79b5..18674469313d 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -127,6 +127,10 @@
 #define ARM_MALI_LPAE_MEMATTR_IMP_DEF	0x88ULL
 #define ARM_MALI_LPAE_MEMATTR_WRITE_ALLOC 0x8DULL

+/* APPLE_DART_PTE_PROT_NO_WRITE actually maps to ARM_LPAE_PTE_AP_RDONLY  */
+#define APPLE_DART_PTE_PROT_NO_WRITE (1<<7)
+#define APPLE_DART_PTE_PROT_NO_READ (1<<8)
+
 /* IOPTE accessors */
 #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))

@@ -381,6 +385,17 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 {
 	arm_lpae_iopte pte;

+#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
+	if (data->iop.fmt == ARM_APPLE_DART) {
+		pte = 0;
+		if (!(prot & IOMMU_WRITE))
+			pte |= APPLE_DART_PTE_PROT_NO_WRITE;
+		if (!(prot & IOMMU_READ))
+			pte |= APPLE_DART_PTE_PROT_NO_READ;
+		return pte;
+	}
+#endif
+
 	if (data->iop.fmt == ARM_64_LPAE_S1 ||
 	    data->iop.fmt == ARM_32_LPAE_S1) {
 		pte = ARM_LPAE_PTE_nG;
@@ -1043,6 +1058,54 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
 	return NULL;
 }

+#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
+static struct io_pgtable *
+apple_dart_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
+{
+	struct arm_lpae_io_pgtable *data;
+
+	if (cfg->ias > 38)
+		return NULL;
+	if (cfg->oas > 36)
+		return NULL;
+
+	if (!cfg->coherent_walk)
+		return NULL;
+
+	cfg->pgsize_bitmap &= SZ_16K;
+	if (!cfg->pgsize_bitmap)
+		return NULL;
+
+	data = arm_lpae_alloc_pgtable(cfg);
+	if (!data)
+		return NULL;
+
+	/*
+	 * the hardware only supports this specific three level pagetable layout with
+	 * the first level being encoded into four hardware registers
+	 */
+	data->start_level = ARM_LPAE_MAX_LEVELS - 2;
+	data->pgd_bits = 13;
+	data->bits_per_level = 11;
+
+	data->pgd = __arm_lpae_alloc_pages(ARM_LPAE_PGD_SIZE(data), GFP_KERNEL,
+					   cfg);
+	if (!data->pgd)
+		goto out_free_data;
+
+	cfg->apple_dart_cfg.pgd[0] = virt_to_phys(data->pgd);
+	cfg->apple_dart_cfg.pgd[1] = virt_to_phys(data->pgd + 0x4000);
+	cfg->apple_dart_cfg.pgd[2] = virt_to_phys(data->pgd + 0x8000);
+	cfg->apple_dart_cfg.pgd[3] = virt_to_phys(data->pgd + 0xc000);
+
+	return &data->iop;
+
+out_free_data:
+	kfree(data);
+	return NULL;
+}
+#endif
+
 struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
 	.alloc	= arm_64_lpae_alloc_pgtable_s1,
 	.free	= arm_lpae_free_pgtable,
@@ -1068,6 +1131,13 @@ struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
 	.free	= arm_lpae_free_pgtable,
 };

+#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
+struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns = {
+	.alloc	= apple_dart_alloc_pgtable,
+	.free	= arm_lpae_free_pgtable,
+};
+#endif
+
 #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST

 static struct io_pgtable_cfg *cfg_cookie __initdata;
diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index 6e9917ce980f..d86590b0673a 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -27,6 +27,9 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
 #ifdef CONFIG_AMD_IOMMU
 	[AMD_IOMMU_V1] = &io_pgtable_amd_iommu_v1_init_fns,
 #endif
+#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
+	[ARM_APPLE_DART] = &io_pgtable_apple_dart_init_fns,
+#endif
 };

 struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index a4c9ca2c31f1..19d9b631d319 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -16,6 +16,7 @@ enum io_pgtable_fmt {
 	ARM_V7S,
 	ARM_MALI_LPAE,
 	AMD_IOMMU_V1,
+	ARM_APPLE_DART,
 	IO_PGTABLE_NUM_FMTS,
 };

@@ -136,6 +137,10 @@ struct io_pgtable_cfg {
 			u64	transtab;
 			u64	memattr;
 		} arm_mali_lpae_cfg;
+
+		struct {
+			u64 pgd[4];
+		} apple_dart_cfg;
 	};
 };

@@ -250,5 +255,6 @@ extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_amd_iommu_v1_init_fns;
+extern struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns;

 #endif /* __IO_PGTABLE_H */
--
2.25.1


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings
  2021-03-20 15:19 [PATCH 0/3] Apple M1 DART IOMMU driver Sven Peter via iommu
  2021-03-20 15:19 ` [PATCH 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter via iommu
@ 2021-03-20 15:20 ` Sven Peter via iommu
  2021-03-22  0:15   ` Rob Herring
  2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
  2021-03-24 15:29 ` Robin Murphy
  3 siblings, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-20 15:20 UTC (permalink / raw)
  To: iommu
  Cc: Arnd Bergmann, devicetree, Will Deacon, Hector Martin,
	linux-kernel, Rob Herring, Sven Peter, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, linux-arm-kernel,
	Mark Kettenis

DART (Device Address Resolution Table) is the iommu found on Apple
ARM SoCs such as the M1.

Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
 .../bindings/iommu/apple,t8103-dart.yaml      | 82 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml

diff --git a/Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml b/Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml
new file mode 100644
index 000000000000..2ec2d905a2ea
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/apple,t8103-dart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple DART IOMMU Implementation
+
+maintainers:
+  - Sven Peter <sven@svenpeter.dev>
+
+description: |+
+  Apple SoCs may contain an implementation of their Device Address
+  Resolution Table which provides a mandatory layer of address
+  translations for various masters external to the CPU.
+
+  Each DART instance is capable of handling up to 16 masters
+  mapped to up tp 16 different virtual address spaces with
+  page-level read/write access control flags.
+
+  This DART IOMMU also raises interrupts in response to various
+  fault conditions.
+
+properties:
+  compatible:
+    const: apple,t8103-dart
+
+  reg:
+    const: 1
+
+  interrupts:
+    const: 1
+
+  clocks:
+    maxItems: 1
+
+  '#iommu-cells':
+    const: 1
+    description:
+      The number of the master connected to this DART.
+
+required:
+  - compatible
+  - reg
+  - '#iommu-cells'
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |+
+    dart1: dart1@382f80000 {
+      compatible = "apple,t8103-dart";
+      reg = <0x3 0x82f80000 0x0 0x4000>;
+      interrupt-parent = <&aic>;
+      interrupts = <AIC_IRQ 781 IRQ_TYPE_LEVEL_HIGH>;
+      #iommu-cells = <1>;
+    };
+
+    master1 {
+      iommus = <&dart1 0>;
+    };
+
+  - |+
+    usb_dart0: usb_dart0@382f80000 {
+      compatible = "apple,t8103-dart";
+      reg = <0x3 0x82f80000 0x0 0x4000>;
+      interrupt-parent = <&aic>;
+      interrupts = <AIC_IRQ 781 IRQ_TYPE_LEVEL_HIGH>;
+      #iommu-cells = <1>;
+    };
+
+    usbdrd_dwc3_0: dwc3@382280000 {
+      compatible = "snps,dwc3";
+      reg = <0x3 0x82280000 0x0 0x100000>;
+      interrupt-parent = <&aic>;
+      interrupts = <AIC_IRQ 777 IRQ_TYPE_LEVEL_HIGH>;
+      iommus = <&usb_dart0 1>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      dr_mode = "peripheral";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d5e4d93a536a..1f9a4f2de88b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1236,6 +1236,12 @@ L:	linux-input@vger.kernel.org
 S:	Odd fixes
 F:	drivers/input/mouse/bcm5974.c

+APPLE DART IOMMU DRIVER
+M:	Sven Peter <sven@svenpeter.dev>
+L:	iommu@lists.linux-foundation.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml
+
 APPLE SMC DRIVER
 M:	Henrik Rydberg <rydberg@bitmath.org>
 L:	linux-hwmon@vger.kernel.org
--
2.25.1


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-20 15:19 [PATCH 0/3] Apple M1 DART IOMMU driver Sven Peter via iommu
  2021-03-20 15:19 ` [PATCH 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter via iommu
  2021-03-20 15:20 ` [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings Sven Peter via iommu
@ 2021-03-21 16:00 ` Mark Kettenis
  2021-03-21 17:22   ` Sven Peter via iommu
                     ` (2 more replies)
  2021-03-24 15:29 ` Robin Murphy
  3 siblings, 3 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-21 16:00 UTC (permalink / raw)
  To: Sven Peter
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> Date: Sat, 20 Mar 2021 15:19:33 +0000
> From: Sven Peter <sven@svenpeter.dev>
> 
> Hi,
> 
> After Hector's initial work [1] to bring up Linux on Apple's M1 it's time to
> bring up more devices. Most peripherals connected to the SoC are behind a iommu
> which Apple calls "Device Address Resolution Table", or DART for short [2].
> Unfortunately, it only shares the name with PowerPC's DART.
> Configuring this iommu is mandatory if these peripherals require DMA access.
> 
> This patchset implements initial support for this iommu. The hardware itself
> uses a pagetable format that's very similar to the one already implement in
> io-pgtable.c. There are some minor modifications, namely some details of the
> PTE format and that there are always three pagetable levels, which I've
> implement as a new format variant.
> 
> I have mainly tested this with the USB controller in device mode which is
> compatible with Linux's dwc3 driver. Some custom PHY initialization (which is
> not yet ready or fully understood) is required though to bring up the ports,
> see e.g. my patches to our m1n1 bootloader [3,4]. If you want to test the same
> setup you will probably need that branch for now and add the nodes from
> the DT binding specification example to your device tree.
> 
> Even though each DART instances could support up to 16 devices usually only
> a single device is actually connected. Different devices generally just use
> an entirely separate DART instance with a seperate MMIO range, IRQ, etc.
> 
> I have just noticed today though that at least the USB DWC3 controller in host
> mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> require which DART instance.
> 
> This means that we might need to support devices attached to two iommus
> simultaneously and just create the same iova mappings. Currently this only
> seems to be required for USB according to Apple's Device Tree.
> 
> I see two options for this and would like to get feedback before
> I implement either one:
> 
>     1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
>        to identify the DART and the second one to identify the master.
>        The DART DT node would then also take two register ranges that would
>        correspond to the two DARTs. Both instances use the same IRQ and the
>        same clocks according to Apple's device tree and my experiments.
>        This would keep a single device node and the DART driver would then
>        simply map iovas in both DARTs if required.
> 
>     2) Keep #iommu-cells as-is but support
>             iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
>        instead.
>        This would then require two devices nodes for the two DART instances and
>        some housekeeping in the DART driver to support mapping iovas in both
>        DARTs.
>        I believe omap-iommu.c supports this setup but I will have to read
>        more code to understand the details there and figure out how to implement
>        this in a sane way.
> 
> I currently prefer the first option but I don't understand enough details of
> the iommu system to actually make an informed decision.
> I'm obviously also open to more options :-)

Hi Sven,

I don't think the first option is going to work for PCIe.  PCIe
devices will have to use "iommu-map" properties to map PCI devices to
the right iommu, and the currently implementation seems to assume that
#iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
that it relies on #iommu-cells = <1>, but it isn't clear how the
rid-base to iommu-base mapping mechanism would work when that isn't
the case.

Now the PCIe DARTs are simpler and seem to have only one "instance"
per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
fine using the first approach.

As I mentioned before, not all DARTs support the full 32-bit aperture.
In particular the PCIe DARTs support a smaller address-space.  It is
not clear whether this is a restriction of the PCIe host controller or
the DART, but the Apple Device Tree has "vm-base" and "vm-size"
properties that encode the base address and size of the aperture.
These single-cell properties which is probably why for the USB DARTs
only "vm-base" is given; since "vm-base" is 0, a 32-bit number
wouldn't be able to encode the full aperture size.  We could make them
64-bit numbers in the Linux device tree though and always be explicit
about the size.  Older Sun SPARC machines used a single "virtual-dma"
property to encode the aperture.  We could do someting similar.  You
would use this property to initialize domain->geometry.aperture_start
and domain->geometry.aperture_end in diff 3/3 of this series.

I think it would make sense to include this in this series, as this
would make adding support for PCIe very easy, and PCIe gives you
aupport for network (both wired and wireless) and the type-A USB ports
on the mini.

Cheers,

Mark

[1] Documentation/devicetree/bindings/pci/pci-iommu.txt
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
@ 2021-03-21 17:22   ` Sven Peter via iommu
  2021-03-21 18:35     ` Mark Kettenis
  2021-03-21 17:28   ` Sven Peter via iommu
  2021-03-23 20:53   ` Rob Herring
  2 siblings, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-21 17:22 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan


[-- Attachment #1.1: Type: text/plain, Size: 3906 bytes --]


Hi Mark,

> On 21. Mar 2021, at 17:00, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> I don't think the first option is going to work for PCIe.  PCIe
> devices will have to use "iommu-map" properties to map PCI devices to
> the right iommu, and the currently implementation seems to assume that
> #iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
> that it relies on #iommu-cells = <1>, but it isn't clear how the
> rid-base to iommu-base mapping mechanism would work when that isn't
> the case.
> 
> Now the PCIe DARTs are simpler and seem to have only one "instance"
> per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
> fine using the first approach.

Good point, I guess that only leaves option two for now then.
Having some DARTs use cells = <1> and others <2> sounds confusing to me.


> 
> As I mentioned before, not all DARTs support the full 32-bit aperture.
> In particular the PCIe DARTs support a smaller address-space.  It is
> not clear whether this is a restriction of the PCIe host controller or
> the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> properties that encode the base address and size of the aperture.
> These single-cell properties which is probably why for the USB DARTs
> only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> wouldn't be able to encode the full aperture size.  We could make them
> 64-bit numbers in the Linux device tree though and always be explicit
> about the size.  Older Sun SPARC machines used a single "virtual-dma"
> property to encode the aperture.  We could do someting similar.  You
> would use this property to initialize domain->geometry.aperture_start
> and domain->geometry.aperture_end in diff 3/3 of this series.
> 
> I think it would make sense to include this in this series, as this
> would make adding support for PCIe very easy, and PCIe gives you
> aupport for network (both wired and wireless) and the type-A USB ports
> on the mini.



Agreed, I'd ideally like to converge on a device tree binding
that won't have to change in the near future.

I've tried to use an address space larger than 32bit and that seems to
work for parts of the dwc3 controller but breaks for the xhci parts because
the upper lines don't seem to be connected there (e.g. if xhci tries to
use <0x1 0xffff0000> I get a fault for <0 0xffff0000>).

Looking at other iommu drivers I have found the following two similar
bindings:

qcom uses a ranges property with a 64bit address and 32 bit size [1]

  apps_iommu: iommu@1e20000 {
      ...
      ranges = <0 0x1e20000 0x40000>;
      ...
  };

and tegra seems to support a dma-window property with 32bit address
and size [2]

  smmu {
          [...]
          dma-window = <0 0x40000000>;    /* IOVA start & length */
          [...]
  };

I believe there already is of_get_dma_window to handle parsing this
in the common iommu code [3] but I can't find any place using it.
It's a little bit more complex that we need since it allows to specify the
number of cells for both the address and the size but it should allow us to
express all possible configurations:

  usb_dart {
      [ ... ]
      #dma-address-cells = <1>;
      #dma-size-cells = <2>;
      dma-window = <0 0x1 0x0>;
      [ ... ]
  };
  pcie_dart {
      [ ... ]
      #dma-address-cells = <1>;
      #dma-size-cells = <1>;
      dma-window = <0x100000 0x3fe00000>;
      [ ... ]
  };

where #dma-address-cells and #dma-size-cells default to
#address-cells and #size-cells respectively if I understand
the code correctly. That way we could also just always use
a 64bit address and size in the DT, e.g.

  pcie_dart {
      [ ... ]
      dma-window = <0 0x100000 0 0x3fe00000>;
      [ ... ]
  };


Best,

Sven


[1] Documentation/devicetree/bindings/iommu/qcom,iommu.txt
[2] Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
[3] drivers/iommu/of_iommu.c

[-- Attachment #1.2: Type: text/html, Size: 6938 bytes --]

[-- Attachment #2: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
  2021-03-21 17:22   ` Sven Peter via iommu
@ 2021-03-21 17:28   ` Sven Peter via iommu
  2021-03-23 20:53   ` Rob Herring
  2 siblings, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-21 17:28 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan



Hi Mark,

Sorry for the spam if you get this message twice. This is pretty embarrassing
but I've just switched mail providers after ProtonMail messed up yesterday and
it looks like my new one defaulted to sending HTML messages even though I only
typed plaintext. This shouldn't have happened in the first place but it
certainly shouldn't happen again now :-(

> On 21. Mar 2021, at 17:00, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> I don't think the first option is going to work for PCIe.  PCIe
> devices will have to use "iommu-map" properties to map PCI devices to
> the right iommu, and the currently implementation seems to assume that
> #iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
> that it relies on #iommu-cells = <1>, but it isn't clear how the
> rid-base to iommu-base mapping mechanism would work when that isn't
> the case.
> 
> Now the PCIe DARTs are simpler and seem to have only one "instance"
> per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
> fine using the first approach.

Good point, I guess that only leaves option two for now then.
Having some DARTs use cells = <1> and others <2> sounds confusing to me.


> 
> As I mentioned before, not all DARTs support the full 32-bit aperture.
> In particular the PCIe DARTs support a smaller address-space.  It is
> not clear whether this is a restriction of the PCIe host controller or
> the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> properties that encode the base address and size of the aperture.
> These single-cell properties which is probably why for the USB DARTs
> only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> wouldn't be able to encode the full aperture size.  We could make them
> 64-bit numbers in the Linux device tree though and always be explicit
> about the size.  Older Sun SPARC machines used a single "virtual-dma"
> property to encode the aperture.  We could do someting similar.  You
> would use this property to initialize domain->geometry.aperture_start
> and domain->geometry.aperture_end in diff 3/3 of this series.
> 
> I think it would make sense to include this in this series, as this
> would make adding support for PCIe very easy, and PCIe gives you
> aupport for network (both wired and wireless) and the type-A USB ports
> on the mini.



Agreed, I'd ideally like to converge on a device tree binding
that won't have to change in the near future.

I've tried to use an address space larger than 32bit and that seems to
work for parts of the dwc3 controller but breaks for the xhci parts because
the upper lines don't seem to be connected there (e.g. if xhci tries to
use <0x1 0xffff0000> I get a fault for <0 0xffff0000>).

Looking at other iommu drivers I have found the following two similar
bindings:

qcom uses a ranges property with a 64bit address and 32 bit size [1]

  apps_iommu: iommu@1e20000 {
      ...
      ranges = <0 0x1e20000 0x40000>;
      ...
  };

and tegra seems to support a dma-window property with 32bit address
and size [2]

  smmu {
          [...]
          dma-window = <0 0x40000000>;    /* IOVA start & length */
          [...]
  };

I believe there already is of_get_dma_window to handle parsing this
in the common iommu code [3] but I can't find any place using it.
It's a little bit more complex that we need since it allows to specify the
number of cells for both the address and the size but it should allow us to
express all possible configurations:

  usb_dart {
      [ ... ]
      #dma-address-cells = <1>;
      #dma-size-cells = <2>;
      dma-window = <0 0x1 0x0>;
      [ ... ]
  };
  pcie_dart {
      [ ... ]
      #dma-address-cells = <1>;
      #dma-size-cells = <1>;
      dma-window = <0x100000 0x3fe00000>;
      [ ... ]
  };

where #dma-address-cells and #dma-size-cells default to
#address-cells and #size-cells respectively if I understand
the code correctly. That way we could also just always use
a 64bit address and size in the DT, e.g.

  pcie_dart {
      [ ... ]
      dma-window = <0 0x100000 0 0x3fe00000>;
      [ ... ]
  };


Best,

Sven


[1] Documentation/devicetree/bindings/iommu/qcom,iommu.txt
[2] Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt
[3] drivers/iommu/of_iommu.c
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-21 17:22   ` Sven Peter via iommu
@ 2021-03-21 18:35     ` Mark Kettenis
  2021-03-22 22:17       ` Sven Peter via iommu
  0 siblings, 1 reply; 36+ messages in thread
From: Mark Kettenis @ 2021-03-21 18:35 UTC (permalink / raw)
  To: Sven Peter
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> Date: Sun, 21 Mar 2021 18:22:15 +0100
> From: "Sven Peter" <sven@svenpeter.dev>
> 
> Hi Mark,
> 
>  On 21. Mar 2021, at 17:00, Mark Kettenis <mark.kettenis@xs4all.nl>
>  wrote:
> 
>  I don't think the first option is going to work for PCIe.  PCIe
>  devices will have to use "iommu-map" properties to map PCI devices to
>  the right iommu, and the currently implementation seems to assume that
>  #iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
>  that it relies on #iommu-cells = <1>, but it isn't clear how the
>  rid-base to iommu-base mapping mechanism would work when that isn't
>  the case.
> 
>  Now the PCIe DARTs are simpler and seem to have only one "instance"
>  per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
>  fine using the first approach.
> 
> Good point, I guess that only leaves option two for now then.
> Having some DARTs use cells = <1> and others <2> sounds confusing to me.

Guess we do need to understand a little bit better how the USB DART
actually works.  My hypothesis (based on our discussion on #asahi) is
that the XHCI host controller and the peripheral controller of the
DWC3 block use different DMA "streams" that are handled by the
different sub-DARTs.

The Corellium folks use a DART + sub-DART model in their driver and a
single node in the device tree that represents both.  That might sense
since the error registers and interrupts are shared.  Maybe it would
make sense to select the appropriate sub-DART based on the DMA stream
ID?

>  As I mentioned before, not all DARTs support the full 32-bit aperture.
>  In particular the PCIe DARTs support a smaller address-space.  It is
>  not clear whether this is a restriction of the PCIe host controller or
>  the DART, but the Apple Device Tree has "vm-base" and "vm-size"
>  properties that encode the base address and size of the aperture.
>  These single-cell properties which is probably why for the USB DARTs
>  only "vm-base" is given; since "vm-base" is 0, a 32-bit number
>  wouldn't be able to encode the full aperture size.  We could make them
>  64-bit numbers in the Linux device tree though and always be explicit
>  about the size.  Older Sun SPARC machines used a single "virtual-dma"
>  property to encode the aperture.  We could do someting similar.  You
>  would use this property to initialize domain->geometry.aperture_start
>  and domain->geometry.aperture_end in diff 3/3 of this series.
> 
>  I think it would make sense to include this in this series, as this
>  would make adding support for PCIe very easy, and PCIe gives you
>  aupport for network (both wired and wireless) and the type-A USB ports
>  on the mini.
> 
> Agreed, I'd ideally like to converge on a device tree binding
> that won't have to change in the near future.
> 
> I've tried to use an address space larger than 32bit and that seems to
> work for parts of the dwc3 controller but breaks for the xhci parts because
> the upper lines don't seem to be connected there (e.g. if xhci tries to
> use <0x1 0xffff0000> I get a fault for <0 0xffff0000>).
> 
> Looking at other iommu drivers I have found the following two similar
> bindings:

Ah, missed those.

> qcom uses a ranges property with a 64bit address and 32 bit size [1]
> 
>   apps_iommu: iommu@1e20000 {
>       ...
>       ranges = <0 0x1e20000 0x40000>;
>       ...
>   };

Doesn't sound like a very good idea to me to repurpose "ranges" for
this...

> 
> and tegra seems to support a dma-window property with 32bit address
> and size [2]
> 
>   smmu {
>           [...]
>           dma-window = <0 0x40000000>;    /* IOVA start & length */
>           [...]
>   };
> 
> I believe there already is of_get_dma_window to handle parsing this
> in the common iommu code [3] but I can't find any place using it.
> It's a little bit more complex that we need since it allows to specify the
> number of cells for both the address and the size but it should allow us to
> express all possible configurations:
> 
>   usb_dart {
>       [ ... ]
>       #dma-address-cells = <1>;
>       #dma-size-cells = <2>;
>       dma-window = <0 0x1 0x0>;
>       [ ... ]
>   };
>   pcie_dart {
>       [ ... ]
>       #dma-address-cells = <1>;
>       #dma-size-cells = <1>;
>       dma-window = <0x100000 0x3fe00000>;
>       [ ... ]
>   };
> 
> where #dma-address-cells and #dma-size-cells default to
> #address-cells and #size-cells respectively if I understand
> the code correctly. That way we could also just always use
> a 64bit address and size in the DT, e.g.
> 
>   pcie_dart {
>       [ ... ]
>       dma-window = <0 0x100000 0 0x3fe00000>;
>       [ ... ]
>   };

That sounds like a serious contender to me!  Hopefully one of the
Linux kernel developers can give this some sort of blessing.

I think it would make sense for us to just rely on the #address-cells
and #size-cells defaults for the M1 device tree.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings
  2021-03-20 15:20 ` [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings Sven Peter via iommu
@ 2021-03-22  0:15   ` Rob Herring
  2021-03-22 18:16     ` Sven Peter via iommu
  0 siblings, 1 reply; 36+ messages in thread
From: Rob Herring @ 2021-03-22  0:15 UTC (permalink / raw)
  To: Sven Peter
  Cc: devicetree, Will Deacon, Arnd Bergmann, Marc Zyngier,
	Hector Martin, linux-kernel, iommu, Rob Herring,
	Mohamed Mediouni, Mark Kettenis, Robin Murphy, linux-arm-kernel,
	Stan Skowronek

On Sat, 20 Mar 2021 15:20:08 +0000, Sven Peter wrote:
> DART (Device Address Resolution Table) is the iommu found on Apple
> ARM SoCs such as the M1.
> 
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>  .../bindings/iommu/apple,t8103-dart.yaml      | 82 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 88 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/apple,t8103-dart.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iommu/apple,t8103-dart.example.dts:23.25-26 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/iommu/apple,t8103-dart.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1456122

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings
  2021-03-22  0:15   ` Rob Herring
@ 2021-03-22 18:16     ` Sven Peter via iommu
  0 siblings, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-22 18:16 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, Will Deacon, Arnd Bergmann, Marc Zyngier,
	Hector Martin, linux-kernel, iommu, Rob Herring,
	Mohamed Mediouni, Mark Kettenis, Robin Murphy, linux-arm-kernel,
	Stan Skowronek

Hi Rob,

On Mon, Mar 22, 2021, at 01:15, Rob Herring wrote:
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.

Sorry about that! It looks like I didn't have yamllint installed.
I have fixed the issues and will re-submit.


Thanks,

Sven

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-21 18:35     ` Mark Kettenis
@ 2021-03-22 22:17       ` Sven Peter via iommu
  2021-03-23 20:00         ` Mark Kettenis
  0 siblings, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-22 22:17 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan


Hi Mark,

On Sun, Mar 21, 2021, at 19:35, Mark Kettenis wrote:
>
> Guess we do need to understand a little bit better how the USB DART
> actually works.  My hypothesis (based on our discussion on #asahi) is
> that the XHCI host controller and the peripheral controller of the
> DWC3 block use different DMA "streams" that are handled by the
> different sub-DARTs.

I've done some more experiments and the situation is unfortunately more
complicated: Most DMA transfers are translated with the first DART.
But sometimes (and I have not been able to figure out the exact conditions)
transfers instead have to go through the second DART. 
This happens e.g. with one of my USB keyboards after a stop EP command
is issued: Suddenly the xhci_ep_ctx struct must be translated through the
second DART.

What this likely means is that we'll need to point both DARTs
to the same pagetables and just issue the TLB maintenance operations
as a group.

> 
> The Corellium folks use a DART + sub-DART model in their driver and a
> single node in the device tree that represents both.  That might sense
> since the error registers and interrupts are shared.  Maybe it would
> make sense to select the appropriate sub-DART based on the DMA stream
> ID?

dwc3 specifically seems to require stream id #1 from the DART
at <0x5 0x02f00000> and stream id #0 from the DART at <0x5 0x02f80000>.
Both of these only share a IRQ line but are otherwise completely independent.
Each has their own error registers, etc. and we need some way to
specify these two DARTs + the appropriate stream ID.

Essentially we have three options to represent this now:

1) Add both DARTs as separate regs, use #iommu-cells = <2> and have the
   first cell select the DART and the second one the stream ID.
   We could allow #iommu-cells = <1> in case only one reg is specified
   for the PCIe DART:

   usb_dart1@502f00000 {
     compatible = "apple,t8103-dart";
     reg = <0x5 0x02f00000 0x0 0x4000>, <0x5 0x02f80000 0x0 0x4000>;
     #iommu-cells = <2>;
     ...
   };

   usb1 {
     iommus = <&usb_dart1 0 1>, <&usb_dart1 1 0>;
     ...
   };

   I prefer this option because we fully describe the DART in a single
   device node here. It also feels natural to group them like this because
   they need to share some properties (like dma-window and the interrupt)
   anyway. 

2) Create two DART nodes which share the same IRQ line and attach them
   both to the master node:

   usb_dart1a@502f00000 {
     compatible = "apple,t8103-dart";
     reg = <0x5 0x02f00000 0x0 0x4000>;
     #iommu-cells = <1>;
     ...
   };
   usb_dart1b@502f80000 {
     compatible = "apple,t8103-dart";
     reg = <0x5 0x02f80000 0x0 0x4000>;
     #iommu-cells = <1>;
     ...
   };

   usb1 {
     iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
     ...
   };

   I dislike this one because attaching those two DARTs to a single device
   seems rather unusual. We'd also have to duplicate the dma-window setting,
   make sure it's the same for both DARTs and there are probably even more
   complications I can't think of right now. It seems like this would also
   make the device tree very verbose and the implementation itself more
   complicated.

3) Introduce another property and let the DART driver take care of
   mirroring the pagetables. I believe this would be similar to
   the sid-remap property:

   usb_dart1@502f00000 {
     compatible = "apple,t8103-dart";
     reg = <0x5 0x02f00000 0x0 0x4000>, <0x5 0x02f80000 0x0 0x4000>;
     #iommu-cells = <1>;
     sid-remap = <0 1>;
   };
   usb1 {
     iommus = <&usb_dart1 0>;
   };

   I slightly dislike this one because we now specify which stream id 
   to use in two places: Once in the device node and another time in the
   new property in the DART node. I also don't think the binding is much
   simpler than the first one.


> > where #dma-address-cells and #dma-size-cells default to
> > #address-cells and #size-cells respectively if I understand
> > the code correctly. That way we could also just always use
> > a 64bit address and size in the DT, e.g.
> > 
> >   pcie_dart {
> >       [ ... ]
> >       dma-window = <0 0x100000 0 0x3fe00000>;
> >       [ ... ]
> >   };
> 
> That sounds like a serious contender to me!  Hopefully one of the
> Linux kernel developers can give this some sort of blessing.
> 
> I think it would make sense for us to just rely on the #address-cells
> and #size-cells defaults for the M1 device tree.
>

Agreed.


Best,

Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-22 22:17       ` Sven Peter via iommu
@ 2021-03-23 20:00         ` Mark Kettenis
  2021-03-23 21:03           ` Sven Peter via iommu
  0 siblings, 1 reply; 36+ messages in thread
From: Mark Kettenis @ 2021-03-23 20:00 UTC (permalink / raw)
  To: Sven Peter
  Cc: arnd, devicetree, will, marcan, linux-kernel, iommu, robh+dt,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> Date: Mon, 22 Mar 2021 23:17:31 +0100
> From: "Sven Peter" <sven@svenpeter.dev>
> 
> Hi Mark,
> 
> On Sun, Mar 21, 2021, at 19:35, Mark Kettenis wrote:
> >
> > Guess we do need to understand a little bit better how the USB DART
> > actually works.  My hypothesis (based on our discussion on #asahi) is
> > that the XHCI host controller and the peripheral controller of the
> > DWC3 block use different DMA "streams" that are handled by the
> > different sub-DARTs.
> 
> I've done some more experiments and the situation is unfortunately more
> complicated: Most DMA transfers are translated with the first DART.
> But sometimes (and I have not been able to figure out the exact conditions)
> transfers instead have to go through the second DART. 
> This happens e.g. with one of my USB keyboards after a stop EP command
> is issued: Suddenly the xhci_ep_ctx struct must be translated through the
> second DART.
> 
> What this likely means is that we'll need to point both DARTs
> to the same pagetables and just issue the TLB maintenance operations
> as a group.
> 
> > 
> > The Corellium folks use a DART + sub-DART model in their driver and a
> > single node in the device tree that represents both.  That might sense
> > since the error registers and interrupts are shared.  Maybe it would
> > make sense to select the appropriate sub-DART based on the DMA stream
> > ID?
> 
> dwc3 specifically seems to require stream id #1 from the DART
> at <0x5 0x02f00000> and stream id #0 from the DART at <0x5 0x02f80000>.
> Both of these only share a IRQ line but are otherwise completely independent.
> Each has their own error registers, etc. and we need some way to
> specify these two DARTs + the appropriate stream ID.
> 
> Essentially we have three options to represent this now:
> 
> 1) Add both DARTs as separate regs, use #iommu-cells = <2> and have the
>    first cell select the DART and the second one the stream ID.
>    We could allow #iommu-cells = <1> in case only one reg is specified
>    for the PCIe DART:
> 
>    usb_dart1@502f00000 {
>      compatible = "apple,t8103-dart";
>      reg = <0x5 0x02f00000 0x0 0x4000>, <0x5 0x02f80000 0x0 0x4000>;
>      #iommu-cells = <2>;
>      ...
>    };
> 
>    usb1 {
>      iommus = <&usb_dart1 0 1>, <&usb_dart1 1 0>;
>      ...
>    };
> 
>    I prefer this option because we fully describe the DART in a single
>    device node here. It also feels natural to group them like this because
>    they need to share some properties (like dma-window and the interrupt)
>    anyway. 
> 
> 2) Create two DART nodes which share the same IRQ line and attach them
>    both to the master node:
> 
>    usb_dart1a@502f00000 {
>      compatible = "apple,t8103-dart";
>      reg = <0x5 0x02f00000 0x0 0x4000>;
>      #iommu-cells = <1>;
>      ...
>    };
>    usb_dart1b@502f80000 {
>      compatible = "apple,t8103-dart";
>      reg = <0x5 0x02f80000 0x0 0x4000>;
>      #iommu-cells = <1>;
>      ...
>    };
> 
>    usb1 {
>      iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
>      ...
>    };
> 
>    I dislike this one because attaching those two DARTs to a single device
>    seems rather unusual. We'd also have to duplicate the dma-window setting,
>    make sure it's the same for both DARTs and there are probably even more
>    complications I can't think of right now. It seems like this would also
>    make the device tree very verbose and the implementation itself more
>    complicated.
> 
> 3) Introduce another property and let the DART driver take care of
>    mirroring the pagetables. I believe this would be similar to
>    the sid-remap property:
> 
>    usb_dart1@502f00000 {
>      compatible = "apple,t8103-dart";
>      reg = <0x5 0x02f00000 0x0 0x4000>, <0x5 0x02f80000 0x0 0x4000>;
>      #iommu-cells = <1>;
>      sid-remap = <0 1>;
>    };
>    usb1 {
>      iommus = <&usb_dart1 0>;
>    };
> 
>    I slightly dislike this one because we now specify which stream id 
>    to use in two places: Once in the device node and another time in the
>    new property in the DART node. I also don't think the binding is much
>    simpler than the first one.

Hi Sven,

The problem with both #1 and #2 is that you end up with two references
to (effectively) different iommu's in the dwc3 device node.  I don't
see how that is compatible with the idea of using a single translation
table for both sub-DARTs.

If you no longer think that is desirable, you'll still have the
problem that you'll need to modify the dwc3 driver code such that it
uses the right IOMMU to do its DMA address translation.  Given what
you write above that sounds really ugly and confusing.  I would
certainly want to avoid doing that in OpenBSD.

So I think #3 is the only realistic option here.  In my opinion it is
perfectly fine for the devicetree to present a workable model of the
hardware instead of a 100% accurate model.

> > > where #dma-address-cells and #dma-size-cells default to
> > > #address-cells and #size-cells respectively if I understand
> > > the code correctly. That way we could also just always use
> > > a 64bit address and size in the DT, e.g.
> > > 
> > >   pcie_dart {
> > >       [ ... ]
> > >       dma-window = <0 0x100000 0 0x3fe00000>;
> > >       [ ... ]
> > >   };
> > 
> > That sounds like a serious contender to me!  Hopefully one of the
> > Linux kernel developers can give this some sort of blessing.
> > 
> > I think it would make sense for us to just rely on the #address-cells
> > and #size-cells defaults for the M1 device tree.
> >
> 
> Agreed.
> 
> 
> Best,
> 
> Sven
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
  2021-03-21 17:22   ` Sven Peter via iommu
  2021-03-21 17:28   ` Sven Peter via iommu
@ 2021-03-23 20:53   ` Rob Herring
  2021-03-23 22:33     ` Mark Kettenis
  2021-03-25  7:53     ` Sven Peter via iommu
  2 siblings, 2 replies; 36+ messages in thread
From: Rob Herring @ 2021-03-23 20:53 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: arnd, Sven Peter, devicetree, will, marcan, linux-kernel, iommu,
	maz, mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote:
> > Date: Sat, 20 Mar 2021 15:19:33 +0000
> > From: Sven Peter <sven@svenpeter.dev>
> > 
> > Hi,
> > 
> > After Hector's initial work [1] to bring up Linux on Apple's M1 it's time to
> > bring up more devices. Most peripherals connected to the SoC are behind a iommu
> > which Apple calls "Device Address Resolution Table", or DART for short [2].
> > Unfortunately, it only shares the name with PowerPC's DART.
> > Configuring this iommu is mandatory if these peripherals require DMA access.
> > 
> > This patchset implements initial support for this iommu. The hardware itself
> > uses a pagetable format that's very similar to the one already implement in
> > io-pgtable.c. There are some minor modifications, namely some details of the
> > PTE format and that there are always three pagetable levels, which I've
> > implement as a new format variant.
> > 
> > I have mainly tested this with the USB controller in device mode which is
> > compatible with Linux's dwc3 driver. Some custom PHY initialization (which is
> > not yet ready or fully understood) is required though to bring up the ports,
> > see e.g. my patches to our m1n1 bootloader [3,4]. If you want to test the same
> > setup you will probably need that branch for now and add the nodes from
> > the DT binding specification example to your device tree.
> > 
> > Even though each DART instances could support up to 16 devices usually only
> > a single device is actually connected. Different devices generally just use
> > an entirely separate DART instance with a seperate MMIO range, IRQ, etc.
> > 
> > I have just noticed today though that at least the USB DWC3 controller in host
> > mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> > require which DART instance.
> > 
> > This means that we might need to support devices attached to two iommus
> > simultaneously and just create the same iova mappings. Currently this only
> > seems to be required for USB according to Apple's Device Tree.
> > 
> > I see two options for this and would like to get feedback before
> > I implement either one:
> > 
> >     1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
> >        to identify the DART and the second one to identify the master.
> >        The DART DT node would then also take two register ranges that would
> >        correspond to the two DARTs. Both instances use the same IRQ and the
> >        same clocks according to Apple's device tree and my experiments.
> >        This would keep a single device node and the DART driver would then
> >        simply map iovas in both DARTs if required.
> > 
> >     2) Keep #iommu-cells as-is but support
> >             iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
> >        instead.
> >        This would then require two devices nodes for the two DART instances and
> >        some housekeeping in the DART driver to support mapping iovas in both
> >        DARTs.
> >        I believe omap-iommu.c supports this setup but I will have to read
> >        more code to understand the details there and figure out how to implement
> >        this in a sane way.
> > 
> > I currently prefer the first option but I don't understand enough details of
> > the iommu system to actually make an informed decision.

Please don't mix what does the h/w look like and what's easy to 
implement in Linux's IOMMU subsytem. It's pretty clear (at least 
from the description here) that option 2 reflects the h/w. 

> > I'm obviously also open to more options :-)
> 
> Hi Sven,
> 
> I don't think the first option is going to work for PCIe.  PCIe
> devices will have to use "iommu-map" properties to map PCI devices to
> the right iommu, and the currently implementation seems to assume that
> #iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
> that it relies on #iommu-cells = <1>, but it isn't clear how the
> rid-base to iommu-base mapping mechanism would work when that isn't
> the case.
> 
> Now the PCIe DARTs are simpler and seem to have only one "instance"
> per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
> fine using the first approach.
> 
> As I mentioned before, not all DARTs support the full 32-bit aperture.
> In particular the PCIe DARTs support a smaller address-space.  It is
> not clear whether this is a restriction of the PCIe host controller or
> the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> properties that encode the base address and size of the aperture.
> These single-cell properties which is probably why for the USB DARTs
> only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> wouldn't be able to encode the full aperture size.  We could make them
> 64-bit numbers in the Linux device tree though and always be explicit
> about the size.  Older Sun SPARC machines used a single "virtual-dma"
> property to encode the aperture.  We could do someting similar.  You
> would use this property to initialize domain->geometry.aperture_start
> and domain->geometry.aperture_end in diff 3/3 of this series.

'dma-ranges' is what should be used here.

Rob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-23 20:00         ` Mark Kettenis
@ 2021-03-23 21:03           ` Sven Peter via iommu
  0 siblings, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-23 21:03 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: Arnd Bergmann, devicetree, will, marcan, linux-kernel, iommu,
	robh+dt, Marc Zyngier, mohamed.mediouni, robin.murphy,
	linux-arm-kernel, stan

Hi Mark,

On Tue, Mar 23, 2021, at 21:00, Mark Kettenis wrote:
> The problem with both #1 and #2 is that you end up with two references
> to (effectively) different iommu's in the dwc3 device node.  I don't
> see how that is compatible with the idea of using a single translation
> table for both sub-DARTs.

I don't have a strong opinion about this fwiw.
Option #1 and #2 seem to have precedence in the already existing
iommu bindings though [1,2,3].
I just want to make sure we've thought through all options and understand
their advantages/disadvantages before we take a decision.


I've been reading some more Linux iommu code and here's how I understand
it now / how I could implement at least #1 with a single shared pagetable.
This might also be possible for option #2, but I'll need to think that through
in more detail.

An iommu domain is a collection of devices that share a virtual address space.
During domain allocation I can just allocate a single DART pagetable and
not have anything point to it for now.

A stream identifies the smallest unit the iommu hardware can differentiate.
For the DART we have 16 of these with a single TCR + the four TTBR register
for each stream.

A device is assigned to individual streams using the "iommus" property. When
a device is attached to a domain we now simply setup the TTBR registers to
point to the iommu domain pagetable. It doesn't matter here if it's a single
stream or multiple ones or even multiple devices sharing a single stream as
long as they're attached to the same domain.

All operations (map, unmap, etc.) now simply first modify the domain
pagetable and then issue the TLB maintenance operations for attached streams.


> 
> If you no longer think that is desirable, you'll still have the
> problem that you'll need to modify the dwc3 driver code such that it
> uses the right IOMMU to do its DMA address translation.  Given what
> you write above that sounds really ugly and confusing.  I would
> certainly want to avoid doing that in OpenBSD.

Yeah, I absolutely don't want to hack on the dwc3 code at all.
That will end up being *very* ugly.



Best,

Sven


[1] Documentation/devicetree/bindings/iommu/iommu.txt "Multiple-master IOMMU"
[2] Documentation/devicetree/bindings/qcom,iommu.txt last example
[3] Documentation/devicetree/bindings/arm,smmu.yaml first example
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-23 20:53   ` Rob Herring
@ 2021-03-23 22:33     ` Mark Kettenis
  2021-03-25  7:53     ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-23 22:33 UTC (permalink / raw)
  To: Rob Herring
  Cc: arnd, sven, devicetree, will, marcan, linux-kernel, iommu, maz,
	mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> Date: Tue, 23 Mar 2021 14:53:46 -0600
> From: Rob Herring <robh@kernel.org>
> 
> On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 20 Mar 2021 15:19:33 +0000
> > > From: Sven Peter <sven@svenpeter.dev>
> > > 
> > > Hi,
> > > 
> > > After Hector's initial work [1] to bring up Linux on Apple's M1 it's time to
> > > bring up more devices. Most peripherals connected to the SoC are behind a iommu
> > > which Apple calls "Device Address Resolution Table", or DART for short [2].
> > > Unfortunately, it only shares the name with PowerPC's DART.
> > > Configuring this iommu is mandatory if these peripherals require DMA access.
> > > 
> > > This patchset implements initial support for this iommu. The hardware itself
> > > uses a pagetable format that's very similar to the one already implement in
> > > io-pgtable.c. There are some minor modifications, namely some details of the
> > > PTE format and that there are always three pagetable levels, which I've
> > > implement as a new format variant.
> > > 
> > > I have mainly tested this with the USB controller in device mode which is
> > > compatible with Linux's dwc3 driver. Some custom PHY initialization (which is
> > > not yet ready or fully understood) is required though to bring up the ports,
> > > see e.g. my patches to our m1n1 bootloader [3,4]. If you want to test the same
> > > setup you will probably need that branch for now and add the nodes from
> > > the DT binding specification example to your device tree.
> > > 
> > > Even though each DART instances could support up to 16 devices usually only
> > > a single device is actually connected. Different devices generally just use
> > > an entirely separate DART instance with a seperate MMIO range, IRQ, etc.
> > > 
> > > I have just noticed today though that at least the USB DWC3 controller in host
> > > mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> > > require which DART instance.
> > > 
> > > This means that we might need to support devices attached to two iommus
> > > simultaneously and just create the same iova mappings. Currently this only
> > > seems to be required for USB according to Apple's Device Tree.
> > > 
> > > I see two options for this and would like to get feedback before
> > > I implement either one:
> > > 
> > >     1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
> > >        to identify the DART and the second one to identify the master.
> > >        The DART DT node would then also take two register ranges that would
> > >        correspond to the two DARTs. Both instances use the same IRQ and the
> > >        same clocks according to Apple's device tree and my experiments.
> > >        This would keep a single device node and the DART driver would then
> > >        simply map iovas in both DARTs if required.
> > > 
> > >     2) Keep #iommu-cells as-is but support
> > >             iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
> > >        instead.
> > >        This would then require two devices nodes for the two DART instances and
> > >        some housekeeping in the DART driver to support mapping iovas in both
> > >        DARTs.
> > >        I believe omap-iommu.c supports this setup but I will have to read
> > >        more code to understand the details there and figure out how to implement
> > >        this in a sane way.
> > > 
> > > I currently prefer the first option but I don't understand enough details of
> > > the iommu system to actually make an informed decision.
> 
> Please don't mix what does the h/w look like and what's easy to 
> implement in Linux's IOMMU subsytem. It's pretty clear (at least 
> from the description here) that option 2 reflects the h/w.

Apple does represent these as a single node in their device tree.  The
two instances share an interrupt and share power/clock gating.  So
they seem to be part of a single hardware block.

> > > I'm obviously also open to more options :-)
> > 
> > Hi Sven,
> > 
> > I don't think the first option is going to work for PCIe.  PCIe
> > devices will have to use "iommu-map" properties to map PCI devices to
> > the right iommu, and the currently implementation seems to assume that
> > #iommu-cells = <1>.  The devictree binding[1] doesn't explicitly state
> > that it relies on #iommu-cells = <1>, but it isn't clear how the
> > rid-base to iommu-base mapping mechanism would work when that isn't
> > the case.
> > 
> > Now the PCIe DARTs are simpler and seem to have only one "instance"
> > per DART.  So if we keep #iommu-cells = <1> for those, you'd still be
> > fine using the first approach.
> > 
> > As I mentioned before, not all DARTs support the full 32-bit aperture.
> > In particular the PCIe DARTs support a smaller address-space.  It is
> > not clear whether this is a restriction of the PCIe host controller or
> > the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> > properties that encode the base address and size of the aperture.
> > These single-cell properties which is probably why for the USB DARTs
> > only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> > wouldn't be able to encode the full aperture size.  We could make them
> > 64-bit numbers in the Linux device tree though and always be explicit
> > about the size.  Older Sun SPARC machines used a single "virtual-dma"
> > property to encode the aperture.  We could do someting similar.  You
> > would use this property to initialize domain->geometry.aperture_start
> > and domain->geometry.aperture_end in diff 3/3 of this series.
> 
> 'dma-ranges' is what should be used here.
> 
> Rob
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-20 15:19 [PATCH 0/3] Apple M1 DART IOMMU driver Sven Peter via iommu
                   ` (2 preceding siblings ...)
  2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
@ 2021-03-24 15:29 ` Robin Murphy
  2021-03-25  7:58   ` Sven Peter via iommu
  3 siblings, 1 reply; 36+ messages in thread
From: Robin Murphy @ 2021-03-24 15:29 UTC (permalink / raw)
  To: Sven Peter, iommu
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, Rob Herring, Mohamed Mediouni, Stan Skowronek,
	Will Deacon, linux-arm-kernel, Mark Kettenis

On 2021-03-20 15:19, Sven Peter wrote:
> Hi,
> 
> After Hector's initial work [1] to bring up Linux on Apple's M1 it's time to
> bring up more devices. Most peripherals connected to the SoC are behind a iommu
> which Apple calls "Device Address Resolution Table", or DART for short [2].
> Unfortunately, it only shares the name with PowerPC's DART.
> Configuring this iommu is mandatory if these peripherals require DMA access.
> 
> This patchset implements initial support for this iommu. The hardware itself
> uses a pagetable format that's very similar to the one already implement in
> io-pgtable.c. There are some minor modifications, namely some details of the
> PTE format and that there are always three pagetable levels, which I've
> implement as a new format variant.
> 
> I have mainly tested this with the USB controller in device mode which is
> compatible with Linux's dwc3 driver. Some custom PHY initialization (which is
> not yet ready or fully understood) is required though to bring up the ports,
> see e.g. my patches to our m1n1 bootloader [3,4]. If you want to test the same
> setup you will probably need that branch for now and add the nodes from
> the DT binding specification example to your device tree.
> 
> Even though each DART instances could support up to 16 devices usually only
> a single device is actually connected. Different devices generally just use
> an entirely separate DART instance with a seperate MMIO range, IRQ, etc.
> 
> I have just noticed today though that at least the USB DWC3 controller in host
> mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> require which DART instance.
> 
> This means that we might need to support devices attached to two iommus
> simultaneously and just create the same iova mappings. Currently this only
> seems to be required for USB according to Apple's Device Tree.
> 
> I see two options for this and would like to get feedback before
> I implement either one:
> 
>      1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
>         to identify the DART and the second one to identify the master.
>         The DART DT node would then also take two register ranges that would
>         correspond to the two DARTs. Both instances use the same IRQ and the
>         same clocks according to Apple's device tree and my experiments.
>         This would keep a single device node and the DART driver would then
>         simply map iovas in both DARTs if required.

This is broadly similar to the approach used by rockchip-iommu and the 
special arm-smmu-nvidia implementation, where there are multiple 
instances which require programming identically, that are abstracted 
behind a single "device". Your case is a little different since you're 
not programming both *entirely* identically, although maybe that's a 
possibility if each respective ID isn't used by anything else on the 
"other" DART?

Overall I tend to view this approach as a bit of a hack because it's not 
really describing the hardware truthfully - just because two distinct 
functional blocks have their IRQ lines wired together doesn't suddenly 
make them a single monolithic block with multiple interfaces - and tends 
to be done for the sake of making the driver implementation easier in 
terms of the Linux IOMMU API (which, note, hasn't evolved all that far 
from its PCI-centric origins and isn't exactly great for arbitrary SoC 
topologies).

>      2) Keep #iommu-cells as-is but support
>              iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
>         instead.
>         This would then require two devices nodes for the two DART instances and
>         some housekeeping in the DART driver to support mapping iovas in both
>         DARTs.
>         I believe omap-iommu.c supports this setup but I will have to read
>         more code to understand the details there and figure out how to implement
>         this in a sane way.

This approach is arguably the most honest, and more robust in terms of 
making fewer assumptions, and is used by at least exynos-iommu and 
omap-iommu. In Linux it currently takes a little bit more housekeeping 
to keep track of linked instances within the driver since the IOMMU API 
holds the notion that any given client device is associated with "an 
IOMMU", but that's always free to change at any time, unlike the design 
of a DT binding.

There's also the funky "root" and "leaf" devices thing that ipmmu-vmsa 
does, although I believe that represents genuine hardware differences 
where the leaves are more like extra TLBs rather than fully-functional 
IOMMU blocks in their own right, so that may not be a relevant model here.

Robin.

> I currently prefer the first option but I don't understand enough details of
> the iommu system to actually make an informed decision.
> I'm obviously also open to more options :-)
> 
> 
> Best regards,
> 
> 
> Sven
> 
> [1] https://lore.kernel.org/linux-arch/20210304213902.83903-1-marcan@marcan.st/
> [2] https://developer.apple.com/library/archive/documentation/DeviceDrivers/Conceptual/IOKitFundamentals/DataMgmt/DataMgmt.html
> [3] https://github.com/svenpeter42/m1n1/commit/1e2661abf5ea2c820297b3ff591235c408d19a34
> [4] https://github.com/svenpeter42/m1n1/tree/usb-uartproxy-console-wip
> 
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] iommu: io-pgtable: add DART pagetable format
  2021-03-20 15:19 ` [PATCH 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter via iommu
@ 2021-03-24 16:37   ` Robin Murphy
  2021-03-25 20:47     ` Sven Peter via iommu
  0 siblings, 1 reply; 36+ messages in thread
From: Robin Murphy @ 2021-03-24 16:37 UTC (permalink / raw)
  To: Sven Peter, iommu
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, Rob Herring, Mohamed Mediouni, Stan Skowronek,
	Will Deacon, linux-arm-kernel, Mark Kettenis

On 2021-03-20 15:19, Sven Peter wrote:
> Apple's DART iommu uses a pagetable format that's very similar to the ones
> already implemented by io-pgtable.c.
> Add a new format variant to support the required differences.

TBH there look to be more differences than similarities, but I guess we 
already opened that door with the Mali format, and nobody likes writing 
pagetable code :)

> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
>   drivers/iommu/Kconfig          | 13 +++++++
>   drivers/iommu/io-pgtable-arm.c | 70 ++++++++++++++++++++++++++++++++++
>   drivers/iommu/io-pgtable.c     |  3 ++
>   include/linux/io-pgtable.h     |  6 +++
>   4 files changed, 92 insertions(+)
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 192ef8f61310..3c95c8524abe 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -39,6 +39,19 @@ config IOMMU_IO_PGTABLE_LPAE
>   	  sizes at both stage-1 and stage-2, as well as address spaces
>   	  up to 48-bits in size.
> 
> +config IOMMU_IO_PGTABLE_APPLE_DART

Does this really need to be configurable? I don't think there's an 
appreciable code saving to be had, and it's not like we do it for any of 
the other sub-formats.

> +	bool "Apple DART Descriptor Format"
> +	select IOMMU_IO_PGTABLE
> +	select IOMMU_IO_PGTABLE_LPAE
> +	depends on ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64)
> +	help
> +	  Enable support for the Apple DART iommu pagetable format.
> +	  This format is a variant of the ARMv7/v8 Long Descriptor
> +	  Format specific to Apple's iommu found in their SoCs.
> +
> +	  Say Y here if you have a Apple SoC like the M1 which
> +	  contains DART iommus.
> +
>   config IOMMU_IO_PGTABLE_LPAE_SELFTEST
>   	bool "LPAE selftests"
>   	depends on IOMMU_IO_PGTABLE_LPAE
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58e79b5..18674469313d 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -127,6 +127,10 @@
>   #define ARM_MALI_LPAE_MEMATTR_IMP_DEF	0x88ULL
>   #define ARM_MALI_LPAE_MEMATTR_WRITE_ALLOC 0x8DULL
> 
> +/* APPLE_DART_PTE_PROT_NO_WRITE actually maps to ARM_LPAE_PTE_AP_RDONLY  */
> +#define APPLE_DART_PTE_PROT_NO_WRITE (1<<7)
Given that there's apparently zero similarity with any of the other 
attributes/permissions, this seems more like a coincidence that probably 
doesn't need to be called out.

> +#define APPLE_DART_PTE_PROT_NO_READ (1<<8)
> +

Do you have XN permission? How about memory type attributes?

>   /* IOPTE accessors */
>   #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> 
> @@ -381,6 +385,17 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
>   {
>   	arm_lpae_iopte pte;
> 
> +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART

As a general tip, prefer IS_ENABLED() to inline #ifdefs.

> +	if (data->iop.fmt == ARM_APPLE_DART) { > +		pte = 0;
> +		if (!(prot & IOMMU_WRITE))
> +			pte |= APPLE_DART_PTE_PROT_NO_WRITE;
> +		if (!(prot & IOMMU_READ))
> +			pte |= APPLE_DART_PTE_PROT_NO_READ;
> +		return pte;
> +	}
> +#endif
> +
>   	if (data->iop.fmt == ARM_64_LPAE_S1 ||
>   	    data->iop.fmt == ARM_32_LPAE_S1) {
>   		pte = ARM_LPAE_PTE_nG;
> @@ -1043,6 +1058,54 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   	return NULL;
>   }
> 
> +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART

Similarly, prefer __maybe_unused rather than #ifdefing functions if they 
don't contain any config-dependent references.

> +static struct io_pgtable *
> +apple_dart_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> +{
> +	struct arm_lpae_io_pgtable *data;
> +
> +	if (cfg->ias > 38)
> +		return NULL;
> +	if (cfg->oas > 36)
> +		return NULL;
> +
> +	if (!cfg->coherent_walk)
> +		return NULL;

For completeness you should probably also reject any quirks, since none 
of them are applicable either.

> +
> +	cfg->pgsize_bitmap &= SZ_16K;

No block mappings?

> +	if (!cfg->pgsize_bitmap)
> +		return NULL;
> +
> +	data = arm_lpae_alloc_pgtable(cfg);
> +	if (!data)
> +		return NULL;
> +
> +	/*
> +	 * the hardware only supports this specific three level pagetable layout with
> +	 * the first level being encoded into four hardware registers
> +	 */
> +	data->start_level = ARM_LPAE_MAX_LEVELS - 2;

The comment implies that walks should start at level 1 (for a 3-level 
table), but the code here says (in an unnecessarily roundabout manner) 
level 2 :/

Which is it?

> +	data->pgd_bits = 13;

What if ias < 38? Couldn't we get away with only allocating as much as 
we actually need?

> +	data->bits_per_level = 11;
> +
> +	data->pgd = __arm_lpae_alloc_pages(ARM_LPAE_PGD_SIZE(data), GFP_KERNEL,
> +					   cfg);
> +	if (!data->pgd)
> +		goto out_free_data;
> +
> +	cfg->apple_dart_cfg.pgd[0] = virt_to_phys(data->pgd);
> +	cfg->apple_dart_cfg.pgd[1] = virt_to_phys(data->pgd + 0x4000);
> +	cfg->apple_dart_cfg.pgd[2] = virt_to_phys(data->pgd + 0x8000);
> +	cfg->apple_dart_cfg.pgd[3] = virt_to_phys(data->pgd + 0xc000);
> +
> +	return &data->iop;
> +
> +out_free_data:
> +	kfree(data);
> +	return NULL;
> +}
> +#endif
> +
>   struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
>   	.alloc	= arm_64_lpae_alloc_pgtable_s1,
>   	.free	= arm_lpae_free_pgtable,
> @@ -1068,6 +1131,13 @@ struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
>   	.free	= arm_lpae_free_pgtable,
>   };
> 
> +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> +struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns = {
> +	.alloc	= apple_dart_alloc_pgtable,
> +	.free	= arm_lpae_free_pgtable,
> +};
> +#endif
> +
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> 
>   static struct io_pgtable_cfg *cfg_cookie __initdata;
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 6e9917ce980f..d86590b0673a 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -27,6 +27,9 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
>   #ifdef CONFIG_AMD_IOMMU
>   	[AMD_IOMMU_V1] = &io_pgtable_amd_iommu_v1_init_fns,
>   #endif
> +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> +	[ARM_APPLE_DART] = &io_pgtable_apple_dart_init_fns,
> +#endif
>   };
> 
>   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index a4c9ca2c31f1..19d9b631d319 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -16,6 +16,7 @@ enum io_pgtable_fmt {
>   	ARM_V7S,
>   	ARM_MALI_LPAE,
>   	AMD_IOMMU_V1,
> +	ARM_APPLE_DART,
>   	IO_PGTABLE_NUM_FMTS,
>   };
> 
> @@ -136,6 +137,10 @@ struct io_pgtable_cfg {
>   			u64	transtab;
>   			u64	memattr;
>   		} arm_mali_lpae_cfg;
> +
> +		struct {
> +			u64 pgd[4];

Nit: in the driver you call the registers "TTBR" rather than "PGD". The 
config here tries to represent hardware/architecture definitions rather 
than internal abstractions.

Robin.

> +		} apple_dart_cfg;
>   	};
>   };
> 
> @@ -250,5 +255,6 @@ extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
>   extern struct io_pgtable_init_fns io_pgtable_amd_iommu_v1_init_fns;
> +extern struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns;
> 
>   #endif /* __IO_PGTABLE_H */
> --
> 2.25.1
> 
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-23 20:53   ` Rob Herring
  2021-03-23 22:33     ` Mark Kettenis
@ 2021-03-25  7:53     ` Sven Peter via iommu
  2021-03-25 11:50       ` Robin Murphy
  2021-03-25 21:41       ` Arnd Bergmann
  1 sibling, 2 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-25  7:53 UTC (permalink / raw)
  To: Rob Herring, Mark Kettenis
  Cc: Arnd Bergmann, devicetree, will, marcan, linux-kernel, iommu,
	Marc Zyngier, mohamed.mediouni, Robin Murphy, linux-arm-kernel,
	stan



On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
> On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote:
> > > Date: Sat, 20 Mar 2021 15:19:33 +0000
> > > From: Sven Peter <sven@svenpeter.dev>
> > > I have just noticed today though that at least the USB DWC3 controller in host
> > > mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> > > require which DART instance.
> > > 
> > > This means that we might need to support devices attached to two iommus
> > > simultaneously and just create the same iova mappings. Currently this only
> > > seems to be required for USB according to Apple's Device Tree.
> > > 
> > > I see two options for this and would like to get feedback before
> > > I implement either one:
> > > 
> > >     1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
> > >        to identify the DART and the second one to identify the master.
> > >        The DART DT node would then also take two register ranges that would
> > >        correspond to the two DARTs. Both instances use the same IRQ and the
> > >        same clocks according to Apple's device tree and my experiments.
> > >        This would keep a single device node and the DART driver would then
> > >        simply map iovas in both DARTs if required.
> > > 
> > >     2) Keep #iommu-cells as-is but support
> > >             iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
> > >        instead.
> > >        This would then require two devices nodes for the two DART instances and
> > >        some housekeeping in the DART driver to support mapping iovas in both
> > >        DARTs.
> > >        I believe omap-iommu.c supports this setup but I will have to read
> > >        more code to understand the details there and figure out how to implement
> > >        this in a sane way.
> > > 
> > > I currently prefer the first option but I don't understand enough details of
> > > the iommu system to actually make an informed decision.
> 
> Please don't mix what does the h/w look like and what's easy to 
> implement in Linux's IOMMU subsytem. It's pretty clear (at least 
> from the description here) that option 2 reflects the h/w. 
> 

Good point, I'll keep that in mind and give option 2 a try.

> > 
> > As I mentioned before, not all DARTs support the full 32-bit aperture.
> > In particular the PCIe DARTs support a smaller address-space.  It is
> > not clear whether this is a restriction of the PCIe host controller or
> > the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> > properties that encode the base address and size of the aperture.
> > These single-cell properties which is probably why for the USB DARTs
> > only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> > wouldn't be able to encode the full aperture size.  We could make them
> > 64-bit numbers in the Linux device tree though and always be explicit
> > about the size.  Older Sun SPARC machines used a single "virtual-dma"
> > property to encode the aperture.  We could do someting similar.  You
> > would use this property to initialize domain->geometry.aperture_start
> > and domain->geometry.aperture_end in diff 3/3 of this series.
> 
> 'dma-ranges' is what should be used here.
> 

The iommu binding documentation [1] mentions that

    The device tree node of the IOMMU device's parent bus must contain a valid
    "dma-ranges" property that describes how the physical address space of the
    IOMMU maps to memory. An empty "dma-ranges" property means that there is a 
    1:1 mapping from IOMMU to memory.

which, if I understand this correctly, means that the 'dma-ranges' for the
parent bus of the iommu should be empty since the DART hardware can see the
full physical address space with a 1:1 mapping.


The documentation also mentions that

     When an "iommus" property is specified in a device tree node, the IOMMU
     will be used for address translation. If a "dma-ranges" property exists
     in the device's parent node it will be ignored.

which means that specifying a 'dma-ranges' in the parent bus of any devices
that use the iommu will just be ignored.

As a concrete example, the PCIe DART IOMMU only allows translations from iovas
within 0x00100000...0x3ff00000 to the entire physical address space (though
realistically it will only map to 16GB RAM starting at 0x800000000 on the M1).

I'm probably just confused or maybe the documentation is outdated but I don't
see how I could specify "this device can only use DMA addresses from
0x00100000...0x3ff00000 but can map these via the iommu to any physical
address" using 'dma-ranges'.

Could you maybe point me to the right direction or give me a small example?
That would help a lot!



Thanks,

Sven


[1] Documentation/devicetree/bindings/iommu/iommu.txt
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-24 15:29 ` Robin Murphy
@ 2021-03-25  7:58   ` Sven Peter via iommu
  0 siblings, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-25  7:58 UTC (permalink / raw)
  To: Robin Murphy, iommu
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, Rob Herring, Mohamed Mediouni, Stan Skowronek,
	Will Deacon, linux-arm-kernel, Mark Kettenis

Hi Robin,


On Wed, Mar 24, 2021, at 16:29, Robin Murphy wrote:
> On 2021-03-20 15:19, Sven Peter wrote:
> > 
> > I have just noticed today though that at least the USB DWC3 controller in host
> > mode uses *two* darts at the same time. I'm not sure yet which parts seem to
> > require which DART instance.
> > 
> > This means that we might need to support devices attached to two iommus
> > simultaneously and just create the same iova mappings. Currently this only
> > seems to be required for USB according to Apple's Device Tree.
> > 
> > I see two options for this and would like to get feedback before
> > I implement either one:
> > 
> >      1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
> >         to identify the DART and the second one to identify the master.
> >         The DART DT node would then also take two register ranges that would
> >         correspond to the two DARTs. Both instances use the same IRQ and the
> >         same clocks according to Apple's device tree and my experiments.
> >         This would keep a single device node and the DART driver would then
> >         simply map iovas in both DARTs if required.
> 
> This is broadly similar to the approach used by rockchip-iommu and the 
> special arm-smmu-nvidia implementation, where there are multiple 
> instances which require programming identically, that are abstracted 
> behind a single "device". Your case is a little different since you're 
> not programming both *entirely* identically, although maybe that's a 
> possibility if each respective ID isn't used by anything else on the 
> "other" DART?

That would be possible. The only difference is that I need to
program ID 0 of the first DART and ID 1 of the second one. Both
of these IDs are only connected to the same USB controller.


> 
> Overall I tend to view this approach as a bit of a hack because it's not 
> really describing the hardware truthfully - just because two distinct 
> functional blocks have their IRQ lines wired together doesn't suddenly 
> make them a single monolithic block with multiple interfaces - and tends 
> to be done for the sake of making the driver implementation easier in 
> terms of the Linux IOMMU API (which, note, hasn't evolved all that far 
> from its PCI-centric origins and isn't exactly great for arbitrary SoC 
> topologies).

Yes, the easier driver implementation was my reason to favour this option.

> 
> >      2) Keep #iommu-cells as-is but support
> >              iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
> >         instead.
> >         This would then require two devices nodes for the two DART instances and
> >         some housekeeping in the DART driver to support mapping iovas in both
> >         DARTs.
> >         I believe omap-iommu.c supports this setup but I will have to read
> >         more code to understand the details there and figure out how to implement
> >         this in a sane way.
> 
> This approach is arguably the most honest, and more robust in terms of 
> making fewer assumptions, and is used by at least exynos-iommu and 
> omap-iommu. In Linux it currently takes a little bit more housekeeping 
> to keep track of linked instances within the driver since the IOMMU API 
> holds the notion that any given client device is associated with "an 
> IOMMU", but that's always free to change at any time, unlike the design 
> of a DT binding.

Sounds good. I'll read those drivers and give it a try for v2.


Thanks,


Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-25  7:53     ` Sven Peter via iommu
@ 2021-03-25 11:50       ` Robin Murphy
  2021-03-25 20:49         ` Sven Peter via iommu
  2021-03-27 15:33         ` Sven Peter via iommu
  2021-03-25 21:41       ` Arnd Bergmann
  1 sibling, 2 replies; 36+ messages in thread
From: Robin Murphy @ 2021-03-25 11:50 UTC (permalink / raw)
  To: Sven Peter, Rob Herring, Mark Kettenis
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, marcan, linux-kernel,
	iommu, mohamed.mediouni, will, linux-arm-kernel, stan

On 2021-03-25 07:53, Sven Peter wrote:
> 
> 
> On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
>> On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote:
>>>> Date: Sat, 20 Mar 2021 15:19:33 +0000
>>>> From: Sven Peter <sven@svenpeter.dev>
>>>> I have just noticed today though that at least the USB DWC3 controller in host
>>>> mode uses *two* darts at the same time. I'm not sure yet which parts seem to
>>>> require which DART instance.
>>>>
>>>> This means that we might need to support devices attached to two iommus
>>>> simultaneously and just create the same iova mappings. Currently this only
>>>> seems to be required for USB according to Apple's Device Tree.
>>>>
>>>> I see two options for this and would like to get feedback before
>>>> I implement either one:
>>>>
>>>>      1) Change #iommu-cells = <1>; to #iommu-cells = <2>; and use the first cell
>>>>         to identify the DART and the second one to identify the master.
>>>>         The DART DT node would then also take two register ranges that would
>>>>         correspond to the two DARTs. Both instances use the same IRQ and the
>>>>         same clocks according to Apple's device tree and my experiments.
>>>>         This would keep a single device node and the DART driver would then
>>>>         simply map iovas in both DARTs if required.
>>>>
>>>>      2) Keep #iommu-cells as-is but support
>>>>              iommus = <&usb_dart1a 1>, <&usb_dart1b 0>;
>>>>         instead.
>>>>         This would then require two devices nodes for the two DART instances and
>>>>         some housekeeping in the DART driver to support mapping iovas in both
>>>>         DARTs.
>>>>         I believe omap-iommu.c supports this setup but I will have to read
>>>>         more code to understand the details there and figure out how to implement
>>>>         this in a sane way.
>>>>
>>>> I currently prefer the first option but I don't understand enough details of
>>>> the iommu system to actually make an informed decision.
>>
>> Please don't mix what does the h/w look like and what's easy to
>> implement in Linux's IOMMU subsytem. It's pretty clear (at least
>> from the description here) that option 2 reflects the h/w.
>>
> 
> Good point, I'll keep that in mind and give option 2 a try.
> 
>>>
>>> As I mentioned before, not all DARTs support the full 32-bit aperture.
>>> In particular the PCIe DARTs support a smaller address-space.  It is
>>> not clear whether this is a restriction of the PCIe host controller or
>>> the DART, but the Apple Device Tree has "vm-base" and "vm-size"
>>> properties that encode the base address and size of the aperture.
>>> These single-cell properties which is probably why for the USB DARTs
>>> only "vm-base" is given; since "vm-base" is 0, a 32-bit number
>>> wouldn't be able to encode the full aperture size.  We could make them
>>> 64-bit numbers in the Linux device tree though and always be explicit
>>> about the size.  Older Sun SPARC machines used a single "virtual-dma"
>>> property to encode the aperture.  We could do someting similar.  You
>>> would use this property to initialize domain->geometry.aperture_start
>>> and domain->geometry.aperture_end in diff 3/3 of this series.
>>
>> 'dma-ranges' is what should be used here.
>>
> 
> The iommu binding documentation [1] mentions that
> 
>      The device tree node of the IOMMU device's parent bus must contain a valid
>      "dma-ranges" property that describes how the physical address space of the
>      IOMMU maps to memory. An empty "dma-ranges" property means that there is a
>      1:1 mapping from IOMMU to memory.
> 
> which, if I understand this correctly, means that the 'dma-ranges' for the
> parent bus of the iommu should be empty since the DART hardware can see the
> full physical address space with a 1:1 mapping.
> 
> 
> The documentation also mentions that
> 
>       When an "iommus" property is specified in a device tree node, the IOMMU
>       will be used for address translation. If a "dma-ranges" property exists
>       in the device's parent node it will be ignored.
> 
> which means that specifying a 'dma-ranges' in the parent bus of any devices
> that use the iommu will just be ignored.

I think that's just wrong and wants updating (or at least clarifying). 
The high-level view now is that we use "dma-ranges" to describe 
limitations imposed by a bridge or interconnect segment, and that can 
certainly happen upstream of an IOMMU. As it happens, I've just recently 
sent a patch for precisely that case[1].

I guess what it might have been trying to say is that "dma-ranges" 
*does* become irrelevant in terms of constraining what physical memory 
is usable for DMA, but that shouldn't imply that its meaning doesn't 
just shift to a different purpose.

> As a concrete example, the PCIe DART IOMMU only allows translations from iovas
> within 0x00100000...0x3ff00000 to the entire physical address space (though
> realistically it will only map to 16GB RAM starting at 0x800000000 on the M1).
> 
> I'm probably just confused or maybe the documentation is outdated but I don't
> see how I could specify "this device can only use DMA addresses from
> 0x00100000...0x3ff00000 but can map these via the iommu to any physical
> address" using 'dma-ranges'.
> 
> Could you maybe point me to the right direction or give me a small example?
> That would help a lot!

PCI is easy, since it's already standard practice to use "dma-ranges" to 
describe host bridge inbound windows. Even if the restriction is really 
out in the host-side interconnect rather than in the bridge itself, to 
all intents and purposes it's indistinguishable so can still be 
described the same way.

The case of a standalone device having fewer address bits wired up than 
both its output and the corresponding IOMMU input might expect is a 
little more awkward, since that often *does* require adding an extra 
level of "bus" to explicitly represent that interconnect link in the DT 
model, e.g. [2].

Robin.

[1] 
https://lore.kernel.org/linux-arm-kernel/720d0a9a42e33148fcac45cd39a727093a32bf32.1614965598.git.robin.murphy@arm.com/
[2] 
https://lore.kernel.org/linux-arm-kernel/20180926132247.10971-23-laurentiu.tudor@nxp.com/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 1/3] iommu: io-pgtable: add DART pagetable format
  2021-03-24 16:37   ` Robin Murphy
@ 2021-03-25 20:47     ` Sven Peter via iommu
  0 siblings, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-25 20:47 UTC (permalink / raw)
  To: Robin Murphy, iommu
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, Rob Herring, Mohamed Mediouni, Stan Skowronek,
	Will Deacon, linux-arm-kernel, Mark Kettenis

Hi Robin,

Thanks for the review!

On Wed, Mar 24, 2021, at 17:37, Robin Murphy wrote:
> On 2021-03-20 15:19, Sven Peter wrote:
> > Apple's DART iommu uses a pagetable format that's very similar to the ones
> > already implemented by io-pgtable.c.
> > Add a new format variant to support the required differences.
> 
> TBH there look to be more differences than similarities, but I guess we 
> already opened that door with the Mali format, and nobody likes writing 
> pagetable code :)

Fair enough. There are some similarities but especially the actual
PTE format is completely different. And yes, I very much prefer to use
well-tested and written pagetable code rather than coming up with
my own if possible :-)


> 
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
> > ---
> >   drivers/iommu/Kconfig          | 13 +++++++
> >   drivers/iommu/io-pgtable-arm.c | 70 ++++++++++++++++++++++++++++++++++
> >   drivers/iommu/io-pgtable.c     |  3 ++
> >   include/linux/io-pgtable.h     |  6 +++
> >   4 files changed, 92 insertions(+)
> > 
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 192ef8f61310..3c95c8524abe 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -39,6 +39,19 @@ config IOMMU_IO_PGTABLE_LPAE
> >   	  sizes at both stage-1 and stage-2, as well as address spaces
> >   	  up to 48-bits in size.
> > 
> > +config IOMMU_IO_PGTABLE_APPLE_DART
> 
> Does this really need to be configurable? I don't think there's an 
> appreciable code saving to be had, and it's not like we do it for any of 
> the other sub-formats.
> 

Probably not, I'll just make it unconditional for v2.

> > +	bool "Apple DART Descriptor Format"
> > +	select IOMMU_IO_PGTABLE
> > +	select IOMMU_IO_PGTABLE_LPAE
> > +	depends on ARM64 || (COMPILE_TEST && !GENERIC_ATOMIC64)
> > +	help
> > +	  Enable support for the Apple DART iommu pagetable format.
> > +	  This format is a variant of the ARMv7/v8 Long Descriptor
> > +	  Format specific to Apple's iommu found in their SoCs.
> > +
> > +	  Say Y here if you have a Apple SoC like the M1 which
> > +	  contains DART iommus.
> > +
> >   config IOMMU_IO_PGTABLE_LPAE_SELFTEST
> >   	bool "LPAE selftests"
> >   	depends on IOMMU_IO_PGTABLE_LPAE
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> > index 87def58e79b5..18674469313d 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -127,6 +127,10 @@
> >   #define ARM_MALI_LPAE_MEMATTR_IMP_DEF	0x88ULL
> >   #define ARM_MALI_LPAE_MEMATTR_WRITE_ALLOC 0x8DULL
> > 
> > +/* APPLE_DART_PTE_PROT_NO_WRITE actually maps to ARM_LPAE_PTE_AP_RDONLY  */
> > +#define APPLE_DART_PTE_PROT_NO_WRITE (1<<7)
> Given that there's apparently zero similarity with any of the other 
> attributes/permissions, this seems more like a coincidence that probably 
> doesn't need to be called out.

Agreed, removed for v2.

> 
> > +#define APPLE_DART_PTE_PROT_NO_READ (1<<8)
> > +
> 
> Do you have XN permission? How about memory type attributes?

I haven't been able to find either of them yet.

There is no (public) documentation for this hardware and this was all
figured out by essentially looking at the pagetables the (unknown)
first-level bootloader left around for us by the time we get to run code
and by flipping bits in HW registers or pagetables and observing what happens.

I only have the framebuffer and USB running right now and neither of
them are able to run code so I can't really find the NX bit if it exists.
I'm not sure if there are any peripherals that can even execute code
from pages mapped through a DART. *Maybe* the GPU but it'll still take
a while until we can tackle that one.

I'll see if I can find something that controls memory attributes though.

The only other way would be to actually reverse engineer Apple code but
that's something I've been deliberately avoiding and I'd really like to
keep it that way.


> 
> >   /* IOPTE accessors */
> >   #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> > 
> > @@ -381,6 +385,17 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
> >   {
> >   	arm_lpae_iopte pte;
> > 
> > +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> 
> As a general tip, prefer IS_ENABLED() to inline #ifdefs.

Thanks, will keep that in mind for the future! This #ifdef here will disappear
in v2 once I remove this from Kconfig.

> 
> > +	if (data->iop.fmt == ARM_APPLE_DART) { > +		pte = 0;
> > +		if (!(prot & IOMMU_WRITE))
> > +			pte |= APPLE_DART_PTE_PROT_NO_WRITE;
> > +		if (!(prot & IOMMU_READ))
> > +			pte |= APPLE_DART_PTE_PROT_NO_READ;
> > +		return pte;
> > +	}
> > +#endif
> > +
> >   	if (data->iop.fmt == ARM_64_LPAE_S1 ||
> >   	    data->iop.fmt == ARM_32_LPAE_S1) {
> >   		pte = ARM_LPAE_PTE_nG;
> > @@ -1043,6 +1058,54 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> >   	return NULL;
> >   }
> > 
> > +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> 
> Similarly, prefer __maybe_unused rather than #ifdefing functions if they 
> don't contain any config-dependent references.

Will also keep that mind for the future since I probably won't need that for v2.

> 
> > +static struct io_pgtable *
> > +apple_dart_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
> > +{
> > +	struct arm_lpae_io_pgtable *data;
> > +
> > +	if (cfg->ias > 38)
> > +		return NULL;
> > +	if (cfg->oas > 36)
> > +		return NULL;
> > +
> > +	if (!cfg->coherent_walk)
> > +		return NULL;
> 
> For completeness you should probably also reject any quirks, since none 
> of them are applicable either.

Good point, added for v2.

> 
> > +
> > +	cfg->pgsize_bitmap &= SZ_16K;
> 
> No block mappings?

Don't think so. I've tried this early on and couldn't get it to work.
The HW just ignored my block mappings and reported faults for any iova
in there. I'll check again this weekend though to be sure.

> 
> > +	if (!cfg->pgsize_bitmap)
> > +		return NULL;
> > +
> > +	data = arm_lpae_alloc_pgtable(cfg);
> > +	if (!data)
> > +		return NULL;
> > +
> > +	/*
> > +	 * the hardware only supports this specific three level pagetable layout with
> > +	 * the first level being encoded into four hardware registers
> > +	 */
> > +	data->start_level = ARM_LPAE_MAX_LEVELS - 2;
> 
> The comment implies that walks should start at level 1 (for a 3-level 
> table), but the code here says (in an unnecessarily roundabout manner) 
> level 2 :/
> 
> Which is it?

Now that I'm reading it again it does sound very confusing. This is essentially
a small workaround to be able to reuse this code.

I'll fix the strange way to encode that we start at level 2 and I'll replace the
comment with a better explanation for v2:

	/*
	 * This is how the hardware actually works:
	 *   First level: four registers pointing to the second level tables.
	 *   Second level: 2048 entries pointing to the third level tables.
	 *   Third level: 2048 entries pointing to pages.
	 *
	 *  And this is how we pretend the hardware works:
	 *    First fake level: Merges first and second level, i.e.
	 *       4x2048 entries pointing to the second fake / third real
	 *      level tables
	 *    Second fake level: Equivalent to the third real level.
	 *       2048 entries pointing to pages.
	 *
	 * We then set up the four hardware registers to point to the first
         * 'fake' pagetable with different offsets (0x0, 0x4000, 0x8000, 0xc000).
	 */

I hope that makes it more clear what happens there.

> 
> > +	data->pgd_bits = 13;
> 
> What if ias < 38? Couldn't we get away with only allocating as much as 
> we actually need?

Good idea, will fix that for v2.

> 
> > +	data->bits_per_level = 11;
> > +
> > +	data->pgd = __arm_lpae_alloc_pages(ARM_LPAE_PGD_SIZE(data), GFP_KERNEL,
> > +					   cfg);
> > +	if (!data->pgd)
> > +		goto out_free_data;
> > +
> > +	cfg->apple_dart_cfg.pgd[0] = virt_to_phys(data->pgd);
> > +	cfg->apple_dart_cfg.pgd[1] = virt_to_phys(data->pgd + 0x4000);
> > +	cfg->apple_dart_cfg.pgd[2] = virt_to_phys(data->pgd + 0x8000);
> > +	cfg->apple_dart_cfg.pgd[3] = virt_to_phys(data->pgd + 0xc000);
> > +
> > +	return &data->iop;
> > +
> > +out_free_data:
> > +	kfree(data);
> > +	return NULL;
> > +}
> > +#endif
> > +
> >   struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> >   	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> >   	.free	= arm_lpae_free_pgtable,
> > @@ -1068,6 +1131,13 @@ struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> >   	.free	= arm_lpae_free_pgtable,
> >   };
> > 
> > +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> > +struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns = {
> > +	.alloc	= apple_dart_alloc_pgtable,
> > +	.free	= arm_lpae_free_pgtable,
> > +};
> > +#endif
> > +
> >   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> > 
> >   static struct io_pgtable_cfg *cfg_cookie __initdata;
> > diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> > index 6e9917ce980f..d86590b0673a 100644
> > --- a/drivers/iommu/io-pgtable.c
> > +++ b/drivers/iommu/io-pgtable.c
> > @@ -27,6 +27,9 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
> >   #ifdef CONFIG_AMD_IOMMU
> >   	[AMD_IOMMU_V1] = &io_pgtable_amd_iommu_v1_init_fns,
> >   #endif
> > +#ifdef CONFIG_IOMMU_IO_PGTABLE_APPLE_DART
> > +	[ARM_APPLE_DART] = &io_pgtable_apple_dart_init_fns,
> > +#endif
> >   };
> > 
> >   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
> > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> > index a4c9ca2c31f1..19d9b631d319 100644
> > --- a/include/linux/io-pgtable.h
> > +++ b/include/linux/io-pgtable.h
> > @@ -16,6 +16,7 @@ enum io_pgtable_fmt {
> >   	ARM_V7S,
> >   	ARM_MALI_LPAE,
> >   	AMD_IOMMU_V1,
> > +	ARM_APPLE_DART,
> >   	IO_PGTABLE_NUM_FMTS,
> >   };
> > 
> > @@ -136,6 +137,10 @@ struct io_pgtable_cfg {
> >   			u64	transtab;
> >   			u64	memattr;
> >   		} arm_mali_lpae_cfg;
> > +
> > +		struct {
> > +			u64 pgd[4];
> 
> Nit: in the driver you call the registers "TTBR" rather than "PGD". The 
> config here tries to represent hardware/architecture definitions rather 
> than internal abstractions.
> 

Renamed for v2.


Thanks!


Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-25 11:50       ` Robin Murphy
@ 2021-03-25 20:49         ` Sven Peter via iommu
  2021-03-27 15:33         ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-25 20:49 UTC (permalink / raw)
  To: Robin Murphy, Rob Herring, Mark Kettenis
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, marcan, linux-kernel,
	iommu, mohamed.mediouni, will, linux-arm-kernel, stan



On Thu, Mar 25, 2021, at 12:50, Robin Murphy wrote:
> On 2021-03-25 07:53, Sven Peter wrote:
> > 
> > 
> > On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
> >> On Sun, Mar 21, 2021 at 05:00:50PM +0100, Mark Kettenis wrote:
> >>>
> >>> As I mentioned before, not all DARTs support the full 32-bit aperture.
> >>> In particular the PCIe DARTs support a smaller address-space.  It is
> >>> not clear whether this is a restriction of the PCIe host controller or
> >>> the DART, but the Apple Device Tree has "vm-base" and "vm-size"
> >>> properties that encode the base address and size of the aperture.
> >>> These single-cell properties which is probably why for the USB DARTs
> >>> only "vm-base" is given; since "vm-base" is 0, a 32-bit number
> >>> wouldn't be able to encode the full aperture size.  We could make them
> >>> 64-bit numbers in the Linux device tree though and always be explicit
> >>> about the size.  Older Sun SPARC machines used a single "virtual-dma"
> >>> property to encode the aperture.  We could do someting similar.  You
> >>> would use this property to initialize domain->geometry.aperture_start
> >>> and domain->geometry.aperture_end in diff 3/3 of this series.
> >>
> >> 'dma-ranges' is what should be used here.
> >>
> > 
> > The iommu binding documentation [1] mentions that
> > 
> >      The device tree node of the IOMMU device's parent bus must contain a valid
> >      "dma-ranges" property that describes how the physical address space of the
> >      IOMMU maps to memory. An empty "dma-ranges" property means that there is a
> >      1:1 mapping from IOMMU to memory.
> > 
> > which, if I understand this correctly, means that the 'dma-ranges' for the
> > parent bus of the iommu should be empty since the DART hardware can see the
> > full physical address space with a 1:1 mapping.
> > 
> > 
> > The documentation also mentions that
> > 
> >       When an "iommus" property is specified in a device tree node, the IOMMU
> >       will be used for address translation. If a "dma-ranges" property exists
> >       in the device's parent node it will be ignored.
> > 
> > which means that specifying a 'dma-ranges' in the parent bus of any devices
> > that use the iommu will just be ignored.
> 
> I think that's just wrong and wants updating (or at least clarifying). 
> The high-level view now is that we use "dma-ranges" to describe 
> limitations imposed by a bridge or interconnect segment, and that can 
> certainly happen upstream of an IOMMU. As it happens, I've just recently 
> sent a patch for precisely that case[1].
> 
> I guess what it might have been trying to say is that "dma-ranges" 
> *does* become irrelevant in terms of constraining what physical memory 
> is usable for DMA, but that shouldn't imply that its meaning doesn't 
> just shift to a different purpose.
> 

Okay, now it makes sense then!

> > As a concrete example, the PCIe DART IOMMU only allows translations from iovas
> > within 0x00100000...0x3ff00000 to the entire physical address space (though
> > realistically it will only map to 16GB RAM starting at 0x800000000 on the M1).
> > 
> > I'm probably just confused or maybe the documentation is outdated but I don't
> > see how I could specify "this device can only use DMA addresses from
> > 0x00100000...0x3ff00000 but can map these via the iommu to any physical
> > address" using 'dma-ranges'.
> > 
> > Could you maybe point me to the right direction or give me a small example?
> > That would help a lot!
> 
> PCI is easy, since it's already standard practice to use "dma-ranges" to 
> describe host bridge inbound windows. Even if the restriction is really 
> out in the host-side interconnect rather than in the bridge itself, to 
> all intents and purposes it's indistinguishable so can still be 
> described the same way.
> 
> The case of a standalone device having fewer address bits wired up than 
> both its output and the corresponding IOMMU input might expect is a 
> little more awkward, since that often *does* require adding an extra 
> level of "bus" to explicitly represent that interconnect link in the DT 
> model, e.g. [2].
> 

Nice, thanks! That's exactly what I was looking for :)


Best,

Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-25  7:53     ` Sven Peter via iommu
  2021-03-25 11:50       ` Robin Murphy
@ 2021-03-25 21:41       ` Arnd Bergmann
  2021-03-26 15:59         ` Mark Kettenis
  1 sibling, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2021-03-25 21:41 UTC (permalink / raw)
  To: Sven Peter
  Cc: Rob Herring, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, Linux ARM,
	Mark Kettenis

On Thu, Mar 25, 2021 at 8:53 AM Sven Peter <sven@svenpeter.dev> wrote:
> On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
>
> I'm probably just confused or maybe the documentation is outdated but I don't
> see how I could specify "this device can only use DMA addresses from
> 0x00100000...0x3ff00000 but can map these via the iommu to any physical
> address" using 'dma-ranges'.

It sounds like this is a holdover from the original powerpc iommu, which also
had a limited set of virtual addresses in the iommu.

I would think it's sufficient to describe it in the iommu itself,
since the limitation
is more "addresses coming into the iommu must be this range" than "this device
must use that address range for talking to the iommu".

If the addresses are allocated by the iommu driver, and each iommu only has
one DMA master attached to it, having a simple range property in the iommu
node should do the trick here. If there might be multiple devices on the same
iommu but with different address ranges (which I don't think is the case), then
it could be part of the reference to the iommu.

         Arnd
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-25 21:41       ` Arnd Bergmann
@ 2021-03-26 15:59         ` Mark Kettenis
  2021-03-26 16:09           ` Arnd Bergmann
  2021-03-26 16:10           ` Sven Peter via iommu
  0 siblings, 2 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-26 15:59 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: robh, sven, devicetree, will, marcan, linux-kernel, iommu, maz,
	mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> From: Arnd Bergmann <arnd@kernel.org>
> Date: Thu, 25 Mar 2021 22:41:09 +0100
> 
> On Thu, Mar 25, 2021 at 8:53 AM Sven Peter <sven@svenpeter.dev> wrote:
> > On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
> >
> > I'm probably just confused or maybe the documentation is outdated but I don't
> > see how I could specify "this device can only use DMA addresses from
> > 0x00100000...0x3ff00000 but can map these via the iommu to any physical
> > address" using 'dma-ranges'.
> 
> It sounds like this is a holdover from the original powerpc iommu,
> which also had a limited set of virtual addresses in the iommu.
> 
> I would think it's sufficient to describe it in the iommu itself,
> since the limitation is more "addresses coming into the iommu must
> be this range" than "this device must use that address range for
> talking to the iommu".
> 
> If the addresses are allocated by the iommu driver, and each iommu
> only has one DMA master attached to it, having a simple range
> property in the iommu node should do the trick here. If there might
> be multiple devices on the same iommu but with different address
> ranges (which I don't think is the case), then it could be part of
> the reference to the iommu.

The ADT has properties on the iommu node that describe the adresses it
accepts for translation ("vm-base" and "vm-size").  So I think we can
safely assume that the same limits apply to all DMA masters that are
attached to it.  We don't know if the range limit is baked into the
silicon or whether it is related to how the firmware sets things up.
Having the properties on the iommu node makes it easy for m1n1 to
update the properties with the right values if necessary.

Some of the DARTs provide a bypass facility.  That code make using the
standard "dma-ranges" property tricky.  That property would need to
contain the bypass address range.  But that would mean that if the
DART driver needs to look at that property to figure out the address
range that supports translation it will need to be able to distinguish
between the translatable address range and the bypass address range.

Cheers,

Mark
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 15:59         ` Mark Kettenis
@ 2021-03-26 16:09           ` Arnd Bergmann
  2021-03-26 16:10           ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Arnd Bergmann @ 2021-03-26 16:09 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: Rob Herring, sven, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Robin Murphy, Linux ARM, Stan Skowronek

On Fri, Mar 26, 2021 at 4:59 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Arnd Bergmann <arnd@kernel.org>
> > Date: Thu, 25 Mar 2021 22:41:09 +0100
> >
> > On Thu, Mar 25, 2021 at 8:53 AM Sven Peter <sven@svenpeter.dev> wrote:
> > > On Tue, Mar 23, 2021, at 21:53, Rob Herring wrote:
> > >
> > > I'm probably just confused or maybe the documentation is outdated but I don't
> > > see how I could specify "this device can only use DMA addresses from
> > > 0x00100000...0x3ff00000 but can map these via the iommu to any physical
> > > address" using 'dma-ranges'.
> >
> > It sounds like this is a holdover from the original powerpc iommu,
> > which also had a limited set of virtual addresses in the iommu.
> >
> > I would think it's sufficient to describe it in the iommu itself,
> > since the limitation is more "addresses coming into the iommu must
> > be this range" than "this device must use that address range for
> > talking to the iommu".
> >
> > If the addresses are allocated by the iommu driver, and each iommu
> > only has one DMA master attached to it, having a simple range
> > property in the iommu node should do the trick here. If there might
> > be multiple devices on the same iommu but with different address
> > ranges (which I don't think is the case), then it could be part of
> > the reference to the iommu.
>
> The ADT has properties on the iommu node that describe the adresses it
> accepts for translation ("vm-base" and "vm-size").  So I think we can
> safely assume that the same limits apply to all DMA masters that are
> attached to it.  We don't know if the range limit is baked into the
> silicon or whether it is related to how the firmware sets things up.
> Having the properties on the iommu node makes it easy for m1n1 to
> update the properties with the right values if necessary.

Ok.

> Some of the DARTs provide a bypass facility.  That code make using the
> standard "dma-ranges" property tricky.  That property would need to
> contain the bypass address range.  But that would mean that if the
> DART driver needs to look at that property to figure out the address
> range that supports translation it will need to be able to distinguish
> between the translatable address range and the bypass address range.

I'm not following here. Do you mean the bus address used for bypass is
different from the upstream address that gets programmed into the iommu
when it is active?

          Arnd
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 15:59         ` Mark Kettenis
  2021-03-26 16:09           ` Arnd Bergmann
@ 2021-03-26 16:10           ` Sven Peter via iommu
  2021-03-26 16:38             ` Arnd Bergmann
  1 sibling, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-26 16:10 UTC (permalink / raw)
  To: Mark Kettenis, Arnd Bergmann
  Cc: Rob Herring, devicetree, Robin Murphy, Hector Martin,
	linux-kernel, iommu, Marc Zyngier, mohamed.mediouni, Will Deacon,
	linux-arm-kernel, stan



On Fri, Mar 26, 2021, at 16:59, Mark Kettenis wrote:
> Some of the DARTs provide a bypass facility.  That code make using the
> standard "dma-ranges" property tricky.  That property would need to
> contain the bypass address range.  But that would mean that if the
> DART driver needs to look at that property to figure out the address
> range that supports translation it will need to be able to distinguish
> between the translatable address range and the bypass address range.

Do we understand if and why we even need to bypass certain streams?
And do you have an example for a node in the ADT that contains this bypass range?

I've only seen nodes with "bypass" and "bypass-adress" but that could just be
some software abstraction Apple uses which doesn't map well to Linux or other OSes
and might not even be required here.


Sven

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 16:10           ` Sven Peter via iommu
@ 2021-03-26 16:38             ` Arnd Bergmann
  2021-03-26 17:06               ` Sven Peter via iommu
  2021-03-26 17:26               ` Mark Kettenis
  0 siblings, 2 replies; 36+ messages in thread
From: Arnd Bergmann @ 2021-03-26 16:38 UTC (permalink / raw)
  To: Sven Peter
  Cc: Rob Herring, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, Linux ARM,
	Mark Kettenis

On Fri, Mar 26, 2021 at 5:10 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Fri, Mar 26, 2021, at 16:59, Mark Kettenis wrote:
> > Some of the DARTs provide a bypass facility.  That code make using the
> > standard "dma-ranges" property tricky.  That property would need to
> > contain the bypass address range.  But that would mean that if the
> > DART driver needs to look at that property to figure out the address
> > range that supports translation it will need to be able to distinguish
> > between the translatable address range and the bypass address range.
>
> Do we understand if and why we even need to bypass certain streams?

My guess is that this is a performance optimization.

There are generally three reasons to want an iommu in the first place:
 - Pass a device down to a guest or user process without giving
   access to all of memory
 - Avoid problems with limitations in the device, typically when it
only supports
   32-bit bus addressing, but the installed memory is larger than 4GB
 - Protect kernel memory from broken drivers

If you care about none of the above, but you do care about data transfer
speed, you are better off just leaving the IOMMU in bypass mode.
I don't think we have to support it if the IOMMU works reliably, but it's
something that users might want.

        Arnd
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 16:38             ` Arnd Bergmann
@ 2021-03-26 17:06               ` Sven Peter via iommu
  2021-03-26 17:26               ` Mark Kettenis
  1 sibling, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-26 17:06 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rob Herring, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, Linux ARM,
	Mark Kettenis



On Fri, Mar 26, 2021, at 17:38, Arnd Bergmann wrote:
> On Fri, Mar 26, 2021 at 5:10 PM Sven Peter <sven@svenpeter.dev> wrote:
> > On Fri, Mar 26, 2021, at 16:59, Mark Kettenis wrote:
> > > Some of the DARTs provide a bypass facility.  That code make using the
> > > standard "dma-ranges" property tricky.  That property would need to
> > > contain the bypass address range.  But that would mean that if the
> > > DART driver needs to look at that property to figure out the address
> > > range that supports translation it will need to be able to distinguish
> > > between the translatable address range and the bypass address range.
> >
> > Do we understand if and why we even need to bypass certain streams?
> 
> My guess is that this is a performance optimization.

Makes sense.

> 
> There are generally three reasons to want an iommu in the first place:
>  - Pass a device down to a guest or user process without giving
>    access to all of memory
>  - Avoid problems with limitations in the device, typically when it
> only supports
>    32-bit bus addressing, but the installed memory is larger than 4GB
>  - Protect kernel memory from broken drivers
> 
> If you care about none of the above, but you do care about data transfer
> speed, you are better off just leaving the IOMMU in bypass mode.
> I don't think we have to support it if the IOMMU works reliably, but it's
> something that users might want.

Right now the IOMMU works very reliably while bypass mode seems to be tricky
at best. I think I partly know how to enable it but it looks like either not
every DART or DART/master combination even supports it or that there is
some additional configuration required to make it work reliably.

I had it working with the USB DART at one point but I needed to enable it in
all 16 streams of the IOMMU even though the pagetables only need to be setup
in one stream as indicated by the ADT.
I couldn't get it to work at all for the framebuffer IOMMU.

I think it's fine to skip it for now until it's either actually required due
to some hardware quirk or once we have users requesting support. Apple uses
almost all IOMMUs without bypass mode if that ADT is to be believed though.


Best,

Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 16:38             ` Arnd Bergmann
  2021-03-26 17:06               ` Sven Peter via iommu
@ 2021-03-26 17:26               ` Mark Kettenis
  2021-03-26 17:34                 ` Robin Murphy
  2021-03-26 20:03                 ` Arnd Bergmann
  1 sibling, 2 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-26 17:26 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: robh, sven, devicetree, will, marcan, linux-kernel, iommu, maz,
	mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> From: Arnd Bergmann <arnd@kernel.org>
> Date: Fri, 26 Mar 2021 17:38:24 +0100
> 
> On Fri, Mar 26, 2021 at 5:10 PM Sven Peter <sven@svenpeter.dev> wrote:
> > On Fri, Mar 26, 2021, at 16:59, Mark Kettenis wrote:
> > > Some of the DARTs provide a bypass facility.  That code make using the
> > > standard "dma-ranges" property tricky.  That property would need to
> > > contain the bypass address range.  But that would mean that if the
> > > DART driver needs to look at that property to figure out the address
> > > range that supports translation it will need to be able to distinguish
> > > between the translatable address range and the bypass address range.
> >
> > Do we understand if and why we even need to bypass certain streams?
> 
> My guess is that this is a performance optimization.
> 
> There are generally three reasons to want an iommu in the first place:
>  - Pass a device down to a guest or user process without giving
>    access to all of memory
>  - Avoid problems with limitations in the device, typically when it
> only supports
>    32-bit bus addressing, but the installed memory is larger than 4GB
>  - Protect kernel memory from broken drivers
> 
> If you care about none of the above, but you do care about data transfer
> speed, you are better off just leaving the IOMMU in bypass mode.
> I don't think we have to support it if the IOMMU works reliably, but it's
> something that users might want.

Another reason might be that a device needs access to large amounts of
physical memory at the same time and the 32-bit address space that the
DART provides is too tight.

In U-Boot I might want to use bypass where it works since there is no
proper IOMMU support in U-Boot.  Generally speaking, the goal is to
keep the code size down in U-Boot.  In OpenBSD I'll probably avoid
bypass mode if I can.

I haven't figured out how the bypass stuff really works.  Corellium
added support for it in their codebase when they added support for
Thunderbolt, and some of the DARTs that seem to be related to
Thunderbolt do indeed have a "bypass" property.  But it is unclear to
me how the different puzzle pieces fit together for Thunderbolt.

Anyway, from my viewpoint having the information about the IOVA
address space sit on the devices makes little sense.  This information
is needed by the DART driver, and there is no direct cnnection from
the DART to the individual devices in the devicetree.  The "iommus"
property makes a connection in the opposite direction.

Cheers,

Mark
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 17:26               ` Mark Kettenis
@ 2021-03-26 17:34                 ` Robin Murphy
  2021-03-26 17:51                   ` Sven Peter via iommu
  2021-03-26 20:03                 ` Arnd Bergmann
  1 sibling, 1 reply; 36+ messages in thread
From: Robin Murphy @ 2021-03-26 17:34 UTC (permalink / raw)
  To: Mark Kettenis, Arnd Bergmann
  Cc: robh, sven, devicetree, maz, marcan, linux-kernel, iommu,
	mohamed.mediouni, will, linux-arm-kernel, stan

On 2021-03-26 17:26, Mark Kettenis wrote:
>> From: Arnd Bergmann <arnd@kernel.org>
>> Date: Fri, 26 Mar 2021 17:38:24 +0100
>>
>> On Fri, Mar 26, 2021 at 5:10 PM Sven Peter <sven@svenpeter.dev> wrote:
>>> On Fri, Mar 26, 2021, at 16:59, Mark Kettenis wrote:
>>>> Some of the DARTs provide a bypass facility.  That code make using the
>>>> standard "dma-ranges" property tricky.  That property would need to
>>>> contain the bypass address range.  But that would mean that if the
>>>> DART driver needs to look at that property to figure out the address
>>>> range that supports translation it will need to be able to distinguish
>>>> between the translatable address range and the bypass address range.
>>>
>>> Do we understand if and why we even need to bypass certain streams?
>>
>> My guess is that this is a performance optimization.
>>
>> There are generally three reasons to want an iommu in the first place:
>>   - Pass a device down to a guest or user process without giving
>>     access to all of memory
>>   - Avoid problems with limitations in the device, typically when it
>> only supports
>>     32-bit bus addressing, but the installed memory is larger than 4GB
>>   - Protect kernel memory from broken drivers
>>
>> If you care about none of the above, but you do care about data transfer
>> speed, you are better off just leaving the IOMMU in bypass mode.
>> I don't think we have to support it if the IOMMU works reliably, but it's
>> something that users might want.
> 
> Another reason might be that a device needs access to large amounts of
> physical memory at the same time and the 32-bit address space that the
> DART provides is too tight.
> 
> In U-Boot I might want to use bypass where it works since there is no
> proper IOMMU support in U-Boot.  Generally speaking, the goal is to
> keep the code size down in U-Boot.  In OpenBSD I'll probably avoid
> bypass mode if I can.
> 
> I haven't figured out how the bypass stuff really works.  Corellium
> added support for it in their codebase when they added support for
> Thunderbolt, and some of the DARTs that seem to be related to
> Thunderbolt do indeed have a "bypass" property.  But it is unclear to
> me how the different puzzle pieces fit together for Thunderbolt.
> 
> Anyway, from my viewpoint having the information about the IOVA
> address space sit on the devices makes little sense.  This information
> is needed by the DART driver, and there is no direct cnnection from
> the DART to the individual devices in the devicetree.  The "iommus"
> property makes a connection in the opposite direction.

What still seems unclear is whether these addressing limitations are a 
property of the DART input interface, the device output interface, or 
the interconnect between them. Although the observable end result 
appears more or less the same either way, they are conceptually 
different things which we have different abstractions to deal with.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 17:34                 ` Robin Murphy
@ 2021-03-26 17:51                   ` Sven Peter via iommu
  2021-03-26 19:59                     ` Arnd Bergmann
  0 siblings, 1 reply; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-26 17:51 UTC (permalink / raw)
  To: Robin Murphy, Mark Kettenis, Arnd Bergmann
  Cc: Rob Herring, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, iommu, mohamed.mediouni, Will Deacon,
	linux-arm-kernel, stan



On Fri, Mar 26, 2021, at 18:34, Robin Murphy wrote:
> On 2021-03-26 17:26, Mark Kettenis wrote:
> > 
> > Anyway, from my viewpoint having the information about the IOVA
> > address space sit on the devices makes little sense.  This information
> > is needed by the DART driver, and there is no direct cnnection from
> > the DART to the individual devices in the devicetree.  The "iommus"
> > property makes a connection in the opposite direction.
> 
> What still seems unclear is whether these addressing limitations are a 
> property of the DART input interface, the device output interface, or 
> the interconnect between them. Although the observable end result 
> appears more or less the same either way, they are conceptually 
> different things which we have different abstractions to deal with.
> 
> Robin.
>

I'm not really sure if there is any way for us to figure out where these
limitation comes from though.

I've done some more experiments and looked at all DART nodes in Apple's Device
Tree though. It seems that most (if not all) masters only connect 32 address
lines even though the iommu can handle a much larger address space. I'll therefore
remove the code to handle the full space for v2 since it's essentially dead
code that can't be tested anyway.


There are some exceptions though:

There are the PCIe DARTs which have a different limitation which could be
encoded as 'dma-ranges' in the pci bus node:

           name         base      size
         dart-apcie1: 00100000  3fe00000
         dart-apcie2: 00100000  3fe00000
         dart-apcie0: 00100000  3fe00000
        dart-apciec0: 00004000  7fffc000
        dart-apciec1: 80000000  7fffc000

Then there are also these display controller DARTs. If we wanted to use dma-ranges
we could just put them in a single sub bus:

              name     base      size
          dart-disp0: 00000000 fc000000
            dart-dcp: 00000000 fc000000
       dart-dispext0: 00000000 fc000000
         dart-dcpext: 00000000 fc000000


And finally we have these strange ones which might eventually each require
another awkward sub-bus if we want to stick to the dma-ranges property.

    name     base      size
  dart-aop: 00030000 ffffffff ("always-on processor")
  dart-pmp: 00000000 bff00000 (no idea yet)
  dart-sio: 0021c000 fbde4000 (at least their Secure Enclave/TPM co-processor)
  dart-ane: 00000000 e0000000 ("Neural Engine", their ML accelerator)


For all we know these limitations could even arise for different reasons.
(the secure enclave one looks like it might be imposed by the code running
on there).


Not really sure to proceed from here. I'll give the dma-ranges options a try
for v2 and see how that one works out but that's not going to help us understand
*why* these limitations exist. 
At least I won't have to change much code if we agree on a different abstraction :)

The important ones for now are probably the USB and the PCIe ones. We'll need the
display ones after that and can probably ignore the strange ones for quite a while.




Best,

Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 17:51                   ` Sven Peter via iommu
@ 2021-03-26 19:59                     ` Arnd Bergmann
  2021-03-26 21:16                       ` Mark Kettenis
  2021-03-27 15:30                       ` Sven Peter via iommu
  0 siblings, 2 replies; 36+ messages in thread
From: Arnd Bergmann @ 2021-03-26 19:59 UTC (permalink / raw)
  To: Sven Peter
  Cc: Rob Herring, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, Linux ARM,
	Mark Kettenis

On Fri, Mar 26, 2021 at 6:51 PM Sven Peter <sven@svenpeter.dev> wrote:
> On Fri, Mar 26, 2021, at 18:34, Robin Murphy wrote:
> > On 2021-03-26 17:26, Mark Kettenis wrote:
> > >
> > > Anyway, from my viewpoint having the information about the IOVA
> > > address space sit on the devices makes little sense.  This information
> > > is needed by the DART driver, and there is no direct cnnection from
> > > the DART to the individual devices in the devicetree.  The "iommus"
> > > property makes a connection in the opposite direction.
> >
> > What still seems unclear is whether these addressing limitations are a
> > property of the DART input interface, the device output interface, or
> > the interconnect between them. Although the observable end result
> > appears more or less the same either way, they are conceptually
> > different things which we have different abstractions to deal with.
>
> I'm not really sure if there is any way for us to figure out where these
> limitation comes from though.

My first guess was that this is done to partition the available address
address space in a way that allows one physical IOTLB to handle
multiple devices that each have their own page table for a subset
of the address space, as was done on old PowerPC IOMMUs.
However, the ranges you list don't really support that model.

> I've done some more experiments and looked at all DART nodes in Apple's Device
> Tree though. It seems that most (if not all) masters only connect 32 address
> lines even though the iommu can handle a much larger address space. I'll therefore
> remove the code to handle the full space for v2 since it's essentially dead
> code that can't be tested anyway.
>
>
> There are some exceptions though:
>
> There are the PCIe DARTs which have a different limitation which could be
> encoded as 'dma-ranges' in the pci bus node:
>
>            name         base      size
>          dart-apcie1: 00100000  3fe00000
>          dart-apcie2: 00100000  3fe00000
>          dart-apcie0: 00100000  3fe00000
>         dart-apciec0: 00004000  7fffc000
>         dart-apciec1: 80000000  7fffc000

This looks like they are reserving some address space in the beginning
and/or the end, and for apciec0, the address space is partitioned into
two equal-sized regions.

> Then there are also these display controller DARTs. If we wanted to use dma-ranges
> we could just put them in a single sub bus:
>
>               name     base      size
>           dart-disp0: 00000000 fc000000
>             dart-dcp: 00000000 fc000000
>        dart-dispext0: 00000000 fc000000
>          dart-dcpext: 00000000 fc000000
>
>
> And finally we have these strange ones which might eventually each require
> another awkward sub-bus if we want to stick to the dma-ranges property.
>
>     name     base      size
>   dart-aop: 00030000 ffffffff ("always-on processor")
>   dart-pmp: 00000000 bff00000 (no idea yet)

Here I also see a "pio-vm-size" property:

    dart-pmp {
      pio-vm-base = <0xc0000000>;
      pio-vm-size = <0x40000000>;
      vm-size = <0xbff00000>;
      ...
   };

Which seems to give 3GB of address space to the normal iotlb,
plus the last 1GB to something else. The vm-base property is also
missing rather than zero, but that could just be part of their syntax
instead of a real difference.

Could it be that there are

>   dart-sio: 0021c000 fbde4000 (at least their Secure Enclave/TPM co-processor)

Same here:
    dart-sio {
       vm-base = <0x0>;
       vm-size = <0xfc000000>;
       pio-vm-base = <0xfd000000>;
      pio-vm-size = <0x2000000>;
      pio-granularity = <0x1000000>;
   }

There are clearly two distinct ranges that split up the 4GB space again,
with a small hole of 16MB (==pio-granularity) at the end of each range.

The "pio" name might indicate that this is a range of addresses that
can be programmed to point at I/O registers in another device, rather
than pointing to RAM.

           Arnd
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 17:26               ` Mark Kettenis
  2021-03-26 17:34                 ` Robin Murphy
@ 2021-03-26 20:03                 ` Arnd Bergmann
  2021-03-26 21:13                   ` Mark Kettenis
  1 sibling, 1 reply; 36+ messages in thread
From: Arnd Bergmann @ 2021-03-26 20:03 UTC (permalink / raw)
  To: Mark Kettenis
  Cc: Rob Herring, sven, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Robin Murphy, Linux ARM, Stan Skowronek

On Fri, Mar 26, 2021 at 6:28 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> I haven't figured out how the bypass stuff really works.  Corellium
> added support for it in their codebase when they added support for
> Thunderbolt, and some of the DARTs that seem to be related to
> Thunderbolt do indeed have a "bypass" property.  But it is unclear to
> me how the different puzzle pieces fit together for Thunderbolt.

As a general observation, bypass mode for Thunderbolt is what enabled
the http://thunderclap.io/ attack. This is extremely useful for debugging
a running kernel from another machine, but it's also something that
should never be done in a production kernel.

         Arnd
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 20:03                 ` Arnd Bergmann
@ 2021-03-26 21:13                   ` Mark Kettenis
  0 siblings, 0 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-26 21:13 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: robh, sven, devicetree, will, marcan, linux-kernel, iommu, maz,
	mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> From: Arnd Bergmann <arnd@kernel.org>
> Date: Fri, 26 Mar 2021 21:03:32 +0100
> 
> On Fri, Mar 26, 2021 at 6:28 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > I haven't figured out how the bypass stuff really works.  Corellium
> > added support for it in their codebase when they added support for
> > Thunderbolt, and some of the DARTs that seem to be related to
> > Thunderbolt do indeed have a "bypass" property.  But it is unclear to
> > me how the different puzzle pieces fit together for Thunderbolt.
> 
> As a general observation, bypass mode for Thunderbolt is what enabled
> the http://thunderclap.io/ attack. This is extremely useful for debugging
> a running kernel from another machine, but it's also something that
> should never be done in a production kernel.

No kidding!  I was surprised to see the bypass support on the
Thunderbolt-related nodes.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 19:59                     ` Arnd Bergmann
@ 2021-03-26 21:16                       ` Mark Kettenis
  2021-03-27 15:30                       ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Mark Kettenis @ 2021-03-26 21:16 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: robh, sven, devicetree, will, marcan, linux-kernel, iommu, maz,
	mohamed.mediouni, robin.murphy, linux-arm-kernel, stan

> From: Arnd Bergmann <arnd@kernel.org>
> Date: Fri, 26 Mar 2021 20:59:58 +0100
> 
> On Fri, Mar 26, 2021 at 6:51 PM Sven Peter <sven@svenpeter.dev> wrote:
> > On Fri, Mar 26, 2021, at 18:34, Robin Murphy wrote:
> > > On 2021-03-26 17:26, Mark Kettenis wrote:
> > > >
> > > > Anyway, from my viewpoint having the information about the IOVA
> > > > address space sit on the devices makes little sense.  This information
> > > > is needed by the DART driver, and there is no direct cnnection from
> > > > the DART to the individual devices in the devicetree.  The "iommus"
> > > > property makes a connection in the opposite direction.
> > >
> > > What still seems unclear is whether these addressing limitations are a
> > > property of the DART input interface, the device output interface, or
> > > the interconnect between them. Although the observable end result
> > > appears more or less the same either way, they are conceptually
> > > different things which we have different abstractions to deal with.
> >
> > I'm not really sure if there is any way for us to figure out where these
> > limitation comes from though.
> 
> My first guess was that this is done to partition the available address
> address space in a way that allows one physical IOTLB to handle
> multiple devices that each have their own page table for a subset
> of the address space, as was done on old PowerPC IOMMUs.
> However, the ranges you list don't really support that model.
> 
> > I've done some more experiments and looked at all DART nodes in Apple's Device
> > Tree though. It seems that most (if not all) masters only connect 32 address
> > lines even though the iommu can handle a much larger address space. I'll therefore
> > remove the code to handle the full space for v2 since it's essentially dead
> > code that can't be tested anyway.
> >
> >
> > There are some exceptions though:
> >
> > There are the PCIe DARTs which have a different limitation which could be
> > encoded as 'dma-ranges' in the pci bus node:
> >
> >            name         base      size
> >          dart-apcie1: 00100000  3fe00000
> >          dart-apcie2: 00100000  3fe00000
> >          dart-apcie0: 00100000  3fe00000
> >         dart-apciec0: 00004000  7fffc000
> >         dart-apciec1: 80000000  7fffc000
> 
> This looks like they are reserving some address space in the beginning
> and/or the end, and for apciec0, the address space is partitioned into
> two equal-sized regions.
> 
> > Then there are also these display controller DARTs. If we wanted to use dma-ranges
> > we could just put them in a single sub bus:
> >
> >               name     base      size
> >           dart-disp0: 00000000 fc000000
> >             dart-dcp: 00000000 fc000000
> >        dart-dispext0: 00000000 fc000000
> >          dart-dcpext: 00000000 fc000000
> >
> >
> > And finally we have these strange ones which might eventually each require
> > another awkward sub-bus if we want to stick to the dma-ranges property.
> >
> >     name     base      size
> >   dart-aop: 00030000 ffffffff ("always-on processor")
> >   dart-pmp: 00000000 bff00000 (no idea yet)
> 
> Here I also see a "pio-vm-size" property:
> 
>     dart-pmp {
>       pio-vm-base = <0xc0000000>;
>       pio-vm-size = <0x40000000>;
>       vm-size = <0xbff00000>;
>       ...
>    };
> 
> Which seems to give 3GB of address space to the normal iotlb,
> plus the last 1GB to something else. The vm-base property is also
> missing rather than zero, but that could just be part of their syntax
> instead of a real difference.

Yes.  It seems like "vm-base" is omitted when it is 0, and "vm-size"
is omitted when the end of the window is at 0xffffffff.

> 
> Could it be that there are
> 
> >   dart-sio: 0021c000 fbde4000 (at least their Secure Enclave/TPM co-processor)
> 
> Same here:
>     dart-sio {
>        vm-base = <0x0>;
>        vm-size = <0xfc000000>;
>        pio-vm-base = <0xfd000000>;
>       pio-vm-size = <0x2000000>;
>       pio-granularity = <0x1000000>;
>    }
> 
> There are clearly two distinct ranges that split up the 4GB space again,
> with a small hole of 16MB (==pio-granularity) at the end of each range.
> 
> The "pio" name might indicate that this is a range of addresses that
> can be programmed to point at I/O registers in another device, rather
> than pointing to RAM.
> 
>            Arnd
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-26 19:59                     ` Arnd Bergmann
  2021-03-26 21:16                       ` Mark Kettenis
@ 2021-03-27 15:30                       ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-27 15:30 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Rob Herring, DTML, Will Deacon, Hector Martin,
	Linux Kernel Mailing List, open list:IOMMU DRIVERS, Marc Zyngier,
	Mohamed Mediouni, Stan Skowronek, Robin Murphy, Linux ARM,
	Mark Kettenis



On Fri, Mar 26, 2021, at 20:59, Arnd Bergmann wrote:
> On Fri, Mar 26, 2021 at 6:51 PM Sven Peter <sven@svenpeter.dev> wrote:
> >   dart-sio: 0021c000 fbde4000 (at least their Secure Enclave/TPM co-processor)
> 
> Same here:
>     dart-sio {
>        vm-base = <0x0>;
>        vm-size = <0xfc000000>;
>        pio-vm-base = <0xfd000000>;
>       pio-vm-size = <0x2000000>;
>       pio-granularity = <0x1000000>;
>    }
> 
> There are clearly two distinct ranges that split up the 4GB space again,
> with a small hole of 16MB (==pio-granularity) at the end of each range.
> 
> The "pio" name might indicate that this is a range of addresses that
> can be programmed to point at I/O registers in another device, rather
> than pointing to RAM.
> 
>            Arnd
>

Very interesting observation!

Mark and I have discussed this a little bit further on IRC. Mark also successfully
used the PCIe DARTs with a DMA window outside of the one specified by vm-base/vm-size
in the ADT.

I believe that the (pio-)vm-base/size properties merely specify the ranges their
allocator uses and do not describe actual hardware limitations. Mark also suggested
that they might reserve memory at the beginning to find bugs similar to how one
might not allow to map memory at 0x0.

I have also done a few more experiments and figured out that if I put the IOMMU
into bypass mode (which doesn't seem to work for all IOMMUs/master combinations
which is why I'll leave it out of this series for now until I figure out more
details) I *can* use the full address space. I think the limitation
is therefore imposed by the translation hardware inside the IOMMU and not by
the bus/the interconnect.

If that's correct I think the right place to enforce this is to just limit
the aperture inside the DART driver to a 32bit address space whenever address
translation is enabled.


Thanks,

Sven

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH 0/3] Apple M1 DART IOMMU driver
  2021-03-25 11:50       ` Robin Murphy
  2021-03-25 20:49         ` Sven Peter via iommu
@ 2021-03-27 15:33         ` Sven Peter via iommu
  1 sibling, 0 replies; 36+ messages in thread
From: Sven Peter via iommu @ 2021-03-27 15:33 UTC (permalink / raw)
  To: Robin Murphy, Rob Herring, Mark Kettenis
  Cc: Arnd Bergmann, devicetree, Marc Zyngier, Hector Martin,
	linux-kernel, iommu, mohamed.mediouni, Will Deacon,
	linux-arm-kernel, stan

Hi Robin,


On Thu, Mar 25, 2021, at 12:50, Robin Murphy wrote:
> On 2021-03-25 07:53, Sven Peter wrote:
> > The iommu binding documentation [1] mentions that
> > 
> >      The device tree node of the IOMMU device's parent bus must contain a valid
> >      "dma-ranges" property that describes how the physical address space of the
> >      IOMMU maps to memory. An empty "dma-ranges" property means that there is a
> >      1:1 mapping from IOMMU to memory.
> > 
> > which, if I understand this correctly, means that the 'dma-ranges' for the
> > parent bus of the iommu should be empty since the DART hardware can see the
> > full physical address space with a 1:1 mapping.
> > 
> > 
> > The documentation also mentions that
> > 
> >       When an "iommus" property is specified in a device tree node, the IOMMU
> >       will be used for address translation. If a "dma-ranges" property exists
> >       in the device's parent node it will be ignored.
> > 
> > which means that specifying a 'dma-ranges' in the parent bus of any devices
> > that use the iommu will just be ignored.
> 
> I think that's just wrong and wants updating (or at least clarifying). 
> The high-level view now is that we use "dma-ranges" to describe 
> limitations imposed by a bridge or interconnect segment, and that can 
> certainly happen upstream of an IOMMU. As it happens, I've just recently 
> sent a patch for precisely that case[1].
> 
> I guess what it might have been trying to say is that "dma-ranges" 
> *does* become irrelevant in terms of constraining what physical memory 
> is usable for DMA, but that shouldn't imply that its meaning doesn't 
> just shift to a different purpose.

Should I add a patch to clarify this paragraph for v2 or submit a separate
one-off patch? I'm not entirely sure about the process here but I could add
a Suggested-by: to the commit if that's fine with you.



Best,

Sven
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-03-27 15:34 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 15:19 [PATCH 0/3] Apple M1 DART IOMMU driver Sven Peter via iommu
2021-03-20 15:19 ` [PATCH 1/3] iommu: io-pgtable: add DART pagetable format Sven Peter via iommu
2021-03-24 16:37   ` Robin Murphy
2021-03-25 20:47     ` Sven Peter via iommu
2021-03-20 15:20 ` [PATCH 2/3] dt-bindings: iommu: add DART iommu bindings Sven Peter via iommu
2021-03-22  0:15   ` Rob Herring
2021-03-22 18:16     ` Sven Peter via iommu
2021-03-21 16:00 ` [PATCH 0/3] Apple M1 DART IOMMU driver Mark Kettenis
2021-03-21 17:22   ` Sven Peter via iommu
2021-03-21 18:35     ` Mark Kettenis
2021-03-22 22:17       ` Sven Peter via iommu
2021-03-23 20:00         ` Mark Kettenis
2021-03-23 21:03           ` Sven Peter via iommu
2021-03-21 17:28   ` Sven Peter via iommu
2021-03-23 20:53   ` Rob Herring
2021-03-23 22:33     ` Mark Kettenis
2021-03-25  7:53     ` Sven Peter via iommu
2021-03-25 11:50       ` Robin Murphy
2021-03-25 20:49         ` Sven Peter via iommu
2021-03-27 15:33         ` Sven Peter via iommu
2021-03-25 21:41       ` Arnd Bergmann
2021-03-26 15:59         ` Mark Kettenis
2021-03-26 16:09           ` Arnd Bergmann
2021-03-26 16:10           ` Sven Peter via iommu
2021-03-26 16:38             ` Arnd Bergmann
2021-03-26 17:06               ` Sven Peter via iommu
2021-03-26 17:26               ` Mark Kettenis
2021-03-26 17:34                 ` Robin Murphy
2021-03-26 17:51                   ` Sven Peter via iommu
2021-03-26 19:59                     ` Arnd Bergmann
2021-03-26 21:16                       ` Mark Kettenis
2021-03-27 15:30                       ` Sven Peter via iommu
2021-03-26 20:03                 ` Arnd Bergmann
2021-03-26 21:13                   ` Mark Kettenis
2021-03-24 15:29 ` Robin Murphy
2021-03-25  7:58   ` Sven Peter via iommu

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).