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

[Jason Gunthorpe <jgg@nvidia.com>]

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

#define STRTAB_SPLIT			8
 
> 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;

num_l1_ents too

> 	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;
> 	...
> };

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.

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

Jason