Linux RISC-V IOMMU Support

Add bindings for the RISC-V IOMMU device drivers.

Co-developed-by Anup Patel <[email protected]>

Signed-off-by Anup Patel <[email protected]>

Signed-off-by Tomasz Jeznach <[email protected]>

Add bindings for the RISC-V IOMMU device drivers.

Co-developed-by Anup Patel <[email protected]>

Signed-off-by Anup Patel <[email protected]>

Signed-off-by Tomasz Jeznach <[email protected]>

---
 .../bindings/iommu/riscv,iommu.yaml           | 150 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 157 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +27 -0

new file mode 100644
index 000000000000..16817525e157
@@ -0,0 +1,150 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V IOMMU Architecture Implementation
+
+maintainers:
+  - Tomasz Jeznach <[email protected]>
+
+description: |
+  The RISC-V IOMMU provides memory address translation and isolation for
+  input and output devices, supporting per-device translation context,
+  shared process address spaces including the ATS and PRI components of
+  the PCIe specification, two stage address translation and MSI remapping.
+  It supports identical translation table format to the RISC-V address
+  translation tables with page level access and protection attributes.
+  Hardware uses in-memory command and fault reporting queues with wired
+  interrupt or MSI notifications.
+
+  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
+
+  For information on assigning RISC-V IOMMU to its peripheral devices,
+  see generic IOMMU bindings.
+
+properties:

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +6 -0

+          - const: riscv,iommu
+      - items:
+          - enum:
+              - pci1efd,edf1
+          - const: riscv,pci-iommu
+

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +73 -0

+      by a master to the IOMMU.
+
+  interrupts:
+    minItems: 1
+    maxItems: 4
+    description:
+      Wired interrupt vectors available for RISC-V IOMMU to notify the
+      RISC-V HARTS. The cause to interrupt vector is software defined
+      using IVEC IOMMU register.
+
+  msi-parent: true
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#iommu-cells'
+
+additionalProperties: false
+
+examples:
+  - |+
+    /* Example 1 (IOMMU device with wired interrupts) */
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    iommu1: iommu@1bccd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        interrupt-parent = <&aplic_smode>;
+        interrupts = <32 IRQ_TYPE_LEVEL_HIGH>,
+                     <33 IRQ_TYPE_LEVEL_HIGH>,
+                     <34 IRQ_TYPE_LEVEL_HIGH>,
+                     <35 IRQ_TYPE_LEVEL_HIGH>;
+        #iommu-cells = <1>;
+    };
+
+    /* Device with two IOMMU device IDs, 0 and 7 */
+    master1 {
+        iommus = <&iommu1 0>, <&iommu1 7>;
+    };
+
+  - |+
+    /* Example 2 (IOMMU device with shared wired interrupt) */
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    iommu2: iommu@1bccd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        interrupt-parent = <&aplic_smode>;
+        interrupts = <32 IRQ_TYPE_LEVEL_HIGH>;
+        #iommu-cells = <1>;
+    };
+
+  - |+
+    /* Example 3 (IOMMU device with MSIs) */
+    iommu3: iommu@1bcdd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        msi-parent = <&imsics_smode>;
+        #iommu-cells = <1>;
+    };
+
+  - |+
+    /* Example 4 (IOMMU PCIe device with MSIs) */
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie@30000000 {
+            device_type = "pci";
+            #address-cells = <3>;

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +9 -0

+
+            /* The IOMMU programming interface uses slot 00:01.0 */
+            iommu0: iommu@1,0 {
+               compatible = "pci1efd,edf1", "riscv,pci-iommu";
+               reg = <0x800 0 0 0 0>;
+               #iommu-cells = <1>;
+            };
+        };
+    };

MAINTAINERS +7 -0

index f6dc90559341..7fcf7c27ef6b 100644
@@ -18958,6 +18958,13 @@ F:	arch/riscv/
 N:	riscv
 K:	riscv
 
+RISC-V IOMMU
+M:	Tomasz Jeznach <[email protected]>
+L:	[email protected]
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
+
 RISC-V MICROCHIP FPGA SUPPORT
 M:	Conor Dooley <[email protected]>
 M:	Daire McNamara <[email protected]>

Signature · 1 line

2.34.1

Quoted 50 lines · Tomasz Jeznach

Add bindings for the RISC-V IOMMU device drivers.

Co-developed-by Anup Patel <[email protected]>

Signed-off-by Anup Patel <[email protected]>

Signed-off-by Tomasz Jeznach <[email protected]>

---
 .../bindings/iommu/riscv,iommu.yaml           | 150 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 157 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/riscv,iommu.yaml

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +33 -0

new file mode 100644
index 000000000000..16817525e157
@@ -0,0 +1,150 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/riscv,iommu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V IOMMU Architecture Implementation
+
+maintainers:
+  - Tomasz Jeznach <[email protected]>
+
+description: |
+  The RISC-V IOMMU provides memory address translation and isolation for
+  input and output devices, supporting per-device translation context,
+  shared process address spaces including the ATS and PRI components of
+  the PCIe specification, two stage address translation and MSI remapping.
+  It supports identical translation table format to the RISC-V address
+  translation tables with page level access and protection attributes.
+  Hardware uses in-memory command and fault reporting queues with wired
+  interrupt or MSI notifications.
+
+  Visit https://github.com/riscv-non-isa/riscv-iommu for more details.
+
+  For information on assigning RISC-V IOMMU to its peripheral devices,
+  see generic IOMMU bindings.
+
+properties:
+  # For PCIe IOMMU hardware compatible property should contain the vendor
+  # and device ID according to the PCI Bus Binding specification.
+  # Since PCI provides built-in identification methods, compatible is not
+  # actually required. For non-PCIe hardware implementations 'riscv,iommu'
+  # should be specified along with 'reg' property providing MMIO location.
+  compatible:

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml line 37

Quoted 4 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +4 -0

+    oneOf:
+      - items:
+          - enum:
+              - qemu,iommu

'qemu,iommu' is too broad. Could be other QEMU implementations of an IOMMU. So 'qemu,riscv-iommu'.

Quoted 6 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +6 -0

+          - const: riscv,iommu
+      - items:
+          - enum:
+              - pci1efd,edf1
+          - const: riscv,pci-iommu
+

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml line 53

Quoted 10 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +10 -0

+  reg:
+    maxItems: 1
+    description:
+      For non-PCI devices this represents base address and size of for the
+      IOMMU memory mapped registers interface.
+      For PCI IOMMU hardware implementation this should represent an address
+      of the IOMMU, as defined in the PCI Bus Binding reference. The reg
+      property is a five-cell address encoded as (phys.hi phys.mid phys.lo
+      size.hi size.lo), where phys.hi should contain the device's BDF as
+      0b00000000 bbbbbbbb dddddfff 00000000. The other cells should be zero.

No need to describe what the PCI address looks like.

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml line 58

Quoted 5 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +5 -0

+
+  '#iommu-cells':
+    const: 1
+    description:
+      Has to be one. The single cell describes the requester id emitted

Drop 'Has to be one'. You already said that with the schema.

Quoted 73 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +73 -0

+      by a master to the IOMMU.
+
+  interrupts:
+    minItems: 1
+    maxItems: 4
+    description:
+      Wired interrupt vectors available for RISC-V IOMMU to notify the
+      RISC-V HARTS. The cause to interrupt vector is software defined
+      using IVEC IOMMU register.
+
+  msi-parent: true
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - '#iommu-cells'
+
+additionalProperties: false
+
+examples:
+  - |+
+    /* Example 1 (IOMMU device with wired interrupts) */
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    iommu1: iommu@1bccd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        interrupt-parent = <&aplic_smode>;
+        interrupts = <32 IRQ_TYPE_LEVEL_HIGH>,
+                     <33 IRQ_TYPE_LEVEL_HIGH>,
+                     <34 IRQ_TYPE_LEVEL_HIGH>,
+                     <35 IRQ_TYPE_LEVEL_HIGH>;
+        #iommu-cells = <1>;
+    };
+
+    /* Device with two IOMMU device IDs, 0 and 7 */
+    master1 {
+        iommus = <&iommu1 0>, <&iommu1 7>;
+    };
+
+  - |+
+    /* Example 2 (IOMMU device with shared wired interrupt) */
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    iommu2: iommu@1bccd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        interrupt-parent = <&aplic_smode>;
+        interrupts = <32 IRQ_TYPE_LEVEL_HIGH>;
+        #iommu-cells = <1>;
+    };
+
+  - |+
+    /* Example 3 (IOMMU device with MSIs) */
+    iommu3: iommu@1bcdd000 {
+        compatible = "qemu,iommu", "riscv,iommu";
+        reg = <0x1bccd000 0x1000>;
+        msi-parent = <&imsics_smode>;
+        #iommu-cells = <1>;
+    };
+
+  - |+
+    /* Example 4 (IOMMU PCIe device with MSIs) */
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie@30000000 {
+            device_type = "pci";
+            #address-cells = <3>;

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml line 141

Quoted 10 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +10 -0

+            #size-cells = <2>;
+            reg = <0x0 0x30000000  0x0 0x1000000>;
+            ranges = <0x02000000 0x0 0x41000000  0x0 0x41000000  0x0 0x0f000000>;
+
+            /*
+             * The IOMMU manages all functions in this PCI domain except
+             * itself. Omit BDF 00:01.0.
+             */
+            iommu-map = <0x0 &iommu0 0x0 0x8
+                         0x9 &iommu0 0x9 0xfff7>;

Bracket each entry.

Quoted 17 lines

Documentation/devicetree/bindings/iommu/riscv,iommu.yaml new file +9 -0

+
+            /* The IOMMU programming interface uses slot 00:01.0 */
+            iommu0: iommu@1,0 {
+               compatible = "pci1efd,edf1", "riscv,pci-iommu";
+               reg = <0x800 0 0 0 0>;
+               #iommu-cells = <1>;
+            };
+        };
+    };

MAINTAINERS +0 -0

index f6dc90559341..7fcf7c27ef6b 100644
@@ -18958,6 +18958,13 @@ F:   arch/riscv/
 N:   riscv
 K:   riscv
 

Quoted 11 lines

MAINTAINERS +7 -1

+RISC-V IOMMU
+M:   Tomasz Jeznach <[email protected]>
+L:   [email protected]
+L:   [email protected]
+S:   Maintained
+F:   Documentation/devicetree/bindings/iommu/riscv,iommu.yaml
+
 RISC-V MICROCHIP FPGA SUPPORT
 M:   Conor Dooley <[email protected]>
 M:   Daire McNamara <[email protected]>
--

Quoted 2 lines

2.34.1

Quoted 4 lines

'qemu,iommu' is too broad. Could be other QEMU implementations of an IOMMU. So 'qemu,riscv-iommu'.

I'm ok with that. Any objections from others?

drivers/iommu/riscv/iommu.c new file +5 -0

+static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
+{
+	u64 ddtp;
+
+	/*

drivers/iommu/riscv/iommu.c new file +7 -0

+			return -EBUSY;
+		riscv_iommu_disable(iommu);
+	}
+
+	/* Configure accesses to in-memory data structures for CPU-native byte order. */
+	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) != !!(iommu->fctl & RISCV_IOMMU_FCTL_BE)) {
+		if (!(iommu->caps & RISCV_IOMMU_CAP_END))

Quoted 5 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c new file +5 -0

+static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
+{
+	u64 ddtp;
+
+	/*

drivers/iommu/riscv/iommu.c line 53

Quoted 10 lines

drivers/iommu/riscv/iommu.c new file +10 -0

+	 * Make sure the IOMMU is switched off or in pass-through mode during regular
+	 * boot flow and disable translation when we boot into a kexec kernel and the
+	 * previous kernel left them enabled.
+	 */
+	ddtp = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_DDTP);
+	if (ddtp & RISCV_IOMMU_DDTP_BUSY)
+		return -EBUSY;
+
+	if (FIELD_GET(RISCV_IOMMU_DDTP_MODE, ddtp) > RISCV_IOMMU_DDTP_MODE_BARE) {
+		if (!is_kdump_kernel())

Is kdump supported for RISC-V architectures? If so, the documentation in Documentation/admin-guide/kdump/kdump.rst might need an update.

There is a possibility of ongoing DMAs during the boot process of the kdump capture kernel because there's a small chance of legacy DMA setups targeting any memory location. Kdump typically allows these ongoing DMA transfers to complete, assuming they were intended for valid memory regions.

The IOMMU subsystem implements a default domain deferred attachment mechanism for this. In the kdump capture kernel, the whole device context tables are copied from the original kernel and will be overridden once the device driver calls the kernel DMA interface for the first time. This assumes that all old DMA transfers are completed after the driver's takeover.

Will you consider this for RISC-V architecture as well?

Quoted 32 lines · Baolu Lu

Quoted 5 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c new file +5 -0

+static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
+{
+	u64 ddtp;
+
+	/*

drivers/iommu/riscv/iommu.c line 53

Quoted 10 lines

drivers/iommu/riscv/iommu.c new file +10 -0

+	 * Make sure the IOMMU is switched off or in pass-through mode during regular
+	 * boot flow and disable translation when we boot into a kexec kernel and the
+	 * previous kernel left them enabled.
+	 */
+	ddtp = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_DDTP);
+	if (ddtp & RISCV_IOMMU_DDTP_BUSY)
+		return -EBUSY;
+
+	if (FIELD_GET(RISCV_IOMMU_DDTP_MODE, ddtp) > RISCV_IOMMU_DDTP_MODE_BARE) {
+		if (!is_kdump_kernel())

Is kdump supported for RISC-V architectures? If so, the documentation in Documentation/admin-guide/kdump/kdump.rst might need an update.

There is a possibility of ongoing DMAs during the boot process of the kdump capture kernel because there's a small chance of legacy DMA setups targeting any memory location. Kdump typically allows these ongoing DMA transfers to complete, assuming they were intended for valid memory regions.

The IOMMU subsystem implements a default domain deferred attachment mechanism for this. In the kdump capture kernel, the whole device context tables are copied from the original kernel and will be overridden once the device driver calls the kernel DMA interface for the first time. This assumes that all old DMA transfers are completed after the driver's takeover.

Quoted 1 line

Will you consider this for RISC-V architecture as well?

It seems we decided not to do that mess in ARM..

New architectures doing kdump should put the iommu in a full blocking state before handing over the next kernel, and this implies that devices drivers need to cleanly suspend their DMAs before going into the next kernel.

Quoted 33 lines · Jason Gunthorpe

Quoted 32 lines · Baolu Lu

Quoted 5 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c new file +5 -0

+static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
+{
+   u64 ddtp;
+
+   /*

drivers/iommu/riscv/iommu.c line 53

Quoted 10 lines

drivers/iommu/riscv/iommu.c new file +10 -0

+    * Make sure the IOMMU is switched off or in pass-through mode during regular
+    * boot flow and disable translation when we boot into a kexec kernel and the
+    * previous kernel left them enabled.
+    */
+   ddtp = riscv_iommu_readq(iommu, RISCV_IOMMU_REG_DDTP);
+   if (ddtp & RISCV_IOMMU_DDTP_BUSY)
+           return -EBUSY;
+
+   if (FIELD_GET(RISCV_IOMMU_DDTP_MODE, ddtp) > RISCV_IOMMU_DDTP_MODE_BARE) {
+           if (!is_kdump_kernel())

Is kdump supported for RISC-V architectures? If so, the documentation in Documentation/admin-guide/kdump/kdump.rst might need an update.

There is a possibility of ongoing DMAs during the boot process of the kdump capture kernel because there's a small chance of legacy DMA setups targeting any memory location. Kdump typically allows these ongoing DMA transfers to complete, assuming they were intended for valid memory regions.

The IOMMU subsystem implements a default domain deferred attachment mechanism for this. In the kdump capture kernel, the whole device context tables are copied from the original kernel and will be overridden once the device driver calls the kernel DMA interface for the first time. This assumes that all old DMA transfers are completed after the driver's takeover.

Quoted 7 lines

Quoted 1 line

Will you consider this for RISC-V architecture as well?

It seems we decided not to do that mess in ARM..

New architectures doing kdump should put the iommu in a full blocking state before handing over the next kernel, and this implies that devices drivers need to cleanly suspend their DMAs before going into the next kernel.

Glad to hear that. :-)

With the above consideration, the driver should consider it an error case where the iommu is not in the blocking state, and it's in the kdump kernel, right?

If so, probably the iommu driver should always return failure when the iommu is not in the blocking state. However, the RISC-V's logic is:

- if this is a kdump kernel, just disable iommu;
- otherwise, failure case.

This logic seems problematic.

Best regards,
baolu

Introduce device driver for PCIe implementation of RISC-V IOMMU architected hardware.

IOMMU hardware and system support for MSI or MSI-X is required by this implementation.

Vendor and device identifiers used in this patch matches QEMU implementation of the RISC-V IOMMU PCIe device, from Rivos VID (0x1efd) range allocated by the PCI-SIG.

MAINTAINERS | added iommu-pci.c already covered by matching pattern.

Link https://lore.kernel.org/qemu-devel/[email protected]/ open here

Co-developed-by Nick Kossifidis<[email protected]>

Signed-off-by Nick Kossifidis<[email protected]>

Signed-off-by Tomasz Jeznach<[email protected]>

---
  drivers/iommu/riscv/Kconfig     |   5 ++
  drivers/iommu/riscv/Makefile    |   1 +
  drivers/iommu/riscv/iommu-pci.c | 119 ++++++++++++++++++++++++++++++++
  3 files changed, 125 insertions(+)

create mode 100644 drivers/iommu/riscv/iommu-pci.c

Advertise IOMMU device and its core API.
Only minimal implementation for single identity domain type, without per-group domain protection.

Signed-off-by Tomasz Jeznach <[email protected]>

---
  drivers/iommu/riscv/iommu.c | 64 +++++++++++++++++++++++++++++++++++++
  1 file changed, 64 insertions(+)

  static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
  {
  	u64 ddtp;

drivers/iommu/riscv/iommu.c +4 -0

@@ -71,6 +126,7 @@ static int riscv_iommu_init_check(struct riscv_iommu_device *iommu)
 
  void riscv_iommu_remove(struct riscv_iommu_device *iommu)
  {
+	iommu_device_unregister(&iommu->iommu);
  	iommu_device_sysfs_remove(&iommu->iommu);
  }
 

@@ -96,8 +152,16 @@ int riscv_iommu_init(struct riscv_iommu_device *iommu)
  		goto err_sysfs;
  	}
 
+	rc = iommu_device_register(&iommu->iommu, &riscv_iommu_ops, iommu->dev);
+	if (rc) {
+		dev_err_probe(iommu->dev, rc, "cannot register iommu interface\n");

Introduce device context allocation and device directory tree management including capabilities discovery sequence, as described in Chapter 2.1 of the RISC-V IOMMU Architecture Specification.

Device directory mode will be auto detected using DDTP WARL property, using highest mode supported by the driver and hardware. If none supported can be configured, driver will fall back to global pass-through.

First level DDTP page can be located in I/O (detected using DDTP WARL) and system memory.

Only simple identity and release (blocking) protection domains are supported by this implementation.

drivers/iommu/riscv/iommu.c +7 -0

+static int riscv_iommu_attach_release_domain(struct iommu_domain *iommu_domain,
+					     struct device *dev)
+{
+	struct riscv_iommu_device *iommu = dev_to_iommu(dev);
+
+	if (iommu->ddt_mode > RISCV_IOMMU_DDTP_MODE_BARE)
+		riscv_iommu_iodir_update(iommu, dev, RISCV_IOMMU_FSC_BARE, 0);

drivers/iommu/riscv/iommu.c +1 -6

@@ -128,6 +489,7 @@ void riscv_iommu_remove(struct riscv_iommu_device *iommu)
  {
  	iommu_device_unregister(&iommu->iommu);
  	iommu_device_sysfs_remove(&iommu->iommu);
+	riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_MODE_OFF);
  }
 
  int riscv_iommu_init(struct riscv_iommu_device *iommu)

@@ -138,12 +500,13 @@ int riscv_iommu_init(struct riscv_iommu_device *iommu)
  	if (rc)
  		return dev_err_probe(iommu->dev, rc, "unexpected device state\n");
 
-	/*
-	 * Placeholder for a complete IOMMU device initialization.
-	 * For now, only bare minimum: enable global identity mapping mode and register sysfs.
-	 */
-	riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_DDTP,
-			   FIELD_PREP(RISCV_IOMMU_DDTP_MODE, RISCV_IOMMU_DDTP_MODE_BARE));

Quoted 21 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c +1 -3

@@ -128,6 +489,7 @@ void riscv_iommu_remove(struct riscv_iommu_device 
 *iommu)
   {
       iommu_device_unregister(&iommu->iommu);
       iommu_device_sysfs_remove(&iommu->iommu);
+    riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_MODE_OFF);
   }
   int riscv_iommu_init(struct riscv_iommu_device *iommu)

@@ -138,12 +500,13 @@ int riscv_iommu_init(struct riscv_iommu_device 
 *iommu)
       if (rc)
           return dev_err_probe(iommu->dev, rc, "unexpected device state\n");
-    /*
-     * Placeholder for a complete IOMMU device initialization.
-     * For now, only bare minimum: enable global identity mapping 

mode and register sysfs.
-     */
-    riscv_iommu_writeq(iommu, RISCV_IOMMU_REG_DDTP, -               FIELD_PREP(RISCV_IOMMU_DDTP_MODE, RISCV_IOMMU_DDTP_MODE_BARE));
+    rc = riscv_iommu_iodir_alloc(iommu);

Quoted 8 lines

+    if (rc)
+        goto err_init;
+
+    rc = riscv_iommu_iodir_set_mode(iommu, RISCV_IOMMU_DDTP_MODE_MAX); +    if (rc)
+        goto err_init;
      rc = iommu_device_sysfs_add(&iommu->iommu, NULL, NULL, "riscv-iommu@%s", dev_name(iommu->dev));

The device directory root page might be allocated in riscv_iommu_iodir_alloc(),

+    if (!iommu->ddt_root) {
+        iommu->ddt_root = riscv_iommu_get_pages(iommu, 0); +        iommu->ddt_phys = __pa(iommu->ddt_root); +    }

But I didn't find any place to free it in the error paths. Did I overlook anything?

Introduce device context allocation and device directory tree management including capabilities discovery sequence, as described in Chapter 2.1 of the RISC-V IOMMU Architecture Specification.

Device directory mode will be auto detected using DDTP WARL property, using highest mode supported by the driver and hardware. If none supported can be configured, driver will fall back to global pass-through.

First level DDTP page can be located in I/O (detected using DDTP WARL) and system memory.

Only simple identity and release (blocking) protection domains are supported by this implementation.

Co-developed-by Nick Kossifidis<[email protected]>

Signed-off-by Nick Kossifidis<[email protected]>

Signed-off-by Tomasz Jeznach<[email protected]>

Introduce device command submission and fault reporting queues, as described in Chapter 3.1 and 3.2 of the RISC-V IOMMU Architecture Specification.

Command and fault queues are instantiated in contiguous system memory local to IOMMU device domain, or mapped from fixed I/O space provided by the hardware implementation. Detection of the location and maximum allowed size of the queue utilize WARL properties of queue base control register. Driver implementation will try to allocate up to 128KB of system memory, while respecting hardware supported maximum queue size.

Interrupts allocation is based on interrupt vectors availability and distributed to all queues in simple round-robin fashion. For hardware Implementation with fixed event type to interrupt vector assignment IVEC WARL property is used to discover such mappings.

Address translation, command and queue fault handling in this change is limited to simple fault reporting without taking any action.

Signed-off-by Tomasz Jeznach<[email protected]>

drivers/iommu/riscv/iommu.c +9 -0

+static struct iommu_domain *riscv_iommu_alloc_paging_domain(struct device *dev)
+{
+	struct riscv_iommu_domain *domain;
+	struct riscv_iommu_device *iommu;
+
+	iommu = dev ? dev_to_iommu(dev) : NULL;
+	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return ERR_PTR(-ENOMEM);

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 770

Quoted 5 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c +5 -0

+#define iommu_domain_to_riscv(iommu_domain) \
+     container_of(iommu_domain, struct riscv_iommu_domain, domain)
+
+#define dev_to_domain(dev) \
+     iommu_domain_to_riscv(dev_iommu_priv_get(dev))

Please use the priv properly and put a struct around it, you'll certainly need this eventually to do the rest of the advanced features.

Quoted 4 lines

drivers/iommu/riscv/iommu.c +4 -0

+static void riscv_iommu_bond_unlink(struct riscv_iommu_domain *domain, struct device *dev)
+{
+     struct riscv_iommu_bond *bond, *found = NULL;
+     unsigned long flags;

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 810

Quoted 5 lines

drivers/iommu/riscv/iommu.c +5 -0

+
+     if (!domain)
+             return;
+
+     spin_lock_irqsave(&domain->lock, flags);

This is never locked from an irq, you don't need to use the irqsave variations.

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 821

Quoted 11 lines

drivers/iommu/riscv/iommu.c +11 -0

+     list_for_each_entry_rcu(bond, &domain->bonds, list) {
+             if (bond->dev == dev) {
+                     list_del_rcu(&bond->list);
+                     found = bond;
+             }
+     }
+     spin_unlock_irqrestore(&domain->lock, flags);
+
+     /* Release and wait for all read-rcu critical sections have completed. */
+     kfree_rcu(found, rcu);
+     synchronize_rcu();

Please no, synchronize_rcu() on a path like this is not so reasonable.. Also you don't need kfree_rcu() if you write it like this.

This still looks better to do what I said before, put the iommu not the dev in the bond struct.

Quoted 9 lines

drivers/iommu/riscv/iommu.c +9 -0

+static struct iommu_domain *riscv_iommu_alloc_paging_domain(struct device *dev)
+{
+     struct riscv_iommu_domain *domain;
+     struct riscv_iommu_device *iommu;
+
+     iommu = dev ? dev_to_iommu(dev) : NULL;
+     domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+     if (!domain)
+             return ERR_PTR(-ENOMEM);

drivers/iommu/riscv/iommu.c static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu, · line 1273

Quoted 10 lines

drivers/iommu/riscv/iommu.c +10 -0

+
+     INIT_LIST_HEAD_RCU(&domain->bonds);
+     spin_lock_init(&domain->lock);
+     domain->numa_node = NUMA_NO_NODE;
+
+     /*
+      * Follow system address translation mode.
+      * RISC-V IOMMU ATP mode values match RISC-V CPU SATP mode values.
+      */
+     domain->pgd_mode = satp_mode >> SATP_MODE_SHIFT;

This seems really strange, the iommu paging domains should be unrelated to what the CPU is doing. There is no connection between these two concepts.

Just pick a size that the iommu supports.

The number of radix levels is a tunable alot of iommus have that we haven't really exposed to anything else yet.

drivers/iommu/riscv/iommu.c static void riscv_iommu_iodir_update(struct riscv_iommu_device *iommu, · line 1309

Quoted 13 lines

drivers/iommu/riscv/iommu.c +13 -0

+     /*
+      * Note: RISC-V Privilege spec mandates that virtual addresses
+      * need to be sign-extended, so if (VA_BITS - 1) is set, all
+      * bits >= VA_BITS need to also be set or else we'll get a
+      * page fault. However the code that creates the mappings
+      * above us (e.g. iommu_dma_alloc_iova()) won't do that for us
+      * for now, so we'll end up with invalid virtual addresses
+      * to map. As a workaround until we get this sorted out
+      * limit the available virtual addresses to VA_BITS - 1.
+      */
+     domain->domain.geometry.aperture_start = 0;
+     domain->domain.geometry.aperture_end = DMA_BIT_MASK(VA_BITS - 1);
+     domain->domain.geometry.force_aperture = true;

Yikes.. This is probably the best solution long term anyhow, unless you need to use the last page in VFIO for some reason.

drivers/iommu/riscv/iommu.c static struct iommu_domain riscv_iommu_identity_domain = { · line 1366

Quoted 8 lines

static int riscv_iommu_device_domain_type(struct device *dev) {

drivers/iommu/riscv/iommu.c +5 -1

-     return IOMMU_DOMAIN_IDENTITY;
+     struct riscv_iommu_device *iommu = dev_to_iommu(dev);
+
+     if (iommu->ddt_mode == RISCV_IOMMU_DDTP_MODE_BARE)
+             return IOMMU_DOMAIN_IDENTITY;
+

Is there even a point of binding an iommu driver if the HW can't support a DDT table? Just return -ENODEV from probe_device?

Logically a iommu block that can't decode the RID has no association at all with a Linux struct device :)

Jason

Quoted 15 lines

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 821

Quoted 11 lines

drivers/iommu/riscv/iommu.c +11 -0

+     list_for_each_entry_rcu(bond, &domain->bonds, list) {
+             if (bond->dev == dev) {
+                     list_del_rcu(&bond->list);
+                     found = bond;
+             }
+     }
+     spin_unlock_irqrestore(&domain->lock, flags);
+
+     /* Release and wait for all read-rcu critical sections have completed. */
+     kfree_rcu(found, rcu);
+     synchronize_rcu();

Please no, synchronize_rcu() on a path like this is not so reasonable.. Also you don't need kfree_rcu() if you write it like this.

Quoted 4 lines

This still looks better to do what I said before, put the iommu not the dev in the bond struct.

I was trying not to duplicate data in bond struct and use whatever is available to be referenced from dev pointer (eg iommu / ids / private iommu dev data).

If I'm reading core iommu code correctly, device pointer and iommu pointers should be valid between _probe_device and _release_device calls. I've moved synchronize_rcu out of the domain attach path to _release_device, LMK if that would be ok for now. I'll have a second another to rework other patches to avoid storing dev pointers at all.

Quoted 2 lines

The number of radix levels is a tunable alot of iommus have that we haven't really exposed to anything else yet.

Makes sense. I've left an option to pick mode from MMU for cases where dev/iommu is not known at allocation time (with iommu_domain_alloc()). I'd guess it's reasonable to assume IOMMU supported page modes will match MMU.

Quoted 15 lines · Tomasz Jeznach

Quoted 15 lines

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 821

Quoted 11 lines

drivers/iommu/riscv/iommu.c +11 -0

+     list_for_each_entry_rcu(bond, &domain->bonds, list) {
+             if (bond->dev == dev) {
+                     list_del_rcu(&bond->list);
+                     found = bond;
+             }
+     }
+     spin_unlock_irqrestore(&domain->lock, flags);
+
+     /* Release and wait for all read-rcu critical sections have completed. */
+     kfree_rcu(found, rcu);
+     synchronize_rcu();

Please no, synchronize_rcu() on a path like this is not so reasonable.. Also you don't need kfree_rcu() if you write it like this.

Quoted 8 lines

Quoted 4 lines

This still looks better to do what I said before, put the iommu not the dev in the bond struct.

I was trying not to duplicate data in bond struct and use whatever is available to be referenced from dev pointer (eg iommu / ids / private iommu dev data).

I'm not sure that is a valuable goal considering the RCU complexity.. But I suppose it would be a bit of a hassle to replicate the ids list into bond structurs. Maybe something to do when you get to ATS since you'll probably want to replicate the ATS RIDs. (see what Intel did, which I think is pretty good)

Quoted 6 lines

If I'm reading core iommu code correctly, device pointer and iommu pointers should be valid between _probe_device and _release_device calls. I've moved synchronize_rcu out of the domain attach path to _release_device, LMK if that would be ok for now. I'll have a second another to rework other patches to avoid storing dev pointers at all.

Yes, that seems better.. I'm happier to see device hot-unplug be slow than un attach

There is another issue with the RCU that I haven't wrapped my head around..

Technically we can have concurrent map/unmap/invalidation along side device attach/detach. Does the invalidation under RCU work correctly?

For detach I think yes:

Inv CPU Detach CPU

write io_pte Update device descriptor
rcu_read_lock
list_for_each
<make invalidation command> <make description inv cmd>
dma_wmb() dma_wmb()
<doorbell> <cmd doorbell>
rcu_read_unlock

                                             list_del_rcu()
                                             <wipe ASID>

In this case I think we never miss an invalidation, the list_del is always after the HW has been fully fenced, so I don't think we can have any issue. Maybe a suprious invalidation if the ASID gets re-used, but who cares.

Attach is different..

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() Update device descriptor <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

As above shows we can "miss" an invalidation. The issue is narrow, the io_pte could still be sitting in write buffers in "Inv CPU" and not yet globally visiable. "Attach CPU" could get the device descriptor installed in the IOMMU and the IOMMU could walk an io_pte that is in the old state. Effectively this is because there is no release/acquire barrier passing the io_pte store from the Inv CPU to the Attach CPU to the IOMMU.

It seems like it should be solvable somehow:
1) Inv CPU releases all the io ptes
2) Attach CPU acquires the io ptes before updating the DDT
3) Inv CPU acquires the RCU list in such a way that either attach CPU will acquire the io_pte or inv CPU will acquire the RCU list.
4) Either invalidation works or we release the new iopte to the SMMU and don't need it.

But #3 is a really weird statement. smb_mb() on both sides may do the job??

Quoted 7 lines

Quoted 2 lines

The number of radix levels is a tunable alot of iommus have that we haven't really exposed to anything else yet.

Makes sense. I've left an option to pick mode from MMU for cases where dev/iommu is not known at allocation time (with iommu_domain_alloc()). I'd guess it's reasonable to assume IOMMU supported page modes will match MMU.

Reasonable, but for this case you'd be best to have a global static that unifies the capability of all the iommu instances. Then you can pick the right thing from the installed iommus, and arguably, that is the right thing to do in all cases as we'd slightly prefer domains that work everywhere in that edge case.

Jason

Quoted 49 lines

For detach I think yes:

Inv CPU Detach CPU

write io_pte Update device descriptor
rcu_read_lock
list_for_each
<make invalidation command> <make description inv cmd>
dma_wmb() dma_wmb()
<doorbell> <cmd doorbell>
rcu_read_unlock

                                             list_del_rcu()
                                             <wipe ASID>

In this case I think we never miss an invalidation, the list_del is always after the HW has been fully fenced, so I don't think we can have any issue. Maybe a suprious invalidation if the ASID gets re-used, but who cares.

Attach is different..

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() Update device descriptor <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

As above shows we can "miss" an invalidation. The issue is narrow, the io_pte could still be sitting in write buffers in "Inv CPU" and not yet globally visiable. "Attach CPU" could get the device descriptor installed in the IOMMU and the IOMMU could walk an io_pte that is in the old state. Effectively this is because there is no release/acquire barrier passing the io_pte store from the Inv CPU to the Attach CPU to the IOMMU.

It seems like it should be solvable somehow:
1) Inv CPU releases all the io ptes
2) Attach CPU acquires the io ptes before updating the DDT
3) Inv CPU acquires the RCU list in such a way that either attach CPU will acquire the io_pte or inv CPU will acquire the RCU list.
4) Either invalidation works or we release the new iopte to the SMMU and don't need it.

Quoted 3 lines

But #3 is a really weird statement. smb_mb() on both sides may do the job??

Actual attach sequence is slightly different.

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() IOTLB.INVAL(PSCID) <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

I've tried to cover this case with riscv_iommu_iotlb_inval() called before the attached domain is visible to the device.

Quoted 67 lines · Tomasz Jeznach

Quoted 49 lines

For detach I think yes:

Inv CPU Detach CPU

write io_pte Update device descriptor
rcu_read_lock
list_for_each
<make invalidation command> <make description inv cmd>
dma_wmb() dma_wmb()
<doorbell> <cmd doorbell>
rcu_read_unlock

                                             list_del_rcu()
                                             <wipe ASID>

In this case I think we never miss an invalidation, the list_del is always after the HW has been fully fenced, so I don't think we can have any issue. Maybe a suprious invalidation if the ASID gets re-used, but who cares.

Attach is different..

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() Update device descriptor <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

As above shows we can "miss" an invalidation. The issue is narrow, the io_pte could still be sitting in write buffers in "Inv CPU" and not yet globally visiable. "Attach CPU" could get the device descriptor installed in the IOMMU and the IOMMU could walk an io_pte that is in the old state. Effectively this is because there is no release/acquire barrier passing the io_pte store from the Inv CPU to the Attach CPU to the IOMMU.

It seems like it should be solvable somehow:
1) Inv CPU releases all the io ptes
2) Attach CPU acquires the io ptes before updating the DDT
3) Inv CPU acquires the RCU list in such a way that either attach CPU will acquire the io_pte or inv CPU will acquire the RCU list.
4) Either invalidation works or we release the new iopte to the SMMU and don't need it.

Quoted 3 lines

But #3 is a really weird statement. smb_mb() on both sides may do the job??

Actual attach sequence is slightly different.

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() IOTLB.INVAL(PSCID) <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

Quoted 2 lines

I've tried to cover this case with riscv_iommu_iotlb_inval() called before the attached domain is visible to the device.

That invalidation shouldn't do anything. If this is the first attach of a PSCID then the PSCID had better already be empty, it won't become non-empty until the DDT entry is installed.

And if it is the second attach then the Inv CPU is already taking care of things, no need to invalidate at all.

Regardless, there is still a theortical race that the IOPTEs haven't been made visible yet because there is still no synchronization with the CPU writing them.

So, I don't think this solves any problem. I belive you need the appropriate kind of CPU barrier here instead of an invalidation.

Jason

Quoted 79 lines · Jason Gunthorpe

Quoted 67 lines · Tomasz Jeznach

Quoted 49 lines

For detach I think yes:

Inv CPU Detach CPU

write io_pte Update device descriptor
rcu_read_lock
list_for_each
<make invalidation command> <make description inv cmd>
dma_wmb() dma_wmb()
<doorbell> <cmd doorbell>
rcu_read_unlock

                                             list_del_rcu()
                                             <wipe ASID>

In this case I think we never miss an invalidation, the list_del is always after the HW has been fully fenced, so I don't think we can have any issue. Maybe a suprious invalidation if the ASID gets re-used, but who cares.

Attach is different..

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() Update device descriptor <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

As above shows we can "miss" an invalidation. The issue is narrow, the io_pte could still be sitting in write buffers in "Inv CPU" and not yet globally visiable. "Attach CPU" could get the device descriptor installed in the IOMMU and the IOMMU could walk an io_pte that is in the old state. Effectively this is because there is no release/acquire barrier passing the io_pte store from the Inv CPU to the Attach CPU to the IOMMU.

It seems like it should be solvable somehow:
1) Inv CPU releases all the io ptes
2) Attach CPU acquires the io ptes before updating the DDT
3) Inv CPU acquires the RCU list in such a way that either attach CPU will acquire the io_pte or inv CPU will acquire the RCU list.
4) Either invalidation works or we release the new iopte to the SMMU and don't need it.

Quoted 3 lines

But #3 is a really weird statement. smb_mb() on both sides may do the job??

Actual attach sequence is slightly different.

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() IOTLB.INVAL(PSCID) <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

Quoted 2 lines

I've tried to cover this case with riscv_iommu_iotlb_inval() called before the attached domain is visible to the device.

That invalidation shouldn't do anything. If this is the first attach of a PSCID then the PSCID had better already be empty, it won't become non-empty until the DDT entry is installed.

And if it is the second attach then the Inv CPU is already taking care of things, no need to invalidate at all.

Quoted 7 lines

Regardless, there is still a theortical race that the IOPTEs haven't been made visible yet because there is still no synchronization with the CPU writing them.

So, I don't think this solves any problem. I belive you need the appropriate kind of CPU barrier here instead of an invalidation.

Yes. There was a small, but still plausible race w/ IOPTEs visibility to the IOMMU.
For v5 I'm adding two barriers to the inval/detach flow, I believe should cover it.

1) In riscv_iommu_iotlb_inval() unconditional dma_wmb() to make any pending writes to PTEs visible to the IOMMU device. This should cover the case when list_add_rcu() update is not yet visible in the _iotlb_inval() sequence, for the first time the domain is attached to the IOMMU.

Inv CPU Attach CPU
write io_pte
dma_wmb (1)
rcu_read_lock
list_for_each // empty list_add_rcu()
smp_wmb (2)
Update device descriptor <make description inv cmd> // PTEs are visible to the HW (*1) dma_wmb()
<cmd doorbell>
rcu_read_unlock

2) In riscv_iommu_bond_link() write memory barrier to ensure list update is visible before IOMMU descriptor update. If stale data has been fetched by the HW, inval CPU will run iotlb-invalidation sequence. There is a possibility that IOMMU will fetch correct PTEs and will receive unnecessary IOTLB inval, but I don't think anyone would care.

Inv CPU Attach CPU
write io_pte list_add_rcu()

                                             smp_wmb (2)
                                             Update device descriptor
                                             <make description inv cmd>
                                             // HW might fetch stale PTEs
                                             dma_wmb()
                                             <cmd doorbell>
  dma_wmb (1)
  rcu_read_lock
    list_for_each // non-empty (*2)
      <make iotlb inval cmd>
      dma_wmb()
      <cmd doorbell>

rcu_read_unlock

3) I've also updated riscv_iommu_bond_unlink() to wipe the PSCID cache on the last domain unlink from the IOMMU.

Thank you for pointing this out. Let me know if that makes sense.

Quoted 133 lines · Tomasz Jeznach

Quoted 79 lines · Jason Gunthorpe

Quoted 67 lines · Tomasz Jeznach

Quoted 49 lines

For detach I think yes:

Inv CPU Detach CPU

write io_pte Update device descriptor
rcu_read_lock
list_for_each
<make invalidation command> <make description inv cmd>
dma_wmb() dma_wmb()
<doorbell> <cmd doorbell>
rcu_read_unlock

                                             list_del_rcu()
                                             <wipe ASID>

In this case I think we never miss an invalidation, the list_del is always after the HW has been fully fenced, so I don't think we can have any issue. Maybe a suprious invalidation if the ASID gets re-used, but who cares.

Attach is different..

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() Update device descriptor <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

As above shows we can "miss" an invalidation. The issue is narrow, the io_pte could still be sitting in write buffers in "Inv CPU" and not yet globally visiable. "Attach CPU" could get the device descriptor installed in the IOMMU and the IOMMU could walk an io_pte that is in the old state. Effectively this is because there is no release/acquire barrier passing the io_pte store from the Inv CPU to the Attach CPU to the IOMMU.

It seems like it should be solvable somehow:
1) Inv CPU releases all the io ptes
2) Attach CPU acquires the io ptes before updating the DDT
3) Inv CPU acquires the RCU list in such a way that either attach CPU will acquire the io_pte or inv CPU will acquire the RCU list.
4) Either invalidation works or we release the new iopte to the SMMU and don't need it.

Quoted 3 lines

But #3 is a really weird statement. smb_mb() on both sides may do the job??

Actual attach sequence is slightly different.

Inv CPU Attach CPU

write io_pte
rcu_read_lock
list_for_each // empty
list_add_rcu() IOTLB.INVAL(PSCID) <make description inv cmd> dma_wmb()
<cmd doorbell>
rcu_read_unlock

Quoted 2 lines

I've tried to cover this case with riscv_iommu_iotlb_inval() called before the attached domain is visible to the device.

That invalidation shouldn't do anything. If this is the first attach of a PSCID then the PSCID had better already be empty, it won't become non-empty until the DDT entry is installed.

And if it is the second attach then the Inv CPU is already taking care of things, no need to invalidate at all.

Quoted 7 lines

Regardless, there is still a theortical race that the IOPTEs haven't been made visible yet because there is still no synchronization with the CPU writing them.

So, I don't think this solves any problem. I belive you need the appropriate kind of CPU barrier here instead of an invalidation.

Yes. There was a small, but still plausible race w/ IOPTEs visibility to the IOMMU.
For v5 I'm adding two barriers to the inval/detach flow, I believe should cover it.

1) In riscv_iommu_iotlb_inval() unconditional dma_wmb() to make any pending writes to PTEs visible to the IOMMU device. This should cover the case when list_add_rcu() update is not yet visible in the _iotlb_inval() sequence, for the first time the domain is attached to the IOMMU.

Inv CPU Attach CPU
write io_pte
dma_wmb (1)
rcu_read_lock
list_for_each // empty list_add_rcu()
smp_wmb (2)
Update device descriptor <make description inv cmd> // PTEs are visible to the HW (*1) dma_wmb()
<cmd doorbell>
rcu_read_unlock

2) In riscv_iommu_bond_link() write memory barrier to ensure list update is visible before IOMMU descriptor update. If stale data has been fetched by the HW, inval CPU will run iotlb-invalidation sequence. There is a possibility that IOMMU will fetch correct PTEs and will receive unnecessary IOTLB inval, but I don't think anyone would care.

Inv CPU Attach CPU
write io_pte list_add_rcu()

                                             smp_wmb (2)
                                             Update device descriptor
                                             <make description inv cmd>
                                             // HW might fetch stale PTEs
                                             dma_wmb()
                                             <cmd doorbell>
  dma_wmb (1)
  rcu_read_lock
    list_for_each // non-empty (*2)
      <make iotlb inval cmd>
      dma_wmb()
      <cmd doorbell>

Quoted 6 lines

rcu_read_unlock

3) I've also updated riscv_iommu_bond_unlink() to wipe the PSCID cache on the last domain unlink from the IOMMU.

Thank you for pointing this out. Let me know if that makes sense.

I'm not an expert in barriers, but I think you need the more expensive "mb" in both cases.

The inv side is both releasing the write and acquiring the list read. IIRC READ_ONCE is not a full acquire?

The Attach side is both releasing the list_add_rcu() and acquiring the iopte.

rcu is still a benefit, there is no cache line sharing and there is only one full barrier, not two, like a spinlock.

And a big fat comment in both sides explaining this :)

Jason

drivers/iommu/riscv/iommu.c +36 -0

+/*
+ * Send IOTLB.INVAL for whole address space for ranges larger than 2MB.
+ * This limit will be replaced with range invalidations, if supported by
+ * the hardware, when RISC-V IOMMU architecture specification update for
+ * range invalidations update will be available.
+ */
+#define RISCV_IOMMU_IOTLB_INVAL_LIMIT	(2 << 20)
+
+static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
+				    unsigned long start, unsigned long end)
+{
+	struct riscv_iommu_bond *bond;
+	struct riscv_iommu_device *iommu, *prev;
+	struct riscv_iommu_command cmd;
+	unsigned long len = end - start + 1;
+	unsigned long iova;
+
+	rcu_read_lock();
+
+	prev = NULL;
+	list_for_each_entry_rcu(bond, &domain->bonds, list) {
+		iommu = dev_to_iommu(bond->dev);
+
+		riscv_iommu_cmd_inval_vma(&cmd);
+		riscv_iommu_cmd_inval_set_pscid(&cmd, domain->pscid);
+		if (len && len >= RISCV_IOMMU_IOTLB_INVAL_LIMIT) {
+			for (iova = start; iova < end; iova += PAGE_SIZE) {
+				riscv_iommu_cmd_inval_set_addr(&cmd, iova);
+				riscv_iommu_cmd_send(iommu, &cmd, 0);
+			}
+		} else {
+			riscv_iommu_cmd_send(iommu, &cmd, 0);
+		}
+
+		/*
+		 * IOTLB invalidation request can be safely omitted if already sent

Quoted 36 lines · Tomasz Jeznach

drivers/iommu/riscv/iommu.c +36 -0

+/*
+ * Send IOTLB.INVAL for whole address space for ranges larger than 2MB.
+ * This limit will be replaced with range invalidations, if supported by
+ * the hardware, when RISC-V IOMMU architecture specification update for
+ * range invalidations update will be available.
+ */
+#define RISCV_IOMMU_IOTLB_INVAL_LIMIT        (2 << 20)
+
+static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
+                                 unsigned long start, unsigned long end)
+{
+     struct riscv_iommu_bond *bond;
+     struct riscv_iommu_device *iommu, *prev;
+     struct riscv_iommu_command cmd;
+     unsigned long len = end - start + 1;
+     unsigned long iova;
+
+     rcu_read_lock();
+
+     prev = NULL;
+     list_for_each_entry_rcu(bond, &domain->bonds, list) {
+             iommu = dev_to_iommu(bond->dev);
+
+             riscv_iommu_cmd_inval_vma(&cmd);
+             riscv_iommu_cmd_inval_set_pscid(&cmd, domain->pscid);
+             if (len && len >= RISCV_IOMMU_IOTLB_INVAL_LIMIT) {
+                     for (iova = start; iova < end; iova += PAGE_SIZE) {
+                             riscv_iommu_cmd_inval_set_addr(&cmd, iova);
+                             riscv_iommu_cmd_send(iommu, &cmd, 0);
+                     }
+             } else {
+                     riscv_iommu_cmd_send(iommu, &cmd, 0);
+             }
+
+             /*
+              * IOTLB invalidation request can be safely omitted if already sent

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 869

Quoted 10 lines

drivers/iommu/riscv/iommu.c +10 -0

+              * to the IOMMU for the same PSCID, and with domain->bonds list
+              * arranged based on the device's IOMMU, it's sufficient to check
+              * last device the invalidation was sent to.
+              */
+             if (iommu == prev)
+                     continue;
+
+             prev = iommu;
+             riscv_iommu_cmd_send(iommu, &cmd, 0);
+     }

I don't quite follow why not moving "if (iommu == prev)" check to the top and removing the last riscv_iommu_cmd_send(). My understanding is that we could make it simply like below:

        prev = NULL;
        list_for_each_entry_rcu(bond, &domain->bonds, list) {
                iommu = dev_to_iommu(bond->dev);
                if (iommu == prev)
                        continue;

                /*
                 * Send an invalidation request to the request queue
                 * without wait.
                 */
                ... ...

                prev = iommu;
        }

Quoted 8 lines

drivers/iommu/riscv/iommu.c +8 -0

+
+     prev = NULL;
+     list_for_each_entry_rcu(bond, &domain->bonds, list) {
+             iommu = dev_to_iommu(bond->dev);
+             if (iommu == prev)
+                     continue;
+
+             prev = iommu;

drivers/iommu/riscv/iommu.c static int riscv_iommu_iodir_set_mode(struct riscv_iommu_device *iommu, · line 882

Quoted 5 lines

drivers/iommu/riscv/iommu.c +5 -0

+             riscv_iommu_cmd_iofence(&cmd);
+             riscv_iommu_cmd_send(iommu, &cmd, RISCV_IOMMU_QUEUE_TIMEOUT);
+     }
+     rcu_read_unlock();
+}

Best regards,
baolu

This patch series introduces support for RISC-V IOMMU architected hardware into the Linux kernel.