Add support for Hisilicon SMMU architecture

RFC PATCH Superseded Zhen Lei 3 participants 16 messages Jun 12, 2014 – Jun 19, 2014

Subsystems (3)

Device tree bindings IOMMU Arm SMMU drivers

lore

Share & tools

lore

Zhen Lei

author

2014-06-12 05:08 UTC #0 lore raw

v2 cover Add support for Hisilicon SMMU architecture

Changes in v2:

- Split Hisilicon smmu implementation in a separate file, hisi-smmu.c
- Refactor arm-smmu.c. Some direct call hardware dependent functions replaced with hooks. And move common struct and marco definition into arm-smmu.h
- Merge the description of Hisilicon private properties into arm,smmu.txt

Hisilicon smmu is similar to arm-smmu, some code can be direct reused. For
example: map and unmap, device tree configuration, and the software framework. I found that, abstract about 11 hardware dependent functions in arm-smmu.c is enough . Abstract means use hooks to replace the direct call of functions. Now, if need to support Hisilicon SMMU or others arm smmu variants, just selective rewrite these hooks. And I add a dt_cfg_probe hook, to allow each variant parse their hardware special configuration. Finally, flush_pgtable is a special case, hardware independent but maybe referenced. So, total 13 hooks.

Additional:
Arnd Bergmann told me to pay attention to the discussion of "devicetree: Add generic IOMMU device tree bindings". But, this subject is still discussing, no conclusion now. Besides, exclude the Hisilicon private properties, all keep the same to arm-smmu. So, I think this version is not relate to the discussion now.

Zhen Lei (3):
iommu/arm: Adjust code to facilitate support arm smmu variants iommu/hisilicon: Add support for Hisilicon Ltd. System MMU
architecture
documentation/iommu: Add description of Hisilicon SMMU private
binding

 .../devicetree/bindings/iommu/arm,smmu.txt         |  16 +
 drivers/iommu/Kconfig                              |   7 +
 drivers/iommu/Makefile                             |   1 +
 drivers/iommu/arm-smmu.c                           | 190 +++---
 drivers/iommu/arm-smmu.h                           | 165 +++++
 drivers/iommu/hisi-smmu.c                          | 662 +++++++++++++++++++++
 6 files changed, 927 insertions(+), 114 deletions(-)
 create mode 100644 drivers/iommu/arm-smmu.h
 create mode 100644 drivers/iommu/hisi-smmu.c

--
1.8.0

Zhen Lei

author

2014-06-12 05:08 UTC #1 lore raw

RFC PATCH v2 1/3 iommu/arm: Adjust code to facilitate support arm smmu variants

There is no harmful for original arm-smmu process. A variant can appropriate rewrite hooks according to the hardware.

1. Pick out hardware dependent functions, replace direct call with hooks
2. Move common struct and marco definition into arm-smmu.h
3. flush_pgtable is a special case, hardware independent but maybe referenced

Signed-off-by Zhen Lei <[email protected]>

---
 drivers/iommu/arm-smmu.c | 186 ++++++++++++++++++-----------------------------
 drivers/iommu/arm-smmu.h | 154 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 226 insertions(+), 114 deletions(-)
 create mode 100644 drivers/iommu/arm-smmu.h

drivers/iommu/arm-smmu.c +72 ~~-114~~

index 1599354..413a1f2 100644
@@ -46,15 +46,7 @@
 #include <linux/amba/bus.h>
 
 #include <asm/pgalloc.h>
-
-/* Maximum number of stream IDs assigned to a single device */
-#define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
-
-/* Maximum number of context banks per SMMU */
-#define ARM_SMMU_MAX_CBS		128
-
-/* Maximum number of mapping groups per SMMU */
-#define ARM_SMMU_MAX_SMRS		128
+#include "arm-smmu.h"
 
 /* SMMU global address space */
 #define ARM_SMMU_GR0(smmu)		((smmu)->base)

@@ -323,96 +315,12 @@
 
 #define FSYNR0_WNR			(1 << 4)
 
-struct arm_smmu_smr {
-	u8				idx;
-	u16				mask;
-	u16				id;
-};
-
-struct arm_smmu_master {
-	struct device_node		*of_node;
-
-	/*
-	 * The following is specific to the master's position in the
-	 * SMMU chain.
-	 */
-	struct rb_node			node;
-	int				num_streamids;
-	u16				streamids[MAX_MASTER_STREAMIDS];
-
-	/*
-	 * We only need to allocate these on the root SMMU, as we
-	 * configure unmatched streams to bypass translation.
-	 */
-	struct arm_smmu_smr		*smrs;
-};
-
-struct arm_smmu_device {
-	struct device			*dev;
-	struct device_node		*parent_of_node;
-
-	void __iomem			*base;
-	unsigned long			size;
-	unsigned long			pagesize;
-
-#define ARM_SMMU_FEAT_COHERENT_WALK	(1 << 0)
-#define ARM_SMMU_FEAT_STREAM_MATCH	(1 << 1)
-#define ARM_SMMU_FEAT_TRANS_S1		(1 << 2)
-#define ARM_SMMU_FEAT_TRANS_S2		(1 << 3)
-#define ARM_SMMU_FEAT_TRANS_NESTED	(1 << 4)
-	u32				features;
-
-#define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
-	u32				options;
-	int				version;
-
-	u32				num_context_banks;
-	u32				num_s2_context_banks;
-	DECLARE_BITMAP(context_map, ARM_SMMU_MAX_CBS);
-	atomic_t			irptndx;
-
-	u32				num_mapping_groups;
-	DECLARE_BITMAP(smr_map, ARM_SMMU_MAX_SMRS);
-
-	unsigned long			input_size;
-	unsigned long			s1_output_size;
-	unsigned long			s2_output_size;
-
-	u32				num_global_irqs;
-	u32				num_context_irqs;
-	unsigned int			*irqs;
-
-	struct list_head		list;
-	struct rb_root			masters;
-};
-
-struct arm_smmu_cfg {
-	struct arm_smmu_device		*smmu;
-	u8				cbndx;
-	u8				irptndx;
-	u32				cbar;
-	pgd_t				*pgd;
-};
-#define INVALID_IRPTNDX			0xff
-
 #define ARM_SMMU_CB_ASID(cfg)		((cfg)->cbndx)
 #define ARM_SMMU_CB_VMID(cfg)		((cfg)->cbndx + 1)
 
-struct arm_smmu_domain {
-	/*
-	 * A domain can span across multiple, chained SMMUs and requires
-	 * all devices within the domain to follow the same translation
-	 * path.
-	 */
-	struct arm_smmu_device		*leaf_smmu;
-	struct arm_smmu_cfg		root_cfg;
-	phys_addr_t			output_mask;
-
-	spinlock_t			lock;
-};
-
 static DEFINE_SPINLOCK(arm_smmu_devices_lock);
 static LIST_HEAD(arm_smmu_devices);
+static struct arm_smmu_hwdep_ops smmu_hwdep_ops;
 
 struct arm_smmu_option_prop {
 	u32 opt;

@@ -555,6 +463,12 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
 	clear_bit(idx, map);
 }
 
+static int arm_smmu_alloc_context(struct arm_smmu_device *smmu,
+			int start, int end, struct arm_smmu_master *master)
+{
+	return __arm_smmu_alloc_bitmap(smmu->context_map, start, end);
+}
+
 /* Wait for any pending TLB invalidations to complete */
 static void arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
 {

@@ -590,7 +504,7 @@ static void arm_smmu_tlb_inv_context(struct arm_smmu_cfg *cfg)
 			       base + ARM_SMMU_GR0_TLBIVMID);
 	}
 
-	arm_smmu_tlb_sync(smmu);
+	smmu_hwdep_ops.tlb_sync(smmu);
 }
 
 static irqreturn_t arm_smmu_context_fault(int irq, void *dev)

@@ -859,6 +773,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
 	struct arm_smmu_device *smmu, *parent;
+	struct arm_smmu_master *master;
 
 	/*
 	 * Walk the SMMU chain to find the root device for this chain.

@@ -873,7 +788,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		smmu_domain->output_mask &= (1ULL << smmu->s2_output_size) - 1;
 	} while ((parent = find_parent_smmu(smmu)));
 
-	if (!find_smmu_master(smmu, dev->of_node)) {
+	master = find_smmu_master(smmu, dev->of_node);
+	if (!master) {
 		dev_err(dev, "unable to find root SMMU for device\n");
 		return -ENODEV;
 	}

@@ -893,8 +809,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		start = smmu->num_s2_context_banks;
 	}
 
-	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
-				      smmu->num_context_banks);
+	ret = smmu_hwdep_ops.alloc_context(smmu, start,
+					smmu->num_context_banks, master);
 	if (IS_ERR_VALUE(ret))
 		return ret;

@@ -907,7 +823,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	}
 
 	irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];
-	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
+	ret = request_irq(irq, smmu_hwdep_ops.context_fault, IRQF_SHARED,
 			  "arm-smmu-context-fault", domain);
 	if (IS_ERR_VALUE(ret)) {
 		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",

@@ -917,7 +833,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	}
 
 	root_cfg->smmu = smmu;
-	arm_smmu_init_context_bank(smmu_domain);
+	smmu_hwdep_ops.init_context_bank(smmu_domain);
 	return ret;
 
 out_free_context:

@@ -925,21 +841,29 @@ out_free_context:
 	return ret;
 }
 
+static void arm_smmu_destroy_context_bank(struct arm_smmu_domain *smmu_domain)
+{
+	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
+	struct arm_smmu_device *smmu = root_cfg->smmu;
+	void __iomem *cb_base;
+
+	/* Disable the context bank and nuke the TLB before freeing it. */
+	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
+	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
+	smmu_hwdep_ops.tlb_inv_context(root_cfg);
+}
+
 static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 {
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 	struct arm_smmu_cfg *root_cfg = &smmu_domain->root_cfg;
 	struct arm_smmu_device *smmu = root_cfg->smmu;
-	void __iomem *cb_base;
 	int irq;
 
 	if (!smmu)
 		return;
 
-	/* Disable the context bank and nuke the TLB before freeing it. */
-	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, root_cfg->cbndx);
-	writel_relaxed(0, cb_base + ARM_SMMU_CB_SCTLR);
-	arm_smmu_tlb_inv_context(root_cfg);
+	smmu_hwdep_ops.destroy_context_bank(smmu_domain);
 
 	if (root_cfg->irptndx != INVALID_IRPTNDX) {
 		irq = smmu->irqs[smmu->num_global_irqs + root_cfg->irptndx];

@@ -1227,7 +1151,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (!master)
 		return -ENODEV;
 
-	return arm_smmu_domain_add_master(smmu_domain, master);
+	return smmu_hwdep_ops.domain_add_master(smmu_domain, master);
 
 err_unlock:
 	spin_unlock_irqrestore(&smmu_domain->lock, flags);

@@ -1241,7 +1165,7 @@ static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 
 	master = find_smmu_master(smmu_domain->leaf_smmu, dev->of_node);
 	if (master)
-		arm_smmu_domain_remove_master(smmu_domain, master);
+		smmu_hwdep_ops.domain_remove_master(smmu_domain, master);
 }
 
 static bool arm_smmu_pte_is_contiguous_range(unsigned long addr,

@@ -1498,7 +1422,7 @@ static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 
 	ret = arm_smmu_handle_mapping(smmu_domain, iova, 0, size, 0);
-	arm_smmu_tlb_inv_context(&smmu_domain->root_cfg);
+	smmu_hwdep_ops.tlb_inv_context(&smmu_domain->root_cfg);
 	return ret ? 0 : size;
 }

@@ -1625,7 +1549,7 @@ static struct iommu_ops arm_smmu_ops = {
 			   PAGE_SIZE),
 };
 
-static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
+static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
 {
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	void __iomem *cb_base;

@@ -1672,8 +1596,10 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
-	arm_smmu_tlb_sync(smmu);
+	smmu_hwdep_ops.tlb_sync(smmu);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+
+	return 0;
 }
 
 static int arm_smmu_id_size_to_bits(int size)

@@ -1839,6 +1765,27 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+static struct arm_smmu_hwdep_ops smmu_hwdep_ops = {
+	.alloc_context		= arm_smmu_alloc_context,
+	.flush_pgtable		= arm_smmu_flush_pgtable,
+	.tlb_sync		= arm_smmu_tlb_sync,
+	.tlb_inv_context	= arm_smmu_tlb_inv_context,
+	.context_fault		= arm_smmu_context_fault,
+	.global_fault		= arm_smmu_global_fault,
+	.init_context_bank	= arm_smmu_init_context_bank,
+	.destroy_context_bank	= arm_smmu_destroy_context_bank,
+	.domain_add_master	= arm_smmu_domain_add_master,
+	.domain_remove_master	= arm_smmu_domain_remove_master,
+	.device_reset		= arm_smmu_device_reset,
+	.device_cfg_probe	= arm_smmu_device_cfg_probe,
+	.dt_cfg_probe		= NULL,
+};
+
+void __attribute__((weak))
+arm_smmu_hwdep_ops_override(struct arm_smmu_hwdep_ops *ops)
+{
+}
+
 static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 {
 	struct resource *res;

@@ -1916,7 +1863,13 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if ((dev_node = of_parse_phandle(dev->of_node, "smmu-parent", 0)))
 		smmu->parent_of_node = dev_node;
 
-	err = arm_smmu_device_cfg_probe(smmu);
+	if (smmu_hwdep_ops.dt_cfg_probe) {
+		err = smmu_hwdep_ops.dt_cfg_probe(smmu, dev);
+		if (err)
+			goto out_put_parent;
+	}
+
+	err = smmu_hwdep_ops.device_cfg_probe(smmu);
 	if (err)
 		goto out_put_parent;

@@ -1933,7 +1886,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 
 	for (i = 0; i < smmu->num_global_irqs; ++i) {
 		err = request_irq(smmu->irqs[i],
-				  arm_smmu_global_fault,
+				  smmu_hwdep_ops.global_fault,
 				  IRQF_SHARED,
 				  "arm-smmu global fault",
 				  smmu);

@@ -1949,7 +1902,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	list_add(&smmu->list, &arm_smmu_devices);
 	spin_unlock(&arm_smmu_devices_lock);
 
-	arm_smmu_device_reset(smmu);
+	err = smmu_hwdep_ops.device_reset(smmu);
+	if (err)
+		goto out_free_irqs;
+
 	return 0;
 
 out_free_irqs:

@@ -2006,7 +1962,7 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 		free_irq(smmu->irqs[i], smmu);
 
 	/* Turn the thing off */
-	writel(sCR0_CLIENTPD,ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
+	writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 	return 0;
 }

@@ -2035,6 +1991,8 @@ static int __init arm_smmu_init(void)
 {
 	int ret;
 
+	arm_smmu_hwdep_ops_override(&smmu_hwdep_ops);
+
 	ret = platform_driver_register(&arm_smmu_driver);
 	if (ret)
 		return ret;

drivers/iommu/arm-smmu.h new file +154 -0

154 lines not on this page · raw message

--
1.8.0

Zhen Lei

author

2014-06-12 05:08 UTC #2 lore raw

RFC PATCH v2 2/3 iommu/hisilicon: Add support for Hisilicon Ltd. System MMU architecture

In reply to Zhen Lei

Here is the major hardware difference compare to arm-smmu specification:
1. Only have global register space 0, no GR1. Actually, some context bank registers have been moved into GR0 to optimize hardware logic.

2. StreamID is 16 bits, highest 8 bits is VMID, lowest 8 bits is ASID. StreamID match is not support, so direct use VMID and ASID to index context bank. First use VMID to index stage2 context bank, then use ASID to index stage1 context bank. In fact, max 256 stage2 context banks, each stage2 context bank relate to 256 stage1 context banks.

|-----------------|            |-----------------|
|stage2 CB VMID0  |----------->|stage1 CB ASID0  |
|-----------------|            |-----------------|
|   ......        |            |   ......        |
|-----------------|            |-----------------|
|stage2 CB VMID255|-----|      |stage2 CB ASID255|
|-----------------|     |      |-----------------|
                        |
                        |
                        |
                        |----->|-----------------|
                               |stage1 CB ASID0  |
                               |-----------------|
                               |   ......        |
                               |-----------------|
                               |stage2 CB ASID255|
                               |-----------------|

3. The base address of stage2 context bank is stored in SMMU_CFG_S2CTBAR, and the base address of stage1 context bank is stored in S2_S1CTBAR(locate in stage2 context bank).

4. All context bank fault share 8 groups of context fault registers. That is, max record 8 context faults. Fault syndrome register recorded StreamID to help software determine which context bank issue fault.

5. When choose stage1 translation and stage2 bypass mode, the register sequence impact output attribute is: S1_SCTLR, CBAR, S2CR(for arm-smmu, process S2CR first). This issue a problem, because total 256 stage1 CBs share a stage2 CB when VMID=0. If some devices use bypass mode(use device built-in attributes), and some devices use map mode(use page table entry specified attributes), smmu can not work properly. The avoidance scheme is occupy another stage2 CB(S2CR) to support bypass mode.

Signed-off-by Zhen Lei <[email protected]>

---
 drivers/iommu/Kconfig     |   7 +
 drivers/iommu/Makefile    |   1 +
 drivers/iommu/arm-smmu.c  |   4 +
 drivers/iommu/arm-smmu.h  |  11 +
 drivers/iommu/hisi-smmu.c | 662 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 685 insertions(+)
 create mode 100644 drivers/iommu/hisi-smmu.c

drivers/iommu/Kconfig +7 -0

index d260605..ef4e851 100644
@@ -305,4 +305,11 @@ config ARM_SMMU
 	  Say Y here if your SoC includes an IOMMU device implementing
 	  the ARM SMMU architecture.
 
+config HISI_SMMU
+	bool "Hisilicon Ltd. System MMU (SMMU) Support"
+	depends on ARM_SMMU
+	help
+	  Say Y here if your SoC includes an IOMMU device implementing
+	  the Hisilicon SMMU architecture.
+
 endif # IOMMU_SUPPORT

drivers/iommu/Makefile +1 -0

index 8893bad..e06e36e 100644
@@ -5,6 +5,7 @@ obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
+obj-$(CONFIG_HISI_SMMU) += hisi-smmu.o
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
 obj-$(CONFIG_INTEL_IOMMU) += iova.o intel-iommu.o
 obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o

drivers/iommu/arm-smmu.c +4 -0

index 413a1f2..c952d72 100644
@@ -968,6 +968,9 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 {
 	struct arm_smmu_domain *smmu_domain = domain->priv;
 
+	if (smmu_domain->num_of_masters)
+		dev_err(smmu_domain->leaf_smmu->dev, "destroy domain with active dev!\n");
+
 	/*
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.

@@ -1972,6 +1975,7 @@ static struct of_device_id arm_smmu_of_match[] = {
 	{ .compatible = "arm,smmu-v2", },
 	{ .compatible = "arm,mmu-400", },
 	{ .compatible = "arm,mmu-500", },
+	{ .compatible = "hisilicon,smmu-v1", },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);

drivers/iommu/arm-smmu.h +11 -0

index 79366ee..2941b39 100644
@@ -24,8 +24,12 @@
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
 
+#ifdef CONFIG_HISI_SMMU
+#define ARM_SMMU_MAX_CBS		256
+#else
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
+#endif
 
 /* Maximum number of mapping groups per SMMU */
 #define ARM_SMMU_MAX_SMRS		128

@@ -58,6 +62,12 @@ struct arm_smmu_device {
 	struct device			*dev;
 	struct device_node		*parent_of_node;
 
+#ifdef CONFIG_HISI_SMMU
+	void __iomem			*s1cbt;
+	void __iomem			*s2cbt;
+	u8				cb_mtcfg[ARM_SMMU_MAX_CBS];
+#endif
+
 	void __iomem			*base;
 	unsigned long			size;
 	unsigned long			pagesize;

@@ -113,6 +123,7 @@ struct arm_smmu_domain {
 	phys_addr_t			output_mask;
 
 	spinlock_t			lock;
+	int				num_of_masters;
 };
 
 /**

drivers/iommu/hisi-smmu.c new file +662 -0

662 lines not on this page · raw message

--
1.8.0

Zhen Lei

author

2014-06-12 05:08 UTC #3 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

In reply to Zhen Lei

This patch adds a description of private properties for the Hisilicon System MMU architecture.

Signed-off-by Zhen Lei <[email protected]>

---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Documentation/devicetree/bindings/iommu/arm,smmu.txt +16 -0

index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.

@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate address but
+                  translate attributes. The StreamIDs list here can not be
+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-back
+                  2, non-cacheable, Normal outer and inner non-cacheable
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass mode.
+                  If omit, vmid=255 is default. If bypass and map mode can
+                  share a same S2CR, config vmid=0.
+
 Example:
 
         smmu {

--
1.8.0

Will Deacon

2014-06-16 16:39 UTC #4 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 28 lines · Zhen Lei

This patch adds a description of private properties for the Hisilicon System MMU architecture.

Signed-off-by Zhen Lei <[email protected]>

---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Documentation/devicetree/bindings/iommu/arm,smmu.txt +5 -0

index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.

@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate address but
+                  translate attributes. The StreamIDs list here can not be

Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71

Quoted 10 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt +10 -0

+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-back
+                  2, non-cacheable, Normal outer and inner non-cacheable
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass mode.
+                  If omit, vmid=255 is default. If bypass and map mode can
+                  share a same S2CR, config vmid=0.

These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters?

Will

leizhen

author

2014-06-17 07:50 UTC #5 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 45 lines · Will Deacon

Quoted 28 lines · Zhen Lei

This patch adds a description of private properties for the Hisilicon System MMU architecture.

Signed-off-by Zhen Lei <[email protected]>

---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Documentation/devicetree/bindings/iommu/arm,smmu.txt +5 -0

index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.

@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate address but
+                  translate attributes. The StreamIDs list here can not be

Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71

Quoted 10 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt +10 -0

+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-back
+                  2, non-cacheable, Normal outer and inner non-cacheable
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass mode.
+                  If omit, vmid=255 is default. If bypass and map mode can
+                  share a same S2CR, config vmid=0.

Quoted 4 lines

Will
.

OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt

The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.

Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c

A big issue should be discussed now. When a master use bypass mode to access memory, this means it can access all non-secure memory. So, if a master can be operated in user mode, that means a user process can access any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine output priviledge attribute. But if use bypass mode and output attribute is Unprivileged, specification have no mention about where to decide memory access is permitted or not. Do you think is a spec design bug or Implemention defined, or find some description anywhere.

Varun Sethi

2014-06-17 09:11 UTC #6 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 83 lines

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of leizhen
Sent: Tuesday, June 17, 2014 1:20 PM
To: Will Deacon
Cc: huxinwei at huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel
Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of Hisilicon SMMU private binding
Quoted 21 lines · Will Deacon
Quoted 20 lines · Zhen Lei
This patch adds a description of private properties for the Hisilicon System MMU architecture.
Signed-off-by Zhen Lei <[email protected]>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16
++++++++++++++++
 1 file changed, 16 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 20
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +0 -0
                   depending on the particular implementation and/or
the
Quoted 8 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 60
Quoted 8 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
                   version of the architecture implemented.
@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate
address but
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 61
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  translate attributes. The StreamIDs list here can
not be
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-
back
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 65
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  2, non-cacheable, Normal outer and inner non-
cacheable
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +4 -0
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass
mode.
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 70
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  If omit, vmid=255 is default. If bypass and map
mode can
Quoted 11 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  share a same S2CR, config vmid=0.
These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters?
Will
.
OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.
Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c

Quoted 9 lines

A big issue should be discussed now. When a master use bypass mode to access memory, this means it can access all non-secure memory. So, if a master can be operated in user mode, that means a user process can access any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine output priviledge attribute. But if use bypass mode and output attribute is Unprivileged, specification have no mention about where to decide memory access is permitted or not. Do you think is a spec design bug or Implemention defined, or find some description anywhere.

You should be using VFIO framework for user space I/O. VFIO uses the iommu_api to setup IOMMU for user space access. This allows you to restrict user space I/O access.

-Varun

Will Deacon

2014-06-17 09:33 UTC #7 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

In reply to leizhen

Quoted 46 lines · leizhen

Quoted 45 lines · Will Deacon

Quoted 28 lines · Zhen Lei

This patch adds a description of private properties for the Hisilicon System MMU architecture.

Signed-off-by Zhen Lei <[email protected]>

---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Documentation/devicetree/bindings/iommu/arm,smmu.txt +5 -0

index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.

@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate address but
+                  translate attributes. The StreamIDs list here can not be

Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71

Quoted 10 lines

Documentation/devicetree/bindings/iommu/arm,smmu.txt +10 -0

+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-back
+                  2, non-cacheable, Normal outer and inner non-cacheable
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass mode.
+                  If omit, vmid=255 is default. If bypass and map mode can
+                  share a same S2CR, config vmid=0.

Quoted 6 lines

Quoted 4 lines
Will
.
OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt

No, don't do that. Delete the properties instead. I'm not a device-tree expert, but these don't feel like things we should be describing there at all.

Quoted 6 lines

The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.

You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in Linux?

Quoted 2 lines

Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c

I don't want to see hisi-smmu.c at all. You need to make your driver fit into the code we already have.

Do you have a specification available describing the hardware you have created?

Quoted 8 lines

A big issue should be discussed now. When a master use bypass mode to access memory, this means it can access all non-secure memory. So, if a master can be operated in user mode, that means a user process can access any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine output priviledge attribute. But if use bypass mode and output attribute is Unprivileged, specification have no mention about where to decide memory access is permitted or not. Do you think is a spec design bug or Implemention defined, or find some description anywhere.

There are various overrides for the privilege controls but, as Varun said, you should be using VFIO for userspace device access.

Will

leizhen

author

2014-06-18 01:28 UTC #8 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 65 lines · Will Deacon

Quoted 46 lines · leizhen
Quoted 45 lines · Will Deacon
Quoted 28 lines · Zhen Lei
This patch adds a description of private properties for the Hisilicon System MMU architecture.
Signed-off-by Zhen Lei <[email protected]>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu.txt +5 -0
index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
                   depending on the particular implementation and/or the
                   version of the architecture implemented.
@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate address but
+                  translate attributes. The StreamIDs list here can not be
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71
Quoted 10 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +10 -0
+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner write-back
+                  2, non-cacheable, Normal outer and inner non-cacheable
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for bypass mode.
+                  If omit, vmid=255 is default. If bypass and map mode can
+                  share a same S2CR, config vmid=0.
These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters?
Quoted 6 lines
Quoted 4 lines
Will
.
OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt
No, don't do that. Delete the properties instead. I'm not a device-tree expert, but these don't feel like things we should be describing there at all.
Quoted 6 lines
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.

Quoted 4 lines

You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in Linux?

The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first.

Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. Add a common API may meet more difficulty.

If I have time, I will try to do it.

Quoted 9 lines

Quoted 2 lines
Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c
I don't want to see hisi-smmu.c at all. You need to make your driver fit into the code we already have.
Do you have a specification available describing the hardware you have created?

Although I have the Hisilicon SMMU specification, but it was written in Chinese. I have told this to hardware engineers.

OK. I will try my best to merge two drivers into one file. Maybe need to use many #if#else.

Quoted 16 lines

Quoted 8 lines
A big issue should be discussed now. When a master use bypass mode to access memory, this means it can access all non-secure memory. So, if a master can be operated in user mode, that means a user process can access any kernel memory. In ARM smmu-v2 specification, SMMU_S2CRn.PRIVCFG is used for determine output priviledge attribute. But if use bypass mode and output attribute is Unprivileged, specification have no mention about where to decide memory access is permitted or not. Do you think is a spec design bug or Implemention defined, or find some description anywhere.
There are various overrides for the privilege controls but, as Varun said, you should be using VFIO for userspace device access.
Will
.

Varun Sethi

2014-06-18 12:03 UTC #9 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Hi Will/Zhen,

Quoted 95 lines

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of leizhen
Sent: Wednesday, June 18, 2014 6:59 AM
To: Will Deacon
Cc: huxinwei at huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel
Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of Hisilicon SMMU private binding
Quoted 23 lines · Will Deacon
Quoted 22 lines · leizhen
Quoted 21 lines · Will Deacon
Quoted 20 lines · Zhen Lei
This patch adds a description of private properties for the Hisilicon System MMU architecture.
Signed-off-by Zhen Lei <[email protected]>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16
++++++++++++++++
 1 file changed, 16 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 20
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +0 -0
                   depending on the particular implementation and/or
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 55
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +0 -0
                   version of the architecture implemented.
@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used
during
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 5 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate
address but
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 61
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  translate attributes. The StreamIDs list here can
not be
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner
write-back
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 65
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  2, non-cacheable, Normal outer and inner non-
cacheable
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +4 -0
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for
bypass mode.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 70
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  If omit, vmid=255 is default. If bypass and map
mode can
Quoted 25 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  share a same S2CR, config vmid=0.
These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters?
Quoted 6 lines
Quoted 4 lines
Will
.
OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt
No, don't do that. Delete the properties instead. I'm not a device-tree expert, but these don't feel like things we should be describing there at all.
Quoted 6 lines
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.
Quoted 3 lines
You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in
Linux?
Quoted 1 line
The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first.

Quoted 6 lines

Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others.
Add a common API may meet more difficulty.
If I have time, I will try to do it.

[Sethi Varun-B16395] We can introduce SMMU specific attributes (to use with iommu set attribute API call) for setting memory cacheability attributes. These could be used for setting the MAIR per context bank. Actually, we would require this for out platform. However, I am not sure how memory attribute translation would work if S2CR is set for bypass?

-Varun

leizhen

author

2014-06-18 12:34 UTC #10 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 103 lines · Varun Sethi

Hi Will/Zhen,
Quoted 95 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of leizhen
Sent: Wednesday, June 18, 2014 6:59 AM
To: Will Deacon
Cc: huxinwei at huawei.com; Catalin Marinas; Zefan Li; linux-arm-kernel
Subject: Re: [PATCH RFC v2 3/3] documentation/iommu: Add description of Hisilicon SMMU private binding
Quoted 23 lines · Will Deacon
Quoted 22 lines · leizhen
Quoted 21 lines · Will Deacon
Quoted 20 lines · Zhen Lei
This patch adds a description of private properties for the Hisilicon System MMU architecture.
Signed-off-by Zhen Lei <[email protected]>
---
 Documentation/devicetree/bindings/iommu/arm,smmu.txt | 16
++++++++++++++++
 1 file changed, 16 insertions(+)
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
index f284b99..75b1351 100644
@@ -15,6 +15,7 @@ conditions.
                         "arm,smmu-v2"
                         "arm,mmu-400"
                         "arm,mmu-500"
+                        "hisilicon,smmu-v1"
 
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 20
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +0 -0
                   depending on the particular implementation and/or
the
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 55
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +0 -0
                   version of the architecture implemented.
@@ -54,6 +55,21 @@ conditions.
                   aliases of secure registers have to be used
during
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 5 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
                   SMMU configuration.
 
+** Hisilicon SMMU private properties:
+
+- smmu-force-memtype : A list of StreamIDs which not translate
address but
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 61
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  translate attributes. The StreamIDs list here can
not be
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 3 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +3 -0
+                  used for map(translation) mode again.
+                  StreamID first, then the type list below:
+                  1, cahceable, WBRAWA, Normal outer and inner
write-back
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 65
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  2, non-cacheable, Normal outer and inner non-
cacheable
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions.
Quoted 4 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt +4 -0
+                  3, device, nGnRE
+                  others, bypass
+
+- smmu-bypass-vmid   : Specify which context bank is used for
bypass mode.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 70
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  If omit, vmid=255 is default. If bypass and map
mode can
Quoted 25 lines
Quoted 7 lines
Quoted 7 lines
Documentation/devicetree/bindings/iommu/arm,smmu.txt conditions. · line 71
Quoted 1 line
Documentation/devicetree/bindings/iommu/arm,smmu.txt +1 -0
+                  share a same S2CR, config vmid=0.
These don't feel like they belong in the device-tree to me. Is the list of StreamIDs described in smmu-force-memtype a property of the hardware configuration, or a software policy to allow you to generate cacheable traffic for particular masters?
Quoted 6 lines
Quoted 4 lines
Will
.
OK, I will put these description into a sperate file, hisilicon,smmu.txt and mark a reference to arm,smmu.txt
No, don't do that. Delete the properties instead. I'm not a device-tree expert, but these don't feel like things we should be describing there at all.
Quoted 6 lines
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.
Quoted 3 lines
You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in
Linux?
Quoted 1 line
The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first.
Quoted 6 lines
Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others.
Add a common API may meet more difficulty.
If I have time, I will try to do it.

Quoted 8 lines

[Sethi Varun-B16395] We can introduce SMMU specific attributes (to use with iommu set attribute API call) for setting memory cacheability attributes. These could be used for setting the MAIR per context bank. Actually, we would require this for out platform. However, I am not sure how memory attribute translation would work if S2CR is set for bypass?
-Varun
.

The ARM SMMU v2 specification describes that:
If the transaction is successfully matched in the Stream mapping table, SMMU_S2CRn determines the initial context. If SMMU_S2CRn specifies Bypass mode:
? The address is not translated, meaning that the output address is identical to the input address. ? Output attributes are a function of the input attributes and the SMMU_S2CRn fields.

Will Deacon

2014-06-18 13:32 UTC #11 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

In reply to leizhen

Quoted 14 lines · leizhen

Quoted 8 lines · Will Deacon
Quoted 6 lines · leizhen
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.
Quoted 4 lines
You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in Linux?

Quoted 8 lines

The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first.
Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. Add a common API may meet more difficulty.
If I have time, I will try to do it.

Ok, so we're agreeing to drop those properties for now?

Quoted 6 lines

Quoted 6 lines
Quoted 2 lines
Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c
I don't want to see hisi-smmu.c at all. You need to make your driver fit into the code we already have.

Quoted 6 lines

Quoted 3 lines
Do you have a specification available describing the hardware you have created?
Although I have the Hisilicon SMMU specification, but it was written in Chinese. I have told this to hardware engineers.

Having access to a specification I can understand would help to assess exactly what you've gone and built.

Quoted 2 lines

OK. I will try my best to merge two drivers into one file. Maybe need to use many #if#else.

The fact of the matter is that you've got a device that isn't architecturally compliant. That leaves me in a fairly undesirable position with a small set of options:

(1) We can duplicate lots of the code we already have and you end up with a separate driver. This is obviously bad because of the code duplication, associated maintenance headaches, driver divergence, etc. It also brings into question the point of having a driver written to the architecture when the hardware has gone off and done something different.

(2) We try to fit your SMMU into the existing driver. I'd like to see what this looks like, but if it's as much #ifdeffery as you suggest, then that's also bad for many of the same reasons as above.

(3) We don't support your device in the Linux kernel. We could just treat your SMMU as a broken ARM SMMU implementation and not support it. If this was a CPU instead of an SMMU this would unquestionably be the correct approach.

Unless you can come up with something compelling for point (2), I'm going to err on the side of (3). So, please, *please*, don't do a rushed job of merging the drivers. You need to convince me why I should bother supporting this device in my driver, which means using clean abstractions and extending the code we already have. If this isn't possible, then perhaps you'll consider following the architecture next time around.

Will

leizhen

author

2014-06-19 01:58 UTC #12 lore raw

RFC PATCH v2 3/3 documentation/iommu: Add description of Hisilicon SMMU private binding

Quoted 71 lines · Will Deacon

Quoted 14 lines · leizhen
Quoted 8 lines · Will Deacon
Quoted 6 lines · leizhen
The latter case. Some masters driver want use cacheable(WB) attribute to access memory, but the masters can not bring cacheable attribute. So, can not use bypass(or transaction) mode. In fact, the master driver can use iommu_map to create map and specify IOMMU_CACHE. But maybe the driver does not want to map, if the memory access is very dynamically, frequently map and unmap will decrease performance.
Quoted 4 lines
You seem to be highlighting a perceived deficiency in the IOMMU API which you're attempting to work-around with new device-tree properties. Instead, why not propose an extension to the IOMMU API in Linux?
Quoted 8 lines
The private properties or new IOMMU APIs, just in order to optimize performance or simplify master driver code, I will consider it later. I think it's good to support base functions first.
Sorry, I just only known ARM SMMU and Hisilicon SMMU, not familiar with others. Add a common API may meet more difficulty.
If I have time, I will try to do it.
Ok, so we're agreeing to drop those properties for now?
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Actually, I think smmu-force-memtype maybe suitable for arm-smmu too. But now, if nobody declear need it, I will just implement it in hisi-smmu.c
I don't want to see hisi-smmu.c at all. You need to make your driver fit into the code we already have.
Quoted 6 lines
Quoted 3 lines
Do you have a specification available describing the hardware you have created?
Although I have the Hisilicon SMMU specification, but it was written in Chinese. I have told this to hardware engineers.
Having access to a specification I can understand would help to assess exactly what you've gone and built.
Quoted 2 lines
OK. I will try my best to merge two drivers into one file. Maybe need to use many #if#else.
The fact of the matter is that you've got a device that isn't architecturally compliant. That leaves me in a fairly undesirable position with a small set of options:
(1) We can duplicate lots of the code we already have and you end up with a separate driver. This is obviously bad because of the code duplication, associated maintenance headaches, driver divergence, etc. It also brings into question the point of having a driver written to the architecture when the hardware has gone off and done something different.
(2) We try to fit your SMMU into the existing driver. I'd like to see what this looks like, but if it's as much #ifdeffery as you suggest, then that's also bad for many of the same reasons as above.
(3) We don't support your device in the Linux kernel. We could just treat your SMMU as a broken ARM SMMU implementation and not support it. If this was a CPU instead of an SMMU this would unquestionably be the correct approach.
Unless you can come up with something compelling for point (2), I'm going to err on the side of (3). So, please, *please*, don't do a rushed job of merging the drivers. You need to convince me why I should bother supporting this device in my driver, which means using clean abstractions and extending the code we already have. If this isn't possible, then perhaps you'll consider following the architecture next time around.

Quoted 4 lines

Will
.

Yeah, I konw. I also a software engineer. A victim of this SMMU.

Will Deacon

2014-06-16 16:37 UTC #13 lore raw

v2 cover Add support for Hisilicon SMMU architecture

In reply to Zhen Lei

Hi there,

Quoted 7 lines · Zhen Lei

Changes in v2:
- Split Hisilicon smmu implementation in a separate file, hisi-smmu.c
- Refactor arm-smmu.c. Some direct call hardware dependent functions replaced with hooks. And move common struct and marco definition into arm-smmu.h
- Merge the description of Hisilicon private properties into arm,smmu.txt

Quoted 8 lines

Hisilicon smmu is similar to arm-smmu, some code can be direct reused. For
example: map and unmap, device tree configuration, and the software framework. I found that, abstract about 11 hardware dependent functions in arm-smmu.c is enough . Abstract means use hooks to replace the direct call of functions. Now, if need to support Hisilicon SMMU or others arm smmu variants, just selective rewrite these hooks. And I add a dt_cfg_probe hook, to allow each variant parse their hardware special configuration. Finally, flush_pgtable is a special case, hardware independent but maybe referenced. So, total 13 hooks.

The fundamental question here is whether or not your SMMU implementation is intended to follow the ARM SMMU architecture specification. Given the changes you've highlighted, it clearly doesn't comply, so then it comes down to how much code can actually be re-used between arm-smmu.c and hisi-smmu.c.

With your current split, I can still see plenty of duplication between the two files (e.g. bits in the SCTLR register). I also recognise a fair number of lines in hisi-smmu.c that I wrote originally.

So, is this supposed to be architecturally compliant with the ARM SMMU spec or is it something completely independent?

Will

leizhen

author

2014-06-17 06:32 UTC #14 lore raw

v2 cover Add support for Hisilicon SMMU architecture

Quoted 28 lines · Will Deacon

Hi there,
Quoted 7 lines · Zhen Lei
Changes in v2:
- Split Hisilicon smmu implementation in a separate file, hisi-smmu.c
- Refactor arm-smmu.c. Some direct call hardware dependent functions replaced with hooks. And move common struct and marco definition into arm-smmu.h
- Merge the description of Hisilicon private properties into arm,smmu.txt
Quoted 8 lines
Hisilicon smmu is similar to arm-smmu, some code can be direct reused. For
example: map and unmap, device tree configuration, and the software framework. I found that, abstract about 11 hardware dependent functions in arm-smmu.c is enough . Abstract means use hooks to replace the direct call of functions. Now, if need to support Hisilicon SMMU or others arm smmu variants, just selective rewrite these hooks. And I add a dt_cfg_probe hook, to allow each variant parse their hardware special configuration. Finally, flush_pgtable is a special case, hardware independent but maybe referenced. So, total 13 hooks.
The fundamental question here is whether or not your SMMU implementation is intended to follow the ARM SMMU architecture specification. Given the changes you've highlighted, it clearly doesn't comply, so then it comes down to how much code can actually be re-used between arm-smmu.c and hisi-smmu.c.
With your current split, I can still see plenty of duplication between the two files (e.g. bits in the SCTLR register). I also recognise a fair number of lines in hisi-smmu.c that I wrote originally.

Quoted 7 lines

So, is this supposed to be architecturally compliant with the ARM SMMU spec or is it something completely independent?
Will
.

Yeah, it doesn't comply, and finally will not comply too. So, I said before:
"similar to" and "arm-smmu variant" maybe inappropriate. It looks like a new smmu hardware implementation.

I also want to reuse your code as much as possible. But in order to keep your code flow clearly, I try to make as few changes as possible. I think make differentiate granule too small is not well. Because all of our codes maybe changed in future, and may any other smmu variants to reuse arm-smmu.c.

Especially, SCTLR register, the hardware dependent feature. I think not suitable for reuse. A bit field differentiation may need a marco to seperate, make the code ugly.

Will Deacon

2014-06-17 09:27 UTC #15 lore raw

v2 cover Add support for Hisilicon SMMU architecture

Quoted 30 lines · leizhen

Quoted 29 lines · Will Deacon
Quoted 7 lines · Zhen Lei
Changes in v2:
- Split Hisilicon smmu implementation in a separate file, hisi-smmu.c
- Refactor arm-smmu.c. Some direct call hardware dependent functions replaced with hooks. And move common struct and marco definition into arm-smmu.h
- Merge the description of Hisilicon private properties into arm,smmu.txt
Quoted 8 lines
Hisilicon smmu is similar to arm-smmu, some code can be direct reused. For
example: map and unmap, device tree configuration, and the software framework. I found that, abstract about 11 hardware dependent functions in arm-smmu.c is enough . Abstract means use hooks to replace the direct call of functions. Now, if need to support Hisilicon SMMU or others arm smmu variants, just selective rewrite these hooks. And I add a dt_cfg_probe hook, to allow each variant parse their hardware special configuration. Finally, flush_pgtable is a special case, hardware independent but maybe referenced. So, total 13 hooks.
The fundamental question here is whether or not your SMMU implementation is intended to follow the ARM SMMU architecture specification. Given the changes you've highlighted, it clearly doesn't comply, so then it comes down to how much code can actually be re-used between arm-smmu.c and hisi-smmu.c.
With your current split, I can still see plenty of duplication between the two files (e.g. bits in the SCTLR register). I also recognise a fair number of lines in hisi-smmu.c that I wrote originally.
So, is this supposed to be architecturally compliant with the ARM SMMU spec or is it something completely independent?

Quoted 7 lines

Quoted 4 lines
Will
.
Yeah, it doesn't comply, and finally will not comply too. So, I said before:
"similar to" and "arm-smmu variant" maybe inappropriate. It looks like a new smmu hardware implementation.

Well, apart from the parts that are identical, sure.

Quoted 8 lines

I also want to reuse your code as much as possible. But in order to keep your code flow clearly, I try to make as few changes as possible. I think make differentiate granule too small is not well. Because all of our codes maybe changed in future, and may any other smmu variants to reuse arm-smmu.c.
Especially, SCTLR register, the hardware dependent feature. I think not suitable for reuse. A bit field differentiation may need a marco to seperate, make the code ugly.

I disagree. You have a register with a same name, with the same bits, in the same positions. That's hopefully not just luck, so the code should be shared between the drivers. I'm not at all happy with simply duplicating what we have and bodging it for your purposes -- that totally defeats the point of having an architecture, even if you're not following it entirely.

Will