Re: [PATCH v5 01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers

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 01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers
Date: Wed, 21 Feb 2024 10:08:18 -0400	[thread overview]
Message-ID: <20240221140818.GA2635804@nvidia.com> (raw)
In-Reply-To: <20240221134923.GA7362@willie-the-truck>

On Wed, Feb 21, 2024 at 01:49:23PM +0000, Will Deacon wrote:

> Very roughly, yes, although I'd go further and just return a bitmap of
> used qwords instead of tracking these bits. Basically, we could have some
> #defines saying which qwords are used by which configs, 

I don't think this will work well for CD's EPD0 case..

static void arm_smmu_get_cd_used(const __le64 *ent, __le64 *used_bits)
{
	used_bits[0] = cpu_to_le64(CTXDESC_CD_0_V);
	if (!(ent[0] & cpu_to_le64(CTXDESC_CD_0_V)))
		return;
	memset(used_bits, 0xFF, sizeof(struct arm_smmu_cd));

	/* EPD0 means T0SZ/TG0/IR0/OR0/SH0/TTB0 are IGNORED */
	if (ent[0] & cpu_to_le64(CTXDESC_CD_0_TCR_EPD0)) {
		used_bits[0] &= ~cpu_to_le64(
			CTXDESC_CD_0_TCR_T0SZ | CTXDESC_CD_0_TCR_TG0 |
			CTXDESC_CD_0_TCR_IRGN0 | CTXDESC_CD_0_TCR_ORGN0 |
			CTXDESC_CD_0_TCR_SH0);
		used_bits[1] &= ~cpu_to_le64(CTXDESC_CD_1_TTB0_MASK);
	}
}

> and then we can
> simplify the algorithm while retaining the ability to reject updates
> to qwords which we're not expecting.

It is not much simplification. arm_smmu_entry_qword_diff() gets a bit
shorter (not that it is complex anyhow) and other stuff gets worse.

> > We'd have to really start doing really hacky things like remove the
> > SHCFG as a used field entirely - but I think if you do that you break
> > the entire logic of the design and also go backwards to having
> > programming that only works if STEs are constructed in certain ways.
> 
> I would actually like to remove SHCFG as a used field. If the encoding
> was less whacky (i.e. if 0b00 always meant "use incoming"), then it would
> be easy, but it shouldn't be too hard to work around that.

But why?

You throw away the entire logic of the design, go back to subtly
coupling the two parts, and *for what*? Exactly what are we trying to
achieve in return? You haven't explained why we are still discussing
this afer 7 months. It really isn't worthwhile.

Jason

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