With this change, each master will now own its own CD table instead of
sharing one with other masters attached to the same domain. Attaching a
stage 1 domain installs CD entries into the master's CD table. SVA
writes its CD entries into each master's CD table if the domain is
shared across masters.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Michael Shavit <mshavit@google.com>
---
Changes in v5:
- Clear the 0th CD entry when the domain is detached. Not clearing it
caused a bug in arm_smmu_write_ctx_desc which doesn't expect the entry
to already be set.
Changes in v4:
- Added comment about the cd_table's dependency on the iommu core's
group mutex.
- Narrowed the range of code for which the domain's init_mutex is held
on attach since it now only protects the arm_smmu_domain_finalise
call.
Changes in v2:
- Allocate CD table when it's first needed instead of on probe.
Changes in v1:
- The master's CD table allocation was previously split to a different
commit. This change now atomically allocates the new CD table, uses
it, and removes the old one.
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 92 ++++++++++-----------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 +-
2 files changed, 49 insertions(+), 50 deletions(-)
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 34bd7815aeb8e..3f32f9a191a5f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1025,7 +1025,7 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid)
unsigned int idx;
struct arm_smmu_l1_ctx_desc *l1_desc;
struct arm_smmu_device *smmu = master->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &master->domain->cd_table;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &master->cd_table;
if (!cdcfg->l1_desc)
return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
@@ -1062,7 +1062,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
u64 val;
bool cd_live;
__le64 *cdptr;
- struct arm_smmu_ctx_desc_cfg *cd_table = &master->domain->cd_table;
+ struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
if (WARN_ON(ssid >= (1 << cd_table->max_cds_bits)))
return -E2BIG;
@@ -1125,14 +1125,13 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_master *master, int ssid,
return 0;
}
-static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain,
- struct arm_smmu_master *master)
+static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master)
{
int ret;
size_t l1size;
size_t max_contexts;
struct arm_smmu_device *smmu = master->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &master->cd_table;
cdcfg->stall_enabled = master->stall_enabled;
cdcfg->max_cds_bits = master->ssid_bits;
@@ -1174,12 +1173,12 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain,
return ret;
}
-static void arm_smmu_free_cd_tables(struct arm_smmu_domain *smmu_domain)
+static void arm_smmu_free_cd_tables(struct arm_smmu_master *master)
{
int i;
size_t size, l1size;
- struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
+ struct arm_smmu_device *smmu = master->smmu;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &master->cd_table;
if (cdcfg->l1_desc) {
size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
@@ -1287,7 +1286,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
if (smmu_domain) {
switch (smmu_domain->stage) {
case ARM_SMMU_DOMAIN_S1:
- cd_table = &smmu_domain->cd_table;
+ cd_table = &master->cd_table;
break;
case ARM_SMMU_DOMAIN_S2:
case ARM_SMMU_DOMAIN_NESTED:
@@ -2077,14 +2076,10 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
free_io_pgtable_ops(smmu_domain->pgtbl_ops);
- /* Free the CD and ASID, if we allocated them */
+ /* Free the ASID or VMID */
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
- struct arm_smmu_ctx_desc_cfg *cd_table = &smmu_domain->cd_table;
-
/* Prevent SVA from touching the CD while we're freeing it */
mutex_lock(&arm_smmu_asid_lock);
- if (cd_table->cdtab)
- arm_smmu_free_cd_tables(smmu_domain);
arm_smmu_free_asid(&smmu_domain->cd);
mutex_unlock(&arm_smmu_asid_lock);
} else {
@@ -2096,7 +2091,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
kfree(smmu_domain);
}
-static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
+static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
struct arm_smmu_master *master,
struct io_pgtable_cfg *pgtbl_cfg)
{
@@ -2115,10 +2110,6 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
if (ret)
goto out_unlock;
- ret = arm_smmu_alloc_cd_tables(smmu_domain, master);
- if (ret)
- goto out_free_asid;
-
cd->asid = (u16)asid;
cd->ttbr = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
cd->tcr = FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
@@ -2130,17 +2121,9 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
cd->mair = pgtbl_cfg->arm_lpae_s1_cfg.mair;
- ret = arm_smmu_write_ctx_desc(master, 0, cd);
- if (ret)
- goto out_free_cd_tables;
-
mutex_unlock(&arm_smmu_asid_lock);
return 0;
-out_free_cd_tables:
- arm_smmu_free_cd_tables(smmu_domain);
-out_free_asid:
- arm_smmu_free_asid(cd);
out_unlock:
mutex_unlock(&arm_smmu_asid_lock);
return ret;
@@ -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;
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
+ * (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;
- }
+
+ mutex_unlock(&smmu_domain->init_mutex);
+ if (ret)
+ return ret;
master->domain = smmu_domain;
@@ -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;
+ }
+ }
+
arm_smmu_install_ste_for_dev(master);
spin_lock_irqsave(&smmu_domain->devices_lock, flags);
@@ -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)
+ arm_smmu_free_cd_tables(master);
kfree(master);
}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 6066a09c01996..1f3b370257779 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -694,6 +694,8 @@ struct arm_smmu_master {
struct arm_smmu_domain *domain;
struct list_head domain_head;
struct arm_smmu_stream *streams;
+ /* Locked by the iommu core using the group mutex */
+ struct arm_smmu_ctx_desc_cfg cd_table;
unsigned int num_streams;
bool ats_enabled;
bool stall_enabled;
@@ -720,11 +722,8 @@ struct arm_smmu_domain {
enum arm_smmu_domain_stage stage;
union {
- struct {
struct arm_smmu_ctx_desc cd;
- struct arm_smmu_ctx_desc_cfg cd_table;
- };
- struct arm_smmu_s2_cfg s2_cfg;
+ struct arm_smmu_s2_cfg s2_cfg;
};
struct iommu_domain domain;
--
2.41.0.640.ga95def55d0-googOn Wed, Aug 9, 2023 at 1:15 AM Michael Shavit <mshavit@google.com> wrote:
>
> -static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> +static int arm_smmu_domain_finalise_cd(struct arm_smmu_domain *smmu_domain,
> struct arm_smmu_master *master,
> struct io_pgtable_cfg *pgtbl_cfg)
> {
> @@ -2115,10 +2110,6 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> if (ret)
> goto out_unlock;
>
> - ret = arm_smmu_alloc_cd_tables(smmu_domain, master);
> - if (ret)
> - goto out_free_asid;
> -
> cd->asid = (u16)asid;
> cd->ttbr = pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
> cd->tcr = FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
> @@ -2130,17 +2121,9 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
> CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
> cd->mair = pgtbl_cfg->arm_lpae_s1_cfg.mair;
>
> - ret = arm_smmu_write_ctx_desc(master, 0, cd);
> - if (ret)
> - goto out_free_cd_tables;
> -
> mutex_unlock(&arm_smmu_asid_lock);
> return 0;
>
> -out_free_cd_tables:
> - arm_smmu_free_cd_tables(smmu_domain);
> -out_free_asid:
> - arm_smmu_free_asid(cd);
> out_unlock:
> mutex_unlock(&arm_smmu_asid_lock);
> return ret;
...
> @@ -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;
> + }
> + }
> +
> arm_smmu_install_ste_for_dev(master);
>
> spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> @@ -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);
All this talk of locking on the other thread made me realize there's
an issue here. We are no longer holding the arm_smmu_asid_lock while
arm_smmu_write_ctx_desc is called due to its move out of
arm_smmu_domain_finalise_s1. This can race with arm_smmu_share_asid
which may modify the asid after we've written it, but before we've
updated the CD's smmu_domain->devices list.
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
On Wed, Aug 9, 2023 at 9:50 PM Will Deacon <will@kernel.org> wrote: > > 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(). There was a time where s1cfg represented the entire STE and carried the entire cd table. We've gotten rid of s1cfg, and now only store arm_smmu_ctx_desc in the arm_smmu_domain for stage 1 domains. arm_smmu_domain_finalise_cd is IMO more clear, especially given the historical baggage around `s1`.
On Thu, Aug 10, 2023 at 05:45:03PM +0800, Michael Shavit wrote: > On Wed, Aug 9, 2023 at 9:50 PM Will Deacon <will@kernel.org> wrote: > > > > 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(). > > There was a time where s1cfg represented the entire STE and carried > the entire cd table. We've gotten rid of s1cfg, and now only store > arm_smmu_ctx_desc in the arm_smmu_domain for stage 1 domains. > arm_smmu_domain_finalise_cd is IMO more clear, especially given the > historical baggage around `s1`. Ok, but it's the inconsistency I object to. I don't think it's clear at all to have arm_smmu_domain_finalise_cd() and arm_smmu_domain_finalise_s2(). The easiest thing is to leave it as-is. Will
On Thu, Aug 10, 2023 at 03:34:49PM +0100, Will Deacon wrote: > On Thu, Aug 10, 2023 at 05:45:03PM +0800, Michael Shavit wrote: > > On Wed, Aug 9, 2023 at 9:50 PM Will Deacon <will@kernel.org> wrote: > > > > > > 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(). > > > > There was a time where s1cfg represented the entire STE and carried > > the entire cd table. We've gotten rid of s1cfg, and now only store > > arm_smmu_ctx_desc in the arm_smmu_domain for stage 1 domains. > > arm_smmu_domain_finalise_cd is IMO more clear, especially given the > > historical baggage around `s1`. > > Ok, but it's the inconsistency I object to. I don't think it's clear at > all to have arm_smmu_domain_finalise_cd() and arm_smmu_domain_finalise_s2(). > > The easiest thing is to leave it as-is. Well the names have become wonky. arm_smmu_domain_finalise_cd() is filling in the struct arm_smmu_ctx_desc which is mostly the precomputed value for the CD table entry, which is mostly redundant copies of the values of the underlying pgtbl_cfg :\ But I agree keeping it named s1 is more consistent with the naming I think we should use - domains are called S1 or S2 domains depending on their IOPTE format. But arm_smmu_domain_assign_asid/vmid is a generally clearer name for both :\ Jason
>
> > @@ -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?
The CD table is only de-allocated when the SMMU device is released, so
this isn't "leaked" anymore than on a successful attachment. In a
previous version of this patch, this CD table was even pre-allocated
at probe time but is deferred to first attach following this
discussion: https://lore.kernel.org/lkml/ZMOzs1%2FxoEPX2+vA@nvidia.com/
.
> > @@ -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'?
cd_table is statically allocated as part of the arm_smmu_master
struct. I suppose it could be allocated by arm_smmu_alloc_cd_tables()
instead?
On Thu, Aug 10, 2023 at 05:23:37PM +0800, Michael Shavit wrote:
> >
> > > @@ -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?
>
> The CD table is only de-allocated when the SMMU device is released, so
> this isn't "leaked" anymore than on a successful attachment. In a
> previous version of this patch, this CD table was even pre-allocated
> at probe time but is deferred to first attach following this
> discussion: https://lore.kernel.org/lkml/ZMOzs1%2FxoEPX2+vA@nvidia.com/
Thanks, that makes sense.
> > > @@ -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'?
>
> cd_table is statically allocated as part of the arm_smmu_master
> struct. I suppose it could be allocated by arm_smmu_alloc_cd_tables()
> instead?
I just mean you could check 'master->cd_table.cdtab' like you do in other
places. The DMA pointer is supposed to be opaque.
Will
© 2016 - 2025 Red Hat, Inc.