From: Nicolin Chen <nicolinc@nvidia•com>
To: Ashish Mhetre <amhetre@nvidia•com>
Cc: <will@kernel•org>, <robin.murphy@arm•com>, <joro@8bytes•org>,
<jgg@ziepe•ca>, <linux-arm-kernel@lists•infradead.org>,
<iommu@lists•linux.dev>, <linux-kernel@vger•kernel.org>,
<linux-tegra@vger•kernel.org>, Jason Gunthorpe <jgg@nvidia•com>
Subject: Re: [PATCH v2 2/2] iommu/arm-smmu-v3: Issue CFGI/TLBI twice on Tegra264
Date: Fri, 29 May 2026 14:10:43 -0700 [thread overview]
Message-ID: <ahoA05k2zBumGqi/@Asurada-Nvidia> (raw)
In-Reply-To: <20260529140830.629738-3-amhetre@nvidia.com>
On Fri, May 29, 2026 at 02:08:30PM +0000, Ashish Mhetre wrote:
> +int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
> + struct arm_smmu_cmdq *cmdq,
> + struct arm_smmu_cmd *cmds, int n,
> + bool sync)
> +{
> + int ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
> +
> + /*
> + * The driver's batch invariants keep a single submission's
> + * opcode class uniform, so checking the first command is enough.
> + */
> + if (!ret && sync && (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
> + arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
> + cmds[0].data[0])))
> + ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
https://sashiko.dev/#/patchset/20260529140830.629738-1-amhetre%40nvidia.com
Sashiko pointed out that the iommufd path might mix commands when
calling arm_smmu_cmdq_issue_cmdlist(), which is valid I think.
> static int arm_smmu_cmdq_issue_cmd_p(struct arm_smmu_device *smmu,
> struct arm_smmu_cmd *cmd, bool sync)
> {
> @@ -863,8 +909,18 @@ static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,
> }
>
> if (cmds->num == CMDQ_BATCH_ENTRIES) {
> + /*
> + * Force a SYNC only when the batch carries commands that
> + * have to be doubled (see ARM_SMMU_OPT_TLBI_TWICE).
> + * The batch holds a uniform opcode class, so checking
> + * the first command is sufficient.
> + */
> + bool need_sync = (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
> + arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
> + cmds->cmds[0].data[0]));
> +
Also, given that this does "force a sync", I think it might be nicer
to go to the force_sync path. One of my ongoing series also needs to
add another force_sync condition, so I think it would be cleaner to
have a helper function.
Maybe try the following diff. That arm_smmu_cmdq_batch_force_sync()
might be added with a preparatory patch, but it's up to you.
--------------------------------------------------------------------
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 1e9f7d2de3441..4c9ce974d31a8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -350,6 +350,18 @@ static int arm_vsmmu_convert_user_cmd(struct arm_vsmmu *vsmmu,
return 0;
}
+static bool arm_vsmmu_can_batch_cmd(struct arm_smmu_device *smmu,
+ struct arm_vsmmu_invalidation_cmd *last,
+ struct arm_vsmmu_invalidation_cmd *next)
+{
+ struct arm_smmu_cmd next_cmd = {
+ .data[0] = le64_to_cpu(next->ucmd.cmd[0]),
+ };
+
+ return arm_smmu_cmd_needs_tlbi_twice(smmu, &last->cmd) ==
+ arm_smmu_cmd_needs_tlbi_twice(smmu, &next_cmd);
+}
+
int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
struct iommu_user_data_array *array)
{
@@ -382,7 +394,8 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
/* FIXME work in blocks of CMDQ_BATCH_ENTRIES and copy each block? */
cur++;
- if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1)
+ if (cur != end && (cur - last) != CMDQ_BATCH_ENTRIES - 1 &&
+ arm_vsmmu_can_batch_cmd(smmu, last, cur))
continue;
/* FIXME always uses the main cmdq rather than trying to group by type */
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index a63155e9e7f28..9b150e3145034 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -820,33 +820,6 @@ static int __arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
return ret;
}
-/*
- * Returns true if @opcode is a CFGI_* or TLBI_* command, i.e. one of the
- * invalidations covered by Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE).
- */
-static bool arm_smmu_cmd_needs_tlbi_twice(u8 opcode)
-{
- switch (opcode) {
- case CMDQ_OP_CFGI_STE:
- case CMDQ_OP_CFGI_ALL:
- case CMDQ_OP_CFGI_CD:
- case CMDQ_OP_CFGI_CD_ALL:
- case CMDQ_OP_TLBI_NH_ALL:
- case CMDQ_OP_TLBI_NH_ASID:
- case CMDQ_OP_TLBI_NH_VA:
- case CMDQ_OP_TLBI_NH_VAA:
- case CMDQ_OP_TLBI_EL2_ALL:
- case CMDQ_OP_TLBI_EL2_ASID:
- case CMDQ_OP_TLBI_EL2_VA:
- case CMDQ_OP_TLBI_S12_VMALL:
- case CMDQ_OP_TLBI_S2_IPA:
- case CMDQ_OP_TLBI_NSNH_ALL:
- return true;
- default:
- return false;
- }
-}
-
int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
struct arm_smmu_cmdq *cmdq,
struct arm_smmu_cmd *cmds, int n,
@@ -858,9 +831,7 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
* The driver's batch invariants keep a single submission's
* opcode class uniform, so checking the first command is enough.
*/
- if (!ret && sync && (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
- arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
- cmds[0].data[0])))
+ if (!ret && sync && arm_smmu_cmd_needs_tlbi_twice(smmu, &cmds[0]))
ret = __arm_smmu_cmdq_issue_cmdlist(smmu, cmdq, cmds, n, sync);
return ret;
@@ -893,34 +864,48 @@ static void arm_smmu_cmdq_batch_init_cmd(struct arm_smmu_device *smmu,
cmds->cmdq = arm_smmu_get_cmdq(smmu, cmd);
}
+static bool arm_smmu_cmdq_batch_force_sync(struct arm_smmu_device *smmu,
+ struct arm_smmu_cmdq_batch *cmds,
+ struct arm_smmu_cmd *cmd)
+{
+ if (!cmds->num)
+ return false;
+
+ /* The batch's pre-assigned cmdq doesn't support the new command */
+ if (!arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd))
+ return true;
+
+ /* Arm erratum 2812531 */
+ if (cmds->num == CMDQ_BATCH_ENTRIES - 1 &&
+ (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
+ return true;
+
+ /*
+ * Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE). The batch holds a
+ * uniform opcode class, so checking the first command is enough.
+ */
+ if ((cmds->num == CMDQ_BATCH_ENTRIES) &&
+ arm_smmu_cmd_needs_tlbi_twice(smmu, &cmds->cmds[0]))
+ return true;
+
+ return false;
+}
+
static void arm_smmu_cmdq_batch_add_cmd_p(struct arm_smmu_device *smmu,
struct arm_smmu_cmdq_batch *cmds,
struct arm_smmu_cmd *cmd)
{
- bool force_sync = (cmds->num == CMDQ_BATCH_ENTRIES - 1) &&
- (smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC);
- bool unsupported_cmd;
+ bool force_sync = arm_smmu_cmdq_batch_force_sync(smmu, cmds, cmd);
- unsupported_cmd = !arm_smmu_cmdq_supports_cmd(cmds->cmdq, cmd);
- if (force_sync || unsupported_cmd) {
+ if (force_sync) {
arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
cmds->num, true);
arm_smmu_cmdq_batch_init_cmd(smmu, cmds, cmd);
}
if (cmds->num == CMDQ_BATCH_ENTRIES) {
- /*
- * Force a SYNC only when the batch carries commands that
- * have to be doubled (see ARM_SMMU_OPT_TLBI_TWICE).
- * The batch holds a uniform opcode class, so checking
- * the first command is sufficient.
- */
- bool need_sync = (smmu->options & ARM_SMMU_OPT_TLBI_TWICE) &&
- arm_smmu_cmd_needs_tlbi_twice(FIELD_GET(CMDQ_0_OP,
- cmds->cmds[0].data[0]));
-
arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
- cmds->num, need_sync);
+ cmds->num, false);
arm_smmu_cmdq_batch_init_cmd(smmu, cmds, cmd);
}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 08d1abaf31ae2..e6afc832c0078 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1219,6 +1219,37 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
struct arm_smmu_cmd *cmds, int n,
bool sync);
+/*
+ * Returns true if @cmd opcode is a CFGI_* or TLBI_* command, i.e. one of the
+ * invalidations covered by Tegra264 erratum (see ARM_SMMU_OPT_TLBI_TWICE).
+ */
+static inline bool arm_smmu_cmd_needs_tlbi_twice(struct arm_smmu_device *smmu,
+ struct arm_smmu_cmd *cmd)
+{
+ if (!(smmu->options & ARM_SMMU_OPT_TLBI_TWICE))
+ return false;
+
+ switch (FIELD_GET(CMDQ_0_OP, cmd->data[0])) {
+ case CMDQ_OP_CFGI_STE:
+ case CMDQ_OP_CFGI_ALL:
+ case CMDQ_OP_CFGI_CD:
+ case CMDQ_OP_CFGI_CD_ALL:
+ case CMDQ_OP_TLBI_NH_ALL:
+ case CMDQ_OP_TLBI_NH_ASID:
+ case CMDQ_OP_TLBI_NH_VA:
+ case CMDQ_OP_TLBI_NH_VAA:
+ case CMDQ_OP_TLBI_EL2_ALL:
+ case CMDQ_OP_TLBI_EL2_ASID:
+ case CMDQ_OP_TLBI_EL2_VA:
+ case CMDQ_OP_TLBI_S12_VMALL:
+ case CMDQ_OP_TLBI_S2_IPA:
+ case CMDQ_OP_TLBI_NSNH_ALL:
+ return true;
+ default:
+ return false;
+ }
+}
+
#ifdef CONFIG_ARM_SMMU_V3_SVA
bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
void arm_smmu_sva_notifier_synchronize(void);
--------------------------------------------------------------------
Nicolinc
next prev parent reply other threads:[~2026-05-29 21:11 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-29 14:08 [PATCH v2 0/2] iommu/arm-smmu-v3: Tegra264 invalidation workaround Ashish Mhetre
2026-05-29 14:08 ` [PATCH v2 1/2] iommu/arm-smmu-v3: Detect Tegra264 erratum Ashish Mhetre
2026-05-29 21:30 ` Nicolin Chen
2026-06-01 9:04 ` Ashish Mhetre
2026-05-29 14:08 ` [PATCH v2 2/2] iommu/arm-smmu-v3: Issue CFGI/TLBI twice on Tegra264 Ashish Mhetre
2026-05-29 21:10 ` Nicolin Chen [this message]
2026-06-01 9:20 ` Ashish Mhetre
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ahoA05k2zBumGqi/@Asurada-Nvidia \
--to=nicolinc@nvidia$(echo .)com \
--cc=amhetre@nvidia$(echo .)com \
--cc=iommu@lists$(echo .)linux.dev \
--cc=jgg@nvidia$(echo .)com \
--cc=jgg@ziepe$(echo .)ca \
--cc=joro@8bytes$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-tegra@vger$(echo .)kernel.org \
--cc=robin.murphy@arm$(echo .)com \
--cc=will@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox