Re: [PATCH v5 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master
From: Will Deacon <will@kernel.org>
Date: 2023-08-09 13:50:34
Also in:
linux-iommu, lkml
On Wed, Aug 09, 2023 at 01:12:02AM +0800, Michael Shavit wrote:
quoted hunk ↗ jump to hunk
@@ -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().
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
+ * (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.
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel