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 11:28:05 -0700	[thread overview]
Message-ID: <Zl9ctWSM6m3gkH5T@Asurada-Nvidia> (raw)
In-Reply-To: <20240604125955.GH19897@nvidia.com>

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:
> > 
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > index 1242a086c9f948..4769780259affc 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > > @@ -612,7 +610,10 @@ struct arm_smmu_s2_cfg {
> > >  };
> > >  
> > >  struct arm_smmu_strtab_cfg {
> > > -	__le64				*strtab;
> > > +	union {
> > > +		struct arm_smmu_ste *linear;
> > > +		__le64 *l1_desc;
> > > +	} strtab;
> > >  	dma_addr_t			strtab_dma;
> > >  	struct arm_smmu_strtab_l1_desc	*l1_desc;
> > >  	unsigned int			num_l1_ents;
> > 
> > It looks like we have two "l1_desc" ptrs now in the same struct:
> > 	strtab.l1_desc	// raw level-1 descriptor memory
> > 	l1_desc		// SW array to store level-2 descriptor memory
> > 
> > And it gets a bit more confusing that they even use the same error
> > prints in arm_smmu_init_strtab_2lvl()...
> 
> Yeah, I noticed that too, but failed to come with better names.. The
> CD has the same issue
> 
> strtab.l1_desc is a pointer to the data structure that the HW fetches
> that is the first level of a 2 level strtab, it stores an encoded
> dma_addr_t.
> 
> cfg.l1_desc is an array of CPU information for each HW L1 entry,
> eventually just being the CPU pointer to the L2 STE table.
> 
> So they are both the l1 array, just one is a CPU pointer and one is a
> HW/DMA pointer.
> 
> Let's call strtab.l1_desc --> strtab.l1_table ?

Yea. This seems to be good.

> > 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?

I was also thinking of an alternative by separating linear/2lvl:

struct arm_smmu_ste {
	__le64 data[8];
};

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

struct arm_smmu_strtab_l1_desc { // so as to drop TRTAB_L1_DESC_DWORDS
	__le64 data;
};

struct arm_smmu_strtab_l2_stes {
	struct arm_smmu_ste *ste;
};

struct arm_smmu_strtab_l1 {
	struct arm_smmu_strtab_l1_desc *l1;
	dma_addr_t l1_dma;
	struct arm_smmu_strtab_l2_stes *l2;
};

struct arm_smmu_device {
	...
	union {
		struct arm_smmu_strtab_linear linear;
		struct arm_smmu_strtab_l1 l1;
	} strtab;
	...
};

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?

Thanks
Nicolin

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