Re: [PATCH v1 2/7] iommu/arm-smmu-v3: Replace s1_cfg with ctx_desc_cfg

From: Nicolin Chen <nicolinc@nvidia.com>
To: Michael Shavit <mshavit@google.com>
Cc: Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	"Joerg Roedel" <joro@8bytes.org>, <jean-philippe@linaro.org>,
	<jgg@nvidia.com>, <baolu.lu@linux.intel.com>,
	<linux-arm-kernel@lists.infradead.org>, <iommu@lists.linux.dev>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/7] iommu/arm-smmu-v3: Replace s1_cfg with ctx_desc_cfg
Date: Thu, 27 Jul 2023 13:57:23 -0700	[thread overview]
Message-ID: <ZMLaM9QjHDu11iKf@Asurada-Nvidia> (raw)
In-Reply-To: <20230727182647.4106140-3-mshavit@google.com>

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