Re: [PATCH v5 04/17] iommu/arm-smmu-v3: Move the STE generation for S1 and S2 domains into functions

From: Jason Gunthorpe <jgg@nvidia.com>
To: Will Deacon <will@kernel.org>
Cc: iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joerg Roedel <jroedel@suse.de>, Moritz Fischer <mdf@kernel.org>,
	Moritz Fischer <moritzf@google.com>,
	Michael Shavit <mshavit@google.com>,
	Nicolin Chen <nicolinc@nvidia.com>,
	patches@lists.linux.dev,
	Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
	Mostafa Saleh <smostafa@google.com>,
	Zhangfei Gao <zhangfei.gao@linaro.org>
Subject: Re: [PATCH v5 04/17] iommu/arm-smmu-v3: Move the STE generation for S1 and S2 domains into functions
Date: Fri, 16 Feb 2024 13:58:21 -0400	[thread overview]
Message-ID: <20240216175821.GF13330@nvidia.com> (raw)
In-Reply-To: <20240216173921.GA2374@willie-the-truck>

On Fri, Feb 16, 2024 at 05:39:22PM +0000, Will Deacon wrote:
> On Fri, Feb 16, 2024 at 01:12:17PM -0400, Jason Gunthorpe wrote:
> > On Tue, Feb 06, 2024 at 11:12:41AM -0400, Jason Gunthorpe wrote:
> > > +static void arm_smmu_make_s2_domain_ste(struct arm_smmu_ste *target,
> > > +					struct arm_smmu_master *master,
> > > +					struct arm_smmu_domain *smmu_domain)
> > > +{
> > > +	struct arm_smmu_s2_cfg *s2_cfg = &smmu_domain->s2_cfg;
> > > +
> > > +	memset(target, 0, sizeof(*target));
> > > +	target->data[0] = cpu_to_le64(
> > > +		STRTAB_STE_0_V |
> > > +		FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S2_TRANS));
> > > +
> > > +	target->data[1] = cpu_to_le64(
> > > +		FIELD_PREP(STRTAB_STE_1_EATS,
> > > +			   master->ats_enabled ? STRTAB_STE_1_EATS_TRANS : 0) |
> > > +		FIELD_PREP(STRTAB_STE_1_SHCFG,
> > > +			   STRTAB_STE_1_SHCFG_NON_SHARABLE));
> > 
> > Just so we are on the same page.. The above NON_SHARABLE is a mistake
> > here since v1.
> > 
> > It is hard to follow arm_smmu_write_strtab_ent() so we all missed that
> > the S2 ends up re-using the qword[1] that was installed by the
> > bypass/abort STE that has to be in place prior to installing the S2.
> 
> Ah! I thought you were inheriting the existing behaviour, but yeah,

Yeah, so did I..

> it's a straight-up bug which I think just makes life a little more
> difficult than it needs to be. If we can keep SHCFG as "use
> incoming" in all configurations, then I do think we can move to a
> per-qword rather than a per-field approach, as mentioned in the
> other part of the thread. I'll try to make some time next week to
> play with it.

I'm sure you can make per-qword work. I think it will be worse code
though because doing so will have to compromise some of the
underpinning logical principles:

 - Used bits reflects actual HW behavior and flows from the
   spec's IGNORED/etc language
 - Make STE functions set the bits that the HW uses and no extra bits
 - The make STE functions create a complete STE

I already tried the naive version where none of the above are
compromised and it does not work. Someone else may have an idea. IMHO
this is really not a valuable avenue to use all of our limited time
on.

I'm getting the existings remarks typed in, it is already turning into
some work, but if you feel strongly please come next week with exactly
you will accept and I will ensure whatever it is gets done if you will
commit to merge it. Even if we have to toss out Michael's version too.

I can always bring back this version for some future shared code
thing.

Thanks,
Jason

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