RE: [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag to arm_smmu_write_strtab_ent()

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: 14 June 2021 11:23
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
> linux-arm-kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
> iommu at lists.linux-foundation.org
> Cc: Linuxarm <linuxarm at huawei.com>; lorenzo.pieralisi at arm.com;
> joro at 8bytes.org; wanghuiqiang <wanghuiqiang at huawei.com>; Guohanjun
> (Hanjun Guo) <guohanjun at huawei.com>; steven.price at arm.com;
> Sami.Mujawar at arm.com; jon at solid-run.com; eric.auger at redhat.com;
> yangyicong <yangyicong at huawei.com>
> Subject: Re: [PATCH v5 5/8] iommu/arm-smmu-v3: Add bypass flag
> to arm_smmu_write_strtab_ent()
> 
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > By default, disable_bypass is set and any dev without an iommu domain
> > installs STE with CFG_ABORT during arm_smmu_init_bypass_stes().
> Introduce
> > a "bypass" flag to arm_smmu_write_strtab_ent() so that we can force it to
> > install CFG_BYPASS STE for specific SIDs. This will be useful in follow
> > up patch to install bypass for IORT RMR SIDs.
> >
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi at huawei.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 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 754bad6092c1..f9195b740f48 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1174,7 +1174,7 @@ static void arm_smmu_sync_ste_for_sid(struct
> arm_smmu_device *smmu, u32 sid)
> >   }
> >
> >   static void arm_smmu_write_strtab_ent(struct arm_smmu_master
> *master, u32 sid,
> > -				      __le64 *dst)
> > +				      __le64 *dst, bool bypass)
> >   {
> >   	/*
> >   	 * This is hideously complicated, but we only really care about
> > @@ -1245,7 +1245,7 @@ static void arm_smmu_write_strtab_ent(struct
> arm_smmu_master *master, u32 sid,
> >
> >   	/* Bypass/fault */
> >   	if (!smmu_domain || !(s1_cfg || s2_cfg)) {
> > -		if (!smmu_domain && disable_bypass)
> > +		if (!smmu_domain && disable_bypass && !bypass)
> 
> Umm...
> 
> >   			val |= FIELD_PREP(STRTAB_STE_0_CFG,
> STRTAB_STE_0_CFG_ABORT);
> >   		else
> >   			val |= FIELD_PREP(STRTAB_STE_0_CFG,
> STRTAB_STE_0_CFG_BYPASS);
> > @@ -1320,7 +1320,7 @@ static void arm_smmu_init_bypass_stes(__le64
> *strtab, unsigned int nent)
> >   	unsigned int i;
> >
> >   	for (i = 0; i < nent; ++i) {
> > -		arm_smmu_write_strtab_ent(NULL, -1, strtab);
> > +		arm_smmu_write_strtab_ent(NULL, -1, strtab, false);
> 
> ...and in particular, an operation named "init_bypass_stes" passing
> bypass=false is just breaking my brain. Can we pull the logic for
> default bypass/fault out to here as part of the refactoring so that it
> actually makes sense?

Agree, it is confusing. Will add the default bypass/fault logic here.

Thanks,
Shameer

> 
> Robin.
> 
> >   		strtab += STRTAB_STE_DWORDS;
> >   	}
> >   }
> > @@ -2097,7 +2097,7 @@ static void arm_smmu_install_ste_for_dev(struct
> arm_smmu_master *master)
> >   		if (j < i)
> >   			continue;
> >
> > -		arm_smmu_write_strtab_ent(master, sid, step);
> > +		arm_smmu_write_strtab_ent(master, sid, step, false);
> >   	}
> >   }
> >
> >