2023-09-15 22:01:43

by Michael Shavit

[permalink] [raw]
Subject: [PATCH v8 0/9] Refactor the SMMU's CD table ownership


Hi all,

This series refactors stage 1 domains so that they describe a single CD
entry. These entries are now inserted into a CD table that is owned by
the arm_smmu_master instead of the domain.
This is conceptually cleaner and unblocks other features, such as
attaching domains with PASID (for unmanaged/dma domains).

This patch series was originally part of a larger patch series that
implemented the set_dev_pasid callback for non-SVA domains but is now
split into a distinct series.

This patch series is also available on gerrit with Jean's SMMU test
engine patches cherry-picked on top for testing:
https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24742/16

Thanks,
Michael Shavit

Changes in v8:
- Rebased off of 6.6-rc1
- Drive-by update of the "Move ctx_desc out of s1_cfg" commit message to be less
vague.
- Link to v7: https://lore.kernel.org/linux-iommu/[email protected]/

Changes in v7:
- Change the amr_smmu_write_ctx_desc_devices helper introduced to
arm_smmu_update_ctx_desc_devices to distinguish from the case where
a potentially new CD entry is written to. Add a comment to clarify
that it is assumed that the operation can't fail and that it's
therefore safe not to handle the return. In contrast, the case where a
new CD entry is written-to does not use the helper and does have to
handle failure.
- Update commit message to be more clear about locking purpose.
- Drop "Skip cd sync if CD table isn't active" commit, and remove
related comment in the "Move CD table to arm_smmu_master patch.
- And some minor cosmetic changes based on v6 feedback.
- Link to v6: https://lore.kernel.org/all/[email protected]/

Changes in v6:
- Undo removal of s1fmt and renaming of s1cdmax
- Unwind the loop in amr_smmu_write_ctx_desc_devices to NULL out the CD
entries we succesfully wrote on failure.
- Add a comment clarifying the different usages of
amr_smmu_write_ctx_desc_devices
- Grab the asid lock while writing the RID CD to prevent a race with
SVA.
- Add the device to the devices list before writing the CD to the table
and installing the CD table.
- Link to v5: https://lore.kernel.org/all/[email protected]/

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.
- Added commit to clean-up now unused master parameter in
arm_smmu_domain_finalise
- Link to v4: https://lore.kernel.org/all/[email protected]/

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.
- Link to v3: https://lore.kernel.org/all/[email protected]/

Changes in v3:
- Add a helper to write a CD to all masters that a domain is attached
to.
- Fixed an issue where an arm_smmu_write_ctx_desc error return wasn't
correctly handled by its caller.
- Link to v2: https://lore.kernel.org/all/[email protected]/

Changes in v2:
- Allocate CD table when it's first needed instead of on probe.
- Minor changes
- Added commit to rename remaining usages of cdcfg to cd_table
- Link to v1: https://lore.kernel.org/all/[email protected]/#r

Changes in v1:
- Replace s1_cfg with arm_smmu_ctx_desc_cfg representing the CD table
- Assume that the CD table is owned by the SMMU master for most
operations. This is forward-compatible with the nested patch series as
these operations wouldn't be called when the installed CD table comes
from nested domains.
- Split off as a distinct patch series from https://lore.kernel.org/all/[email protected]/

Michael Shavit (9):
iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
iommu/arm-smmu-v3: Replace s1_cfg with cdtab_cfg
iommu/arm-smmu-v3: Encapsulate ctx_desc_cfg init in alloc_cd_tables
iommu/arm-smmu-v3: move stall_enabled to the cd table
iommu/arm-smmu-v3: Refactor write_ctx_desc
iommu/arm-smmu-v3: Move CD table to arm_smmu_master
iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise
iommu/arm-smmu-v3: Update comment about STE liveness
iommu/arm-smmu-v3: Rename cdcfg to cd_table

.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 41 ++-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 251 +++++++++---------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 +-
3 files changed, 166 insertions(+), 143 deletions(-)


base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
--
2.42.0.459.ge4e396fd5e-goog


2023-09-16 02:27:33

by Michael Shavit

[permalink] [raw]
Subject: [PATCH v8 2/9] iommu/arm-smmu-v3: Replace s1_cfg with cdtab_cfg

Remove struct arm_smmu_s1_cfg. This is really just a CD table with a
bit of extra information. Move other attributes of the CD table that
were held there into the existing CD table structure, struct
arm_smmu_ctx_desc_cfg, and replace all usages of arm_smmu_s1_cfg with
arm_smmu_ctx_desc_cfg.

For clarity, use the name "cd_table" for the variables pointing to
arm_smmu_ctx_desc_cfg in the new code instead of cdcfg. A later patch
will make this fully consistent.

Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Nicolin Chen <[email protected]>
Signed-off-by: Michael Shavit <[email protected]>
---

(no changes since v6)

Changes in v6:
- Undo removal of s1fmt and renaming of s1cdmax

Changes in v3:
- Updated commit messages again
- Replace more usages of cdcfg with cdtable (lines that were already
touched by this commit anyways).

Changes in v2:
- Updated commit message

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 41 ++++++++++-----------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 +---
2 files changed, 22 insertions(+), 26 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 6ddc23332a1a8..1ccff6d87edf7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1033,9 +1033,9 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
unsigned int idx;
struct arm_smmu_l1_ctx_desc *l1_desc;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->s1_cfg.cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;

- if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
+ if (cdcfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;

idx = ssid >> CTXDESC_SPLIT;
@@ -1071,7 +1071,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
bool cd_live;
__le64 *cdptr;

- if (WARN_ON(ssid >= (1 << smmu_domain->s1_cfg.s1cdmax)))
+ if (WARN_ON(ssid >= (1 << smmu_domain->cd_table.s1cdmax)))
return -E2BIG;

cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
@@ -1138,19 +1138,18 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
size_t l1size;
size_t max_contexts;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &cfg->cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;

- max_contexts = 1 << cfg->s1cdmax;
+ max_contexts = 1 << cdcfg->s1cdmax;

if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
max_contexts <= CTXDESC_L2_ENTRIES) {
- cfg->s1fmt = STRTAB_STE_0_S1FMT_LINEAR;
+ cdcfg->s1fmt = STRTAB_STE_0_S1FMT_LINEAR;
cdcfg->num_l1_ents = max_contexts;

l1size = max_contexts * (CTXDESC_CD_DWORDS << 3);
} else {
- cfg->s1fmt = STRTAB_STE_0_S1FMT_64K_L2;
+ cdcfg->s1fmt = STRTAB_STE_0_S1FMT_64K_L2;
cdcfg->num_l1_ents = DIV_ROUND_UP(max_contexts,
CTXDESC_L2_ENTRIES);

@@ -1186,7 +1185,7 @@ static void arm_smmu_free_cd_tables(struct arm_smmu_domain *smmu_domain)
int i;
size_t size, l1size;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->s1_cfg.cdcfg;
+ struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;

if (cdcfg->l1_desc) {
size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
@@ -1276,7 +1275,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
u64 val = le64_to_cpu(dst[0]);
bool ste_live = false;
struct arm_smmu_device *smmu = NULL;
- struct arm_smmu_s1_cfg *s1_cfg = NULL;
+ struct arm_smmu_ctx_desc_cfg *cd_table = NULL;
struct arm_smmu_s2_cfg *s2_cfg = NULL;
struct arm_smmu_domain *smmu_domain = NULL;
struct arm_smmu_cmdq_ent prefetch_cmd = {
@@ -1294,7 +1293,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:
- s1_cfg = &smmu_domain->s1_cfg;
+ cd_table = &smmu_domain->cd_table;
break;
case ARM_SMMU_DOMAIN_S2:
case ARM_SMMU_DOMAIN_NESTED:
@@ -1325,7 +1324,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
val = STRTAB_STE_0_V;

/* Bypass/fault */
- if (!smmu_domain || !(s1_cfg || s2_cfg)) {
+ if (!smmu_domain || !(cd_table || s2_cfg)) {
if (!smmu_domain && disable_bypass)
val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
else
@@ -1344,7 +1343,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
return;
}

- if (s1_cfg) {
+ if (cd_table) {
u64 strw = smmu->features & ARM_SMMU_FEAT_E2H ?
STRTAB_STE_1_STRW_EL2 : STRTAB_STE_1_STRW_NSEL1;

@@ -1360,10 +1359,10 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
!master->stall_enabled)
dst[1] |= cpu_to_le64(STRTAB_STE_1_S1STALLD);

- val |= (s1_cfg->cdcfg.cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) |
+ val |= (cd_table->cdtab_dma & STRTAB_STE_0_S1CTXPTR_MASK) |
FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_S1_TRANS) |
- FIELD_PREP(STRTAB_STE_0_S1CDMAX, s1_cfg->s1cdmax) |
- FIELD_PREP(STRTAB_STE_0_S1FMT, s1_cfg->s1fmt);
+ FIELD_PREP(STRTAB_STE_0_S1CDMAX, cd_table->s1cdmax) |
+ FIELD_PREP(STRTAB_STE_0_S1FMT, cd_table->s1fmt);
}

if (s2_cfg) {
@@ -2064,11 +2063,11 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)

/* Free the CD and ASID, if we allocated them */
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
+ 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 (cfg->cdcfg.cdtab)
+ if (cd_table->cdtab)
arm_smmu_free_cd_tables(smmu_domain);
arm_smmu_free_asid(&smmu_domain->cd);
mutex_unlock(&arm_smmu_asid_lock);
@@ -2088,7 +2087,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
int ret;
u32 asid;
struct arm_smmu_device *smmu = smmu_domain->smmu;
- struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg;
+ struct arm_smmu_ctx_desc_cfg *cd_table = &smmu_domain->cd_table;
struct arm_smmu_ctx_desc *cd = &smmu_domain->cd;
typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;

@@ -2101,7 +2100,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
if (ret)
goto out_unlock;

- cfg->s1cdmax = master->ssid_bits;
+ cd_table->s1cdmax = master->ssid_bits;

smmu_domain->stall_enabled = master->stall_enabled;

@@ -2441,7 +2440,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
ret = -EINVAL;
goto out_unlock;
} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
- master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) {
+ master->ssid_bits != smmu_domain->cd_table.s1cdmax) {
ret = -EINVAL;
goto out_unlock;
} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
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 9389780db1f34..def1de62a59c6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -595,11 +595,8 @@ struct arm_smmu_ctx_desc_cfg {
dma_addr_t cdtab_dma;
struct arm_smmu_l1_ctx_desc *l1_desc;
unsigned int num_l1_ents;
-};
-
-struct arm_smmu_s1_cfg {
- struct arm_smmu_ctx_desc_cfg cdcfg;
u8 s1fmt;
+ /* log2 of the maximum number of CDs supported by this table */
u8 s1cdmax;
};

@@ -725,7 +722,7 @@ struct arm_smmu_domain {
union {
struct {
struct arm_smmu_ctx_desc cd;
- struct arm_smmu_s1_cfg s1_cfg;
+ struct arm_smmu_ctx_desc_cfg cd_table;
};
struct arm_smmu_s2_cfg s2_cfg;
};
--
2.42.0.459.ge4e396fd5e-goog

2023-09-16 22:54:26

by Michael Shavit

[permalink] [raw]
Subject: [PATCH v8 6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master

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.

Also add the device to the devices list before writing the CD to the
table so that SVA will know that the CD needs to be re-written to this
device's CD table as well if it decides to update the CD's ASID
concurrently with this function.

Tested-by: Nicolin Chen <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Michael Shavit <[email protected]>
---

(no changes since v7)

Changes in v7:
- Update commit message to be more clear about locking purpose.
- Removed redundant newline

Changes in v6:
- Grab the asid lock while writing the RID CD to prevent a race with
SVA.
- Add the device to the devices list before writing the CD to the table
and installing the CD table.
- Undo arm_smmu_finalise_s1 rename
- Minor comment fix
- Consistently check cdtab pointer instead of cdtab_dma

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 | 100 +++++++++++---------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 +-
2 files changed, 58 insertions(+), 49 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 d8919d3afdabb..25ce62d25732c 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->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
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->s1cdmax)))
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->s1cdmax = master->ssid_bits;
@@ -1176,12 +1175,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);
@@ -1289,7 +1288,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:
@@ -2057,14 +2056,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 {
@@ -2095,10 +2090,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) |
@@ -2110,17 +2101,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, IOMMU_NO_PASID, 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;
@@ -2384,6 +2367,14 @@ 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);
+ /*
+ * Clearing the CD entry isn't strictly required to detach the domain
+ * since the table is uninstalled anyway, but it 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, IOMMU_NO_PASID, NULL);
}

static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -2418,23 +2409,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.s1cdmax) {
+ } 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;

@@ -2448,16 +2430,42 @@ 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);

- arm_smmu_install_ste_for_dev(master);
-
spin_lock_irqsave(&smmu_domain->devices_lock, flags);
list_add(&master->domain_head, &smmu_domain->devices);
spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

+ 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;
+ goto out_list_del;
+ }
+ }
+
+ /*
+ * Prevent SVA from concurrently modifying the CD or writing to
+ * the CD entry
+ */
+ mutex_lock(&arm_smmu_asid_lock);
+ ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
+ mutex_unlock(&arm_smmu_asid_lock);
+ if (ret) {
+ master->domain = NULL;
+ goto out_list_del;
+ }
+ }
+
+ arm_smmu_install_ste_for_dev(master);
+
arm_smmu_enable_ats(master);
+ return 0;
+
+out_list_del:
+ spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+ list_del(&master->domain_head);
+ spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

-out_unlock:
- mutex_unlock(&smmu_domain->init_mutex);
return ret;
}

@@ -2702,6 +2710,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)
+ 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 d2fc0a9793e54..961205ba86d25 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -695,6 +695,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;
@@ -721,11 +723,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.42.0.459.ge4e396fd5e-goog

2023-09-20 12:06:41

by Nicolin Chen

[permalink] [raw]
Subject: Re: [PATCH v8 0/9] Refactor the SMMU's CD table ownership

On Fri, Sep 15, 2023 at 09:17:31PM +0800, Michael Shavit wrote:
>
> Hi all,
>
> This series refactors stage 1 domains so that they describe a single CD
> entry. These entries are now inserted into a CD table that is owned by
> the arm_smmu_master instead of the domain.
> This is conceptually cleaner and unblocks other features, such as
> attaching domains with PASID (for unmanaged/dma domains).
>
> This patch series was originally part of a larger patch series that
> implemented the set_dev_pasid callback for non-SVA domains but is now
> split into a distinct series.
>
> This patch series is also available on gerrit with Jean's SMMU test
> engine patches cherry-picked on top for testing:
> https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24742/16
>
> Thanks,
> Michael Shavit
>
> Changes in v8:
> - Rebased off of 6.6-rc1
> - Drive-by update of the "Move ctx_desc out of s1_cfg" commit message to be less
> vague.
> - Link to v7: https://lore.kernel.org/linux-iommu/[email protected]/

Hmm. I recall that I gave Tested-by to v7, yet some of the patches
in this v8 don't include that. Anyway, I retested with this v8:

Tested-by: Nicolin Chen <[email protected]>

2023-10-11 23:49:13

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v8 0/9] Refactor the SMMU's CD table ownership

On Fri, Sep 15, 2023 at 09:17:31PM +0800, Michael Shavit wrote:
>
> Hi all,
>
> This series refactors stage 1 domains so that they describe a single CD
> entry. These entries are now inserted into a CD table that is owned by
> the arm_smmu_master instead of the domain.
> This is conceptually cleaner and unblocks other features, such as
> attaching domains with PASID (for unmanaged/dma domains).
>
> This patch series was originally part of a larger patch series that
> implemented the set_dev_pasid callback for non-SVA domains but is now
> split into a distinct series.
>
> This patch series is also available on gerrit with Jean's SMMU test
> engine patches cherry-picked on top for testing:
> https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24742/16
>
> Thanks,
> Michael Shavit
>
> Michael Shavit (9):
> iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
> iommu/arm-smmu-v3: Replace s1_cfg with cdtab_cfg
> iommu/arm-smmu-v3: Encapsulate ctx_desc_cfg init in alloc_cd_tables
> iommu/arm-smmu-v3: move stall_enabled to the cd table
> iommu/arm-smmu-v3: Refactor write_ctx_desc
> iommu/arm-smmu-v3: Move CD table to arm_smmu_master
> iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise
> iommu/arm-smmu-v3: Update comment about STE liveness
> iommu/arm-smmu-v3: Rename cdcfg to cd_table

Will, can you take this please?

Thanks,
Jason

2023-10-12 14:51:00

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v8 0/9] Refactor the SMMU's CD table ownership

On Wed, Oct 11, 2023 at 08:48:48PM -0300, Jason Gunthorpe wrote:
> On Fri, Sep 15, 2023 at 09:17:31PM +0800, Michael Shavit wrote:
> >
> > Hi all,
> >
> > This series refactors stage 1 domains so that they describe a single CD
> > entry. These entries are now inserted into a CD table that is owned by
> > the arm_smmu_master instead of the domain.
> > This is conceptually cleaner and unblocks other features, such as
> > attaching domains with PASID (for unmanaged/dma domains).
> >
> > This patch series was originally part of a larger patch series that
> > implemented the set_dev_pasid callback for non-SVA domains but is now
> > split into a distinct series.
> >
> > This patch series is also available on gerrit with Jean's SMMU test
> > engine patches cherry-picked on top for testing:
> > https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/24742/16
> >
> > Thanks,
> > Michael Shavit
> >
> > Michael Shavit (9):
> > iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
> > iommu/arm-smmu-v3: Replace s1_cfg with cdtab_cfg
> > iommu/arm-smmu-v3: Encapsulate ctx_desc_cfg init in alloc_cd_tables
> > iommu/arm-smmu-v3: move stall_enabled to the cd table
> > iommu/arm-smmu-v3: Refactor write_ctx_desc
> > iommu/arm-smmu-v3: Move CD table to arm_smmu_master
> > iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise
> > iommu/arm-smmu-v3: Update comment about STE liveness
> > iommu/arm-smmu-v3: Rename cdcfg to cd_table
>
> Will, can you take this please?

On my list to look at today!

Will

2023-10-12 18:07:45

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v8 0/9] Refactor the SMMU's CD table ownership

On Fri, 15 Sep 2023 21:17:31 +0800, Michael Shavit wrote:
> This series refactors stage 1 domains so that they describe a single CD
> entry. These entries are now inserted into a CD table that is owned by
> the arm_smmu_master instead of the domain.
> This is conceptually cleaner and unblocks other features, such as
> attaching domains with PASID (for unmanaged/dma domains).
>
> This patch series was originally part of a larger patch series that
> implemented the set_dev_pasid callback for non-SVA domains but is now
> split into a distinct series.
>
> [...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/9] iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
https://git.kernel.org/will/c/987a878e09c6
[2/9] iommu/arm-smmu-v3: Replace s1_cfg with cdtab_cfg
https://git.kernel.org/will/c/1f8588834016
[3/9] iommu/arm-smmu-v3: Encapsulate ctx_desc_cfg init in alloc_cd_tables
https://git.kernel.org/will/c/e3aad74c51a7
[4/9] iommu/arm-smmu-v3: move stall_enabled to the cd table
https://git.kernel.org/will/c/1228cc509fc6
[5/9] iommu/arm-smmu-v3: Refactor write_ctx_desc
https://git.kernel.org/will/c/24503148c545
[6/9] iommu/arm-smmu-v3: Move CD table to arm_smmu_master
https://git.kernel.org/will/c/10e4968cd511
[7/9] iommu/arm-smmu-v3: Cleanup arm_smmu_domain_finalise
https://git.kernel.org/will/c/5e14313df2c8
[8/9] iommu/arm-smmu-v3: Update comment about STE liveness
https://git.kernel.org/will/c/6032f58498b7
[9/9] iommu/arm-smmu-v3: Rename cdcfg to cd_table
https://git.kernel.org/will/c/475918e9c4eb

Cheers,
--
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev