[PATCH v4 0/7] soc: ti: Add and use PVU on K3-AM65 for DMA isolation

[PATCH v4 0/7] soc: ti: Add and use PVU on K3-AM65 for DMA isolation

[Jan Kiszka]

Changes in v4:
 - reorder patch queue, moving all DTS changes to the back
 - limit activation to IOT2050 Advanced variants
 - move DMA pool to allow firmware-based expansion it up to 512M

Changes in v3:
 - fix ti,am654-pvu.yaml according to review comments
 - address review comments on ti,am65-pci-host.yaml
 - differentiate between different compatibles in ti,am65-pci-host.yaml
 - move pvu nodes to k3-am65-main.dtsi
 - reorder patch series, pulling bindings and generic DT bits to the front

Changes in v2:
 - fix dt_bindings_check issues (patch 1)
 - address first review comments (patch 2)
 - extend ti,am65-pci-host bindings for PVU (new patch 3)

Only few of the K3 SoCs have an IOMMU and, thus, can isolate the system
against DMA-based attacks of external PCI devices. The AM65 is without
an IOMMU, but it comes with something close to it: the Peripheral
Virtualization Unit (PVU).

The PVU was originally designed to establish static compartments via a
hypervisor, isolate those DMA-wise against each other and the host and
even allow remapping of guest-physical addresses. But it only provides
a static translation region, not page-granular mappings. Thus, it cannot
be handled transparently like an IOMMU.

Now, to use the PVU for the purpose of isolated PCI devices from the
Linux host, this series takes a different approach. It defines a
restricted-dma-pool for the PCI host, using swiotlb to map all DMA
buffers from a static memory carve-out. And to enforce that the devices
actually follow this, a special PVU soc driver is introduced. The driver
permits access to the GIC ITS and otherwise waits for other drivers that
detect devices with constrained DMA to register pools with the PVU.

For the AM65, the first (and possibly only) driver where this is
introduced is the pci-keystone host controller. Finally, this series
configures the IOT2050 devices (all have MiniPCIe or M.2 extension
slots) to make use of this protection scheme.

Due to the cross-cutting nature of these changes, multiple subsystems
are affected. However, I wanted to present the whole thing in one series
to allow everyone to review with the complete picture in hands. If
preferred, I can also split the series up, of course.

Jan

CC: Bjorn Helgaas <bhelgaas@google.com>
CC: "Krzysztof Wilczyński" <kw@linux.com>
CC: linux-pci@vger.kernel.org
CC: Lorenzo Pieralisi <lpieralisi@kernel.org>

Jan Kiszka (7):
  dt-bindings: soc: ti: Add AM65 peripheral virtualization unit
  dt-bindings: PCI: ti,am65: Extend for use with PVU
  soc: ti: Add IOMPU-like PVU driver
  PCI: keystone: Add supported for PVU-based DMA isolation on AM654
  arm64: dts: ti: k3-am65-main: Add PVU nodes
  arm64: dts: ti: k3-am65-main: Add VMAP registers to PCI root complexes
  arm64: dts: ti: iot2050: Enforce DMA isolation for devices behind PCI
    RC on Advanced

 .../bindings/pci/ti,am65-pci-host.yaml        |  52 +-
 .../bindings/soc/ti/ti,am654-pvu.yaml         |  51 ++
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi      |  38 +-
 .../ti/k3-am6548-iot2050-advanced-common.dtsi |  21 +-
 drivers/pci/controller/dwc/pci-keystone.c     | 101 ++++
 drivers/soc/ti/Kconfig                        |   4 +
 drivers/soc/ti/Makefile                       |   1 +
 drivers/soc/ti/ti-pvu.c                       | 487 ++++++++++++++++++
 include/linux/ti-pvu.h                        |  16 +
 9 files changed, 754 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/ti/ti,am654-pvu.yaml
 create mode 100644 drivers/soc/ti/ti-pvu.c
 create mode 100644 include/linux/ti-pvu.h

-- 
2.43.0

[PATCH v4 1/7] dt-bindings: soc: ti: Add AM65 peripheral virtualization unit

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The PVU allows to define a limited set of mappings for incoming DMA
requests to the system memory. It is not a real IOMMU, thus hooked up
under the TI SoC bindings.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/soc/ti/ti,am654-pvu.yaml         | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/ti/ti,am654-pvu.yaml

diff --git a/Documentation/devicetree/bindings/soc/ti/ti,am654-pvu.yaml b/Documentation/devicetree/bindings/soc/ti/ti,am654-pvu.yaml
new file mode 100644
index 000000000000..e4a5fc47d674
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/ti/ti,am654-pvu.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) Siemens AG, 2024
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/ti/ti,am654-pvu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI AM654 Peripheral Virtualization Unit
+
+maintainers:
+  - Jan Kiszka <jan.kiszka@siemens.com>
+
+properties:
+  compatible:
+    enum:
+      - ti,am654-pvu
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: cfg
+      - const: tlbif
+
+  interrupts:
+    items:
+      - description: fault interrupt
+
+  interrupt-names:
+    items:
+      - const: pvu
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+
+additionalProperties: false
+
+examples:
+  - |
+    iommu@30f80000 {
+        compatible = "ti,am654-pvu";
+        reg = <0x30f80000 0x1000>,
+              <0x36000000 0x100000>;
+        reg-names = "cfg", "tlbif";
+        interrupts-extended = <&intr_main_navss 390>;
+        interrupt-names = "pvu";
+    };
-- 
2.43.0

[PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
to specific regions of host memory. Add the optional property
"memory-regions" to point to such regions of memory when PVU is used.

Since the PVU deals with system physical addresses, utilizing the PVU
with PCIe devices also requires setting up the VMAP registers to map the
Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
mapped to the system physical address. Hence, describe the VMAP
registers which are optionally unless the PVU shall used for PCIe.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
CC: "Krzysztof Wilczyński" <kw@linux.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: linux-pci@vger.kernel.org
---
 .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
index 0a9d10532cc8..d8182bad92de 100644
--- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
@@ -19,16 +19,6 @@ properties:
       - ti,am654-pcie-rc
       - ti,keystone-pcie
 
-  reg:
-    maxItems: 4
-
-  reg-names:
-    items:
-      - const: app
-      - const: dbics
-      - const: config
-      - const: atu
-
   interrupts:
     maxItems: 1
 
@@ -84,12 +74,48 @@ if:
       enum:
         - ti,am654-pcie-rc
 then:
+  properties:
+    reg:
+      minItems: 4
+      maxItems: 6
+
+    reg-names:
+      minItems: 4
+      items:
+        - const: app
+        - const: dbics
+        - const: config
+        - const: atu
+        - const: vmap_lp
+        - const: vmap_hp
+
+    memory-region:
+      minItems: 1
+      description: |
+        phandle to one or more restricted DMA pools to be used for all devices
+        behind this controller. The regions should be defined according to
+        reserved-memory/shared-dma-pool.yaml.
+      items:
+        maxItems: 1
+
   required:
     - dma-coherent
     - power-domains
     - msi-map
     - num-viewport
 
+else:
+  properties:
+    reg:
+      maxItems: 4
+
+    reg-names:
+      items:
+        - const: app
+        - const: dbics
+        - const: config
+        - const: atu
+
 unevaluatedProperties: false
 
 examples:
@@ -104,8 +130,10 @@ examples:
         reg =  <0x5500000 0x1000>,
                <0x5501000 0x1000>,
                <0x10000000 0x2000>,
-               <0x5506000 0x1000>;
-        reg-names = "app", "dbics", "config", "atu";
+               <0x5506000 0x1000>,
+               <0x2900000 0x1000>,
+               <0x2908000 0x1000>;
+        reg-names = "app", "dbics", "config", "atu", "vmap_lp", "vmap_hp";
         power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
         #address-cells = <3>;
         #size-cells = <2>;
-- 
2.43.0

[PATCH v4 3/7] soc: ti: Add IOMPU-like PVU driver

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The TI Peripheral Virtualization Unit (PVU) permits to define a limited
set of mappings for DMA requests on the system memory. Unlike with an
IOMMU, there is no fallback to a memory-backed page table, only a fixed
set of register-backed TLBs. Emulating an IOMMU behavior appears to be
the more fragile the more fragmentation of pending requests occur.

Therefore, this driver does not expose the PVU as an IOMMU. It rather
introduces a simple, static interface to devices that are under
restricted-dma-pool constraints. They can register their pools with the
PVUs, enabling only those pools to work for DMA. As also MSI is issued
as DMA, the PVU already register the related translator region of the
AM654 as valid DMA target.

This driver is the essential building block for limiting DMA from
untrusted devices to clearly defined memory regions in the absence of a
real IOMMU (SMMU).

Co-developed-by: Diogo Ivo <diogo.ivo@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/soc/ti/Kconfig  |   4 +
 drivers/soc/ti/Makefile |   1 +
 drivers/soc/ti/ti-pvu.c | 487 ++++++++++++++++++++++++++++++++++++++++
 include/linux/ti-pvu.h  |  16 ++
 4 files changed, 508 insertions(+)
 create mode 100644 drivers/soc/ti/ti-pvu.c
 create mode 100644 include/linux/ti-pvu.h

diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
index 1a93001c9e36..af7173ad84de 100644
--- a/drivers/soc/ti/Kconfig
+++ b/drivers/soc/ti/Kconfig
@@ -82,6 +82,10 @@ config TI_PRUSS
 	  processors on various TI SoCs. It's safe to say N here if you're
 	  not interested in the PRU or if you are unsure.
 
+config TI_PVU
+	bool "TI Peripheral Virtualization Unit driver"
+	depends on ARCH_K3 && DMA_RESTRICTED_POOL
+
 endif # SOC_TI
 
 config TI_SCI_INTA_MSI_DOMAIN
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index cb800a745e66..ecff3fd8c433 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_TI_K3_RINGACC)		+= k3-ringacc.o
 obj-$(CONFIG_TI_K3_SOCINFO)		+= k3-socinfo.o
 obj-$(CONFIG_TI_PRUSS)			+= pruss.o
 obj-$(CONFIG_POWER_AVS_OMAP)		+= smartreflex.o
+obj-$(CONFIG_TI_PVU)			+= ti-pvu.o
diff --git a/drivers/soc/ti/ti-pvu.c b/drivers/soc/ti/ti-pvu.c
new file mode 100644
index 000000000000..5b71d503051f
--- /dev/null
+++ b/drivers/soc/ti/ti-pvu.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI Peripheral Virtualization Unit driver for static DMA isolation
+ *
+ * Copyright (c) 2024, Siemens AG
+ */
+
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/ti-pvu.h>
+
+#define PVU_CONFIG					0x4
+#define PVU_ENABLE					0x10
+#define PVU_VIRTID_MAP1					0x14
+#define PVU_VIRTID_MAP2					0x18
+#define PVU_EXCEPTION_LOGGING_CONTROL			0x120
+#define PVU_EXCEPTION_LOGGING_HEADER0			0x124
+#define PVU_EXCEPTION_LOGGING_HEADER1			0x128
+#define PVU_EXCEPTION_LOGGING_DATA0			0x12c
+#define PVU_EXCEPTION_LOGGING_DATA1			0x130
+#define PVU_EXCEPTION_LOGGING_DATA2			0x134
+#define PVU_EXCEPTION_LOGGING_DATA2_SECURE		BIT(0)
+#define PVU_EXCEPTION_LOGGING_DATA2_PRIV		BIT(1)
+#define PVU_EXCEPTION_LOGGING_DATA2_CACHEABLE		BIT(2)
+#define PVU_EXCEPTION_LOGGING_DATA2_DEBUG		BIT(3)
+#define PVU_EXCEPTION_LOGGING_DATA2_READ		BIT(4)
+#define PVU_EXCEPTION_LOGGING_DATA2_WRITE		BIT(5)
+#define PVU_EXCEPTION_LOGGING_DATA3			0x138
+#define PVU_EXCEPTION_ENABLE_SET			0x148
+#define PVU_EOI_REG					0x150
+
+#define PVU_CHAIN			0x0
+#define  PVU_CHAIN_EN			BIT(31)
+#define  PVU_CHAIN_LOG_DIS		BIT(30)
+#define  PVU_CHAIN_FAULT		BIT(29)
+#define  PVU_CHAIN_MASK			0xfff
+#define PVU_ENTRY0			0x20
+#define PVU_ENTRY1			0x24
+#define  PVU_ENTRY1_RESERVED_MASK	0xffff0000
+#define  PVU_ENTRY1_VBASE_H_MASK	0xffff
+#define PVU_ENTRY2			0x28
+#define  PVU_ENTRY2_RESERVED_MASK	0x1fd00080
+#define  PVU_ENTRY2_INVALID		(0U << 30)
+#define  PVU_ENTRY2_VALID		(2U << 30)
+#define  PVU_ENTRY2_MODE_MASK		0xc0000000
+#define  PVU_ENTRY2_PSIZE_SHIFT		16
+#define  PVU_ENTRY2_PSIZE_MASK		0xf
+#define  PVU_ENTRY2_PERM_SX		BIT(15)
+#define  PVU_ENTRY2_PERM_SW		BIT(14)
+#define  PVU_ENTRY2_PERM_SR		BIT(13)
+#define  PVU_ENTRY2_PERM_UX		BIT(12)
+#define  PVU_ENTRY2_PERM_UW		BIT(11)
+#define  PVU_ENTRY2_PERM_UR		BIT(10)
+#define  PVU_ENTRY2_MEM_WRITETHROUGH	(2 << 8)
+#define  PVU_ENTRY2_OUTER_SHARABLE	BIT(4)
+#define  PVU_ENTRY2_IS_NOALLOC		(0 << 2)
+#define  PVU_ENTRY2_OS_NOALLOC		(0 << 0)
+#define PVU_ENTRY4			0x30
+#define PVU_ENTRY5			0x34
+#define  PVU_ENTRY5_RESERVED_MASK	0xffff0000
+#define  PVU_ENTRY5_PBASE_H_MASK	0xffff
+#define PVU_ENTRY6			0x38
+#define  PVU_ENTRY6_RESERVED_MASK	0xffffffe0
+
+#define NUM_VIRTIDS			1
+
+static const struct regmap_config pvu_cfg_regmap_cfg = {
+	.name = "pvu-cfg",
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = PVU_EOI_REG,
+};
+
+enum pvu_cfg_regfields {
+	PVU_TLBS,
+	PVU_TLB_ENTRIES,
+	PVU_ENABLED,
+	PVU_DMA_CNT,
+	PVU_DMA_CL0,
+	PVU_DMA_CL1,
+	PVU_DMA_CL2,
+	PVU_DMA_CL3,
+	PVU_MAX_VIRTID,
+	PVU_EXC_SRC_ID,
+	PVU_EXC_CODE,
+	PVU_EXC_ADDR_L,
+	PVU_EXC_ADDR_H,
+	PVU_EXC_PRIV_ID,
+	PVU_EXC_PROPS,
+	PVU_EXC_ROUTE_ID,
+	PVU_EXC_BYTE_CNT,
+	PVU_EXC_ENABLE,
+	PVU_EOI,
+	PVU_MAX_CFG_FIELDS,
+};
+
+static const struct reg_field pvu_cfg_reg_fields[] = {
+	[PVU_TLBS] = REG_FIELD(PVU_CONFIG, 0, 15),
+	[PVU_TLB_ENTRIES] = REG_FIELD(PVU_CONFIG, 16, 23),
+	[PVU_ENABLED] = REG_FIELD(PVU_ENABLE, 0, 0),
+	[PVU_DMA_CNT] = REG_FIELD(PVU_VIRTID_MAP1, 0, 11),
+	[PVU_DMA_CL0] = REG_FIELD(PVU_VIRTID_MAP1, 16, 17),
+	[PVU_DMA_CL1] = REG_FIELD(PVU_VIRTID_MAP1, 18, 19),
+	[PVU_DMA_CL2] = REG_FIELD(PVU_VIRTID_MAP1, 20, 21),
+	[PVU_DMA_CL3] = REG_FIELD(PVU_VIRTID_MAP1, 22, 23),
+	[PVU_MAX_VIRTID] = REG_FIELD(PVU_VIRTID_MAP2, 0, 11),
+	[PVU_EXC_SRC_ID] = REG_FIELD(PVU_EXCEPTION_LOGGING_HEADER0, 8, 23),
+	[PVU_EXC_CODE] = REG_FIELD(PVU_EXCEPTION_LOGGING_HEADER1, 16, 23),
+	[PVU_EXC_ADDR_L] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA0, 0, 31),
+	[PVU_EXC_ADDR_H] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA1, 0, 15),
+	[PVU_EXC_PRIV_ID] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA2, 0, 7),
+	[PVU_EXC_PROPS] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA2, 8, 13),
+	[PVU_EXC_ROUTE_ID] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA2, 16, 27),
+	[PVU_EXC_BYTE_CNT] = REG_FIELD(PVU_EXCEPTION_LOGGING_DATA3, 0, 9),
+	[PVU_EXC_ENABLE] = REG_FIELD(PVU_EXCEPTION_ENABLE_SET, 0, 0),
+	[PVU_EOI] = REG_FIELD(PVU_EOI_REG, 0, 15),
+};
+
+struct ti_pvu {
+	struct list_head entry;
+	struct platform_device *pdev;
+	struct regmap *cfg;
+	struct regmap_field *cfg_fields[PVU_MAX_CFG_FIELDS];
+	void __iomem *tlbif_base;
+	unsigned int num_tlbs;
+	unsigned int num_entries;
+};
+
+static const char *pvu_excp_code_string[] = {
+	"PVU miss",
+	"maximum VirtID violation",
+	"<reserved>",
+	"read permission violation",
+	"write permission violation",
+	"execute permission violation",
+	"prefetch permission violation",
+	"<unknown>",
+};
+
+static const u64 pvu_page_size[] = {
+	4 * 1024ULL,
+	16 * 1024ULL,
+	64 * 1024ULL,
+	2 * 1024 * 1024ULL,
+	32 * 1024 * 1024ULL,
+	512 * 1024 * 1024ULL,
+	1 * 1024 * 1024 * 1024ULL,
+	16 * 1024 * 1024 * 1024ULL
+};
+
+static DEFINE_MUTEX(ti_pvu_lock);
+static LIST_HEAD(ti_pvu_list);
+
+static unsigned int pvu_field_read(struct ti_pvu *pvu, enum pvu_cfg_regfields f)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_field_read(pvu->cfg_fields[f], &val);
+	if (ret)
+		dev_err(&pvu->pdev->dev, "failed to read field\n");
+
+	return val;
+}
+
+static void pvu_field_write(struct ti_pvu *pvu, enum pvu_cfg_regfields f,
+			   unsigned int val)
+{
+	int ret;
+
+	ret = regmap_field_write(pvu->cfg_fields[f], val);
+	if (ret)
+		dev_err(&pvu->pdev->dev, "failed to write field\n");
+}
+
+static irqreturn_t pvu_fault_isr(int irq, void *dev_id)
+{
+	u32 code, bytes, route_id, priv_id, props;
+	struct ti_pvu *pvu = dev_id;
+	const char *code_str;
+	u64 address;
+
+	code = pvu_field_read(pvu, PVU_EXC_CODE);
+	code_str = pvu_excp_code_string[
+		min(code, (u32)ARRAY_SIZE(pvu_excp_code_string) - 1)];
+
+	dev_err(&pvu->pdev->dev, "fault detected, code %d (%s)\n",
+		code, code_str);
+
+	address = pvu_field_read(pvu, PVU_EXC_ADDR_L);
+	address |= (u64)pvu_field_read(pvu, PVU_EXC_ADDR_H) << 32;
+
+	bytes = pvu_field_read(pvu, PVU_EXC_BYTE_CNT);
+
+	route_id = pvu_field_read(pvu, PVU_EXC_ROUTE_ID);
+	priv_id = pvu_field_read(pvu, PVU_EXC_PRIV_ID);
+	props = pvu_field_read(pvu, PVU_EXC_PROPS);
+
+	dev_err(&pvu->pdev->dev,
+		"  address 0x%016llx size %d route-ID %d priv-ID %d flags %c%c%c%c%c%c\n",
+		address, bytes, route_id, priv_id,
+		(props & PVU_EXCEPTION_LOGGING_DATA2_WRITE)     ? 'W' : '-',
+		(props & PVU_EXCEPTION_LOGGING_DATA2_READ)      ? 'R' : '-',
+		(props & PVU_EXCEPTION_LOGGING_DATA2_DEBUG)     ? 'D' : '-',
+		(props & PVU_EXCEPTION_LOGGING_DATA2_CACHEABLE) ? 'C' : '-',
+		(props & PVU_EXCEPTION_LOGGING_DATA2_PRIV)      ? 'P' : '-',
+		(props & PVU_EXCEPTION_LOGGING_DATA2_SECURE)    ? 'S' : '-');
+
+	pvu_field_write(pvu, PVU_EOI, 0);
+
+	return IRQ_HANDLED;
+}
+
+static int pvu_get_free_entry(struct ti_pvu *pvu)
+{
+	unsigned int n;
+	u32 val;
+
+	for (n = 0; n < pvu->num_entries; n++) {
+		val = readl(pvu->tlbif_base + n * 0x20 + PVU_ENTRY2);
+		if ((val & PVU_ENTRY2_MODE_MASK) == PVU_ENTRY2_INVALID)
+			return n;
+	}
+	return -ENOSPC;
+}
+
+static void pvu_write_entry(struct ti_pvu *pvu, unsigned int entry,
+			    u64 addr, u32 psize)
+{
+	void __iomem *entry_base = pvu->tlbif_base + entry * 0x20;
+	u32 val;
+
+	writel((u32)addr, entry_base + PVU_ENTRY0);
+
+	val = readl(entry_base + PVU_ENTRY1);
+	val &= PVU_ENTRY1_RESERVED_MASK;
+	val |= (addr >> 32) & PVU_ENTRY1_VBASE_H_MASK;
+	writel(val, entry_base + PVU_ENTRY1);
+
+	writel((u32)addr, entry_base + PVU_ENTRY4);
+
+	val = readl(entry_base + PVU_ENTRY5);
+	val &= PVU_ENTRY5_RESERVED_MASK;
+	val |= (addr >> 32) & PVU_ENTRY5_PBASE_H_MASK;
+	writel(val, entry_base + PVU_ENTRY5);
+
+	val = readl(entry_base + PVU_ENTRY6);
+	val &= PVU_ENTRY6_RESERVED_MASK;
+	writel(val, entry_base + PVU_ENTRY6);
+
+	val = readl(entry_base + PVU_ENTRY2);
+	val &= PVU_ENTRY2_RESERVED_MASK;
+	val |= psize << PVU_ENTRY2_PSIZE_SHIFT;
+	val |= PVU_ENTRY2_VALID |
+		PVU_ENTRY2_PERM_UR | PVU_ENTRY2_PERM_SR |
+		PVU_ENTRY2_PERM_UW | PVU_ENTRY2_PERM_SW |
+		PVU_ENTRY2_PERM_UX | PVU_ENTRY2_PERM_SX |
+		PVU_ENTRY2_MEM_WRITETHROUGH | PVU_ENTRY2_OUTER_SHARABLE |
+		PVU_ENTRY2_IS_NOALLOC | PVU_ENTRY2_OS_NOALLOC;
+	writel(val, entry_base + PVU_ENTRY2);
+}
+
+static int pvu_create_region(struct ti_pvu *pvu, u64 addr, u64 size)
+{
+	u64 page_size;
+	int psize;
+	int entry;
+
+	while (size > 0) {
+		entry = pvu_get_free_entry(pvu);
+		if (entry < 0) {
+			dev_err(&pvu->pdev->dev, "ran out of TLB entries\n");
+			return -ENOSPC;
+		}
+
+		for (psize = ARRAY_SIZE(pvu_page_size) - 1; psize >= 0; psize--) {
+			page_size = pvu_page_size[psize];
+			if (size >= page_size && (addr & (page_size - 1)) == 0)
+				break;
+		}
+		if (psize < 0) {
+			dev_err(&pvu->pdev->dev, "unaligned region provided\n");
+			return -EINVAL;
+		}
+
+		pvu_write_entry(pvu, entry, addr, psize);
+		dev_info(&pvu->pdev->dev,
+			 "created TLB entry 0.%d: 0x%08llx, psize %d (0x%08llx)\n",
+			 entry, addr, psize, page_size);
+
+		size -= page_size;
+		addr += page_size;
+	}
+
+	return 0;
+}
+
+static void pvu_remove_region(struct ti_pvu *pvu, u64 addr, u64 size)
+{
+	void __iomem *entry_base;
+	unsigned int n, psize;
+	u64 entry_addr;
+	u32 entry2;
+
+	for (n = 0; n < pvu->num_entries; n++) {
+		entry_base = pvu->tlbif_base + n * 0x20;
+		entry2 = readl(entry_base + PVU_ENTRY2);
+		if ((entry2 & PVU_ENTRY2_MODE_MASK) != PVU_ENTRY2_VALID)
+			continue;
+
+		entry_addr = readl(entry_base + PVU_ENTRY0);
+		entry_addr |= (u64)(readl(entry_base + PVU_ENTRY1) &
+			PVU_ENTRY1_VBASE_H_MASK) << 32;
+
+		psize = (entry2 >> PVU_ENTRY2_PSIZE_SHIFT) &
+			PVU_ENTRY2_PSIZE_MASK;
+		if (psize >= ARRAY_SIZE(pvu_page_size))
+			continue;
+
+		if (entry_addr >= addr &&
+		    (entry_addr + pvu_page_size[psize]) <= (addr + size)) {
+			entry2 &= ~PVU_ENTRY2_MODE_MASK;
+			entry2 |= PVU_ENTRY2_INVALID;
+			writel(entry2, entry_base + PVU_ENTRY2);
+
+			dev_info(&pvu->pdev->dev,
+				 "removed TLB entry 0.%d\n", n);
+		}
+	}
+}
+
+int ti_pvu_create_region(unsigned int virt_id, const struct resource *region)
+{
+	struct ti_pvu *pvu;
+	int err = 0;
+
+	if (virt_id >= NUM_VIRTIDS)
+		return -EINVAL;
+
+	mutex_lock(&ti_pvu_lock);
+
+	list_for_each_entry(pvu, &ti_pvu_list, entry) {
+		err = pvu_create_region(pvu, region->start,
+					region->end + 1 - region->start);
+		if (err)
+			break;
+	}
+
+	mutex_unlock(&ti_pvu_lock);
+
+	return err;
+}
+
+int ti_pvu_remove_region(unsigned int virt_id, const struct resource *region)
+{
+	struct ti_pvu *pvu;
+
+	if (virt_id >= NUM_VIRTIDS)
+		return -EINVAL;
+
+	mutex_lock(&ti_pvu_lock);
+
+	list_for_each_entry(pvu, &ti_pvu_list, entry) {
+		pvu_remove_region(pvu, region->start,
+				  region->end + 1 - region->start);
+	}
+
+	mutex_unlock(&ti_pvu_lock);
+
+	return 0;
+}
+
+static int ti_pvu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *its_node;
+	void __iomem *base;
+	struct ti_pvu *pvu;
+	u32 val;
+	int ret;
+
+	pvu = devm_kzalloc(dev, sizeof(*pvu), GFP_KERNEL);
+	if (!pvu)
+		return -ENOMEM;
+
+	pvu->pdev = pdev;
+
+	base = devm_platform_ioremap_resource_byname(pdev, "cfg");
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pvu->cfg = devm_regmap_init_mmio(dev, base, &pvu_cfg_regmap_cfg);
+	if (IS_ERR(pvu->cfg))
+		return dev_err_probe(dev, PTR_ERR(pvu->cfg), "failed to init cfg regmap");
+
+	ret = devm_regmap_field_bulk_alloc(dev, pvu->cfg, pvu->cfg_fields,
+					   pvu_cfg_reg_fields, PVU_MAX_CFG_FIELDS);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to alloc cfg regmap fields");
+
+	pvu->num_tlbs = pvu_field_read(pvu, PVU_TLBS);
+	pvu->num_entries = pvu_field_read(pvu, PVU_TLB_ENTRIES);
+	dev_info(dev, "TLBs: %d, entries per TLB: %d\n", pvu->num_tlbs,
+		 pvu->num_entries);
+
+	pvu->tlbif_base = devm_platform_ioremap_resource_byname(pdev, "tlbif");
+	if (IS_ERR(pvu->tlbif_base))
+		return PTR_ERR(pvu->tlbif_base);
+
+	its_node = of_find_compatible_node(0, 0, "arm,gic-v3-its");
+	if (its_node) {
+		u32 pre_its_window[2];
+
+		ret = of_property_read_u32_array(its_node,
+						 "socionext,synquacer-pre-its",
+						 pre_its_window,
+						 ARRAY_SIZE(pre_its_window));
+		if (ret) {
+			dev_err(dev, "failed to read pre-its property\n");
+			return ret;
+		}
+
+		ret = pvu_create_region(pvu, pre_its_window[0],
+					pre_its_window[1]);
+		if (ret)
+			return ret;
+	}
+
+	val = readl(pvu->tlbif_base + PVU_CHAIN);
+	val |= PVU_CHAIN_EN;
+	writel(val, pvu->tlbif_base + PVU_CHAIN);
+
+	pvu_field_write(pvu, PVU_DMA_CNT, 0);
+	pvu_field_write(pvu, PVU_DMA_CL0, 0);
+	pvu_field_write(pvu, PVU_DMA_CL1, 0);
+	pvu_field_write(pvu, PVU_DMA_CL2, 0);
+	pvu_field_write(pvu, PVU_DMA_CL3, 0);
+	pvu_field_write(pvu, PVU_MAX_VIRTID, NUM_VIRTIDS);
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get irq\n");
+
+	ret = devm_request_irq(dev, ret, pvu_fault_isr, 0, dev_name(dev), pvu);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to request irq\n");
+
+	pvu_field_write(pvu, PVU_EXC_ENABLE, 1);
+	pvu_field_write(pvu, PVU_ENABLED, 1);
+
+	dev_set_drvdata(dev, pvu);
+
+	mutex_lock(&ti_pvu_lock);
+	list_add(&pvu->entry, &ti_pvu_list);
+	mutex_unlock(&ti_pvu_lock);
+
+	return 0;
+}
+
+static void ti_pvu_remove(struct platform_device *pdev)
+{
+	struct ti_pvu *pvu = dev_get_drvdata(&pdev->dev);
+
+	mutex_lock(&ti_pvu_lock);
+	list_del(&pvu->entry);
+	mutex_unlock(&ti_pvu_lock);
+}
+
+static const struct of_device_id ti_pvu_of_match[] = {
+	{ .compatible = "ti,am654-pvu", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ti_pvu_of_match);
+
+static struct platform_driver ti_pvu_driver = {
+	.driver = {
+		.name = "ti-pvu",
+		.of_match_table = ti_pvu_of_match,
+	},
+	.probe = ti_pvu_probe,
+	.remove_new = ti_pvu_remove,
+};
+module_platform_driver(ti_pvu_driver);
diff --git a/include/linux/ti-pvu.h b/include/linux/ti-pvu.h
new file mode 100644
index 000000000000..acd4d9e0dc86
--- /dev/null
+++ b/include/linux/ti-pvu.h
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TI Peripheral Virtualization Unit driver for static DMA isolation
+ *
+ * Copyright (c) 2024, Siemens AG
+ */
+
+#ifndef _LINUX_TI_PVU_H
+#define _LINUX_TI_PVU_H
+
+#include <linux/ioport.h>
+
+int ti_pvu_create_region(unsigned int virt_id, const struct resource *region);
+int ti_pvu_remove_region(unsigned int virt_id, const struct resource *region);
+
+#endif /* _LINUX_TI_PVU_H */
-- 
2.43.0

[PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The AM654 lacks an IOMMU, thus does not support isolating DMA requests
from untrusted PCI devices to selected memory regions this way. Use
static PVU-based protection instead.

For this, we use the availability of restricted-dma-pool memory regions
as trigger and register those as valid DMA targets with the PVU. In
addition, we need to enable the mapping of requester IDs to VirtIDs in
the PCI RC. We only use a single VirtID so far, catching all devices.
This may be extended later on.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
CC: "Krzysztof Wilczyński" <kw@linux.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: linux-pci@vger.kernel.org
---
 drivers/pci/controller/dwc/pci-keystone.c | 101 ++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index 2219b1a866fa..96b871656da4 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -19,6 +19,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/msi.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
 #include <linux/phy/phy.h>
@@ -26,6 +27,7 @@
 #include <linux/regmap.h>
 #include <linux/resource.h>
 #include <linux/signal.h>
+#include <linux/ti-pvu.h>
 
 #include "../../pci.h"
 #include "pcie-designware.h"
@@ -111,6 +113,16 @@
 
 #define PCI_DEVICE_ID_TI_AM654X		0xb00c
 
+#define KS_PCI_VIRTID			0
+
+#define PCIE_VMAP_xP_CTRL		0x0
+#define PCIE_VMAP_xP_REQID		0x4
+#define PCIE_VMAP_xP_VIRTID		0x8
+
+#define PCIE_VMAP_xP_CTRL_EN		BIT(0)
+
+#define PCIE_VMAP_xP_VIRTID_VID_MASK	0xfff
+
 struct ks_pcie_of_data {
 	enum dw_pcie_device_mode mode;
 	const struct dw_pcie_host_ops *host_ops;
@@ -1125,6 +1137,89 @@ static const struct of_device_id ks_pcie_of_match[] = {
 	{ },
 };
 
+#ifdef CONFIG_TI_PVU
+static const char *ks_vmap_res[] = {"vmap_lp", "vmap_hp"};
+
+static int ks_init_restricted_dma(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct of_phandle_iterator it;
+	bool init_vmap = false;
+	struct resource phys;
+	struct resource *res;
+	void __iomem *base;
+	unsigned int n;
+	u32 val;
+	int err;
+
+	of_for_each_phandle(&it, err, dev->of_node, "memory-region",
+			    NULL, 0) {
+		if (!of_device_is_compatible(it.node, "restricted-dma-pool"))
+			continue;
+
+		err = of_address_to_resource(it.node, 0, &phys);
+		if (err < 0) {
+			dev_err(dev, "failed to parse memory region %pOF: %d\n",
+				it.node, err);
+			continue;
+		}
+
+		err = ti_pvu_create_region(KS_PCI_VIRTID, &phys);
+		if (err < 0)
+			return err;
+
+		init_vmap = true;
+	}
+
+	if (init_vmap) {
+		for (n = 0; n < ARRAY_SIZE(ks_vmap_res); n++) {
+			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+							   ks_vmap_res[n]);
+			base = devm_pci_remap_cfg_resource(dev, res);
+			if (IS_ERR(base))
+				return PTR_ERR(base);
+
+			writel(0, base + PCIE_VMAP_xP_REQID);
+
+			val = readl(base + PCIE_VMAP_xP_VIRTID);
+			val &= ~PCIE_VMAP_xP_VIRTID_VID_MASK;
+			val |= KS_PCI_VIRTID;
+			writel(val, base + PCIE_VMAP_xP_VIRTID);
+
+			val = readl(base + PCIE_VMAP_xP_CTRL);
+			val |= PCIE_VMAP_xP_CTRL_EN;
+			writel(val, base + PCIE_VMAP_xP_CTRL);
+		}
+	}
+
+	return 0;
+}
+
+static void ks_release_restricted_dma(struct platform_device *pdev)
+{
+	struct of_phandle_iterator it;
+	struct resource phys;
+	int err;
+
+	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
+			    NULL, 0) {
+		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
+		    of_address_to_resource(it.node, 0, &phys) == 0)
+			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
+
+	}
+}
+#else
+static inline int ks_init_restricted_dma(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static inline void ks_release_restricted_dma(struct platform_device *pdev)
+{
+}
+#endif
+
 static int ks_pcie_probe(struct platform_device *pdev)
 {
 	const struct dw_pcie_host_ops *host_ops;
@@ -1273,6 +1368,10 @@ static int ks_pcie_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_get_sync;
 
+	ret = ks_init_restricted_dma(pdev);
+	if (ret < 0)
+		goto err_get_sync;
+
 	switch (mode) {
 	case DW_PCIE_RC_TYPE:
 		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
@@ -1354,6 +1453,8 @@ static void ks_pcie_remove(struct platform_device *pdev)
 	int num_lanes = ks_pcie->num_lanes;
 	struct device *dev = &pdev->dev;
 
+	ks_release_restricted_dma(pdev);
+
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
 	ks_pcie_disable_phy(ks_pcie);
-- 
2.43.0

[PATCH v4 5/7] arm64: dts: ti: k3-am65-main: Add PVU nodes

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Add nodes for the two PVUs of the AM65. Keep them disabled, though,
because the board has to additionally define DMA pools and the devices
to be isolated.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index ba43325c0eec..2582dad68dff 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -841,6 +841,26 @@ main_cpts_mux: refclk-mux {
 				assigned-clock-parents = <&k3_clks 118 5>;
 			};
 		};
+
+		ti_pvu0: iommu@30f80000 {
+			compatible = "ti,am654-pvu";
+			reg = <0 0x30f80000 0 0x1000>,
+			<0 0x36000000 0 0x100000>;
+			reg-names = "cfg", "tlbif";
+			interrupts-extended = <&intr_main_navss 390>;
+			interrupt-names = "pvu";
+			status = "disabled";
+		};
+
+		ti_pvu1: iommu@30f81000 {
+			compatible = "ti,am654-pvu";
+			reg = <0 0x30f81000 0 0x1000>,
+			<0 0x36100000 0 0x100000>;
+			reg-names = "cfg", "tlbif";
+			interrupts-extended = <&intr_main_navss 389>;
+			interrupt-names = "pvu";
+			status = "disabled";
+		};
 	};
 
 	main_gpio0: gpio@600000 {
-- 
2.43.0

[PATCH v4 6/7] arm64: dts: ti: k3-am65-main: Add VMAP registers to PCI root complexes

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Rewrap the long lines at this chance.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
index 2582dad68dff..08a11ab38fbd 100644
--- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi
@@ -895,8 +895,13 @@ main_gpio1: gpio@601000 {
 
 	pcie0_rc: pcie@5500000 {
 		compatible = "ti,am654-pcie-rc";
-		reg = <0x0 0x5500000 0x0 0x1000>, <0x0 0x5501000 0x0 0x1000>, <0x0 0x10000000 0x0 0x2000>, <0x0 0x5506000 0x0 0x1000>;
-		reg-names = "app", "dbics", "config", "atu";
+		reg = <0x0 0x5500000 0x0 0x1000>,
+		      <0x0 0x5501000 0x0 0x1000>,
+		      <0x0 0x10000000 0x0 0x2000>,
+		      <0x0 0x5506000 0x0 0x1000>,
+		      <0x0 0x2900000 0x0 0x1000>,
+		      <0x0 0x2908000 0x0 0x1000>;
+		reg-names = "app", "dbics", "config", "atu", "vmap_lp", "vmap_hp";
 		power-domains = <&k3_pds 120 TI_SCI_PD_EXCLUSIVE>;
 		#address-cells = <3>;
 		#size-cells = <2>;
@@ -916,8 +921,13 @@ pcie0_rc: pcie@5500000 {
 
 	pcie1_rc: pcie@5600000 {
 		compatible = "ti,am654-pcie-rc";
-		reg = <0x0 0x5600000 0x0 0x1000>, <0x0 0x5601000 0x0 0x1000>, <0x0 0x18000000 0x0 0x2000>, <0x0 0x5606000 0x0 0x1000>;
-		reg-names = "app", "dbics", "config", "atu";
+		reg = <0x0 0x5600000 0x0 0x1000>,
+		      <0x0 0x5601000 0x0 0x1000>,
+		      <0x0 0x18000000 0x0 0x2000>,
+		      <0x0 0x5606000 0x0 0x1000>,
+		      <0x0 0x2910000 0x0 0x1000>,
+		      <0x0 0x2918000 0x0 0x1000>;
+		reg-names = "app", "dbics", "config", "atu", "vmap_lp", "vmap_hp";
 		power-domains = <&k3_pds 121 TI_SCI_PD_EXCLUSIVE>;
 		#address-cells = <3>;
 		#size-cells = <2>;
-- 
2.43.0

[PATCH v4 7/7] arm64: dts: ti: iot2050: Enforce DMA isolation for devices behind PCI RC on Advanced

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Reserve a 64M memory region and ensure that all PCI devices do their DMA
only inside that region. This is configured via a restricted-dma-pool
and enforced with the help of the first PVU.

The pool size may be adjusted via DT fixup during boot by the firmware.
The location is chosen to allow up to 512M for special needs without
causing conflicts.

Applying this isolation which is not totally free in terms of overhead
and memory consumption makes only sense for variants that support secure
booting. These are the advanced-class variants.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 .../ti/k3-am6548-iot2050-advanced-common.dtsi | 21 ++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-common.dtsi b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-common.dtsi
index ae842b85b70d..dc75e3ea9a6b 100644
--- a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-common.dtsi
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (c) Siemens AG, 2018-2021
+ * Copyright (c) Siemens AG, 2018-2024
  *
  * Authors:
  *   Le Jin <le.jin@siemens.com>
@@ -20,6 +20,25 @@ memory@80000000 {
 		/* 2G RAM */
 		reg = <0x00000000 0x80000000 0x00000000 0x80000000>;
 	};
+
+	reserved-memory {
+		pci_restricted_dma_region: restricted-dma@c0000000 {
+			compatible = "restricted-dma-pool";
+			reg = <0 0xc0000000 0 0x4000000>;
+		};
+	};
+};
+
+&pcie0_rc {
+	memory-region = <&pci_restricted_dma_region>;
+};
+
+&pcie1_rc {
+	memory-region = <&pci_restricted_dma_region>;
+};
+
+&ti_pvu0 {
+	status = "okay";
 };
 
 &main_pmx0 {
-- 
2.43.0

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Siddharth Vadapalli]

On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
> to specific regions of host memory. Add the optional property
> "memory-regions" to point to such regions of memory when PVU is used.
> 
> Since the PVU deals with system physical addresses, utilizing the PVU
> with PCIe devices also requires setting up the VMAP registers to map the
> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
> mapped to the system physical address. Hence, describe the VMAP
> registers which are optionally unless the PVU shall used for PCIe.

The last line above seems to be accidentally modified when compared to
the one at:
https://lore.kernel.org/r/ada462d5-157a-4e11-ba25-d412a2bb678f@ti.com/
"Hence, describe the VMAP registers which are optionally
configured whenever PVU is used for PCIe."

If you intended to modify it, then the sentence appears distorted.

Regards,
Siddharth.

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 04.09.24 12:16, Siddharth Vadapalli wrote:
> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>> to specific regions of host memory. Add the optional property
>> "memory-regions" to point to such regions of memory when PVU is used.
>>
>> Since the PVU deals with system physical addresses, utilizing the PVU
>> with PCIe devices also requires setting up the VMAP registers to map the
>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>> mapped to the system physical address. Hence, describe the VMAP
>> registers which are optionally unless the PVU shall used for PCIe.
> 
> The last line above seems to be accidentally modified when compared to
> the one at:
> https://lore.kernel.org/r/ada462d5-157a-4e11-ba25-d412a2bb678f@ti.com/
> "Hence, describe the VMAP registers which are optionally
> configured whenever PVU is used for PCIe."
> 
> If you intended to modify it, then the sentence appears distorted.
> 

Yeah, I wanted to make the original sentence clearer but failed:
"...which are optional unless the PVU shall be used for PCIe."

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
> to specific regions of host memory. Add the optional property
> "memory-regions" to point to such regions of memory when PVU is used.
> 
> Since the PVU deals with system physical addresses, utilizing the PVU
> with PCIe devices also requires setting up the VMAP registers to map the
> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
> mapped to the system physical address. Hence, describe the VMAP
> registers which are optionally unless the PVU shall used for PCIe.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
> CC: "Krzysztof Wilczyński" <kw@linux.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: linux-pci@vger.kernel.org
> ---
>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>  1 file changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> index 0a9d10532cc8..d8182bad92de 100644
> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> @@ -19,16 +19,6 @@ properties:
>        - ti,am654-pcie-rc
>        - ti,keystone-pcie
>  
> -  reg:
> -    maxItems: 4
> -
> -  reg-names:
> -    items:
> -      - const: app
> -      - const: dbics
> -      - const: config
> -      - const: atu


Nothing improved here.

> -
>    interrupts:
>      maxItems: 1
>  
> @@ -84,12 +74,48 @@ if:
>        enum:
>          - ti,am654-pcie-rc
>  then:
> +  properties:
> +    reg:
> +      minItems: 4
> +      maxItems: 6
> +
> +    reg-names:
> +      minItems: 4
> +      items:
> +        - const: app
> +        - const: dbics
> +        - const: config
> +        - const: atu
> +        - const: vmap_lp
> +        - const: vmap_hp
> +
> +    memory-region:
> +      minItems: 1

Missing maxItems

> +      description: |
> +        phandle to one or more restricted DMA pools to be used for all devices
> +        behind this controller. The regions should be defined according to
> +        reserved-memory/shared-dma-pool.yaml.
> +      items:
> +        maxItems: 1

And this feels redundant.

Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 05.09.24 08:32, Krzysztof Kozlowski wrote:
> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>> to specific regions of host memory. Add the optional property
>> "memory-regions" to point to such regions of memory when PVU is used.
>>
>> Since the PVU deals with system physical addresses, utilizing the PVU
>> with PCIe devices also requires setting up the VMAP registers to map the
>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>> mapped to the system physical address. Hence, describe the VMAP
>> registers which are optionally unless the PVU shall used for PCIe.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: linux-pci@vger.kernel.org
>> ---
>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> index 0a9d10532cc8..d8182bad92de 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> @@ -19,16 +19,6 @@ properties:
>>        - ti,am654-pcie-rc
>>        - ti,keystone-pcie
>>  
>> -  reg:
>> -    maxItems: 4
>> -
>> -  reg-names:
>> -    items:
>> -      - const: app
>> -      - const: dbics
>> -      - const: config
>> -      - const: atu
> 
> 
> Nothing improved here.

Yes, explained the background to you. Sorry, if you do not address my
replies, I'm lost with your feedback.

> 
>> -
>>    interrupts:
>>      maxItems: 1
>>  
>> @@ -84,12 +74,48 @@ if:
>>        enum:
>>          - ti,am654-pcie-rc
>>  then:
>> +  properties:
>> +    reg:
>> +      minItems: 4
>> +      maxItems: 6
>> +
>> +    reg-names:
>> +      minItems: 4
>> +      items:
>> +        - const: app
>> +        - const: dbics
>> +        - const: config
>> +        - const: atu
>> +        - const: vmap_lp
>> +        - const: vmap_hp
>> +
>> +    memory-region:
>> +      minItems: 1
> 
> Missing maxItems

Same as above. Please address my answers.

> 
>> +      description: |
>> +        phandle to one or more restricted DMA pools to be used for all devices
>> +        behind this controller. The regions should be defined according to
>> +        reserved-memory/shared-dma-pool.yaml.
>> +      items:
>> +        maxItems: 1
> 
> And this feels redundant.

I can drop that if it's not needed. Unfortunately, schemas are far from
being intuitive to me.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On 05/09/2024 08:40, Jan Kiszka wrote:
> On 05.09.24 08:32, Krzysztof Kozlowski wrote:
>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>> to specific regions of host memory. Add the optional property
>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>
>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>> with PCIe devices also requires setting up the VMAP registers to map the
>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>> mapped to the system physical address. Hence, describe the VMAP
>>> registers which are optionally unless the PVU shall used for PCIe.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>> CC: linux-pci@vger.kernel.org
>>> ---
>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> index 0a9d10532cc8..d8182bad92de 100644
>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> @@ -19,16 +19,6 @@ properties:
>>>        - ti,am654-pcie-rc
>>>        - ti,keystone-pcie
>>>  
>>> -  reg:
>>> -    maxItems: 4
>>> -
>>> -  reg-names:
>>> -    items:
>>> -      - const: app
>>> -      - const: dbics
>>> -      - const: config
>>> -      - const: atu
>>
>>
>> Nothing improved here.
> 
> Yes, explained the background to you. Sorry, if you do not address my
> replies, I'm lost with your feedback.

My magic ball could not figure out the problem, so did not provide the
answer.

I gave you the exact code which illustrates how to do it. If you do it
that way: it works. If you do it other way: it might not work. However
without seeing anything, magic ball was silent, so I am not
participating in game: would you be so kind to give more information so
I won't waste my day in asking what is wrong.

Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On 04/09/2024 12:00, Jan Kiszka wrote:
> +
> +    reg-names:
> +      minItems: 4
> +      items:
> +        - const: app
> +        - const: dbics
> +        - const: config
> +        - const: atu
> +        - const: vmap_lp
> +        - const: vmap_hp
> +
> +    memory-region:

This also did not improve. You did not address any feedback from v3.

Missed feedback:

This *must* be defined in top-level.
I still think this must have some sort of maxItems. I accept your
explanation that you could have multiple memory pools, but I don't think
2147000 pools is possible. Make it 4, 8 or 32.

> +      minItems: 1
> +      description: |
> +        phandle to one or more restricted DMA pools to be used for all devices
> +        behind this controller. The regions should be defined according to
> +        reserved-memory/shared-dma-pool.yaml.
> +      items:
> +        maxItems: 1



Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 05.09.24 08:53, Krzysztof Kozlowski wrote:
> On 05/09/2024 08:40, Jan Kiszka wrote:
>> On 05.09.24 08:32, Krzysztof Kozlowski wrote:
>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>>> to specific regions of host memory. Add the optional property
>>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>>
>>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>>> with PCIe devices also requires setting up the VMAP registers to map the
>>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>>> mapped to the system physical address. Hence, describe the VMAP
>>>> registers which are optionally unless the PVU shall used for PCIe.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>>> CC: linux-pci@vger.kernel.org
>>>> ---
>>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> index 0a9d10532cc8..d8182bad92de 100644
>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>> @@ -19,16 +19,6 @@ properties:
>>>>        - ti,am654-pcie-rc
>>>>        - ti,keystone-pcie
>>>>  
>>>> -  reg:
>>>> -    maxItems: 4
>>>> -
>>>> -  reg-names:
>>>> -    items:
>>>> -      - const: app
>>>> -      - const: dbics
>>>> -      - const: config
>>>> -      - const: atu
>>>
>>>
>>> Nothing improved here.
>>
>> Yes, explained the background to you. Sorry, if you do not address my
>> replies, I'm lost with your feedback.
> 
> My magic ball could not figure out the problem, so did not provide the
> answer.
> 
> I gave you the exact code which illustrates how to do it. If you do it
> that way: it works. If you do it other way: it might not work. However

The link you provided was unfortunately not self-explanatory because if 
I - apparently - do it like that example, I'm getting the errors below.

> without seeing anything, magic ball was silent, so I am not
> participating in game: would you be so kind to give more information so
> I won't waste my day in asking what is wrong.

With my patch:

# make ... dtbs_check
  DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb
  OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb
  OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb
  DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb

With this revert on top:

diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
index d8182bad92de..dd753dae24c6 100644
--- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
+++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
@@ -19,6 +19,16 @@ properties:
       - ti,am654-pcie-rc
       - ti,keystone-pcie
 
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: app
+      - const: dbics
+      - const: config
+      - const: atu
+
   interrupts:
     maxItems: 1
 
@@ -104,18 +114,6 @@ then:
     - msi-map
     - num-viewport
 
-else:
-  properties:
-    reg:
-      maxItems: 4
-
-    reg-names:
-      items:
-        - const: app
-        - const: dbics
-        - const: config
-        - const: atu
-
 unevaluatedProperties: false
 
 examples:

# make ... dtbs_check
  DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
.../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg: [[0, 89128960, 0, 4096], [0, 89133056, 0, 4096], [0, 268435456, 0, 8192], [0, 89153536, 0, 4096], [0, 42991616, 0, 4096], [0, 43024384, 0, 4096]] is too long
        from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
.../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long
        from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
.../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg: [[0, 90177536, 0, 4096], [0, 90181632, 0, 4096], [0, 402653184, 0, 8192], [0, 90202112, 0, 4096], [0, 43057152, 0, 4096], [0, 43089920, 0, 4096]] is too long
        from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
.../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long
        from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
  DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
...


Which magic dtschema spell am I missing to make this work like you 
suggest?

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 05.09.24 08:57, Krzysztof Kozlowski wrote:
> On 04/09/2024 12:00, Jan Kiszka wrote:
>> +
>> +    reg-names:
>> +      minItems: 4
>> +      items:
>> +        - const: app
>> +        - const: dbics
>> +        - const: config
>> +        - const: atu
>> +        - const: vmap_lp
>> +        - const: vmap_hp
>> +
>> +    memory-region:
> 
> This also did not improve. You did not address any feedback from v3.
> 
> Missed feedback:
> 
> This *must* be defined in top-level.

Answered in the other reply.

> I still think this must have some sort of maxItems. I accept your
> explanation that you could have multiple memory pools, but I don't think
> 2147000 pools is possible. Make it 4, 8 or 32.

Can do - if you can explain the benefit of that.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On 05/09/2024 09:15, Jan Kiszka wrote:
> On 05.09.24 08:53, Krzysztof Kozlowski wrote:
>> On 05/09/2024 08:40, Jan Kiszka wrote:
>>> On 05.09.24 08:32, Krzysztof Kozlowski wrote:
>>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>>>> to specific regions of host memory. Add the optional property
>>>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>>>
>>>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>>>> with PCIe devices also requires setting up the VMAP registers to map the
>>>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>>>> mapped to the system physical address. Hence, describe the VMAP
>>>>> registers which are optionally unless the PVU shall used for PCIe.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>>>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>>>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>>>> CC: linux-pci@vger.kernel.org
>>>>> ---
>>>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>> index 0a9d10532cc8..d8182bad92de 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>> @@ -19,16 +19,6 @@ properties:
>>>>>        - ti,am654-pcie-rc
>>>>>        - ti,keystone-pcie
>>>>>  
>>>>> -  reg:
>>>>> -    maxItems: 4
>>>>> -
>>>>> -  reg-names:
>>>>> -    items:
>>>>> -      - const: app
>>>>> -      - const: dbics
>>>>> -      - const: config
>>>>> -      - const: atu
>>>>
>>>>
>>>> Nothing improved here.
>>>
>>> Yes, explained the background to you. Sorry, if you do not address my
>>> replies, I'm lost with your feedback.
>>
>> My magic ball could not figure out the problem, so did not provide the
>> answer.
>>
>> I gave you the exact code which illustrates how to do it. If you do it
>> that way: it works. If you do it other way: it might not work. However
> 
> The link you provided was unfortunately not self-explanatory because if 
> I - apparently - do it like that example, I'm getting the errors below.
> 
>> without seeing anything, magic ball was silent, so I am not
>> participating in game: would you be so kind to give more information so
>> I won't waste my day in asking what is wrong.
> 
> With my patch:
> 
> # make ... dtbs_check
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb
>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb
>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb
>   DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb
> 
> With this revert on top:
> 
> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> index d8182bad92de..dd753dae24c6 100644
> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
> @@ -19,6 +19,16 @@ properties:
>        - ti,am654-pcie-rc
>        - ti,keystone-pcie
>  
> +  reg:
> +    maxItems: 4
> +
> +  reg-names:
> +    items:
> +      - const: app
> +      - const: dbics
> +      - const: config
> +      - const: atu

There is nothing like that in that example.
https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44

> +
>    interrupts:
>      maxItems: 1
>  
> @@ -104,18 +114,6 @@ then:
>      - msi-map
>      - num-viewport
>  
> -else:
> -  properties:
> -    reg:
> -      maxItems: 4
> -
> -    reg-names:
> -      items:
> -        - const: app
> -        - const: dbics
> -        - const: config
> -        - const: atu

Neither this.

Each case MUST be covered, look:
https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191

> -
>  unevaluatedProperties: false
>  
>  examples:
> 
> # make ... dtbs_check
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
> .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg: [[0, 89128960, 0, 4096], [0, 89133056, 0, 4096], [0, 268435456, 0, 8192], [0, 89153536, 0, 4096], [0, 42991616, 0, 4096], [0, 43024384, 0, 4096]] is too long
>         from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
> .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5500000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long
>         from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
> .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg: [[0, 90177536, 0, 4096], [0, 90181632, 0, 4096], [0, 402653184, 0, 8192], [0, 90202112, 0, 4096], [0, 43057152, 0, 4096], [0, 43089920, 0, 4096]] is too long
>         from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
> .../arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb: pcie@5600000: reg-names: ['app', 'dbics', 'config', 'atu', 'vmap_lp', 'vmap_hp'] is too long
>         from schema $id: http://devicetree.org/schemas/pci/ti,am65-pci-host.yaml#
>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
> ...
> 
> 
> Which magic dtschema spell am I missing to make this work like you 
> suggest?

follow the example. You do it entirely different so you have different
result. Code works deterministically.

Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On 05/09/2024 09:16, Jan Kiszka wrote:
> On 05.09.24 08:57, Krzysztof Kozlowski wrote:
>> On 04/09/2024 12:00, Jan Kiszka wrote:
>>> +
>>> +    reg-names:
>>> +      minItems: 4
>>> +      items:
>>> +        - const: app
>>> +        - const: dbics
>>> +        - const: config
>>> +        - const: atu
>>> +        - const: vmap_lp
>>> +        - const: vmap_hp
>>> +
>>> +    memory-region:
>>
>> This also did not improve. You did not address any feedback from v3.
>>
>> Missed feedback:
>>
>> This *must* be defined in top-level.
> 
> Answered in the other reply.

I don't see anywhere answer, but regardless your binding is not an
exception. Please follow rules for every binding, not expect something
special here.

Best regards,
Krzysztof

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Krzysztof Kozlowski]

On 05/09/2024 09:50, Krzysztof Kozlowski wrote:
> On 05/09/2024 09:15, Jan Kiszka wrote:
>> On 05.09.24 08:53, Krzysztof Kozlowski wrote:
>>> On 05/09/2024 08:40, Jan Kiszka wrote:
>>>> On 05.09.24 08:32, Krzysztof Kozlowski wrote:
>>>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>>>>> to specific regions of host memory. Add the optional property
>>>>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>>>>
>>>>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>>>>> with PCIe devices also requires setting up the VMAP registers to map the
>>>>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>>>>> mapped to the system physical address. Hence, describe the VMAP
>>>>>> registers which are optionally unless the PVU shall used for PCIe.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>>>>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>>>>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>>>>> CC: linux-pci@vger.kernel.org
>>>>>> ---
>>>>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>>>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>> index 0a9d10532cc8..d8182bad92de 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>> @@ -19,16 +19,6 @@ properties:
>>>>>>        - ti,am654-pcie-rc
>>>>>>        - ti,keystone-pcie
>>>>>>  
>>>>>> -  reg:
>>>>>> -    maxItems: 4
>>>>>> -
>>>>>> -  reg-names:
>>>>>> -    items:
>>>>>> -      - const: app
>>>>>> -      - const: dbics
>>>>>> -      - const: config
>>>>>> -      - const: atu
>>>>>
>>>>>
>>>>> Nothing improved here.
>>>>
>>>> Yes, explained the background to you. Sorry, if you do not address my
>>>> replies, I'm lost with your feedback.
>>>
>>> My magic ball could not figure out the problem, so did not provide the
>>> answer.
>>>
>>> I gave you the exact code which illustrates how to do it. If you do it
>>> that way: it works. If you do it other way: it might not work. However
>>
>> The link you provided was unfortunately not self-explanatory because if 
>> I - apparently - do it like that example, I'm getting the errors below.
>>
>>> without seeing anything, magic ball was silent, so I am not
>>> participating in game: would you be so kind to give more information so
>>> I won't waste my day in asking what is wrong.
>>
>> With my patch:
>>
>> # make ... dtbs_check
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb
>>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb
>>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb
>>   DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb
>>
>> With this revert on top:
>>
>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> index d8182bad92de..dd753dae24c6 100644
>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>> @@ -19,6 +19,16 @@ properties:
>>        - ti,am654-pcie-rc
>>        - ti,keystone-pcie
>>  
>> +  reg:
>> +    maxItems: 4
>> +
>> +  reg-names:
>> +    items:
>> +      - const: app
>> +      - const: dbics
>> +      - const: config
>> +      - const: atu
> 
> There is nothing like that in that example.
> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
> 
>> +
>>    interrupts:
>>      maxItems: 1
>>  
>> @@ -104,18 +114,6 @@ then:
>>      - msi-map
>>      - num-viewport
>>  
>> -else:
>> -  properties:
>> -    reg:
>> -      maxItems: 4
>> -
>> -    reg-names:
>> -      items:
>> -        - const: app
>> -        - const: dbics
>> -        - const: config
>> -        - const: atu
> 
> Neither this.
> 
> Each case MUST be covered, look:
> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191

Actually your case fits better another example from UFS, so take that one:

https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml#L39

Best regards,
Krzysztof

Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

[Bjorn Helgaas]

[+cc Kishon, just in case you have time/interest ;)]

On Wed, Sep 04, 2024 at 12:00:13PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
> from untrusted PCI devices to selected memory regions this way. Use
> static PVU-based protection instead.
> 
> For this, we use the availability of restricted-dma-pool memory regions
> as trigger and register those as valid DMA targets with the PVU.

I guess the implication is that DMA *outside* the restricted-dma-pool
just gets dropped, and the Requester would see Completion Timeouts or
something for reads?

> In
> addition, we need to enable the mapping of requester IDs to VirtIDs in
> the PCI RC. We only use a single VirtID so far, catching all devices.
> This may be extended later on.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
> CC: "Krzysztof Wilczyński" <kw@linux.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: linux-pci@vger.kernel.org

Regrettably we don't really have anybody taking care of pci-keystone.c
(at least per MAINTAINERS).

> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 101 ++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 2219b1a866fa..96b871656da4 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -19,6 +19,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/msi.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_pci.h>
>  #include <linux/phy/phy.h>
> @@ -26,6 +27,7 @@
>  #include <linux/regmap.h>
>  #include <linux/resource.h>
>  #include <linux/signal.h>
> +#include <linux/ti-pvu.h>
>  
>  #include "../../pci.h"
>  #include "pcie-designware.h"
> @@ -111,6 +113,16 @@
>  
>  #define PCI_DEVICE_ID_TI_AM654X		0xb00c
>  
> +#define KS_PCI_VIRTID			0
> +
> +#define PCIE_VMAP_xP_CTRL		0x0
> +#define PCIE_VMAP_xP_REQID		0x4
> +#define PCIE_VMAP_xP_VIRTID		0x8
> +
> +#define PCIE_VMAP_xP_CTRL_EN		BIT(0)
> +
> +#define PCIE_VMAP_xP_VIRTID_VID_MASK	0xfff
> +
>  struct ks_pcie_of_data {
>  	enum dw_pcie_device_mode mode;
>  	const struct dw_pcie_host_ops *host_ops;
> @@ -1125,6 +1137,89 @@ static const struct of_device_id ks_pcie_of_match[] = {
>  	{ },
>  };
>  
> +#ifdef CONFIG_TI_PVU
> +static const char *ks_vmap_res[] = {"vmap_lp", "vmap_hp"};
> +
> +static int ks_init_restricted_dma(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct of_phandle_iterator it;
> +	bool init_vmap = false;
> +	struct resource phys;
> +	struct resource *res;
> +	void __iomem *base;
> +	unsigned int n;
> +	u32 val;
> +	int err;
> +
> +	of_for_each_phandle(&it, err, dev->of_node, "memory-region",
> +			    NULL, 0) {
> +		if (!of_device_is_compatible(it.node, "restricted-dma-pool"))
> +			continue;
> +
> +		err = of_address_to_resource(it.node, 0, &phys);
> +		if (err < 0) {
> +			dev_err(dev, "failed to parse memory region %pOF: %d\n",
> +				it.node, err);
> +			continue;
> +		}
> +
> +		err = ti_pvu_create_region(KS_PCI_VIRTID, &phys);
> +		if (err < 0)
> +			return err;
> +
> +		init_vmap = true;
> +	}

  if (!init_vmap)
    return 0;

would unindent the following.

> +
> +	if (init_vmap) {
> +		for (n = 0; n < ARRAY_SIZE(ks_vmap_res); n++) {

Since the only use of ks_vmap_res is here, this might be more readable
if there were a helper that would be called twice with the constant
strings, e.g.,

  helper(pdev, "vmap_lp");
  helper(pdev, "vmap_hp");

> +			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +							   ks_vmap_res[n]);

Seems like we should check "res" for error before using it?

> +			base = devm_pci_remap_cfg_resource(dev, res);
> +			if (IS_ERR(base))
> +				return PTR_ERR(base);
> +
> +			writel(0, base + PCIE_VMAP_xP_REQID);
> +
> +			val = readl(base + PCIE_VMAP_xP_VIRTID);
> +			val &= ~PCIE_VMAP_xP_VIRTID_VID_MASK;
> +			val |= KS_PCI_VIRTID;
> +			writel(val, base + PCIE_VMAP_xP_VIRTID);
> +
> +			val = readl(base + PCIE_VMAP_xP_CTRL);
> +			val |= PCIE_VMAP_xP_CTRL_EN;
> +			writel(val, base + PCIE_VMAP_xP_CTRL);

Since there's no explicit use of "restricted-dma-pool" elsewhere in
this patch, I assume the setup above causes the controller to drop any
DMA accesses outside that pool?  I think a comment about how the
controller behavior is being changed would be useful.  Basically the
same comment as for the commit log.

Would there be any value in a dmesg note about a restriction being
enforced?  Seems like it's dependent on both CONFIG_TI_PVU and some DT
properties, and since those are invisible in the log, maybe a note
would help understand/debug any issues?

> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void ks_release_restricted_dma(struct platform_device *pdev)
> +{
> +	struct of_phandle_iterator it;
> +	struct resource phys;
> +	int err;
> +
> +	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
> +			    NULL, 0) {
> +		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> +		    of_address_to_resource(it.node, 0, &phys) == 0)
> +			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);

I guess it's not important to undo the PCIE_VMAP_xP_CTRL_EN and
related setup that was done by ks_init_restricted_dma()?

> +	}
> +}
> +#else
> +static inline int ks_init_restricted_dma(struct platform_device *pdev)
> +{
> +	return 0;
> +}
> +
> +static inline void ks_release_restricted_dma(struct platform_device *pdev)
> +{
> +}
> +#endif
> +
>  static int ks_pcie_probe(struct platform_device *pdev)
>  {
>  	const struct dw_pcie_host_ops *host_ops;
> @@ -1273,6 +1368,10 @@ static int ks_pcie_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_get_sync;
>  
> +	ret = ks_init_restricted_dma(pdev);
> +	if (ret < 0)
> +		goto err_get_sync;
> +
>  	switch (mode) {
>  	case DW_PCIE_RC_TYPE:
>  		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
> @@ -1354,6 +1453,8 @@ static void ks_pcie_remove(struct platform_device *pdev)
>  	int num_lanes = ks_pcie->num_lanes;
>  	struct device *dev = &pdev->dev;
>  
> +	ks_release_restricted_dma(pdev);
> +
>  	pm_runtime_put(dev);
>  	pm_runtime_disable(dev);
>  	ks_pcie_disable_phy(ks_pcie);
> -- 
> 2.43.0
> 

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Bjorn Helgaas]

On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
> to specific regions of host memory. Add the optional property
> "memory-regions" to point to such regions of memory when PVU is used.
> 
> Since the PVU deals with system physical addresses, utilizing the PVU
> with PCIe devices also requires setting up the VMAP registers to map the
> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
> mapped to the system physical address. Hence, describe the VMAP
> registers which are optionally unless the PVU shall used for PCIe.

s/optionally/optional/
s/shall used/should be/ ?  (Not sure that's the sense you intend)

Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

[Jan Kiszka]

On 05.09.24 18:33, Bjorn Helgaas wrote:
> [+cc Kishon, just in case you have time/interest ;)]
> 
> On Wed, Sep 04, 2024 at 12:00:13PM +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
>> from untrusted PCI devices to selected memory regions this way. Use
>> static PVU-based protection instead.
>>
>> For this, we use the availability of restricted-dma-pool memory regions
>> as trigger and register those as valid DMA targets with the PVU.
> 
> I guess the implication is that DMA *outside* the restricted-dma-pool
> just gets dropped, and the Requester would see Completion Timeouts or
> something for reads?

I cannot tell what happens on the PCI bus in that case, maybe someone 
from TI can help out.

On the host side, the PVU will record an error and raise an interrupt 
which will make the driver report that to the kernel log. That's quite 
similar to what IOMMU drivers do on translation faults.

> 
>> In
>> addition, we need to enable the mapping of requester IDs to VirtIDs in
>> the PCI RC. We only use a single VirtID so far, catching all devices.
>> This may be extended later on.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: linux-pci@vger.kernel.org
> 
> Regrettably we don't really have anybody taking care of pci-keystone.c
> (at least per MAINTAINERS).
> 
>> ---
>>  drivers/pci/controller/dwc/pci-keystone.c | 101 ++++++++++++++++++++++
>>  1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 2219b1a866fa..96b871656da4 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/msi.h>
>>  #include <linux/of.h>
>> +#include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_pci.h>
>>  #include <linux/phy/phy.h>
>> @@ -26,6 +27,7 @@
>>  #include <linux/regmap.h>
>>  #include <linux/resource.h>
>>  #include <linux/signal.h>
>> +#include <linux/ti-pvu.h>
>>  
>>  #include "../../pci.h"
>>  #include "pcie-designware.h"
>> @@ -111,6 +113,16 @@
>>  
>>  #define PCI_DEVICE_ID_TI_AM654X		0xb00c
>>  
>> +#define KS_PCI_VIRTID			0
>> +
>> +#define PCIE_VMAP_xP_CTRL		0x0
>> +#define PCIE_VMAP_xP_REQID		0x4
>> +#define PCIE_VMAP_xP_VIRTID		0x8
>> +
>> +#define PCIE_VMAP_xP_CTRL_EN		BIT(0)
>> +
>> +#define PCIE_VMAP_xP_VIRTID_VID_MASK	0xfff
>> +
>>  struct ks_pcie_of_data {
>>  	enum dw_pcie_device_mode mode;
>>  	const struct dw_pcie_host_ops *host_ops;
>> @@ -1125,6 +1137,89 @@ static const struct of_device_id ks_pcie_of_match[] = {
>>  	{ },
>>  };
>>  
>> +#ifdef CONFIG_TI_PVU
>> +static const char *ks_vmap_res[] = {"vmap_lp", "vmap_hp"};
>> +
>> +static int ks_init_restricted_dma(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_iterator it;
>> +	bool init_vmap = false;
>> +	struct resource phys;
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	unsigned int n;
>> +	u32 val;
>> +	int err;
>> +
>> +	of_for_each_phandle(&it, err, dev->of_node, "memory-region",
>> +			    NULL, 0) {
>> +		if (!of_device_is_compatible(it.node, "restricted-dma-pool"))
>> +			continue;
>> +
>> +		err = of_address_to_resource(it.node, 0, &phys);
>> +		if (err < 0) {
>> +			dev_err(dev, "failed to parse memory region %pOF: %d\n",
>> +				it.node, err);
>> +			continue;
>> +		}
>> +
>> +		err = ti_pvu_create_region(KS_PCI_VIRTID, &phys);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		init_vmap = true;
>> +	}
> 
>   if (!init_vmap)
>     return 0;
> 
> would unindent the following.
> 
>> +
>> +	if (init_vmap) {
>> +		for (n = 0; n < ARRAY_SIZE(ks_vmap_res); n++) {
> 
> Since the only use of ks_vmap_res is here, this might be more readable
> if there were a helper that would be called twice with the constant
> strings, e.g.,
> 
>   helper(pdev, "vmap_lp");
>   helper(pdev, "vmap_hp");

OK.

> 
>> +			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +							   ks_vmap_res[n]);
> 
> Seems like we should check "res" for error before using it?

Oh, unfinished constructions.

> 
>> +			base = devm_pci_remap_cfg_resource(dev, res);
>> +			if (IS_ERR(base))
>> +				return PTR_ERR(base);
>> +
>> +			writel(0, base + PCIE_VMAP_xP_REQID);
>> +
>> +			val = readl(base + PCIE_VMAP_xP_VIRTID);
>> +			val &= ~PCIE_VMAP_xP_VIRTID_VID_MASK;
>> +			val |= KS_PCI_VIRTID;
>> +			writel(val, base + PCIE_VMAP_xP_VIRTID);
>> +
>> +			val = readl(base + PCIE_VMAP_xP_CTRL);
>> +			val |= PCIE_VMAP_xP_CTRL_EN;
>> +			writel(val, base + PCIE_VMAP_xP_CTRL);
> 
> Since there's no explicit use of "restricted-dma-pool" elsewhere in
> this patch, I assume the setup above causes the controller to drop any
> DMA accesses outside that pool?  I think a comment about how the
> controller behavior is being changed would be useful.  Basically the
> same comment as for the commit log.

Right, this is what will happen. Will add some comment.

> 
> Would there be any value in a dmesg note about a restriction being
> enforced?  Seems like it's dependent on both CONFIG_TI_PVU and some DT
> properties, and since those are invisible in the log, maybe a note
> would help understand/debug any issues?

This is what you will see when there are reserved region and PVU in 
play:

keystone-pcie 5600000.pcie: assigned reserved memory node restricted-dma@c0000000
ti-pvu 30f80000.iommu: created TLB entry 0.2: 0xc0000000, psize 4 (0x02000000)
ti-pvu 30f80000.iommu: created TLB entry 0.3: 0xc2000000, psize 4 (0x02000000)
...
ath9k 0000:01:00.0: assigned reserved memory node restricted-dma@c0000000

> 
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ks_release_restricted_dma(struct platform_device *pdev)
>> +{
>> +	struct of_phandle_iterator it;
>> +	struct resource phys;
>> +	int err;
>> +
>> +	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
>> +			    NULL, 0) {
>> +		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
>> +		    of_address_to_resource(it.node, 0, &phys) == 0)
>> +			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
> 
> I guess it's not important to undo the PCIE_VMAP_xP_CTRL_EN and
> related setup that was done by ks_init_restricted_dma()?
> 

Right, I didn't find a reason to do that.

>> +	}
>> +}
>> +#else
>> +static inline int ks_init_restricted_dma(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline void ks_release_restricted_dma(struct platform_device *pdev)
>> +{
>> +}
>> +#endif
>> +
>>  static int ks_pcie_probe(struct platform_device *pdev)
>>  {
>>  	const struct dw_pcie_host_ops *host_ops;
>> @@ -1273,6 +1368,10 @@ static int ks_pcie_probe(struct platform_device *pdev)
>>  	if (ret < 0)
>>  		goto err_get_sync;
>>  
>> +	ret = ks_init_restricted_dma(pdev);
>> +	if (ret < 0)
>> +		goto err_get_sync;
>> +
>>  	switch (mode) {
>>  	case DW_PCIE_RC_TYPE:
>>  		if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_HOST)) {
>> @@ -1354,6 +1453,8 @@ static void ks_pcie_remove(struct platform_device *pdev)
>>  	int num_lanes = ks_pcie->num_lanes;
>>  	struct device *dev = &pdev->dev;
>>  
>> +	ks_release_restricted_dma(pdev);
>> +
>>  	pm_runtime_put(dev);
>>  	pm_runtime_disable(dev);
>>  	ks_pcie_disable_phy(ks_pcie);
>> -- 
>> 2.43.0
>>

Thanks,
Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

[Bjorn Helgaas]

On Thu, Sep 05, 2024 at 09:07:36PM +0200, Jan Kiszka wrote:
> On 05.09.24 18:33, Bjorn Helgaas wrote:
> > [+cc Kishon, just in case you have time/interest ;)]
> > 
> > On Wed, Sep 04, 2024 at 12:00:13PM +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> The AM654 lacks an IOMMU, thus does not support isolating DMA requests
> >> from untrusted PCI devices to selected memory regions this way. Use
> >> static PVU-based protection instead.
> >>
> >> For this, we use the availability of restricted-dma-pool memory regions
> >> as trigger and register those as valid DMA targets with the PVU.
> > 
> > I guess the implication is that DMA *outside* the restricted-dma-pool
> > just gets dropped, and the Requester would see Completion Timeouts or
> > something for reads?
> 
> I cannot tell what happens on the PCI bus in that case, maybe someone 
> from TI can help out.
> 
> On the host side, the PVU will record an error and raise an interrupt 
> which will make the driver report that to the kernel log. That's quite 
> similar to what IOMMU drivers do on translation faults.

The main thing is that the DMA doesn't complete, as you mentioned
below.

> > Since there's no explicit use of "restricted-dma-pool" elsewhere in
> > this patch, I assume the setup above causes the controller to drop any
> > DMA accesses outside that pool?  I think a comment about how the
> > controller behavior is being changed would be useful.  Basically the
> > same comment as for the commit log.
> 
> Right, this is what will happen. Will add some comment.
> 
> > Would there be any value in a dmesg note about a restriction being
> > enforced?  Seems like it's dependent on both CONFIG_TI_PVU and some DT
> > properties, and since those are invisible in the log, maybe a note
> > would help understand/debug any issues?
> 
> This is what you will see when there are reserved region and PVU in 
> play:
> 
> keystone-pcie 5600000.pcie: assigned reserved memory node restricted-dma@c0000000
> ti-pvu 30f80000.iommu: created TLB entry 0.2: 0xc0000000, psize 4 (0x02000000)
> ti-pvu 30f80000.iommu: created TLB entry 0.3: 0xc2000000, psize 4 (0x02000000)
> ...
> ath9k 0000:01:00.0: assigned reserved memory node restricted-dma@c0000000

Looks reasonable and solves my concern.

> >> +	of_for_each_phandle(&it, err, pdev->dev.of_node, "memory-region",
> >> +			    NULL, 0) {
> >> +		if (of_device_is_compatible(it.node, "restricted-dma-pool") &&
> >> +		    of_address_to_resource(it.node, 0, &phys) == 0)
> >> +			ti_pvu_remove_region(KS_PCI_VIRTID, &phys);
> > 
> > I guess it's not important to undo the PCIE_VMAP_xP_CTRL_EN and
> > related setup that was done by ks_init_restricted_dma()?
> > 
> 
> Right, I didn't find a reason to do that.

OK, as long as you considered it :)

Bjorn

Re: [PATCH v4 4/7] PCI: keystone: Add supported for PVU-based DMA isolation on AM654

[Jan Kiszka]

On 05.09.24 21:07, Jan Kiszka wrote:
> On 05.09.24 18:33, Bjorn Helgaas wrote:
>>> +			res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> +							   ks_vmap_res[n]);
>>
>> Seems like we should check "res" for error before using it?
> 
> Oh, unfinished constructions.

In fact, devm_pci_remap_cfg_resource takes care of res == NULL.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 05.09.24 09:56, Krzysztof Kozlowski wrote:
> On 05/09/2024 09:50, Krzysztof Kozlowski wrote:
>> On 05/09/2024 09:15, Jan Kiszka wrote:
>>> On 05.09.24 08:53, Krzysztof Kozlowski wrote:
>>>> On 05/09/2024 08:40, Jan Kiszka wrote:
>>>>> On 05.09.24 08:32, Krzysztof Kozlowski wrote:
>>>>>> On Wed, Sep 04, 2024 at 12:00:11PM +0200, Jan Kiszka wrote:
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> The PVU on the AM65 SoC is capable of restricting DMA from PCIe devices
>>>>>>> to specific regions of host memory. Add the optional property
>>>>>>> "memory-regions" to point to such regions of memory when PVU is used.
>>>>>>>
>>>>>>> Since the PVU deals with system physical addresses, utilizing the PVU
>>>>>>> with PCIe devices also requires setting up the VMAP registers to map the
>>>>>>> Requester ID of the PCIe device to the CBA Virtual ID, which in turn is
>>>>>>> mapped to the system physical address. Hence, describe the VMAP
>>>>>>> registers which are optionally unless the PVU shall used for PCIe.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>> CC: Lorenzo Pieralisi <lpieralisi@kernel.org>
>>>>>>> CC: "Krzysztof Wilczyński" <kw@linux.com>
>>>>>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>>>>>> CC: linux-pci@vger.kernel.org
>>>>>>> ---
>>>>>>>  .../bindings/pci/ti,am65-pci-host.yaml        | 52 ++++++++++++++-----
>>>>>>>  1 file changed, 40 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>>> index 0a9d10532cc8..d8182bad92de 100644
>>>>>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>>>>>> @@ -19,16 +19,6 @@ properties:
>>>>>>>        - ti,am654-pcie-rc
>>>>>>>        - ti,keystone-pcie
>>>>>>>  
>>>>>>> -  reg:
>>>>>>> -    maxItems: 4
>>>>>>> -
>>>>>>> -  reg-names:
>>>>>>> -    items:
>>>>>>> -      - const: app
>>>>>>> -      - const: dbics
>>>>>>> -      - const: config
>>>>>>> -      - const: atu
>>>>>>
>>>>>>
>>>>>> Nothing improved here.
>>>>>
>>>>> Yes, explained the background to you. Sorry, if you do not address my
>>>>> replies, I'm lost with your feedback.
>>>>
>>>> My magic ball could not figure out the problem, so did not provide the
>>>> answer.
>>>>
>>>> I gave you the exact code which illustrates how to do it. If you do it
>>>> that way: it works. If you do it other way: it might not work. However
>>>
>>> The link you provided was unfortunately not self-explanatory because if 
>>> I - apparently - do it like that example, I'm getting the errors below.
>>>
>>>> without seeing anything, magic ball was silent, so I am not
>>>> participating in game: would you be so kind to give more information so
>>>> I won't waste my day in asking what is wrong.
>>>
>>> With my patch:
>>>
>>> # make ... dtbs_check
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6528-iot2050-basic-pg2.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dtb
>>>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-ekey-pcie.dtb
>>>   OVL [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2-bkey-usb3.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-pg2.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dtb
>>>   DTC [C] arch/arm64/boot/dts/ti/k3-am654-base-board.dtb
>>>
>>> With this revert on top:
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> index d8182bad92de..dd753dae24c6 100644
>>> --- a/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/ti,am65-pci-host.yaml
>>> @@ -19,6 +19,16 @@ properties:
>>>        - ti,am654-pcie-rc
>>>        - ti,keystone-pcie
>>>  
>>> +  reg:
>>> +    maxItems: 4
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: app
>>> +      - const: dbics
>>> +      - const: config
>>> +      - const: atu
>>
>> There is nothing like that in that example.
>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44
>>
>>> +
>>>    interrupts:
>>>      maxItems: 1
>>>  
>>> @@ -104,18 +114,6 @@ then:
>>>      - msi-map
>>>      - num-viewport
>>>  
>>> -else:
>>> -  properties:
>>> -    reg:
>>> -      maxItems: 4
>>> -
>>> -    reg-names:
>>> -      items:
>>> -        - const: app
>>> -        - const: dbics
>>> -        - const: config
>>> -        - const: atu
>>
>> Neither this.
>>
>> Each case MUST be covered, look:
>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L191
> 
> Actually your case fits better another example from UFS, so take that one:
> 
> https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/samsung,exynos-ufs.yaml#L39
> 

Via lots of trial-and-error, I think I now understood the magic behind
this: You may reduce maxItems in a condition, but you cannot increase
it. That's why all my intuition-based attempts failed before. Not sure
if this is a specified property of the meta schema or just an oddity of
the validator.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v4 2/7] dt-bindings: PCI: ti,am65: Extend for use with PVU

[Jan Kiszka]

On 05.09.24 09:16, Jan Kiszka wrote:
> On 05.09.24 08:57, Krzysztof Kozlowski wrote:
>> On 04/09/2024 12:00, Jan Kiszka wrote:
>>> +
>>> +    reg-names:
>>> +      minItems: 4
>>> +      items:
>>> +        - const: app
>>> +        - const: dbics
>>> +        - const: config
>>> +        - const: atu
>>> +        - const: vmap_lp
>>> +        - const: vmap_hp
>>> +
>>> +    memory-region:
>>
>> This also did not improve. You did not address any feedback from v3.
>>
>> Missed feedback:
>>
>> This *must* be defined in top-level.
> 
> Answered in the other reply.
> 
>> I still think this must have some sort of maxItems. I accept your
>> explanation that you could have multiple memory pools, but I don't think
>> 2147000 pools is possible. Make it 4, 8 or 32.
> 
> Can do - if you can explain the benefit of that.
> 

It turned out during further investigations that swiotlb actually only
allows a single restricted DMA pool per device. So this discussion
auto-resolves, bindings and code will be adjusted in v5.

Jan

-- 
Siemens AG, Technology
Linux Expert Center