Received-SPF: pass (google.com: domain of linux-kernel+bounces-179221-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.118.232 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.118.232; helo=mail.nvidia.com; pr=C
Date: Tue, 14 May 2024 15:20:24 -0700
From: Nicolin Chen <nicolinc@nvidia.com>
To: Jason Gunthorpe <jgg@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 v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned
 VINTF
Message-ID: <ZkPjqDJvkXc84At3@nvidia.com>
References: <cover.1715147377.git.nicolinc@nvidia.com>
 <062cf0a1e2b8ec6f068262cc68498b8d72b04bcc.1715147377.git.nicolinc@nvidia.com>
 <ZkDo9US02pD6vysO@nvidia.com>
 <ZkE+Fa4BS+LTxvgi@nvidia.com>
 <ZkOAAdugARILEeBh@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZkOAAdugARILEeBh@nvidia.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 May 2024 22:20:45.9705
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 7f9ddeaa-2898-4758-3343-08dc74641a03
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.118.232];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource:
	BN3PEPF0000B36D.namprd21.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB6438

On Tue, May 14, 2024 at 12:15:13PM -0300, Jason Gunthorpe wrote:
> On Sun, May 12, 2024 at 03:09:25PM -0700, Nicolin Chen wrote:
> > > > -static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)
> > > > +static struct arm_smmu_cmdq *
> > > > +arm_smmu_get_cmdq(struct arm_smmu_device *smmu, u8 opcode)
> > > >  {
> > > >  	if (arm_smmu_has_tegra241_cmdqv(smmu))
> > > > -		return tegra241_cmdqv_get_cmdq(smmu);
> > > > +		return tegra241_cmdqv_get_cmdq(smmu, opcode);
> > > 
> > > It is worth a comment descrbing opcode here, I think.. At least the
> > > nesting invalidation will send mixed batches.
> > 
> > Right, this makes the "opcode" look bad, though we know that the
> > "opcode" in the nesting invalidation doesn't matter because VCMDQ
> > in that case supports all commands with HYP_OWN=1.
> 
> Yeah, it isn't a real problem, it just looks a little messy and
> should have a small comment someplace at least..
>  
> > A CMD_SYNC, on the other hand, is outside the batch struct. And
> > here is another assumption that CMD_SYNC is always supported by a
> > VCMDQ..
> > 
> > I could clarify the "opcode" here with these assumptions. Or maybe
> > we should think think of a better alternative?
> 
> I don't think it really needs to be more complex, but we should
> document that invalidation is going to be special and doesn't quite
> follow this rule.

Yea. I just added this:

@@ -333,10 +333,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)
+static struct arm_smmu_cmdq *
+arm_smmu_get_cmdq(struct arm_smmu_device *smmu, u8 opcode)
 {
+	/*
+	 * TEGRA241 CMDQV has two modes to execute commands: host and guest.
+	 * The host mode supports all the opcodes, while the guest mode only
+	 * supports a few invalidation ones (check tegra241_vintf_support_cmd)
+	 * and also a CMD_SYNC added by arm_smmu_cmdq_issue_cmdlist(..., true).
+	 *
+	 * Here pass in the representing opcode for either a single command or
+	 * an arm_smmu_cmdq_batch, assuming that this SMMU driver will only add
+	 * same type of commands into a batch as it does today or it will only
+	 * mix supported invalidation commands in a batch.
+	 */
 	if (arm_smmu_has_tegra241_cmdqv(smmu))
-		return tegra241_cmdqv_get_cmdq(smmu);
+		return tegra241_cmdqv_get_cmdq(smmu, opcode);
 
 	return &smmu->cmdq;
 }

Thanks
Nicolin