linux-kernel - Re: [PATCH v1 6/7] iommu/arm-smmu-v3: Refactor write_ctx

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: <ZMLjqfp6M6n7HAxl@Asurada-Nvidia>
Date:   Thu, 27 Jul 2023 14:37:45 -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 6/7] iommu/arm-smmu-v3: Refactor write_ctx_desc

On Fri, Jul 28, 2023 at 02:26:22AM +0800, Michael Shavit wrote:

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 968559d625c40..57073d278cd7e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -45,10 +45,12 @@ static struct arm_smmu_ctx_desc *
>  arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
>  {
>         int ret;
> +       unsigned long flags;
>         u32 new_asid;
>         struct arm_smmu_ctx_desc *cd;
>         struct arm_smmu_device *smmu;
>         struct arm_smmu_domain *smmu_domain;
> +       struct arm_smmu_master *master;

It seems that the coding style at these struct lines is listing
from shorter to longer, like a Christmas tree? If so, we should
place "master" before "smmu_domain".

> @@ -80,7 +82,11 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
>          * be some overlap between use of both ASIDs, until we invalidate the
>          * TLB.
>          */
> -       arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
> +       spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> +       list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +               arm_smmu_write_ctx_desc(master, 0, cd);
> +       }

+	list_for_each_entry(master, &smmu_domain->devices, domain_head)
+		arm_smmu_write_ctx_desc(master, 0, cd);

> @@ -248,8 +260,10 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
>                           struct mm_struct *mm)
>  {
>         int ret;
> +       unsigned long flags;
>         struct arm_smmu_ctx_desc *cd;
>         struct arm_smmu_mmu_notifier *smmu_mn;
> +       struct arm_smmu_master *master;

For the coding style topic, similarly, "master" before "smmu_mn".

> 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 af7949b62327b..b211424a85fb2 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -971,14 +971,12 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
>         arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
>  }
> 
> -static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
> +static void arm_smmu_sync_cd(struct arm_smmu_master *master,
>                              int ssid, bool leaf)
>  {
>         size_t i;
> -       unsigned long flags;
> -       struct arm_smmu_master *master;
>         struct arm_smmu_cmdq_batch cmds;
> -       struct arm_smmu_device *smmu = smmu_domain->smmu;
> +       struct arm_smmu_device *smmu;

	struct arm_smmu_device *smmu = master->smmu;

Then ...

> @@ -987,19 +985,15 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
>                 },
>         };
> 
> -       if (!smmu_domain->cd_table.installed)
> +       if (!master->domain->cd_table.installed)
>                 return;
> 
> +       smmu = master->smmu;

... no need of this line.

> @@ -1029,14 +1023,12 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst,
>         WRITE_ONCE(*dst, cpu_to_le64(val));
>  }
> 
> -static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
> -                                  u32 ssid)
> +static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
>  {
>         __le64 *l1ptr;
>         unsigned int idx;
>         struct arm_smmu_l1_ctx_desc *l1_desc;
> -       struct arm_smmu_device *smmu = smmu_domain->smmu;

	struct arm_smmu_device *smmu = master->smmu;

Then ...

> @@ -1044,19 +1036,19 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
>         idx = ssid >> CTXDESC_SPLIT;
>         l1_desc = &cdcfg->l1_desc[idx];
>         if (!l1_desc->l2ptr) {
> -               if (arm_smmu_alloc_cd_leaf_table(smmu, l1_desc))
> +               if (arm_smmu_alloc_cd_leaf_table(master->smmu, l1_desc))

... no need to change this.

> @@ -1101,11 +1094,11 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
>                 cdptr[3] = cpu_to_le64(cd->mair);
> 
>                 /*
> -                * STE is live, and the SMMU might read dwords of this CD in any
> -                * order. Ensure that it observes valid values before reading
> -                * V=1.
> +                * STE may be live, and the SMMU might read dwords of this CD
> +                * in any order. Ensure that it observes valid values before
> +                * reading V=1.

This seems to be true only after the following patch? If so, we
should move this part over there too.

Thanks
Nicolin