Add Tegra241 (Grace) CMDQV Support (part 1/2)

The driver currently calls arm_smmu_get_cmdq() helper in different places, although they are all called from the arm_smmu_cmdq_issue_cmdlist().

Allow to pass in the cmdq pointer, instead of calling arm_smmu_get_cmdq() every time.

This will also help CMDQV extension in NVIDIA Tegra241 SoC, as its driver will maintain its own cmdq pointers, then need to redirect arm_smmu->cmdq to one of its vcmdqs upon seeing a supported command.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

Quoted 8 lines · Nicolin Chen

The CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 ++++++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

This seems fine, other than the misplaced hunk

Reviewed-by Jason Gunthorpe <[email protected]>

But it might be tidier like the below. There is already a function that is called to build the sync that has the q, just build it directly there and avoid going through the ent?

static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -1

-					 struct arm_smmu_queue *q, u32 prod)
+					 struct arm_smmu_queue *q, u32 prod,
+					 bool msi)
 {

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

+	if (!msi || !(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
+		return;
+	}

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -3

@@ -402,9 +395,6 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 	u64 cmd[CMDQ_ENT_DWORDS];
 	u32 cons = readl_relaxed(q->cons_reg);
 	u32 idx = FIELD_GET(CMDQ_CONS_ERR, cons);
-	struct arm_smmu_cmdq_ent cmd_sync = {
-		.opcode = CMDQ_OP_CMD_SYNC,
-	};
 
 	dev_err(smmu->dev, "CMDQ error (cons 0x%08x): %s\n", cons,
 		idx < ARRAY_SIZE(cerror_str) ?  cerror_str[idx] : "Unknown");

@@ -437,11 +427,8 @@ void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 	for (i = 0; i < ARRAY_SIZE(cmd); ++i)

Quoted 1 line

queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);

Here is the only caller for "msi=false". Maybe we could just do:

(quoted diff) +1 -0

+	arm_smmu_cmdq_build_sync_cmd(cmd, smmu, q, cons);

So, no need of "bool msi"? It would slightly change the behavior though, a SYNC for ARM_SMMU_OPT_MSIPOLL should be still a SYNC.

Quoted 5 lines · Nicolin Chen

Quoted 1 line

queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);

Here is the only caller for "msi=false". Maybe we could just do:

(quoted diff) +1 -0

+	arm_smmu_cmdq_build_sync_cmd(cmd, smmu, q, cons);

Quoted 2 lines

So, no need of "bool msi"? It would slightly change the behavior though, a SYNC for ARM_SMMU_OPT_MSIPOLL should be still a SYNC.

I don't know, I didn't try to figure out what to stick for prod in that case. It is probably OK to convert an error entry into a MSI sync if it works out?

The CMDQV extension in NVIDIA Tegra241 SoC resues the arm_smmu_cmdq structure while the queue location isn't same as smmu->cmdq.

Add a cmdq argument to arm_smmu_cmdq_init() function and shares its define in the header for CMDQV driver to use.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 +++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

Quoted 11 lines · Nicolin Chen

The CMDQV extension in NVIDIA Tegra241 SoC resues the arm_smmu_cmdq structure while the queue location isn't same as smmu->cmdq.

Add a cmdq argument to arm_smmu_cmdq_init() function and shares its define in the header for CMDQV driver to use.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 +++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

I would squash this patch and the next together:

iommu/arm-smm-v3: Make symbols public

The symbols arm_smmu_cmdq_init() and __arm_smmu_cmdq_skip_err() need to be used by the tegra241-cmdqv.c compilation unit in the next patch. Remove the static and put prototypes in the header.

But the code is fine

Reviewed-by Jason Gunthorpe <[email protected]>

Allow __arm_smmu_cmdq_skip_err function to be reused by NVIDIA Tegra241 CMDQV unit since it will use the same data structure for q. And include the CMDQ_QUIRK_SYNC_CS_NONE_ONLY quirk when inserting a CMD_SYNC.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 +++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2

index 538850059bdd..5111859347d5 100644
@@ -379,8 +379,8 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 	arm_smmu_cmdq_build_cmd(cmd, &ent);
 }
 
-static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
-				     struct arm_smmu_queue *q)
+void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
+			      struct arm_smmu_queue *q)
 {
 	static const char * const cerror_str[] = {
 		[CMDQ_ERR_CERROR_NONE_IDX]	= "No error",

Quoted 29 lines · Nicolin Chen

Allow __arm_smmu_cmdq_skip_err function to be reused by NVIDIA Tegra241 CMDQV unit since it will use the same data structure for q. And include the CMDQ_QUIRK_SYNC_CS_NONE_ONLY quirk when inserting a CMD_SYNC.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 +++++--
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2

index 538850059bdd..5111859347d5 100644
@@ -379,8 +379,8 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
 	arm_smmu_cmdq_build_cmd(cmd, &ent);
 }
 
-static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
-				     struct arm_smmu_queue *q)
+void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
+			      struct arm_smmu_queue *q)
 {
 	static const char * const cerror_str[] = {
 		[CMDQ_ERR_CERROR_NONE_IDX]	= "No error",

@@ -428,6 +428,9 @@ static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
 	for (i = 0; i < ARRAY_SIZE(cmd); ++i)
 		dev_err(smmu->dev, "\t0x%016llx\n", (unsigned long long)cmd[i]);
 

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, · line 432

Quoted 2 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -0

+	if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY)
+		cmd_sync.sync.cs_none = true;

This hunk should be in "iommu/arm-smmu-v3: Add CS_NONE quirk" ?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -2

@@ -3105,12 +3108,10 @@ static struct iommu_ops arm_smmu_ops = {
 };
 
 /* Probing and initialisation functions */
-static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
-				   struct arm_smmu_queue *q,

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +43 -0

+#define vintf_warn(fmt, ...) \
+	dev_warn(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_err(fmt, ...) \
+	dev_err(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_info(fmt, ...) \
+	dev_info(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_dbg(fmt, ...) \
+	dev_dbg(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+
+#define vcmdq_warn(fmt, ...)                                                   \
+	({                                                                     \
+		struct tegra241_vintf *vintf = vcmdq->vintf;                   \
+		if (vintf)                                                     \
+			vintf_warn("VCMDQ%u/LVCMDQ%u: " fmt,                   \
+				   vcmdq->idx, vcmdq->lidx,                    \
+				   ##__VA_ARGS__);                             \
+		else                                                           \
+			dev_warn(vcmdq->cmdqv->dev, "VCMDQ%u: " fmt,           \
+				 vcmdq->idx, ##__VA_ARGS__);                   \
+	})
+#define vcmdq_err(fmt, ...)                                                    \
+	({                                                                     \
+		struct tegra241_vintf *vintf = vcmdq->vintf;                   \
+		if (vintf)                                                     \
+			vintf_err("VCMDQ%u/LVCMDQ%u: " fmt,                    \
+				  vcmdq->idx, vcmdq->lidx,                     \
+				  ##__VA_ARGS__);                              \
+		else                                                           \
+			dev_err(vcmdq->cmdqv->dev, "VCMDQ%u: " fmt,            \
+				vcmdq->idx, ##__VA_ARGS__);                    \
+	})
+#define vcmdq_info(fmt, ...)                                                   \
+	({                                                                     \
+		struct tegra241_vintf *vintf = vcmdq->vintf;                   \
+		if (vintf)                                                     \
+			vintf_info("VCMDQ%u/LVCMDQ%u: " fmt,                   \
+				   vcmdq->idx, vcmdq->lidx,                    \
+				   ##__VA_ARGS__);                             \
+		else                                                           \
+			dev_info(vcmdq->cmdqv->dev, "VCMDQ%u: " fmt,           \
+				 vcmdq->idx, ##__VA_ARGS__);                   \
+	})
+#define vcmdq_dbg(fmt, ...)                                                    \

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +9 -0

+
+/* Configuring and polling helpers */
+#define tegra241_cmdqv_write_config(_owner, _OWNER, _regval)                   \
+	({                                                                     \
+		bool _en = (_regval) & _OWNER##_EN;                            \
+		u32 _status;                                                   \
+		int _ret;                                                      \
+		writel((_regval), _owner->base + TEGRA241_##_OWNER##_CONFIG);  \
+		_ret = readl_poll_timeout(                                     \

+/**
+ * struct tegra241_cmdqv - CMDQ-V for SMMUv3
+ * @smmu: SMMUv3 pointer
+ * @dev: Device pointer

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +39 -0

+static void tegra241_cmdqv_handle_vintf0_error(struct tegra241_cmdqv *cmdqv)
+{
+	struct tegra241_vintf *vintf = cmdqv->vintfs[0];
+	int i;
+
+	/* Cache status to bypass VCMDQs until error is recovered */
+	atomic_set(&vintf->status, vintf_readl(STATUS));
+
+	for (i = 0; i < 4; i++) {
+		u32 lvcmdq_err_map = vintf_readl_relaxed(CMDQ_ERR_MAP(i));
+
+		while (lvcmdq_err_map) {
+			int lidx = ffs(lvcmdq_err_map) - 1;
+			struct tegra241_vcmdq *vcmdq = vintf->vcmdqs[lidx];
+			u32 gerrorn, gerror;
+
+			lvcmdq_err_map &= ~BIT(lidx);
+
+			__arm_smmu_cmdq_skip_err(cmdqv->smmu, &vcmdq->cmdq.q);
+
+			gerrorn = vcmdq_page0_readl_relaxed(GERRORN);
+			gerror = vcmdq_page0_readl_relaxed(GERROR);
+
+			vcmdq_page0_writel(gerror, GERRORN);
+		}
+	}
+
+	/* Now error status should be clean, cache it again */
+	atomic_set(&vintf->status, vintf_readl(STATUS));
+}
+
+static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)
+{
+	struct tegra241_cmdqv *cmdqv = (struct tegra241_cmdqv *)devid;
+	u32 vintf_errs[2];
+	u32 vcmdq_errs[4];
+
+	vintf_errs[0] = cmdqv_readl_relaxed(VINTF_ERR_MAP);
+	vintf_errs[1] = cmdqv_readl_relaxed(VINTF_ERR_MAP + 0x4);

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +21 -0

+	}
+	vcmdq_page0_writel_relaxed(0, PROD);
+	vcmdq_page0_writel_relaxed(0, CONS);
+	vcmdq_page1_writeq_relaxed(0, BASE);
+	vcmdq_page1_writeq_relaxed(0, CONS_INDX_BASE);
+
+	gerrorn = vcmdq_page0_readl_relaxed(GERRORN);
+	gerror = vcmdq_page0_readl_relaxed(GERROR);
+	if (gerror != gerrorn) {
+		vcmdq_info("Uncleared error detected, resetting\n");
+		vcmdq_page0_writel(gerror, GERRORN);
+	}
+
+	vcmdq_dbg("deinited\n");
+}
+
+static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq)
+{
+	int ret;
+
+	/* Configure and enable the vcmdq */

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +31 -0

+static struct tegra241_vcmdq *
+tegra241_vintf_lvcmdq_alloc(struct tegra241_vintf *vintf, u16 lidx)
+{
+	struct tegra241_cmdqv *cmdqv = vintf->cmdqv;
+	struct tegra241_vcmdq *vcmdq;
+	int ret;
+
+	vcmdq = devm_kzalloc(cmdqv->dev, sizeof(*vcmdq), GFP_KERNEL);
+	if (!vcmdq)
+		return ERR_PTR(-ENOMEM);
+
+	ret = tegra241_vintf_lvcmdq_init(vintf, lidx, vcmdq);
+	if (ret)
+		goto free_vcmdq;
+
+	/* Setup struct arm_smmu_cmdq data members */
+	ret = tegra241_vcmdq_alloc_smmu_cmdq(vcmdq);
+	if (ret)
+		goto deinit_lvcmdq;
+
+	ret = tegra241_vcmdq_hw_init(vcmdq);
+	if (ret)
+		goto free_queue;
+
+	vcmdq_dbg("allocated\n");
+	return vcmdq;
+free_queue:
+	tegra241_vcmdq_free_smmu_cmdq(vcmdq);
+deinit_lvcmdq:
+	tegra241_vintf_lvcmdq_deinit(vcmdq);
+free_vcmdq:

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 103

Quoted 9 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +9 -0

+/* MMIO helpers */
+#define cmdqv_readl(reg) \
+	readl(cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_readl_relaxed(reg) \
+	readl_relaxed(cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_writel(val, reg) \
+	writel((val), cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_writel_relaxed(val, reg) \
+	writel_relaxed((val), cmdqv->base + TEGRA241_CMDQV_##reg)

Please don't hide access to a stack variable in a macro, and I'm not keen on the ##reg scheme either - it makes it much harder to search for things.

Really this all seems like alot of overkill to make a little bit of shorthand. It is not so wordy just to type it out:

readl(vintf->base + TEGRA241_VINTF_CONFIG)

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +4 -0

+/* Logging helpers */
+#define cmdqv_warn(fmt, ...) \
+	dev_warn(cmdqv->dev, "CMDQV: " fmt, ##__VA_ARGS__)
+#define cmdqv_err(fmt, ...) \

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 146

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +5 -0

+	dev_err(cmdqv->dev, "CMDQV: " fmt, ##__VA_ARGS__)
+#define cmdqv_info(fmt, ...) \
+	dev_info(cmdqv->dev, "CMDQV: " fmt, ##__VA_ARGS__)
+#define cmdqv_dbg(fmt, ...) \
+	dev_dbg(cmdqv->dev, "CMDQV: " fmt, ##__VA_ARGS__)

Really not sure these are necessary, same remark about the stack variable.

Also cmdqv->dev is the wrong thing to print, this is part of the smmu driver, it should print cmdqv->smmu->dev for consistency

Quoted 14 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +14 -0

+#define vintf_warn(fmt, ...) \
+	dev_warn(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_err(fmt, ...) \
+	dev_err(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_info(fmt, ...) \
+	dev_info(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_dbg(fmt, ...) \
+	dev_dbg(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+
+#define vcmdq_warn(fmt, ...)                                                   \
+	({                                                                     \
+		struct tegra241_vintf *vintf = vcmdq->vintf;                   \
+		if (vintf)                                                     \
+			vintf_warn("VCMDQ%u/LVCMDQ%u: " fmt,                   \

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 167

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+				   vcmdq->idx, vcmdq->lidx,                    \
+				   ##__VA_ARGS__);                             \
+		else                                                           \
+			dev_warn(vcmdq->cmdqv->dev, "VCMDQ%u: " fmt,           \
+				 vcmdq->idx, ##__VA_ARGS__);                   \
+	})

Some of these are barely used, is it worth all these macros??

Quoted 14 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +14 -0

+
+/* Configuring and polling helpers */
+#define tegra241_cmdqv_write_config(_owner, _OWNER, _regval)                   \
+	({                                                                     \
+		bool _en = (_regval) & _OWNER##_EN;                            \
+		u32 _status;                                                   \
+		int _ret;                                                      \
+		writel((_regval), _owner->base + TEGRA241_##_OWNER##_CONFIG);  \
+		_ret = readl_poll_timeout(                                     \
+			_owner->base + TEGRA241_##_OWNER##_STATUS, _status,    \
+			_en ? (_regval) & _OWNER##_ENABLED :                   \
+			      !((_regval) & _OWNER##_ENABLED),                 \
+			1, ARM_SMMU_POLL_TIMEOUT_US);                          \
+		if (_ret)                                                      \

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 219

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +5 -0

+			_owner##_err("failed to %sable, STATUS = 0x%08X\n",    \
+				     _en ? "en" : "dis", _status);             \
+		atomic_set(&_owner->status, _status);                          \
+		_ret;                                                          \
+	})

I feel like this could be an actual inline function without the macro wrapper with a little fiddling.

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 226

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +7 -0

+
+#define cmdqv_write_config(_regval) \
+	tegra241_cmdqv_write_config(cmdqv, CMDQV, _regval)
+#define vintf_write_config(_regval) \
+	tegra241_cmdqv_write_config(vintf, VINTF, _regval)
+#define vcmdq_write_config(_regval) \
+	tegra241_cmdqv_write_config(vcmdq, VCMDQ, _regval)

More hidden access to stack values

Quoted 4 lines

+/**
+ * struct tegra241_cmdqv - CMDQ-V for SMMUv3
+ * @smmu: SMMUv3 pointer
+ * @dev: Device pointer

This should probably be clarified as the device pointer to the ACPI companion device

Quoted 2 lines

(quoted diff) +2 -0

+struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
+{

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 391

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +7 -0

+	/* Use SMMU CMDQ if vintfs[0] is uninitialized */
+	if (!FIELD_GET(VINTF_ENABLED, atomic_read(&vintf->status)))
+		return &smmu->cmdq;
+
+	/* Use SMMU CMDQ if vintfs[0] has error status */
+	if (FIELD_GET(VINTF_STATUS, atomic_read(&vintf->status)))
+		return &smmu->cmdq;

Why atomic_read? The unlocked interaction with tegra241_cmdqv_handle_vintf0_error() doesn't seem especially sane IMHO

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 414

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +8 -0

+static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
+{
+	u32 gerrorn, gerror;
+
+	if (vcmdq_write_config(0)) {
+		vcmdq_err("GERRORN=0x%X\n", vcmdq_page0_readl_relaxed(GERRORN));
+		vcmdq_err("GERROR=0x%X\n", vcmdq_page0_readl_relaxed(GERROR));
+		vcmdq_err("CONS=0x%X\n", vcmdq_page0_readl_relaxed(CONS));

Less prints, include a unique message about why this is being printed..

Quoted 25 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +25 -0

+	}
+	vcmdq_page0_writel_relaxed(0, PROD);
+	vcmdq_page0_writel_relaxed(0, CONS);
+	vcmdq_page1_writeq_relaxed(0, BASE);
+	vcmdq_page1_writeq_relaxed(0, CONS_INDX_BASE);
+
+	gerrorn = vcmdq_page0_readl_relaxed(GERRORN);
+	gerror = vcmdq_page0_readl_relaxed(GERROR);
+	if (gerror != gerrorn) {
+		vcmdq_info("Uncleared error detected, resetting\n");
+		vcmdq_page0_writel(gerror, GERRORN);
+	}
+
+	vcmdq_dbg("deinited\n");
+}
+
+static int tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq)
+{
+	int ret;
+
+	/* Configure and enable the vcmdq */
+	tegra241_vcmdq_hw_deinit(vcmdq);
+
+	vcmdq_page1_writeq_relaxed(vcmdq->cmdq.q.q_base, BASE);
+

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 445

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+	ret = vcmdq_write_config(VCMDQ_EN);
+	if (ret) {
+		vcmdq_err("GERRORN=0x%X\n", vcmdq_page0_readl_relaxed(GERRORN));
+		vcmdq_err("GERROR=0x%X\n", vcmdq_page0_readl_relaxed(GERROR));
+		vcmdq_err("CONS=0x%X\n", vcmdq_page0_readl_relaxed(CONS));
+		return ret;

Same print?

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 489

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +8 -0

+static void tegra241_vcmdq_free_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
+{
+	struct tegra241_cmdqv *cmdqv = vcmdq->cmdqv;
+	struct arm_smmu_queue *q = &vcmdq->cmdq.q;
+	size_t nents = 1 << q->llq.max_n_shift;
+
+	dmam_free_coherent(cmdqv->smmu->dev, (nents * CMDQ_ENT_DWORDS) << 3,
+			   q->base, q->base_dma);

If we are calling dmam_free, do we really need devm at all?

Quoted 1 line

+tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id)

id is a u32.

It might be clearer to just pass in the struct
acpi_iort_node *?

Quoted 9 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +9 -0

+{
+	struct tegra241_cmdqv *cmdqv;
+
+	cmdqv = tegra241_cmdqv_find_resource(smmu, id);
+	if (!cmdqv)
+		return NULL;
+
+	if (tegra241_cmdqv_probe(cmdqv)) {
+		if (cmdqv->irq > 0)

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 811

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +4 -0

+			devm_free_irq(smmu->dev, cmdqv->irq, cmdqv);
+		devm_iounmap(smmu->dev, cmdqv->base);
+		devm_kfree(smmu->dev, cmdqv);
+		return NULL;

Oh. Please don't use devm at all in this code then, it is not attached to a probed driver with the proper scope, devm isn't going to work in sensible way.

(Removing chunks that I simply ack)

Quoted 15 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 103

Quoted 9 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +9 -0

+/* MMIO helpers */
+#define cmdqv_readl(reg) \
+	readl(cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_readl_relaxed(reg) \
+	readl_relaxed(cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_writel(val, reg) \
+	writel((val), cmdqv->base + TEGRA241_CMDQV_##reg)
+#define cmdqv_writel_relaxed(val, reg) \
+	writel_relaxed((val), cmdqv->base + TEGRA241_CMDQV_##reg)

Please don't hide access to a stack variable in a macro, and I'm not keen on the ##reg scheme either - it makes it much harder to search for things.

I can pass in cmdqv/vintf/vcmdq pointers, if it would be better.

Quoted 4 lines

Really this all seems like alot of overkill to make a little bit of shorthand. It is not so wordy just to type it out:

readl(vintf->base + TEGRA241_VINTF_CONFIG)

vintf_readl(vintf, CONFIG) is much shorter. Doing so reduced the line breaks at quite a lot places, so overall the driver looks a lot cleaner to me.

I can probably change these logging helpers to inline functions.

Quoted 14 lines

Quoted 14 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +14 -0

+#define vintf_warn(fmt, ...) \
+	dev_warn(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_err(fmt, ...) \
+	dev_err(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_info(fmt, ...) \
+	dev_info(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+#define vintf_dbg(fmt, ...) \
+	dev_dbg(vintf->cmdqv->dev, "VINTF%u: " fmt, vintf->idx, ##__VA_ARGS__)
+
+#define vcmdq_warn(fmt, ...)                                                   \
+	({                                                                     \
+		struct tegra241_vintf *vintf = vcmdq->vintf;                   \
+		if (vintf)                                                     \
+			vintf_warn("VCMDQ%u/LVCMDQ%u: " fmt,                   \

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 167

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+				   vcmdq->idx, vcmdq->lidx,                    \
+				   ##__VA_ARGS__);                             \
+		else                                                           \
+			dev_warn(vcmdq->cmdqv->dev, "VCMDQ%u: " fmt,           \
+				 vcmdq->idx, ##__VA_ARGS__);                   \
+	})

Some of these are barely used, is it worth all these macros??

Only vcmdq_warn isn't called. But I think it would be useful. I could also find a place to call it, if that's a must.

Quoted 14 lines

Quoted 14 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +14 -0

+
+/* Configuring and polling helpers */
+#define tegra241_cmdqv_write_config(_owner, _OWNER, _regval)                   \
+	({                                                                     \
+		bool _en = (_regval) & _OWNER##_EN;                            \
+		u32 _status;                                                   \
+		int _ret;                                                      \
+		writel((_regval), _owner->base + TEGRA241_##_OWNER##_CONFIG);  \
+		_ret = readl_poll_timeout(                                     \
+			_owner->base + TEGRA241_##_OWNER##_STATUS, _status,    \
+			_en ? (_regval) & _OWNER##_ENABLED :                   \
+			      !((_regval) & _OWNER##_ENABLED),                 \
+			1, ARM_SMMU_POLL_TIMEOUT_US);                          \
+		if (_ret)                                                      \

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 219

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +5 -0

+			_owner##_err("failed to %sable, STATUS = 0x%08X\n",    \
+				     _en ? "en" : "dis", _status);             \
+		atomic_set(&_owner->status, _status);                          \
+		_ret;                                                          \
+	})

I feel like this could be an actual inline function without the macro wrapper with a little fiddling.

It would be unrolled to three mostly identical inline functions:
tegra241_cmdqv_write_config(cmdqv, regval) tegra241_vintf_write_config(vintf, regval) tegra241_vcmdq_write_config(vcmdq, regval)

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 226

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+#define cmdqv_write_config(_regval) \
+	tegra241_cmdqv_write_config(cmdqv, CMDQV, _regval)
+#define vintf_write_config(_regval) \
+	tegra241_cmdqv_write_config(vintf, VINTF, _regval)
+#define vcmdq_write_config(_regval) \
+	tegra241_cmdqv_write_config(vcmdq, VCMDQ, _regval)

More hidden access to stack values

Btw, any reason for forbidding this practice? It will break the build if something goes wrong, which seems to be pretty easy to catch.

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 391

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +7 -0

+	/* Use SMMU CMDQ if vintfs[0] is uninitialized */
+	if (!FIELD_GET(VINTF_ENABLED, atomic_read(&vintf->status)))
+		return &smmu->cmdq;
+
+	/* Use SMMU CMDQ if vintfs[0] has error status */
+	if (FIELD_GET(VINTF_STATUS, atomic_read(&vintf->status)))
+		return &smmu->cmdq;

Why atomic_read? The unlocked interaction with tegra241_cmdqv_handle_vintf0_error() doesn't seem especially sane IMHO

Race between this get_cmdq() and the isr. Any alternative practice?

Quoted 11 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 414

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +8 -0

+static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq)
+{
+	u32 gerrorn, gerror;
+
+	if (vcmdq_write_config(0)) {
+		vcmdq_err("GERRORN=0x%X\n", vcmdq_page0_readl_relaxed(GERRORN));
+		vcmdq_err("GERROR=0x%X\n", vcmdq_page0_readl_relaxed(GERROR));
+		vcmdq_err("CONS=0x%X\n", vcmdq_page0_readl_relaxed(CONS));

Less prints, include a unique message about why this is being printed..

Something must be wrong if disabling VCMDQ fails, so the prints of error register values would be helpful. And "failed to disable" is already printed by the vcmdq_write_config() call. I can merge them into one vcmdq_err call though.

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 489

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +8 -0

+static void tegra241_vcmdq_free_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
+{
+	struct tegra241_cmdqv *cmdqv = vcmdq->cmdqv;
+	struct arm_smmu_queue *q = &vcmdq->cmdq.q;
+	size_t nents = 1 << q->llq.max_n_shift;
+
+	dmam_free_coherent(cmdqv->smmu->dev, (nents * CMDQ_ENT_DWORDS) << 3,
+			   q->base, q->base_dma);

If we are calling dmam_free, do we really need devm at all?

Hmm. This is a part of SMMU's probe/device_reset().

Quoted 8 lines

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +8 -0

+	struct tegra241_cmdqv *cmdqv;
+
+	cmdqv = tegra241_cmdqv_find_resource(smmu, id);
+	if (!cmdqv)
+		return NULL;
+
+	if (tegra241_cmdqv_probe(cmdqv)) {
+		if (cmdqv->irq > 0)

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 811

Quoted 4 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +4 -0

+			devm_free_irq(smmu->dev, cmdqv->irq, cmdqv);
+		devm_iounmap(smmu->dev, cmdqv->base);
+		devm_kfree(smmu->dev, cmdqv);
+		return NULL;

Oh. Please don't use devm at all in this code then, it is not attached to a probed driver with the proper scope, devm isn't going to work in sensible way.

Mind elaborating "it is not"? This function is called by arm_smmu_device_acpi_probe and arm_smmu_device_probe.

Quoted 7 lines · Nicolin Chen

Quoted 5 lines · Jason Gunthorpe

On Mon, Apr 29, 2024 at 09:43:48PM -0700, Nicolin Chen wrote:
Really this all seems like alot of overkill to make a little bit of shorthand. It is not so wordy just to type it out:

readl(vintf->base + TEGRA241_VINTF_CONFIG)

Quoted 3 lines

vintf_readl(vintf, CONFIG) is much shorter. Doing so reduced the line breaks at quite a lot places, so overall the driver looks a lot cleaner to me.

We don't have the strict 80 column limit now, it would be fine to go a few extra to avoid the breaks.

Certainly preferred to these readability damaging macros.

Quoted 1 line

I can probably change these logging helpers to inline functions.

Just call the normal logging functions directly.

Just call the normal logging functions, there are so few callers typing out the VCMDQ%u is not going to be so bad

Quoted 4 lines

It would be unrolled to three mostly identical inline functions:
tegra241_cmdqv_write_config(cmdqv, regval) tegra241_vintf_write_config(vintf, regval) tegra241_vcmdq_write_config(vcmdq, regval)

Expand the parameters in the caller:

__do_write_config(owner->base, &owner->status, _CMDQV_EN, TEGRA241_CMDQ_CONFIG,
TEGRA241_CMDQ_STATUS, _CMDQ_ENABLED)

Quoted 9 lines

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 226

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+#define cmdqv_write_config(_regval) \
+	tegra241_cmdqv_write_config(cmdqv, CMDQV, _regval)
+#define vintf_write_config(_regval) \
+	tegra241_cmdqv_write_config(vintf, VINTF, _regval)
+#define vcmdq_write_config(_regval) \
+	tegra241_cmdqv_write_config(vcmdq, VCMDQ, _regval)

More hidden access to stack values

Quoted 3 lines

Btw, any reason for forbidding this practice? It will break the build if something goes wrong, which seems to be pretty easy to catch.

It is the kernel consensus not to do that. function-like-macros should act like functions and not reach into some other stack frame. It makes it very hard to follow the calling function if you can't follow where the references are.

Quoted 11 lines

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 391

Quoted 7 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +7 -0

+	/* Use SMMU CMDQ if vintfs[0] is uninitialized */
+	if (!FIELD_GET(VINTF_ENABLED, atomic_read(&vintf->status)))
+		return &smmu->cmdq;
+
+	/* Use SMMU CMDQ if vintfs[0] has error status */
+	if (FIELD_GET(VINTF_STATUS, atomic_read(&vintf->status)))
+		return &smmu->cmdq;

Why atomic_read? The unlocked interaction with tegra241_cmdqv_handle_vintf0_error() doesn't seem especially sane IMHO

Quoted 1 line

Race between this get_cmdq() and the isr. Any alternative practice?

It doesn't fix any real race, I'm not sure what this is supposed to be doing. The cmdq becomes broken and you get an ISR, so before the ISR it will still post but get stuck, during the ISR it will avoid posting, and after it will go back to posting?

Why? Just always post to the Q and let the ISR fix it?

Quoted 11 lines

Quoted 10 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 811

Quoted 6 lines

drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +6 -0

+	if (tegra241_cmdqv_probe(cmdqv)) {
+		if (cmdqv->irq > 0)
+			devm_free_irq(smmu->dev, cmdqv->irq, cmdqv);
+		devm_iounmap(smmu->dev, cmdqv->base);
+		devm_kfree(smmu->dev, cmdqv);
+		return NULL;

Oh. Please don't use devm at all in this code then, it is not attached to a probed driver with the proper scope, devm isn't going to work in sensible way.

Quoted 2 lines

Mind elaborating "it is not"? This function is called by arm_smmu_device_acpi_probe and arm_smmu_device_probe.

Normal devm usage will unwind the devm allocations when probe fails.

That doesn't happen here, you open coded the unwind above, and then you have open coded freeing in another place anyhow.

So just don't use it. There is no value if the places where it should work automatically are not functioning.

Quoted 6 lines

It doesn't fix any real race, I'm not sure what this is supposed to be doing. The cmdq becomes broken and you get an ISR, so before the ISR it will still post but get stuck, during the ISR it will avoid posting, and after it will go back to posting?

Why? Just always post to the Q and let the ISR fix it?

Yes, we could do so. I was thinking of the worst case by giving the guest OS a chance to continue (though in a slower mode), if something unrecoverable happens to the VINTF/VCMDQ part.

Quoted 2 lines

So just don't use it. There is no value if the places where it should work automatically are not functioning.

I thought devm could work when rmmod too, not only when the probe fails..

Quoted 7 lines · Nicolin Chen

Quoted 6 lines

It doesn't fix any real race, I'm not sure what this is supposed to be doing. The cmdq becomes broken and you get an ISR, so before the ISR it will still post but get stuck, during the ISR it will avoid posting, and after it will go back to posting?

Why? Just always post to the Q and let the ISR fix it?

Quoted 3 lines

Yes, we could do so. I was thinking of the worst case by giving the guest OS a chance to continue (though in a slower mode), if something unrecoverable happens to the VINTF/VCMDQ part.

Does that happn? The stuck vcmdq will have stuck entries on it no matter what, can we actually fully recover from that? Ie re-issue the commands on another queue?

Quoted 5 lines

Quoted 2 lines

So just don't use it. There is no value if the places where it should work automatically are not functioning.

I thought devm could work when rmmod too, not only when the probe fails..

It is limited to cases when the probing driver of the passed struct device unbinds, including probe failure.

(quoted diff) +5 -1

-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
+static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf,
+					u64 *cmds, int n)
+{
+	int i;
+

Quoted 11 lines · Nicolin Chen

(quoted diff) +10 -1

-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)
+static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf,
+					u64 *cmds, int n)
+{
+	int i;
+
+	/* VINTF owned by hypervisor can execute any command */
+	if (vintf->hyp_own)
+		return true;
+
+	/* Guest-owned VINTF must Check against the list of supported CMDs */

Review

Quoted 5 lines

(quoted diff) +5 -0

+	for (i = 0; i < n; i++) {
+		switch (FIELD_GET(CMDQ_0_OP, cmds[i * CMDQ_ENT_DWORDS])) {
+		case CMDQ_OP_TLBI_NH_ASID:
+		case CMDQ_OP_TLBI_NH_VA:
+		case CMDQ_OP_ATC_INV:

So CMDQ only works if not ARM_SMMU_FEAT_E2H? Probably worth mentioning that too along with the discussion about HYP

Review

Quoted 9 lines

(quoted diff) +8 -0

+			continue;
+		default:
+			return false;
+		}
+	}
+
+	return true;
+}

For a performance path this looping seems disappointing.. The callers don't actually mix different command type. Is there something preventing adding a parameter at the callers?

Actually looking at this more closely, isn't the command q selection in the wrong place?

Ie this batch stuff:

static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
struct arm_smmu_cmdq_batch *cmds, struct arm_smmu_cmdq_ent *cmd)

{
	int index;

	if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
	    (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC)) {
		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmds, cmds->num, true);
		cmds->num = 0;
	}

	if (cmds->num == CMDQ_BATCH_ENTRIES) {
		arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmds, cmds->num, false);
		cmds->num = 0;
	}

	index = cmds->num * CMDQ_ENT_DWORDS;
	if (unlikely(arm_smmu_cmdq_build_cmd(&cmds->cmds[index], cmd))) {
		dev_warn(smmu->dev, "ignoring unknown CMDQ opcode 0x%x\n",
			 cmd->opcode);
		return;
	}

Has to push everything, across all the iterations of add/submut, onto the same CMDQ otherwise the SYNC won't be properly flushing?

But each arm_smmu_cmdq_issue_cmdlist() calls its own get q function. Yes, they probably return the same Q since we are probably on the same CPU, but it seems logically wrong (and slower!) to organize it like this.

I would expect the Q to be selected when the struct arm_smmu_cmdq_batch is allocated on the stack, and be the same for the entire batch operation. Not only do we spend less time trying to compute the Q to use we have a built in guarentee that every command will be on the same Q as the fenching SYNC.

Something sort of like this as another patch?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -2

index 268da20baa4e9c..d8c9597878315a 100644
@@ -357,11 +357,22 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 	return 0;
 }
 
-static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
-					       u64 *cmds, int n)

Quoted 2 lines

Has to push everything, across all the iterations of add/submut, onto the same CMDQ otherwise the SYNC won't be properly flushing?

ECMDQ seems to have such a limitation, but VCMDQs can get away as HW can insert a SYNC to a queue that doesn't end with a SYNC.

Quoted 5 lines

But each arm_smmu_cmdq_issue_cmdlist() calls its own get q function. Yes, they probably return the same Q since we are probably on the same CPU, but it seems logically wrong (and slower!) to organize it like this.

Quoted 5 lines

I would expect the Q to be selected when the struct arm_smmu_cmdq_batch is allocated on the stack, and be the same for the entire batch operation. Not only do we spend less time trying to compute the Q to use we have a built in guarentee that every command will be on the same Q as the fenching SYNC.

This seems to be helpful to ECMDQ. The current version disables the preempts, which feels costly to me.

Quoted 20 lines

Something sort of like this as another patch?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -2

index 268da20baa4e9c..d8c9597878315a 100644
@@ -357,11 +357,22 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 	return 0;
 }
 
-static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
-					       u64 *cmds, int n)
+enum required_cmds {
+	CMDS_ALL,
+	/*
+	 * Commands will be one of:
+	 *  CMDQ_OP_ATC_INV, CMDQ_OP_TLBI_EL2_VA, CMDQ_OP_TLBI_NH_VA,
+	 *  CMDQ_OP_TLBI_EL2_ASID, CMDQ_OP_TLBI_NH_ASID, CMDQ_OP_TLBI_S2_IPA,
+	 *  CMDQ_OP_TLBI_S12_VMALL, CMDQ_OP_SYNC
+	 */

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 369

Quoted 2 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -0

+	CMDS_INVALIDATION,
+};

Hmm, guest-owned VCMDQs don't support EL2 commands. So, it feels to be somehow complicated to decouple them further in the callers of arm_smmu_cmdq_batch_add(). And I am not sure if there is a use case of guest issuing CMDQ_OP_TLBI_S2_IPA/CMDQ_OP_TLBI_S12_VMALL either, HW surprisingly supports these two though.

Perhaps we could just scan the first command in the batch, giving a faith that no one will covertly sneak different commands in it?

Otherwise, there has to be a get_suported_cmdq callback so batch or its callers can avoid adding unsupported commands at the first place.

Quoted 5 lines · Nicolin Chen

Quoted 2 lines

Has to push everything, across all the iterations of add/submut, onto the same CMDQ otherwise the SYNC won't be properly flushing?

ECMDQ seems to have such a limitation, but VCMDQs can get away as HW can insert a SYNC to a queue that doesn't end with a SYNC.

That seems like a strange thing to do in HW, but I recall you mentioned it once before. Still, I'm not sure there is any merit in relying on it?

Quoted 20 lines

Quoted 20 lines

Something sort of like this as another patch?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -2

index 268da20baa4e9c..d8c9597878315a 100644
@@ -357,11 +357,22 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 	return 0;
 }
 
-static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
-					       u64 *cmds, int n)
+enum required_cmds {
+	CMDS_ALL,
+	/*
+	 * Commands will be one of:
+	 *  CMDQ_OP_ATC_INV, CMDQ_OP_TLBI_EL2_VA, CMDQ_OP_TLBI_NH_VA,
+	 *  CMDQ_OP_TLBI_EL2_ASID, CMDQ_OP_TLBI_NH_ASID, CMDQ_OP_TLBI_S2_IPA,
+	 *  CMDQ_OP_TLBI_S12_VMALL, CMDQ_OP_SYNC
+	 */

Quoted 8 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 369

Quoted 2 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -0

+	CMDS_INVALIDATION,
+};

Hmm, guest-owned VCMDQs don't support EL2 commands. So, it feels to be somehow complicated to decouple them further in the callers of arm_smmu_cmdq_batch_add(). And I am not sure if there is a use case of guest issuing CMDQ_OP_TLBI_S2_IPA/CMDQ_OP_TLBI_S12_VMALL either, HW surprisingly supports these two though.

These are the max commands that could be issued, but they are all gated based on the feature bits. The ones VCMDQ don't support are not going to be issued because of the feature bits. You could test and enforce this when probing the ECMDQ parts.

Quoted 3 lines · Nicolin Chen

Otherwise, there has to be a get_suported_cmdq callback so batch or its callers can avoid adding unsupported commands at the first place.

If you really feel strongly the invalidation could be split into S1/S2/S1_VM groupings that align with the feature bits and that could be passed down from one step above. But I don't think the complexity is really needed. It is better to deal with it through the feature mechanism.

Quoted 10 lines · Jason Gunthorpe

Quoted 3 lines · Nicolin Chen

Otherwise, there has to be a get_suported_cmdq callback so batch or its callers can avoid adding unsupported commands at the first place.

If you really feel strongly the invalidation could be split into S1/S2/S1_VM groupings that align with the feature bits and that could be passed down from one step above. But I don't think the complexity is really needed. It is better to deal with it through the feature mechanism.

Hmm, I tried following your design by passing in a CMD_TYPE_xxx to the tegra241_cmdqv_get_cmdq(), but I found a little painful to accommodate these two cases:
1. TLBI_NH_ASID is issued via arm_smmu_cmdq_issue_cmdlist(), so we should not mark it as CMD_TYPE_ALL. Yet, this function is used by other commands too. So, either we pass in a type from higher callers, or simply check the opcode in that function.

So, it feels to me that checking against the opcode is still a straightforward solution. And what I ended up with is somewhat similar to this v6, yet this time it only checks at batch init call as your design does.