linux-kernel - Re: [PATCH v6 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] [thread-next>] [day] [month] [year] [list]

Date: Wed, 1 May 2024 09:32:44 -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 v6 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned
 VINTF

On Tue, Apr 30, 2024 at 09:17:58PM -0300, Jason Gunthorpe wrote:
> On Tue, Apr 30, 2024 at 11:58:44AM -0700, Nicolin Chen wrote:
> 
> > > Has to push everything, across all the iterations of add/submut, onto
> > > the same CMDQ otherwise the SYNC won't be properly flushing?
> > 
> > ECMDQ seems to have such a limitation, but VCMDQs can get away
> > as HW can insert a SYNC to a queue that doesn't end with a SYNC.
> 
> That seems like a strange thing to do in HW, but I recall you
> mentioned it once before. Still, I'm not sure there is any merit in
> relying on it?

I was hoping to get some idea from the designer. Yet, at this
moment, let's say there's likely no merit besides SW can care
less and stay simpler, AFAIK.

Robin previously remarked that there could be some performance
impact from such a feature, so I think adding your patch would
be nicer.

> > > Something sort of like this as another patch?
> > > 
> > > 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..d8c9597878315a 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -357,11 +357,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,
> > > -					       u64 *cmds, int n)
> > > +enum required_cmds {
> > > +	CMDS_ALL,
> > > +	/*
> > > +	 * Commands will be one of:
> > > +	 *  CMDQ_OP_ATC_INV, CMDQ_OP_TLBI_EL2_VA, CMDQ_OP_TLBI_NH_VA,
> > > +	 *  CMDQ_OP_TLBI_EL2_ASID, CMDQ_OP_TLBI_NH_ASID, CMDQ_OP_TLBI_S2_IPA,
> > > +	 *  CMDQ_OP_TLBI_S12_VMALL, CMDQ_OP_SYNC
> > > +	 */
> > > +	CMDS_INVALIDATION,
> > > +};
> > 
> > Hmm, guest-owned VCMDQs don't support EL2 commands. So, it feels
> > to be somehow complicated to decouple them further in the callers
> > of arm_smmu_cmdq_batch_add(). And I am not sure if there is a use
> > case of guest issuing CMDQ_OP_TLBI_S2_IPA/CMDQ_OP_TLBI_S12_VMALL
> > either, HW surprisingly supports these two though.
> 
> These are the max commands that could be issued, but they are all
> gated based on the feature bits. The ones VCMDQ don't support are not
> going to be issued because of the feature bits. You could test and
> enforce this when probing the ECMDQ parts.

Ah, I see. So cmdqv's probe() could check the smmu->features
against ARM_SMMU_FEAT_E2H, and disable VINTF0 right away, in
case of guest-owned while E2H is present.

And we could do the same for ARM_SMMU_FEAT_TRANS_S2, stating
that the driver does not expect use cases of issuing S2 TLBI
commands from the guest OS.

Thanks
Nicolin