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 03:52:07PM +0000, Mostafa Saleh wrote:

> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index ab415e107054c1..6b4f1a664288db 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1661,8 +1661,8 @@ static int arm_smmu_init_l2_strtab(struct arm_smmu_device *smmu, u32 sid)
> >  	if (desc->l2ptr)
> >  		return 0;
> >  
> > -	size = 1 << (STRTAB_SPLIT + ilog2(STRTAB_STE_DWORDS) + 3);
> > -	strtab = &cfg->strtab[(sid >> STRTAB_SPLIT) * STRTAB_L1_DESC_DWORDS];
> 
> I believe also STRTAB_L1_DESC_DWORDS isn’t needed as l1_desc has the same size.
> Especially I already see sizeof(*cfg->l1_desc) used in some places instead of the macro.
> So we can remove it also as STRTAB_STE_DWORDS.

I didn't want to do this without another type, but sure, it is not
hard. I ended up with:

struct arm_smmu_ste {
	__le64 data[STRTAB_STE_DWORDS];
};

struct arm_smmu_strtab_l2 {
	struct arm_smmu_ste stes[1 << STRTAB_SPLIT];
};

struct arm_smmu_strtab_l1 {
	__le64 l2ptr;
};
#define STRTAB_MAX_L1_ENTRIES (1 << 17)

struct arm_smmu_strtab_cfg {
       union {
	       struct {
		       struct arm_smmu_ste *table;
		       dma_addr_t ste_dma;
		       unsigned int num_ents;
	       } linear;
	       struct {
		       struct arm_smmu_strtab_l1 *l1tab;
		       struct arm_smmu_strtab_l2 **l2ptrs;
		       dma_addr_t l1_dma;
		       unsigned int num_l1_ents;
	       } l2;
       };
       u64				strtab_base;
       u32				strtab_base_cfg;
};

And we drop STRTAB_STE_DWORDS and STRTAB_L1_SZ_SHIFT.

> > @@ -3285,7 +3282,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
> >  	u32 size;
> >  	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
> >  
> > -	size = (1 << smmu->sid_bits) * (STRTAB_STE_DWORDS << 3);
> > +	size = (1 << smmu->sid_bits) * sizeof(cfg->strtab.linear[0]);
> 
> nit: maybe be consistent with "sizeof(struct arm_smmu_ste)" which was used earlier
> this patch and "sizeof(cfg->strtab.linear[0])"

I went with the struct version in all cases

Thanks,
Jason