Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF

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
Date: Tue, 14 May 2024 15:20:24 -0700	[thread overview]
Message-ID: <ZkPjqDJvkXc84At3@nvidia.com> (raw)
In-Reply-To: <ZkOAAdugARILEeBh@nvidia.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
