linux-kernel - Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Add boolean bypass_ste and skip

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]

Message-ID: <ZRHTw0+DCYGjFX0P@Asurada-Nvidia>
Date:   Mon, 25 Sep 2023 11:38:59 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     <will@...nel.org>, <robin.murphy@....com>, <joro@...tes.org>,
        <jean-philippe@...aro.org>, <mshavit@...gle.com>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>
Subject: Re: [PATCH v4 1/2] iommu/arm-smmu-v3: Add boolean bypass_ste and
 skip_cdtab flags

On Mon, Sep 25, 2023 at 02:57:08PM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 20, 2023 at 01:52:03PM -0700, Nicolin Chen wrote:
> > If a master has only a default substream, it can skip CD/translation table
> > allocations when being attached to an IDENTITY domain, by simply setting
> > STE to the "bypass" mode (STE.Config[2:0] == 0b100).
> > 
> > If a master has multiple substreams, it will still need a CD table for the
> > non-default substreams when being attached to an IDENTITY domain, in which
> > case the STE.Config is set to the "stage-1 translate" mode while STE.S1DSS
> > field instead is set to the "bypass" mode (STE.S1DSS[1:0] == 0b01).
> > 
> > If a master is attached to a stage-2 domain, it does not need a CD table,
> > while the STE.Config is set to the "stage-2 translate" mode.
> > 
> > Add boolean bypass_ste and skip_cdtab flags in arm_smmu_attach_dev(), to
> > handle clearly the cases above, which also corrects the conditions at the
> > ats_enabled setting and arm_smmu_alloc_cd_tables() callback to cover the
> > second use case.
> > 
> > Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 35 ++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> > 
> > 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 df6409017127..dbe11997b4b9 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2381,6 +2381,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  	struct arm_smmu_device *smmu;
> >  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> >  	struct arm_smmu_master *master;
> > +	bool byapss_ste, skip_cdtab;
> >  
> >  	if (!fwspec)
> >  		return -ENOENT;
> > @@ -2416,6 +2417,24 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  
> >  	master->domain = smmu_domain;
> >  
> > +	/*
> > +	 * When master attaches ARM_SMMU_DOMAIN_BYPASS to its single substream,
> > +	 * set STE.Config to "bypass" and skip a CD table allocation. Otherwise,
> > +	 * set STE.Config to "stage-1 translate" and allocate a CD table for its
> > +	 * multiple stage-1 substream support, unless with a stage-2 domain in
> > +	 * which case set STE.config to "stage-2 translate" and skip a CD table.
> > +	 */
> 
> It might be clearer like this:
> 
> static bool arm_smmu_domain_needs_cdtab(struct arm_smmu_domain *smmu_domain,
> 					struct arm_smmu_master *master)
> {
> 	switch (smmu_domain->stage) {
> 	/*
>          * The SMMU can support IOMMU_DOMAIN_IDENTITY either by programming
>          * STE.Config to 0b100 (bypass) or by configuring STE.Config to 0b101
>          * (S1 translate) and setting STE.S1DSS[1:0] to 0b01 "bypass". The
>          * latter requires allocating a CD table.
> 	 *
> 	 * The 0b100 config has the drawback that ATS and PASID cannot be used,
>          * however it could be higher performance. Select the "S1 translation"
>          * option if we might need those features.
> 	 */
> 	case ARM_SMMU_DOMAIN_BYPASS:
> 		return master->ssid_bits || arm_smmu_ats_supported(master);
> 	case ARM_SMMU_DOMAIN_S1:
> 	case ARM_SMMU_DOMAIN_NESTED:
> 		return true;
> 	case ARM_SMMU_DOMAIN_S2:
> 		return false;
> 	}
> 	return false;
> }
>
> Then the below is
> 
>        if (needs_cdtab || smm_domain->stage != ARM_SMMU_DOMAIN_BYPASS)
> 		master->ats_enabled = arm_smmu_ats_supported(master);
> 
> And the CD table should be sync'd to the result of arm_smmu_domain_needs_cdtab()..

Ack.

> It looks like there is still some kind of logic missing as we need to
> know if there are any PASIDs using the cd table here:
> 
> if (!master->cd_table_empty && !needs_cdtab)
>    return -EBUSY;
> 
> if (needs_ctab && !master->cd_table.cdtab)
>      ret = arm_smmu_alloc_cd_tables(master);
> 
> if (!needs_ctab && master->cd_table.cdtab)
>     arm_smmu_dealloc_cd_tables(master);
> 
> And add master->cd_table_emty to the arm_smmu_domain_needs_cdtab bypass logic.

OK. I will give it a try.

> Also, are these patches are out of order, this should come last since
> the arm_smmu_write_strtab_ent hasn't learned yet how to do
> STRTAB_STE_1_S1DSS_BYPASS?

Hmm. It could although it's a status quo IMHO -- in practical
only ARM_SMMU_DOMAIN_BYPASS would be changed by the 1st patch
to allocating a CD table that still doesn't work since this
STRTAB_STE_1_S1DSS_BYPASS is unset.

Thanks
Nic