Received-SPF: pass (google.com: domain of linux-kernel+bounces-164129-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 30 Apr 2024 11:22:01 -0300
From: Jason Gunthorpe <jgg@nvidia.com>
To: Nicolin Chen <nicolinc@nvidia.com>
Cc: will@kernel.org, robin.murphy@arm.com, joro@8bytes.org,
	thierry.reding@gmail.com, vdumpa@nvidia.com, jonathanh@nvidia.com,
	linux-kernel@vger.kernel.org, iommu@lists.linux.dev,
	linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org
Subject: Re: [PATCH v6 2/6] iommu/arm-smmu-v3: Add CS_NONE quirk
Message-ID: <20240430142201.GQ941030@nvidia.com>
References: <cover.1714451595.git.nicolinc@nvidia.com>
 <81d79f51c69604a38ea4f72c8ac2c573c52e8609.1714451595.git.nicolinc@nvidia.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <81d79f51c69604a38ea4f72c8ac2c573c52e8609.1714451595.git.nicolinc@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?2Mt07unKBqidaWZacqXNkwwgkNtU3fc4G1KfGUVgsSdQgV7qXraIT8Unz1fB?=
 =?us-ascii?Q?3pIWb8C2zU1BHXnGm+7/XyOE4+HRv0kWq2scGRXJ38mt8vWF+5Ix5obcItUj?=
 =?us-ascii?Q?2B65N75M7sM+h6fWkUjI0u9ZkwiXnEm3enP7ylh7cLA9t18NDi80RkRST9G+?=
 =?us-ascii?Q?ovSqP1EfZtDL3qpbVm03yxXTbJNO1xMOhAqn208Dr+Djciho7gl0+bCPt60Z?=
 =?us-ascii?Q?O5WFvhZhOPSd1hMVRIxty0okzA3xPrf2ILi69D3B+pmS8bcMkrhm9CYHqya1?=
 =?us-ascii?Q?9JjpAdsRrRtxrgSYEgVMDcXLR1x2dZjOg0xaBtl+icWKHqhUuHXDTCSqjiWb?=
 =?us-ascii?Q?FaAyXOk6OcxXbmdffTft6EGmqYGMVSRuQeoe9ElUKTL0eVHOUSdqoLb8pS8t?=
 =?us-ascii?Q?XxqbelX6ySH76vo+ESQJBfueb1igvvoYDL2UtxZOS0TuPkbOUI8QE4oV9b+a?=
 =?us-ascii?Q?AUt2TRzRkZ49tLFaT8FhOtPJF5VnB7w0/fUUZAxF6+J3RKiw44ziWwcp7j9t?=
 =?us-ascii?Q?jxMMZ7AGaTv3xKqVbWlCrBELBTyNY6BlteCvDdgswlLCCkgsoLyjTptUXHPv?=
 =?us-ascii?Q?i5PhSTlVqejVFAUQ69nlgmE5DDtbQvG9swIHzMc8tRTBeq6JuA+w97EWB6jy?=
 =?us-ascii?Q?qxR1vjnQ2tZCBeRwpVxT4W0LrKND4jBlZKUiKNOzzFpig7/dpGj5zAf4+EEG?=
 =?us-ascii?Q?BshEjVDbnr3mqzw4wyRfxtfd+6avEMXXVAurwnf3G2Wu3FsMzLadayGsuWzE?=
 =?us-ascii?Q?W19ay9I+SufLeqPM8fLOPEA4K+5E0WoNhN47+e/XptqNiiD+FFj2BYeXMvAn?=
 =?us-ascii?Q?AXQW67MRW7SjoGsuS44FI2/2y77yVOIef1jTw95aVprrJQyZ5KH4WlVL9BF2?=
 =?us-ascii?Q?0IU+n6qeIN939Phoh2Oe6gOvDmMFI5kA+CuxtTmUraZGpovBUCshlvWIk/PO?=
 =?us-ascii?Q?QLYtvLKbF2OLW3IvnJNr6UkPpFixaT/PhPnlnVwtBwiQ0cmKeRlQMRJOS81E?=
 =?us-ascii?Q?3dUpuj8LN63do8/a3hAMVG5ywJl+mvvxPIW+T0N4khvOPeAkgzyZ/timas+0?=
 =?us-ascii?Q?Syr8kBIB8yVRsQ9NhEzfDXkh0ZOQOpT2JqnMflO8LYO39iIRBrFsagD+Q8Qq?=
 =?us-ascii?Q?Up5G7Nl09guLt5VLJlK8B19E5pmF93D8qJRkl4VOHL/8xo/DGW65V+/xcoiQ?=
 =?us-ascii?Q?BbM9OHreYPh+DdIng34hYG0d8+0JBfjAhSpO/rKWTbHk8DC6LXlK2k5+uh5T?=
 =?us-ascii?Q?6xG304b/mGtZxP7WWDiBcUHHfCCtlIAgsfye+vIzjOBaz3aJT8xm+b9QGg7E?=
 =?us-ascii?Q?UpNQuau5XxQsU+RxqJ5CdqWDBvi2ABRTLIKB3mqHc12z8qd4MWShLMEgq1Lt?=
 =?us-ascii?Q?YyiNeAntl0lS9PSyMfm1n1MSVlWUzAJzOwDtI0kIdMkLyof+S5v8ZNBigoxY?=
 =?us-ascii?Q?csTdosc6fmBnDu5bpDJG//zXtfwbEPjHRlRQ5S1jUw5IlMNkFDKImGWB+TX/?=
 =?us-ascii?Q?GMrO3gRbp2UMUkm7y5eYL67ksmX165dCEaDMpRXF47d+cNKTQqAJvrjXW4Y2?=
 =?us-ascii?Q?rh1yH3QSk2DB29dPMv8u+AVeCQGTfv4rNFnzoVy9?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b833b8bb-34a4-448c-4f56-08dc6920e7a0
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3849.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 14:22:02.7283
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xoiPpf8L7jJ3s50SmGwOY8DXKyVs3tNzwdx0j+Zbxa+Vn3OimPl7UJwSJU6zA2yO
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB7690

On Mon, Apr 29, 2024 at 09:43:45PM -0700, Nicolin Chen wrote:
> 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 <nicolinc@nvidia.com>
> ---
>  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 <jgg@nvidia.com>

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?

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 268da20baa4e9c..fa9cb0f49bf1ee 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -338,18 +338,6 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
 		cmd[0] |= FIELD_PREP(CMDQ_RESUME_0_RESP, ent->resume.resp);
 		cmd[1] |= FIELD_PREP(CMDQ_RESUME_1_STAG, ent->resume.stag);
 		break;
-	case CMDQ_OP_CMD_SYNC:
-		if (ent->sync.cs_none) {
-			cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
-		} else if (ent->sync.msiaddr) {
-			cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ);
-			cmd[1] |= ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK;
-		} else {
-			cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
-		}
-		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH);
-		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
-		break;
 	default:
 		return -ENOENT;
 	}
@@ -367,25 +355,30 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu,
 }
 
 static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,
-					 struct arm_smmu_queue *q, u32 prod)
+					 struct arm_smmu_queue *q, u32 prod,
+					 bool msi)
 {
-	struct arm_smmu_cmdq_ent ent = {
-		.opcode = CMDQ_OP_CMD_SYNC,
-	};
+	memset(cmd, 0, 1 << CMDQ_ENT_SZ_SHIFT);
+	cmd[0] = FIELD_PREP(CMDQ_0_OP, CMDQ_OP_CMD_SYNC) |
+		 FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
+		 FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB);
+	if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);
+		return;
+	}
+
+	if (!msi || !(smmu->options & ARM_SMMU_OPT_MSIPOLL)) {
+		cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_SEV);
+		return;
+	}
 
 	/*
 	 * Beware that Hi16xx adds an extra 32 bits of goodness to its MSI
 	 * payload, so the write will zero the entire command on that platform.
 	 */
-	if (smmu->options & ARM_SMMU_OPT_MSIPOLL) {
-		ent.sync.msiaddr = q->base_dma + Q_IDX(&q->llq, prod) *
-				   q->ent_dwords * 8;
-	}
-
-	if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY)
-		ent.sync.cs_none = true;
-
-	arm_smmu_cmdq_build_cmd(cmd, &ent);
+	cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ);
+	cmd[1] = (q->base_dma + Q_IDX(&q->llq, prod) * q->ent_dwords * 8) &
+		 CMDQ_SYNC_1_MSIADDR_MASK;
 }
 
 void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
@@ -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)
 		dev_err(smmu->dev, "\t0x%016llx\n", (unsigned long long)cmd[i]);
 
-	if (q->quirks & CMDQ_QUIRK_SYNC_CS_NONE_ONLY)
-		cmd_sync.sync.cs_none = true;
-
 	/* Convert the erroneous command into a CMD_SYNC */
-	arm_smmu_cmdq_build_cmd(cmd, &cmd_sync);
+	arm_smmu_cmdq_build_sync_cmd(cmd, smmu, q, 0, false);
 
 	queue_write(Q_ENT(q, cons), cmd, q->ent_dwords);
 }
@@ -812,7 +799,8 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
 	arm_smmu_cmdq_write_entries(cmdq, cmds, llq.prod, n);
 	if (sync) {
 		prod = queue_inc_prod_n(&llq, n);
-		arm_smmu_cmdq_build_sync_cmd(cmd_sync, smmu, &cmdq->q, prod);
+		arm_smmu_cmdq_build_sync_cmd(cmd_sync, smmu, &cmdq->q, prod,
+					     true);
 		queue_write(Q_ENT(&cmdq->q, prod), cmd_sync, CMDQ_ENT_DWORDS);
 
 		/*
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 9412fa4ff5e045..b1ce1986e61101 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -520,10 +520,6 @@ struct arm_smmu_cmdq_ent {
 		} resume;
 
 		#define CMDQ_OP_CMD_SYNC	0x46
-		struct {
-			u64			msiaddr;
-			bool			cs_none;
-		} sync;
 	};
 };