Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

From: Jason Gunthorpe <jgg@nvidia.com>
To: Michael Shavit <mshavit@google.com>
Cc: Nicolin Chen <nicolinc@nvidia.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Joerg Roedel <joro@8bytes.org>,
	jean-philippe@linaro.org, baolu.lu@linux.intel.com,
	linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master
Date: Mon, 17 Jul 2023 09:29:21 -0300	[thread overview]
Message-ID: <ZLU0IZJr6WSjknHf@nvidia.com> (raw)
In-Reply-To: <CAKHBV24zQ+4waZgsYV08LzeMkjze1gTcwvEv5uL8RM1GcBgrzg@mail.gmail.com>

On Mon, Jul 17, 2023 at 06:06:19PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:21 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > patch 2 should delete arm_smmu_s1_cfg and just put
> > arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
> > a weird name for the contex descriptor table, but it is much less
> > weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.
> 
> s1fmt is fairly trivial to replace but s1cdmax requires inversing
> previous computations. I don't really buy that getting rid of it
> simplifies anything, even if it's technically redundant.

Then store s1cdmax in the arm_smmu_ctx_desc_cfg and still get rid of
arm_smmu_s1_cfg

The point is to get rid of "s1_cfg" entirely as language in the driver
*because it makes zero sense now*. Prior to your restructuring it was
sort of the STE to use for the S1 page table which had an embedded
CD. After all this change it isn't realated to a S1 page table anymore
at all.

> > patch 3 I don't understand, we should not add something called
> > s1_cfg/s2_cfg to the master. The master should have
> > 'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'
> 
> This was simply meant to be a more convenient way of finding the
> currently active cdtable from the

There is only one (meaningful) choice though, the cdtable is either
the master's default CD table or it isn't. You detect that by checking
for NULL ste_domain

> > My suggestion for patch structure is to start by cleaning up the CD
> > table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
> > redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
> > 'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
> > and then as the last step just move the arm_smmu_ctx_desc_cfg from the
> > iommu_domain to the master.
> >
> > And that is a nice little series on its own - you end up with a shared
> > CD table in the master, and no CD table in any domains.
> 
> I don't entirely buy that refactoring s1_cfg is worth the extra
> effort, nor that it should be tied to this patch series. This series
> already makes s1_cfg behave as the CD table; whether we want to
> entirely get rid of pre-computed data useful for writing an STE sounds
> like a separate cleanup.

s1_cfg is a terrible name to describe the cd table. Please do the
tiny bit extra to get rid of it for clarity. 

Jason

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