Re: [PATCH 1/7] iommu/arm-smmu-v3: Split struct arm_smmu_strtab_cfg.strtab

From: Nicolin Chen <nicolinc@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: <iommu@lists.linux.dev>, Joerg Roedel <joro@8bytes.org>,
	<linux-arm-kernel@lists.infradead.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>,
	Michael Shavit <mshavit@google.com>, <patches@lists.linux.dev>,
	Ryan Roberts <ryan.roberts@arm.com>,
	Mostafa Saleh <smostafa@google.com>
Subject: Re: [PATCH 1/7] iommu/arm-smmu-v3: Split struct arm_smmu_strtab_cfg.strtab
Date: Tue, 4 Jun 2024 12:28:11 -0700	[thread overview]
Message-ID: <Zl9qy9XA7Wivk3/J@Asurada-Nvidia> (raw)
In-Reply-To: <20240604190247.GP19897@nvidia.com>

On Tue, Jun 04, 2024 at 04:02:47PM -0300, Jason Gunthorpe wrote:
> On Tue, Jun 04, 2024 at 11:28:05AM -0700, Nicolin Chen wrote:
> > On Tue, Jun 04, 2024 at 09:59:55AM -0300, Jason Gunthorpe wrote:
> > > On Tue, Jun 04, 2024 at 01:32:20AM -0700, Nicolin Chen wrote:
> > > > On Mon, Jun 03, 2024 at 07:31:27PM -0300, Jason Gunthorpe wrote:
> > > > The "struct arm_smmu_strtab_l1_desc" seems to be only used at one
> > > > place in arm_smmu_init_l2_strtab(). So, how about:
> > > 
> > > I didn't do it but, it would make some of the maths more obvious 
> > > if we encoded the table structure in the types:
> > > 
> > > struct arm_smmu_strtab_l2_stes {
> > > 	struct arm_smmu_ste l2[256];
> > > };
> > 
> > I personally prefer this one, though why 256?
> 
> #define STRTAB_SPLIT			8

Oh right! And similarly, "struct arm_smmu_cd l2[1024]".

> > struct arm_smmu_strtab_linear {
> > 	struct arm_smmu_ste *ste;
> > 	dma_addr_t ste_dma;
> > };

> > struct arm_smmu_strtab_l1 {
> > 	struct arm_smmu_strtab_l1_desc *l1;
> 
> num_l1_ents too

Yea, missed that.

> > struct arm_smmu_device {
> > 	...
> > 	union {
> > 		struct arm_smmu_strtab_linear linear;
> > 		struct arm_smmu_strtab_l1 l1;
> > 	} strtab;
> > 	...
> > };
> 
> Yes! That is quite readable and understandable! I was relucant to do
> much more than just the small change Will asked about, and even that
> expanded.. Let me see if I can reasonably squeeze that into a small
> number of patches.

Yea, I hesitated too at the beginning, until I saw Mostafa suggest
something further.

> > Only arm_smmu_device_reset() really needs strtab_base/_cfg values
> > that we could compute them over there, given that there are quite
> > amount of smmu->features checking already?
> 
> Certainly could do, but that seems to have less advantage..

Well, either way sounds good to me. I was just considering that we
already did something similar with s1/s2/cd cfg values. Yet, not a
problem to continue storing these two.

Thanks
Nicolin

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