[v1,2/7] iommu/arm-smmu-v3: Replace s1_cfg with ctx_desc_cfg
Commit Message
Remove or move s1_cfg fields that are redundant with those found in
arm_smmu_ctx_desc_cfg. The arm_smmu_ctx_desc_cfg member is named
cd_table to make it more obvious that it represents a cd table.
Signed-off-by: Michael Shavit <mshavit@google.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 45 +++++++++++----------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 ++---
2 files changed, 26 insertions(+), 29 deletions(-)
Comments
On Fri, Jul 28, 2023 at 02:26:18AM +0800, Michael Shavit wrote:
> Remove or move s1_cfg fields that are redundant with those found in
> arm_smmu_ctx_desc_cfg. The arm_smmu_ctx_desc_cfg member is named
> cd_table to make it more obvious that it represents a cd table.
Though the "cd_table" is clear, it doesn't feel very obvious to me
that "struct arm_smmu_ctx_desc_cfg" means CD table, so a mismatch
with "cd_table". How about renaming to "struct arm_smmu_cdtab_cfg",
similar to "struct arm_smmu_strtab_cfg"?
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 45 +++++++++++----------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 ++---
> 2 files changed, 26 insertions(+), 29 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 bb277ff86f65f..8cf4987dd9ec7 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1033,9 +1033,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
> unsigned int idx;
> struct arm_smmu_l1_ctx_desc *l1_desc;
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> - struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->s1_cfg.cdcfg;
> + struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
[<<<]
> @@ -1276,7 +1273,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
> u64 val = le64_to_cpu(dst[0]);
> bool ste_live = false;
> struct arm_smmu_device *smmu = NULL;
> - struct arm_smmu_s1_cfg *s1_cfg = NULL;
> + struct arm_smmu_ctx_desc_cfg *cd_table = NULL;
[>>>]
It'd be nicer to align all the variables to "cd_table" like the
2nd piece here. And if we rename the struct name too:
struct arm_smmu_cdtab_cfg *cd_table = xxxx;
> -struct arm_smmu_s1_cfg {
> - struct arm_smmu_ctx_desc_cfg cdcfg;
> - u8 s1fmt;
> - u8 s1cdmax;
> + /* log2 of the maximum number of CDs supported by this table */
> + u8 max_cds_bits;
Though "s1fmt" is redundant, "max_cds_bits" doesn't seem to be.
It'd be nicer to separate them in the commit message to why we
remove s1fmt and why we rename s1cdmax.
Thanks
Nicolin
On Fri, Jul 28, 2023 at 4:57 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> It'd be nicer to align all the variables to "cd_table" like the
> 2nd piece here. And if we rename the struct name too:
>
> struct arm_smmu_cdtab_cfg *cd_table = xxxx;
I agree that renaming these would be nice. There's 36 usages of cdcfg
in arm-smmu-v3.c, and 6 usages of arm_smmu_ctx_desc_cfg.
I can rename the struct since we'll be touching many of those in this
patch anyways, but I'm a bit concerned of the churn from updating all
the cdcfg usages.
> Though "s1fmt" is redundant, "max_cds_bits" doesn't seem to be.
>
> It'd be nicer to separate them in the commit message to why we
> remove s1fmt and why we rename s1cdmax.
Will fix!
On Fri, Jul 28, 2023 at 03:47:45PM +0800, Michael Shavit wrote:
> On Fri, Jul 28, 2023 at 4:57 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> > It'd be nicer to align all the variables to "cd_table" like the
> > 2nd piece here. And if we rename the struct name too:
> >
> > struct arm_smmu_cdtab_cfg *cd_table = xxxx;
>
> I agree that renaming these would be nice. There's 36 usages of cdcfg
> in arm-smmu-v3.c, and 6 usages of arm_smmu_ctx_desc_cfg.
> I can rename the struct since we'll be touching many of those in this
> patch anyways, but I'm a bit concerned of the churn from updating all
> the cdcfg usages.
Will was not keen on churn for clarity so it seem better to be
thoughtful about what is touched to get this merged.
Jason
On Fri, Jul 28, 2023 at 09:22:52AM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 28, 2023 at 03:47:45PM +0800, Michael Shavit wrote:
> > On Fri, Jul 28, 2023 at 4:57 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> >
> > > It'd be nicer to align all the variables to "cd_table" like the
> > > 2nd piece here. And if we rename the struct name too:
> > >
> > > struct arm_smmu_cdtab_cfg *cd_table = xxxx;
> >
> > I agree that renaming these would be nice. There's 36 usages of cdcfg
> > in arm-smmu-v3.c, and 6 usages of arm_smmu_ctx_desc_cfg.
> > I can rename the struct since we'll be touching many of those in this
> > patch anyways, but I'm a bit concerned of the churn from updating all
> > the cdcfg usages.
>
> Will was not keen on churn for clarity so it seem better to be
> thoughtful about what is touched to get this merged.
Still, it would be odd to have "cdcfg" and "cd_table" at the same
time. If we have to be conservative, perhaps we should just align
with the old naming: "struct arm_smmu_ctx_desc_cfg *cdcfg;"...
Nicolin
On Fri, Jul 28, 2023 at 11:41:26AM -0700, Nicolin Chen wrote:
> On Fri, Jul 28, 2023 at 09:22:52AM -0300, Jason Gunthorpe wrote:
> > On Fri, Jul 28, 2023 at 03:47:45PM +0800, Michael Shavit wrote:
> > > On Fri, Jul 28, 2023 at 4:57 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > >
> > > > It'd be nicer to align all the variables to "cd_table" like the
> > > > 2nd piece here. And if we rename the struct name too:
> > > >
> > > > struct arm_smmu_cdtab_cfg *cd_table = xxxx;
> > >
> > > I agree that renaming these would be nice. There's 36 usages of cdcfg
> > > in arm-smmu-v3.c, and 6 usages of arm_smmu_ctx_desc_cfg.
> > > I can rename the struct since we'll be touching many of those in this
> > > patch anyways, but I'm a bit concerned of the churn from updating all
> > > the cdcfg usages.
> >
> > Will was not keen on churn for clarity so it seem better to be
> > thoughtful about what is touched to get this merged.
>
> Still, it would be odd to have "cdcfg" and "cd_table" at the same
> time. If we have to be conservative, perhaps we should just align
> with the old naming: "struct arm_smmu_ctx_desc_cfg *cdcfg;"...
Yeah, I think changing to cd_table in the places touched makes alot of
sense
Jason
On Sat, Jul 29, 2023 at 2:54 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > Still, it would be odd to have "cdcfg" and "cd_table" at the same
> > time. If we have to be conservative, perhaps we should just align
> > with the old naming: "struct arm_smmu_ctx_desc_cfg *cdcfg;"...
>
> Yeah, I think changing to cd_table in the places touched makes alot of
> sense
A bit confused by the "Yeah" reply given the quote... Are we ok
keeping the v1 version of this patch w.r.t. to cd_table/cdcfg and
struct naming?
On Sun, Jul 30, 2023 at 07:24:28PM +0800, Michael Shavit wrote:
> On Sat, Jul 29, 2023 at 2:54 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > Still, it would be odd to have "cdcfg" and "cd_table" at the same
> > > time. If we have to be conservative, perhaps we should just align
> > > with the old naming: "struct arm_smmu_ctx_desc_cfg *cdcfg;"...
> >
> > Yeah, I think changing to cd_table in the places touched makes alot of
> > sense
>
> A bit confused by the "Yeah" reply given the quote... Are we ok
> keeping the v1 version of this patch w.r.t. to cd_table/cdcfg and
> struct naming?
I think you should optimistically use the name "cd_table" in any place
you touch. Leave cdcfg as is if you don't touch it. Consider a final
patch at the end to fix the cdcfgs, and see if Willy agrees.
Jason
@@ -1033,9 +1033,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
unsigned int idx;
struct arm_smmu_l1_ctx_desc *l1_desc;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->s1_cfg.cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
- if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
+ if (!cdcfg->l1_desc)
return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
idx = ssid >> CTXDESC_SPLIT;
@@ -1071,7 +1071,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
bool cd_live;
__le64 *cdptr;
- if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax)))
+ if (WARN_ON(ssid >= (1 << smmu_domain->cd_table.max_cds_bits)))
return -E2BIG;
cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
@@ -1138,19 +1138,16 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
size_t l1size;
size_t max_contexts;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &cfg->cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
- max_contexts = 1 << cfg->s1cdmax;
+ max_contexts = 1 << cdcfg->max_cds_bits;
if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
max_contexts <= CTXDESC_L2_ENTRIES) {
- cfg->s1fmt = STRTAB_STE_0_S1FMT_LINEAR;
cdcfg->num_l1_ents = max_contexts;
l1size = max_contexts * (CTXDESC_CD_DWORDS << 3);
} else {
- cfg->s1fmt = STRTAB_STE_0_S1FMT_64K_L2;
cdcfg->num_l1_ents = DIV_ROUND_UP(max_contexts,
CTXDESC_L2_ENTRIES);
@@ -1186,7 +1183,7 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_domain *smmu_domain)
int i;
size_t size, l1size;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->s1_cfg.cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
if (cdcfg->l1_desc) {
size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
@@ -1276,7 +1273,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
u64 val = le64_to_cpu(dst[0]);
bool ste_live = false;
struct arm_smmu_device *smmu = NULL;
- struct arm_smmu_s1_cfg *s1_cfg = NULL;
+ struct arm_smmu_ctx_desc_cfg *cd_table = NULL;
struct arm_smmu_s2_cfg *s2_cfg = NULL;
struct arm_smmu_domain *smmu_domain = NULL;
struct arm_smmu_cmdq_ent prefetch_cmd = {
@@ -1294,7 +1291,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
if (smmu_domain) {
switch (smmu_domain->stage) {
case ARM_SMMU_DOMAIN_S1:
- s1_cfg = &smmu_domain->s1_cfg;
+ cd_table = &smmu_domain->cd_table;
break;
case ARM_SMMU_DOMAIN_S2:
case ARM_SMMU_DOMAIN_NESTED:
@@ -1325,7 +1322,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
val = STRTAB_STE_0_V;
/* Bypass/fault */
- if (!smmu_domain || !(s1_cfg || s2_cfg)) {
+ if (!smmu_domain || !(cd_table || s2_cfg)) {
if (!smmu_domain && disable_bypass)
val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
else
@@ -1344,7 +1341,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
return;
}
- if (s1_cfg) {
+ if (cd_table) {
u64 strw = smmu->features & ARM_SMMU_FEAT_E2H ?
STRTAB_STE_1_STRW_EL2 : STRTAB_STE_1_STRW_NSEL1;
@@ -1360,10 +1357,14 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
!master->stall_enabled)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);
- val |= (s1_cfg->cdcfg.cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) |
- FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S1_TRANS) |
- FIELD_PREP(STRTAB_STE_0_S1CDMAX, s1_cfg->s1cdmax) |
- FIELD_PREP(STRTAB_STE_0_S1FMT, s1_cfg->s1fmt);
+ val |= (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) |
+ FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S1_TRANS) |
+ FIELD_PREP(STRTAB_STE_0_S1CDMAX,
+ cd_table->max_cds_bits) |
+ FIELD_PREP(STRTAB_STE_0_S1FMT,
+ cd_table->l1_desc ?
+ STRTAB_STE_0_S1FMT_64K_L2 :
+ STRTAB_STE_0_S1FMT_LINEAR);
}
if (s2_cfg) {
@@ -2082,11 +2083,11 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
/* Free the CD and ASID, if we allocated them */
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
/* Prevent SVA from touching the CD while we're freeing it */
mutex_lock(&arm_smmu_asid_lock);
- if (cfg->cdcfg.cdtab)
+ if (cdcfg->cdtab)
arm_smmu_free_cd_tables(smmu_domain);
arm_smmu_free_asid(&smmu_domain->cd);
mutex_unlock(&arm_smmu_asid_lock);
@@ -2106,7 +2107,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
int ret;
u32 asid;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;
@@ -2119,7 +2120,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
if (ret)
goto out_unlock;
- cfg->s1cdmax = master->ssid_bits;
+ cdcfg->max_cds_bits = master->ssid_bits;
smmu_domain->stall_enabled = master->stall_enabled;
@@ -2457,7 +2458,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
ret = -EINVAL;
goto out_unlock;
} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
- master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
+ master->ssid_bits != smmu_domain->cd_table.max_cds_bits) {
ret = -EINVAL;
goto out_unlock;
} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
@@ -595,12 +595,8 @@ struct arm_smmu_ctx_desc_cfg {
dma_addr_t cdtab_dma;
struct arm_smmu_l1_ctx_desc *l1_desc;
unsigned int num_l1_ents;
-};
-
-struct arm_smmu_s1_cfg {
- struct arm_smmu_ctx_desc_cfg cdcfg;
- u8 s1fmt;
- u8 s1cdmax;
+ /* log2 of the maximum number of CDs supported by this table */
+ u8 max_cds_bits;
};
struct arm_smmu_s2_cfg {
@@ -725,7 +721,7 @@ struct arm_smmu_domain {
union {
struct {
struct arm_smmu_ctx_desc cd;
- struct arm_smmu_s1_cfg s1_cfg;
+ struct arm_smmu_ctx_desc_cfg cd_table;
};
struct arm_smmu_s2_cfg s2_cfg;
};