[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230809135024.GD4226@willie-the-truck>
Date: Wed, 9 Aug 2023 14:50:25 +0100
From: Will Deacon <will@...nel.org>
To: Michael Shavit <mshavit@...gle.com>
Cc: iommu@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, robin.murphy@....com,
nicolinc@...dia.com, jgg@...dia.com, jean-philippe@...aro.org
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
Powered by blists - more mailing lists