| Message ID | 1-v1-1b720dce51d1+4f44-smmuv3_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tidy some minor things in the stream table/cd table area | expand |
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()... The "struct arm_smmu_strtab_l1_desc" seems to be only used at one place in arm_smmu_init_l2_strtab(). So, how about: struct arm_smmu_strtab_cfg { union { struct arm_smmu_ste *linear; __le64 *l1_desc; } strtab; dma_addr_t strtab_dma; struct arm_smmu_ste **l2ptrs; Thanks Nicolin
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 ? > 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]; }; Jason
Hi Jason, On Mon, Jun 03, 2024 at 07:31:27PM -0300, Jason Gunthorpe wrote: > This is being used as both an array of STEs and an array of L1 > descriptors. > > Give the two usages different names and correct types. > > Remove STRTAB_STE_DWORDS as most usages were indexing or sizing an array > of struct arm_smmu_ste. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 +++++++++------------ > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++---- > 2 files changed, 14 insertions(+), 16 deletions(-) > > 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. > + size = (1 << STRTAB_SPLIT) * sizeof(struct arm_smmu_ste); > + strtab = &cfg->strtab.l1_desc[sid >> STRTAB_SPLIT]; > > desc->span = STRTAB_SPLIT + 1; > desc->l2ptr = dmam_alloc_coherent(smmu->dev, size, &desc->l2ptr_dma, > @@ -2409,8 +2409,7 @@ arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid) > return &cfg->l1_desc[idx1].l2ptr[idx2]; > } else { > /* Simple linear lookup */ > - return (struct arm_smmu_ste *)&cfg > - ->strtab[sid * STRTAB_STE_DWORDS]; > + return &cfg->strtab.linear[sid]; > } > } > > @@ -3225,17 +3224,15 @@ static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu) > { > unsigned int i; > struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; > - void *strtab = smmu->strtab_cfg.strtab; > > cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, > sizeof(*cfg->l1_desc), GFP_KERNEL); > if (!cfg->l1_desc) > return -ENOMEM; > > - for (i = 0; i < cfg->num_l1_ents; ++i) { > - arm_smmu_write_strtab_l1_desc(strtab, &cfg->l1_desc[i]); > - strtab += STRTAB_L1_DESC_DWORDS << 3; > - } > + for (i = 0; i < cfg->num_l1_ents; ++i) > + arm_smmu_write_strtab_l1_desc( > + &smmu->strtab_cfg.strtab.l1_desc[i], &cfg->l1_desc[i]); > > return 0; > } > @@ -3267,7 +3264,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) > l1size); > return -ENOMEM; > } > - cfg->strtab = strtab; > + cfg->strtab.l1_desc = strtab; > > /* Configure strtab_base_cfg for 2 levels */ > reg = FIELD_PREP(STRTAB_BASE_CFG_FMT, STRTAB_BASE_CFG_FMT_2LVL); > @@ -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])" > strtab = dmam_alloc_coherent(smmu->dev, size, &cfg->strtab_dma, > GFP_KERNEL); > if (!strtab) { > @@ -3294,7 +3291,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) > size); > return -ENOMEM; > } > - cfg->strtab = strtab; > + cfg->strtab.linear = strtab; > cfg->num_l1_ents = 1 << smmu->sid_bits; > > /* Configure strtab_base_cfg for a linear table covering all SIDs */ > 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 > @@ -206,10 +206,8 @@ > #define STRTAB_L1_DESC_SPAN GENMASK_ULL(4, 0) > #define STRTAB_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 6) > > -#define STRTAB_STE_DWORDS 8 > - > struct arm_smmu_ste { > - __le64 data[STRTAB_STE_DWORDS]; > + __le64 data[8]; > }; > > #define STRTAB_STE_0_V (1UL << 0) > @@ -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; I agree with Nicolin, that it is confusing to have both l1_desc, I guess a rename is sufficient. > + } strtab; > dma_addr_t strtab_dma; > struct arm_smmu_strtab_l1_desc *l1_desc; > unsigned int num_l1_ents; > -- > 2.45.2 > Thanks, Mostafa
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
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
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
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
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]; + size = (1 << STRTAB_SPLIT) * sizeof(struct arm_smmu_ste); + strtab = &cfg->strtab.l1_desc[sid >> STRTAB_SPLIT]; desc->span = STRTAB_SPLIT + 1; desc->l2ptr = dmam_alloc_coherent(smmu->dev, size, &desc->l2ptr_dma, @@ -2409,8 +2409,7 @@ arm_smmu_get_step_for_sid(struct arm_smmu_device *smmu, u32 sid) return &cfg->l1_desc[idx1].l2ptr[idx2]; } else { /* Simple linear lookup */ - return (struct arm_smmu_ste *)&cfg - ->strtab[sid * STRTAB_STE_DWORDS]; + return &cfg->strtab.linear[sid]; } } @@ -3225,17 +3224,15 @@ static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu) { unsigned int i; struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; - void *strtab = smmu->strtab_cfg.strtab; cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, sizeof(*cfg->l1_desc), GFP_KERNEL); if (!cfg->l1_desc) return -ENOMEM; - for (i = 0; i < cfg->num_l1_ents; ++i) { - arm_smmu_write_strtab_l1_desc(strtab, &cfg->l1_desc[i]); - strtab += STRTAB_L1_DESC_DWORDS << 3; - } + for (i = 0; i < cfg->num_l1_ents; ++i) + arm_smmu_write_strtab_l1_desc( + &smmu->strtab_cfg.strtab.l1_desc[i], &cfg->l1_desc[i]); return 0; } @@ -3267,7 +3264,7 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) l1size); return -ENOMEM; } - cfg->strtab = strtab; + cfg->strtab.l1_desc = strtab; /* Configure strtab_base_cfg for 2 levels */ reg = FIELD_PREP(STRTAB_BASE_CFG_FMT, STRTAB_BASE_CFG_FMT_2LVL); @@ -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]); strtab = dmam_alloc_coherent(smmu->dev, size, &cfg->strtab_dma, GFP_KERNEL); if (!strtab) { @@ -3294,7 +3291,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) size); return -ENOMEM; } - cfg->strtab = strtab; + cfg->strtab.linear = strtab; cfg->num_l1_ents = 1 << smmu->sid_bits; /* Configure strtab_base_cfg for a linear table covering all SIDs */ 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 @@ -206,10 +206,8 @@ #define STRTAB_L1_DESC_SPAN GENMASK_ULL(4, 0) #define STRTAB_L1_DESC_L2PTR_MASK GENMASK_ULL(51, 6) -#define STRTAB_STE_DWORDS 8 - struct arm_smmu_ste { - __le64 data[STRTAB_STE_DWORDS]; + __le64 data[8]; }; #define STRTAB_STE_0_V (1UL << 0) @@ -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;
This is being used as both an array of STEs and an array of L1 descriptors. Give the two usages different names and correct types. Remove STRTAB_STE_DWORDS as most usages were indexing or sizing an array of struct arm_smmu_ste. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 +++++++++------------ drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++---- 2 files changed, 14 insertions(+), 16 deletions(-)