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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMLaM9QjHDu11iKf@Asurada-Nvidia>
Date:   Thu, 27 Jul 2023 13:57:23 -0700
From:   Nicolin Chen <nicolinc@...dia.com>
To:     Michael Shavit <mshavit@...gle.com>
CC:     Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
        "Joerg Roedel" <joro@...tes.org>, <jean-philippe@...aro.org>,
        <jgg@...dia.com>, <baolu.lu@...ux.intel.com>,
        <linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/7] iommu/arm-smmu-v3: Replace s1_cfg with
 ctx_desc_cfg

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