Messages in this thread | | | Date | Wed, 9 Aug 2023 14:50:25 +0100 | From | Will Deacon <> | Subject | Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master |
| |
On Wed, Aug 09, 2023 at 01:12:02AM +0800, Michael Shavit wrote: > @@ -2203,7 +2186,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, > ias = min_t(unsigned long, ias, VA_BITS); > oas = smmu->ias; > fmt = ARM_64_LPAE_S1; > - finalise_stage_fn = arm_smmu_domain_finalise_s1; > + finalise_stage_fn = arm_smmu_domain_finalise_cd;
Why is this a better name? Now we have inconsistency with arm_smmu_domain_finalise_s2().
> break; > case ARM_SMMU_DOMAIN_NESTED: > case ARM_SMMU_DOMAIN_S2: > @@ -2402,6 +2385,16 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master) > master->domain = NULL; > master->ats_enabled = false; > arm_smmu_install_ste_for_dev(master); > + /* > + * The table is uninstalled before clearing the CD to prevent an > + * unnecessary sync in arm_smmu_write_ctx_desc. Although clearing the > + * CD entry isn't strictly required to detach the domain since the > + * table is uninstalled anyway, it's more proper and helps avoid > + * confusion in the call to arm_smmu_write_ctx_desc on the next attach
You can remove the "it's more proper" part.
> + * (which expects the entry to be empty). > + */ > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->cd_table.cdtab) > + arm_smmu_write_ctx_desc(master, 0, NULL); > } > > static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > @@ -2436,22 +2429,14 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (!smmu_domain->smmu) { > smmu_domain->smmu = smmu; > ret = arm_smmu_domain_finalise(domain, master); > - if (ret) { > + if (ret) > smmu_domain->smmu = NULL; > - goto out_unlock; > - } > - } else if (smmu_domain->smmu != smmu) { > - ret = -EINVAL; > - goto out_unlock; > - } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > - master->ssid_bits != smmu_domain->cd_table.max_cds_bits) { > + } else if (smmu_domain->smmu != smmu) > ret = -EINVAL; > - goto out_unlock; > - } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && > - smmu_domain->cd_table.stall_enabled != master->stall_enabled) { > - ret = -EINVAL; > - goto out_unlock; > - }
Removing these checks on the domain is pretty nice.
> @@ -2465,6 +2450,22 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (smmu_domain->stage != ARM_SMMU_DOMAIN_BYPASS) > master->ats_enabled = arm_smmu_ats_supported(master); > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + if (!master->cd_table.cdtab) { > + ret = arm_smmu_alloc_cd_tables(master); > + if (ret) { > + master->domain = NULL; > + return ret; > + } > + } > + > + ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd); > + if (ret) { > + master->domain = NULL; > + return ret;
Can you leak the cd tables here if you just allocated them?
> @@ -2472,10 +2473,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > arm_smmu_enable_ats(master); > - > -out_unlock: > - mutex_unlock(&smmu_domain->init_mutex); > - return ret; > + return 0; > } > > static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova, > @@ -2719,6 +2717,8 @@ static void arm_smmu_release_device(struct device *dev) > arm_smmu_detach_dev(master); > arm_smmu_disable_pasid(master); > arm_smmu_remove_master(master); > + if (master->cd_table.cdtab_dma)
Why are you checking 'cdtab_dma' here instead of just 'cdtab'?
Will
| |