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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 14 May 2024 15:20:24 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: Jason Gunthorpe <jgg@...dia.com>
CC: <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
	<thierry.reding@...il.com>, <vdumpa@...dia.com>, <jonathanh@...dia.com>,
	<linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned
 VINTF

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