Messages in this thread | | | Date | Thu, 27 Jul 2023 14:37:45 -0700 | From | Nicolin Chen <> | 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
| |