[PATCH v4 00/13] Add PASID support to SMMUv3 unmanaged domains

[PATCH v4 00/13] Add PASID support to SMMUv3 unmanaged domains

[Michael Shavit]

Hi all,

This patch series implements the set_dev_pasid operation for DMA
and UNMANAGED iommu domains.

The bulk of the series involves a refactor of stage 1 domains so that
they describe a single CD entry. On attach, stage 1 domains are inserted
into a CD table that is now owned by the arm_smmu_master struct. This is
a pre-requisite to the set_dev_pasid implementation but also results in
a conceptually cleaner arm_smmu_domain. Note that this does not preclude
from attaching domains that represent a CD table, such as for the
proposed iommufd NESTED domains.

The last few patches of the series make drive-by cleanups to the smmu
SVA implementation. A follow-up patch-series is planned to further take
advantage of these refactorings so that the SVA set_dev_pasid
implementation can directly rely on the arm-smmu-v3.c's set_dev_pasid
implementation. See discussion on patch 14 of the v2 series.

This patch series is also available on gerrit with Jean's SMMU test
engine patches cherry-picked on top:
https://linux-review.googlesource.com/id/I0fcd9adc058d1c58a12d2599cc82fba73da7697a
This allowed testing of basic SVA functionality (e.g.: attaching, page
fault handling, and detaching).

Thanks,
Michael Shavit

Changelog
v4:
 * Fix build warning and error on patch 07. The error was introduced
   during a v1->v2 rebase and hidden by patch 09 which removed the
   offending line.
v3:
https://lore.kernel.org/all/20230614154304.2860121-1-mshavit@google.com/
 * Dropped the bulk of the SVA refactoring to re-work as a follow-up
   series.
 * Reworded cover letter to omit dropped changes.
 * Rebased on 6.4 tip
v2:
https://lore.kernel.org/all/20230606120854.4170244-1-mshavit@google.com/
 * Reworded cover letter and commits based on v1 feedback.
 * Split and reworked `iommu/arm-smmu-v3: Move cdtable to arm_smmu_master`
 * Added SVA clean-up and refactor.
 * A few other small bug fixes and cosmetics.
v1:
https://lore.kernel.org/all/20230510205054.2667898-1-mshavit@google.com/


Michael Shavit (13):
  iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg
  iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master
  iommu/arm-smmu-v3: Refactor write_strtab_ent
  iommu/arm-smmu-v3: Refactor write_ctx_desc
  iommu/arm-smmu-v3: Use the master-owned s1_cfg
  iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats
  iommu/arm-smmu-v3: Keep track of attached ssids
  iommu/arm-smmu-v3: Add helper for atc invalidation
  iommu/arm-smmu-v3: Implement set_dev_pasid
  iommu/arm-smmu-v3-sva: Remove bond refcount
  iommu/arm-smmu-v3-sva: Clean unused iommu_sva
  iommu/arm-smmu-v3-sva: Remove arm_smmu_bond
  iommu/arm-smmu-v3-sva: Add check when enabling sva

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 156 +++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 447 ++++++++++++------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  49 +-
 3 files changed, 414 insertions(+), 238 deletions(-)


base-commit: b6dad5178ceaf23f369c3711062ce1f2afc33644
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 01/13] iommu/arm-smmu-v3: Move ctx_desc out of s1_cfg

[Michael Shavit]

s1_cfg describes the CD table that is inserted into an SMMU's STEs. It's
weird for s1_cfg to also own ctx_desc which describes a CD that is
inserted into that table. It is more appropriate for arm_smmu_domain to
own ctx_desc.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 23 +++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 28 ++++++++++---------
 3 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index a5a63b1c947eb..968559d625c40 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -62,7 +62,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 		return cd;
 	}
 
-	smmu_domain = container_of(cd, struct arm_smmu_domain, s1_cfg.cd);
+	smmu_domain = container_of(cd, struct arm_smmu_domain, cd);
 	smmu = smmu_domain->smmu;
 
 	ret = xa_alloc(&arm_smmu_asid_xa, &new_asid, cd,
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 3fd83fb757227..beff04b897718 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1863,7 +1863,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 	 * careful, 007.
 	 */
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid);
+		arm_smmu_tlb_inv_asid(smmu, smmu_domain->cd.asid);
 	} else {
 		cmd.opcode	= CMDQ_OP_TLBI_S12_VMALL;
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
@@ -1946,7 +1946,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		cmd.opcode	= smmu_domain->smmu->features & ARM_SMMU_FEAT_E2H ?
 				  CMDQ_OP_TLBI_EL2_VA : CMDQ_OP_TLBI_NH_VA;
-		cmd.tlbi.asid	= smmu_domain->s1_cfg.cd.asid;
+		cmd.tlbi.asid	= smmu_domain->cd.asid;
 	} else {
 		cmd.opcode	= CMDQ_OP_TLBI_S2_IPA;
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
@@ -2077,7 +2077,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 		mutex_lock(&arm_smmu_asid_lock);
 		if (cfg->cdcfg.cdtab)
 			arm_smmu_free_cd_tables(smmu_domain);
-		arm_smmu_free_asid(&cfg->cd);
+		arm_smmu_free_asid(&smmu_domain->cd);
 		mutex_unlock(&arm_smmu_asid_lock);
 	} else {
 		struct arm_smmu_s2_cfg *cfg = &smmu_domain->s2_cfg;
@@ -2096,13 +2096,14 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	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 *cd = &smmu_domain->cd;
 	typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;
 
-	refcount_set(&cfg->cd.refs, 1);
+	refcount_set(&cd->refs, 1);
 
 	/* Prevent SVA from modifying the ASID until it is written to the CD */
 	mutex_lock(&arm_smmu_asid_lock);
-	ret = xa_alloc(&arm_smmu_asid_xa, &asid, &cfg->cd,
+	ret = xa_alloc(&arm_smmu_asid_xa, &asid, cd,
 		       XA_LIMIT(1, (1 << smmu->asid_bits) - 1), GFP_KERNEL);
 	if (ret)
 		goto out_unlock;
@@ -2115,23 +2116,23 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		goto out_free_asid;
 
-	cfg->cd.asid	= (u16)asid;
-	cfg->cd.ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
-	cfg->cd.tcr	= FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
+	cd->asid	= (u16)asid;
+	cd->ttbr	= pgtbl_cfg->arm_lpae_s1_cfg.ttbr;
+	cd->tcr		= FIELD_PREP(CTXDESC_CD_0_TCR_T0SZ, tcr->tsz) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_TG0, tcr->tg) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_IRGN0, tcr->irgn) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_ORGN0, tcr->orgn) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_SH0, tcr->sh) |
 			  FIELD_PREP(CTXDESC_CD_0_TCR_IPS, tcr->ips) |
 			  CTXDESC_CD_0_TCR_EPD1 | CTXDESC_CD_0_AA64;
-	cfg->cd.mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
+	cd->mair	= pgtbl_cfg->arm_lpae_s1_cfg.mair;
 
 	/*
 	 * Note that this will end up calling arm_smmu_sync_cd() before
 	 * the master has been added to the devices list for this domain.
 	 * This isn't an issue because the STE hasn't been installed yet.
 	 */
-	ret = arm_smmu_write_ctx_desc(smmu_domain, 0, &cfg->cd);
+	ret = arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
 	if (ret)
 		goto out_free_cd_tables;
 
@@ -2141,7 +2142,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 out_free_cd_tables:
 	arm_smmu_free_cd_tables(smmu_domain);
 out_free_asid:
-	arm_smmu_free_asid(&cfg->cd);
+	arm_smmu_free_asid(cd);
 out_unlock:
 	mutex_unlock(&arm_smmu_asid_lock);
 	return ret;
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 b574c58a34876..68d519f21dbd8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -593,7 +593,6 @@ struct arm_smmu_ctx_desc_cfg {
 
 struct arm_smmu_s1_cfg {
 	struct arm_smmu_ctx_desc_cfg	cdcfg;
-	struct arm_smmu_ctx_desc	cd;
 	u8				s1fmt;
 	u8				s1cdmax;
 };
@@ -707,25 +706,28 @@ enum arm_smmu_domain_stage {
 };
 
 struct arm_smmu_domain {
-	struct arm_smmu_device		*smmu;
-	struct mutex			init_mutex; /* Protects smmu pointer */
+	struct arm_smmu_device			*smmu;
+	struct mutex				init_mutex; /* Protects smmu pointer */
 
-	struct io_pgtable_ops		*pgtbl_ops;
-	bool				stall_enabled;
-	atomic_t			nr_ats_masters;
+	struct io_pgtable_ops			*pgtbl_ops;
+	bool					stall_enabled;
+	atomic_t				nr_ats_masters;
 
-	enum arm_smmu_domain_stage	stage;
+	enum arm_smmu_domain_stage		stage;
 	union {
-		struct arm_smmu_s1_cfg	s1_cfg;
-		struct arm_smmu_s2_cfg	s2_cfg;
+		struct {
+		struct arm_smmu_ctx_desc	cd;
+		struct arm_smmu_s1_cfg		s1_cfg;
+		};
+		struct arm_smmu_s2_cfg		s2_cfg;
 	};
 
-	struct iommu_domain		domain;
+	struct iommu_domain			domain;
 
-	struct list_head		devices;
-	spinlock_t			devices_lock;
+	struct list_head			devices;
+	spinlock_t				devices_lock;
 
-	struct list_head		mmu_notifiers;
+	struct list_head			mmu_notifiers;
 };
 
 static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

Except for Nested domains, arm_smmu_master will own the STEs that are
inserted into the arm_smmu_device's STE table.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 28 +++++++++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
 2 files changed, 18 insertions(+), 11 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 beff04b897718..023769f5ca79a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1126,15 +1126,16 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
 	return 0;
 }
 
-static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
+static int arm_smmu_init_s1_cfg(struct arm_smmu_master *master,
+				struct arm_smmu_s1_cfg *cfg)
 {
 	int ret;
 	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_device *smmu = master->smmu;
 	struct arm_smmu_ctx_desc_cfg *cdcfg = &cfg->cdcfg;
 
+	cfg->s1cdmax = master->ssid_bits;
 	max_contexts = 1 << cfg->s1cdmax;
 
 	if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
@@ -1175,12 +1176,11 @@ 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_device *smmu,
+				    struct arm_smmu_ctx_desc_cfg *cdcfg)
 {
 	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;
 
 	if (cdcfg->l1_desc) {
 		size = CTXDESC_L2_ENTRIES * (CTXDESC_CD_DWORDS << 3);
@@ -2076,7 +2076,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 		/* Prevent SVA from touching the CD while we're freeing it */
 		mutex_lock(&arm_smmu_asid_lock);
 		if (cfg->cdcfg.cdtab)
-			arm_smmu_free_cd_tables(smmu_domain);
+			arm_smmu_free_cd_tables(smmu, &cfg->cdcfg);
 		arm_smmu_free_asid(&smmu_domain->cd);
 		mutex_unlock(&arm_smmu_asid_lock);
 	} else {
@@ -2108,11 +2108,9 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		goto out_unlock;
 
-	cfg->s1cdmax = master->ssid_bits;
-
 	smmu_domain->stall_enabled = master->stall_enabled;
 
-	ret = arm_smmu_alloc_cd_tables(smmu_domain);
+	ret = arm_smmu_init_s1_cfg(master, cfg);
 	if (ret)
 		goto out_free_asid;
 
@@ -2140,7 +2138,7 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	return 0;
 
 out_free_cd_tables:
-	arm_smmu_free_cd_tables(smmu_domain);
+	arm_smmu_free_cd_tables(smmu, &cfg->cdcfg);
 out_free_asid:
 	arm_smmu_free_asid(cd);
 out_unlock:
@@ -2704,6 +2702,13 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	    smmu->features & ARM_SMMU_FEAT_STALL_FORCE)
 		master->stall_enabled = true;
 
+	ret = arm_smmu_init_s1_cfg(master, &master->owned_s1_cfg);
+	if (ret) {
+		arm_smmu_disable_pasid(master);
+		arm_smmu_remove_master(master);
+		goto err_free_master;
+	}
+
 	return &smmu->iommu;
 
 err_free_master:
@@ -2719,6 +2724,7 @@ static void arm_smmu_release_device(struct device *dev)
 	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
 	arm_smmu_detach_dev(master);
+	arm_smmu_free_cd_tables(master->smmu, &master->owned_s1_cfg.cdcfg);
 	arm_smmu_disable_pasid(master);
 	arm_smmu_remove_master(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 68d519f21dbd8..053cc14c23969 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -688,6 +688,7 @@ struct arm_smmu_master {
 	struct arm_smmu_domain		*domain;
 	struct list_head		domain_head;
 	struct arm_smmu_stream		*streams;
+	struct arm_smmu_s1_cfg		owned_s1_cfg;
 	unsigned int			num_streams;
 	bool				ats_enabled;
 	bool				stall_enabled;
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Nicolin Chen]

Hi Michael,

On Wed, Jun 21, 2023 at 02:37:14PM +0800, Michael Shavit wrote:
 
> Except for Nested domains, arm_smmu_master will own the STEs that are
> inserted into the arm_smmu_device's STE table.

I think that the master still owns an STE when attached to a
nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
an opaque object to the STE in the guest, the host still has
a real STE for the nested configuration somewhere -- and it's
likely still to be owned by the master that's attached to the
opaque NESTED iommu_domain in the host kernel.

> -static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain)
> +static int arm_smmu_init_s1_cfg(struct arm_smmu_master *master,
> +                               struct arm_smmu_s1_cfg *cfg)

We here pass in an s1_cfg ptr because we expect someone else
rather than the master could own the s1_cfg?

But the final codeline by the end of this series seems that
only master owns an s1_cfg. So perhaps we could re-organize
the patches to clean this away, as the cfg always comes from
a 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 68d519f21dbd8..053cc14c23969 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -688,6 +688,7 @@ struct arm_smmu_master {
>         struct arm_smmu_domain          *domain;
>         struct list_head                domain_head;
>         struct arm_smmu_stream          *streams;
> +       struct arm_smmu_s1_cfg          owned_s1_cfg;

I am a bit confused by this naming. If only master would own
an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
s1_cfg pointer in the next patch.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Thu, Jul 13, 2023 at 9:22 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> > Except for Nested domains, arm_smmu_master will own the STEs that are
> > inserted into the arm_smmu_device's STE table.
>
> I think that the master still owns an STE when attached to a
> nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
> an opaque object to the STE in the guest, the host still has
> a real STE for the nested configuration somewhere -- and it's
> likely still to be owned by the master that's attached to the
> opaque NESTED iommu_domain in the host kernel.

> I am a bit confused by this naming. If only master would own
> an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
> s1_cfg pointer in the next patch.

Could be that the naming is causing some confusion. This owned_s1_cfg
is very different from the s1_cfg set-up by Nested domains in your
patch series. It's better to think of it as the default s1_cfg used
for DMA/SVA/UNMANAGED domains. Because stage 1 domains represent a
single page table, it doesn't make sense for them to own an entire CD
table. In contrast, nested domains map an entire CD table and it
therefore makes sense for them to own the s1_cfg representing that
table.
Would renaming this as default_s1_cfg make more sense?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Thu, Jul 13, 2023 at 04:34:56PM +0800, Michael Shavit wrote:
> On Thu, Jul 13, 2023 at 9:22 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> >
> > > Except for Nested domains, arm_smmu_master will own the STEs that are
> > > inserted into the arm_smmu_device's STE table.
> >
> > I think that the master still owns an STE when attached to a
> > nested domain. Though an IOMMU_DOMAIN_NESTED iommu_domain is
> > an opaque object to the STE in the guest, the host still has
> > a real STE for the nested configuration somewhere -- and it's
> > likely still to be owned by the master that's attached to the
> > opaque NESTED iommu_domain in the host kernel.
> 
> > I am a bit confused by this naming. If only master would own
> > an s1_cfg, perhaps we can just make it "s1_cfg" and drop the
> > s1_cfg pointer in the next patch.
> 
> Could be that the naming is causing some confusion. This owned_s1_cfg
> is very different from the s1_cfg set-up by Nested domains in your
> patch series. It's better to think of it as the default s1_cfg used
> for DMA/SVA/UNMANAGED domains. Because stage 1 domains represent a
> single page table, it doesn't make sense for them to own an entire CD
> table. In contrast, nested domains map an entire CD table and it
> therefore makes sense for them to own the s1_cfg representing that
> table.
> Would renaming this as default_s1_cfg make more sense?

It would make alot more sense if the STE value used by an unmanaged S1
domain was located in/near the unmanaged domain or called 'unmanaged
S1 STE' or something if it really has to be in the master. Why does
this even need to be stored, can't we compute it?

Notice that we have unmanaged domains that use a CD and SVA domains
typically would be on a CD, so it is a bit weird already.

I'd think the basic mental model should be to extract the STE from the
thing you intend to install. Either the default CD table, or from the
iommu_domain. ie some 'get STE from iommu_domain' function?

There shouldn't be a concept of a "default" or "owned" STE value..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> It would make alot more sense if the STE value used by an unmanaged S1
> domain was located in/near the unmanaged domain or called 'unmanaged
> S1 STE' or something if it really has to be in the master. Why does
> this even need to be stored, can't we compute it?

struct s1_cfg* and struct s2_cfg* are precisely what is used to
compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
will write the s1_cfg's CD table dma_pointer into the STE's
STRTAB_STE_0_CFG field. When neither are set, the STE fields are
written to enable bypass (or abort depending on the config).

> I'd think the basic mental model should be to extract the STE from the
> thing you intend to install. Either the default CD table, or from the
> iommu_domain. ie some 'get STE from iommu_domain' function?

I don't follow this. When we attach a domain with pasid (whether
through SVA or the set_dev_pasid API) , we don't want to install an
entirely new CD table. We want to write something (page-table pointer)
to a common CD table. Where should the s1_cfg which owns that common
table live? I thought we concluded that it should be owned by the
arm_smmu_master rather than any domain (to avoid dependencies between
domains a-la aux-domain). With this change, even attach_dev with a DMA
or UNMANAGED domain is now just preparing a single entry into this
common CD table.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Fri, Jul 14, 2023 at 12:16 AM Michael Shavit <mshavit@google.com> wrote:
> domains a-la aux-domain). With this change, even attach_dev with a DMA
> or UNMANAGED domain is now just preparing a single entry into this
> common CD table.

I have to correct this part, arm-smmu-v3.c's attach_dev()
implementation does both: it writes to the pasid=0 entry of the
owned_s1_cfg's CD table, and then installs that CD table to the STE.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > It would make alot more sense if the STE value used by an unmanaged S1
> > domain was located in/near the unmanaged domain or called 'unmanaged
> > S1 STE' or something if it really has to be in the master. Why does
> > this even need to be stored, can't we compute it?
> 
> struct s1_cfg* and struct s2_cfg* are precisely what is used to
> compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> will write the s1_cfg's CD table dma_pointer into the STE's
> STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> written to enable bypass (or abort depending on the config).

I guess I never really understood why these were precomputed and
stored at all. Even more confusing is why we need to keep pointers to
them anywhere? Compute the STE and CDE directly from the source data
when you need it?

eg If I want to install an IDENITY domain into a STE then I compute
the STE for identity and go ahead and do it.

> > I'd think the basic mental model should be to extract the STE from the
> > thing you intend to install. Either the default CD table, or from the
> > iommu_domain. ie some 'get STE from iommu_domain' function?
> 
> I don't follow this. When we attach a domain with pasid (whether
> through SVA or the set_dev_pasid API) , we don't want to install an
> entirely new CD table. 

The master object owns an optional CD table. If it is exsists it is
used by every domain that is attached to that master.

In the code flow there are two entry points to attach a domain, attach
to a PASID or attach to a RID.

For attach to PASID the code should always force the master to have a
CD table and then attach the domain to the CD table.

For attach to RID the code should do a bunch of checks and decide if
it should force the master to have a CD table and attach the domain to
that, or directly attach the domain to the STE.

When the master gains a CD table then the CD table object becomes
attached to the STE. In all cases we should be able to point to the
object the STE points at and don't need a cfg or pointer to cfg since
the object itself can provide the cfg.

In all cases when you go to compute a STE you figure out what object
is attached to it (CD or domain), compute the correct STE for that
object, the set it. Same for he CDE, query the correct CDE from the
iommu_domain when you attach it to the table.

There should be no such thing as a "default" STE, and I question if it
makes sense to even precompute the s1/s2_cfg values during finalize at
all..

> We want to write something (page-table pointer) to a common CD
> table. Where should the s1_cfg which owns that common table live? 

I would suggest a 'cd table struct' that as all the stuff related to
the CD table, including an API to cacluate the STE this CD table
requires. If not in actual code with a real struct, then in a logical
sense in that a chunk of the master struct is the "CD table".

> I thought we concluded that it should be owned by the
> arm_smmu_master rather than any domain (to avoid dependencies
> between domains a-la aux-domain). 

Yes, I'm not saying anything against that, just how and where the STE
and CDE values flow around.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Nicolin Chen]

On Thu, Jul 13, 2023 at 01:41:38PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> > On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > It would make alot more sense if the STE value used by an unmanaged S1
> > > domain was located in/near the unmanaged domain or called 'unmanaged
> > > S1 STE' or something if it really has to be in the master. Why does
> > > this even need to be stored, can't we compute it?
> > 
> > struct s1_cfg* and struct s2_cfg* are precisely what is used to
> > compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> > will write the s1_cfg's CD table dma_pointer into the STE's
> > STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> > written to enable bypass (or abort depending on the config).
> 
> I guess I never really understood why these were precomputed and
> stored at all. Even more confusing is why we need to keep pointers to
> them anywhere? Compute the STE and CDE directly from the source data
> when you need it?
> 
> eg If I want to install an IDENITY domain into a STE then I compute
> the STE for identity and go ahead and do it.

I think it'd be clear if the master stores STE value directly to
get rid of s1_cfg/s2_cfg in the master struct. We fill in those
domain-related STE fields when the domain attaches to the master
and then call arm_smmu_write_strtab().

For struct arm_smmu_domain, it still has a union of a CD (for an
S1 domain) or an s2_cfg (for an S2 domain). Or we could select
a better naming for s2_cfg too, since s1_cfg is gone.

Does this match with your expectation?

> > > I'd think the basic mental model should be to extract the STE from the
> > > thing you intend to install. Either the default CD table, or from the
> > > iommu_domain. ie some 'get STE from iommu_domain' function?
> > 
> > I don't follow this. When we attach a domain with pasid (whether
> > through SVA or the set_dev_pasid API) , we don't want to install an
> > entirely new CD table. 
> 
> The master object owns an optional CD table. If it is exsists it is
> used by every domain that is attached to that master.
>
> In the code flow there are two entry points to attach a domain, attach
> to a PASID or attach to a RID.
> 
> For attach to PASID the code should always force the master to have a
> CD table and then attach the domain to the CD table.
> 
> For attach to RID the code should do a bunch of checks and decide if
> it should force the master to have a CD table and attach the domain to
> that, or directly attach the domain to the STE.

I think a master own a CD table in both cases, though the table
could have a single entry or multiple entries, depending on its
ssid_bits/cdmax value. And since a master own the CD table, we
could preallocate the CD table in arm_smmu_probe_device(), like
Michael does in this series. And at the same time, the s1ctxptr
field of the STE could be setup too. Then we insert the CD from
a domain into the CD table during the domain attaching.

Yet two cases that would waste the preallocated CD table:
1) If a master with ssid_bits=0 gets attached to an IDENITY S1
   domain, it sets CONFIG=BYPASS in the STE, which wastes the
   single-entry CD table, unlike currently the driver bypassing
   the allocation of a CD table at all.
2) When enabling nested translation, we should replace all the
   S1 fields in the STE with guest/user values. So, the kernel-
   level CD table (either single-entry or multi-entry) in the
   master struct will not be used. Although this seems to be
   unchanged since currently the driver wastes this too in the
   default domain?

With that, I still feel it clear and flexible. And I can simply
add my use case of attaching an IDENITY domain to the default
substream when having a multi-entry CD table.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Thu, Jul 13, 2023 at 12:54:47PM -0700, Nicolin Chen wrote:
> On Thu, Jul 13, 2023 at 01:41:38PM -0300, Jason Gunthorpe wrote:
> > On Fri, Jul 14, 2023 at 12:16:16AM +0800, Michael Shavit wrote:
> > > On Thu, Jul 13, 2023 at 10:29 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > > It would make alot more sense if the STE value used by an unmanaged S1
> > > > domain was located in/near the unmanaged domain or called 'unmanaged
> > > > S1 STE' or something if it really has to be in the master. Why does
> > > > this even need to be stored, can't we compute it?
> > > 
> > > struct s1_cfg* and struct s2_cfg* are precisely what is used to
> > > compute an STE. For example, when s1_cfg is set, arm_smmu_write_strtab
> > > will write the s1_cfg's CD table dma_pointer into the STE's
> > > STRTAB_STE_0_CFG field. When neither are set, the STE fields are
> > > written to enable bypass (or abort depending on the config).
> > 
> > I guess I never really understood why these were precomputed and
> > stored at all. Even more confusing is why we need to keep pointers to
> > them anywhere? Compute the STE and CDE directly from the source data
> > when you need it?
> > 
> > eg If I want to install an IDENITY domain into a STE then I compute
> > the STE for identity and go ahead and do it.
> 
> I think it'd be clear if the master stores STE value directly to
> get rid of s1_cfg/s2_cfg in the master struct. We fill in those
> domain-related STE fields when the domain attaches to the master
> and then call arm_smmu_write_strtab().

Right, though master effectively records the STE when it writes it to
the steering table. If we need another copy I don't know.

> For struct arm_smmu_domain, it still has a union of a CD (for an
> S1 domain) or an s2_cfg (for an S2 domain). Or we could select
> a better naming for s2_cfg too, since s1_cfg is gone.

By spec it should be called CD and STE value, but frankly I like CDTE
and STE a little better (context descriptor table entry) as it evokes
a sense of similarity. I don't care for the words 's1_cfg' and
's2_cfg' to represent the CD and STE, that is pretty confusing now
that we have more uses for the CD and STE's than simple s1/s2 IO page
tables.

> I think a master own a CD table in both cases, though the table
> could have a single entry or multiple entries

Yes.

> depending on its ssid_bits/cdmax value. And since a master own the
> CD table, we could preallocate the CD table in
> arm_smmu_probe_device(), like Michael does in this series. And at
> the same time, the s1ctxptr field of the STE could be setup
> too. Then we insert the CD from a domain into the CD table during
> the domain attaching.

We insert the CDTE from the domain into the CD table, and generate a
STE for the CD table and insert it ino the Steering table.

> Yet two cases that would waste the preallocated CD table:
> 1) If a master with ssid_bits=0 gets attached to an IDENITY S1
>    domain, it sets CONFIG=BYPASS in the STE, which wastes the
>    single-entry CD table, unlike currently the driver bypassing
>    the allocation of a CD table at all.
> 2) When enabling nested translation, we should replace all the
>    S1 fields in the STE with guest/user values. So, the kernel-
>    level CD table (either single-entry or multi-entry) in the
>    master struct will not be used. Although this seems to be
>    unchanged since currently the driver wastes this too in the
>    default domain?

As we discussed in another thread dynamic resizing of the CD table
makes some sense. Eg keep it at one entry until PASID mode is enabled
then resize it to the max PASID bits.

But I view that as an improvement beyond here. It seems fine for now
to pre allocate the full CD table and waste the memory. PASID capable
devices are rare.

> With that, I still feel it clear and flexible. And I can simply
> add my use case of attaching an IDENITY domain to the default
> substream when having a multi-entry CD table.

Yes, with the above note about dynamically resizing the CD table, I
think we generally have the idea that programming the CD, eg by
installing an entry, can cause the CD tables's STE to (atomically)
change.

Eg to toggle the S1DSS bit if the PASID 0 is IDENTITY, or to resize
the table if we exceed the PASID space.

Longer term we'd also need a way to setup IDENTITY domains for !0
substreams as well too (eg for SIOV). It is too bad the CDTE doesn't
have a bit for bypass. I suppose it will need dummy 1:1 IO page tables
or something.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Nicolin Chen]

On Thu, Jul 13, 2023 at 08:48:32PM -0300, Jason Gunthorpe wrote:
 
> > For struct arm_smmu_domain, it still has a union of a CD (for an
> > S1 domain) or an s2_cfg (for an S2 domain). Or we could select
> > a better naming for s2_cfg too, since s1_cfg is gone.
> 
> By spec it should be called CD and STE value, but frankly I like CDTE
> and STE a little better (context descriptor table entry) as it evokes
> a sense of similarity. I don't care for the words 's1_cfg' and
> 's2_cfg' to represent the CD and STE, that is pretty confusing now
> that we have more uses for the CD and STE's than simple s1/s2 IO page
> tables.

We have STE and PTE, so CDTE sounds legit to me, though probably
it'd be safer by just following the spec? We can always add kdoc
and comments to make things clear.

@Michael,
Would it be possible for you to update a v5, following Jason's
suggestion overall? And I think we can have a smaller refactor
series first without set_dev_pasid, to have a common codeline
sooner for us to add new features, such as set_dev_pasid, the
use case of IDENTITY default substream, and the nesting series.
I will help testing with some pasid/non-pasid use cases too.

> > Yet two cases that would waste the preallocated CD table:
> > 1) If a master with ssid_bits=0 gets attached to an IDENITY S1
> >    domain, it sets CONFIG=BYPASS in the STE, which wastes the
> >    single-entry CD table, unlike currently the driver bypassing
> >    the allocation of a CD table at all.
> > 2) When enabling nested translation, we should replace all the
> >    S1 fields in the STE with guest/user values. So, the kernel-
> >    level CD table (either single-entry or multi-entry) in the
> >    master struct will not be used. Although this seems to be
> >    unchanged since currently the driver wastes this too in the
> >    default domain?
> 
> As we discussed in another thread dynamic resizing of the CD table
> makes some sense. Eg keep it at one entry until PASID mode is enabled
> then resize it to the max PASID bits.

I see.

> But I view that as an improvement beyond here. It seems fine for now
> to pre allocate the full CD table and waste the memory. PASID capable
> devices are rare.

Yea, that'd be easier for us to move forward other features :)

> > With that, I still feel it clear and flexible. And I can simply
> > add my use case of attaching an IDENITY domain to the default
> > substream when having a multi-entry CD table.
> 
> Yes, with the above note about dynamically resizing the CD table, I
> think we generally have the idea that programming the CD, eg by
> installing an entry, can cause the CD tables's STE to (atomically)
> change.
> 
> Eg to toggle the S1DSS bit if the PASID 0 is IDENTITY, or to resize
> the table if we exceed the PASID space.

Yea, we have enable_pasid() so probably can resize over there.

> Longer term we'd also need a way to setup IDENTITY domains for !0
> substreams as well too (eg for SIOV). It is too bad the CDTE doesn't
> have a bit for bypass. I suppose it will need dummy 1:1 IO page tables
> or something.

Oh. I haven't thought about that far. Noted it down.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Nicolin Chen]

On Thu, Jul 13, 2023 at 08:48:32PM -0300, Jason Gunthorpe wrote:
 
> > For struct arm_smmu_domain, it still has a union of a CD (for an
> > S1 domain) or an s2_cfg (for an S2 domain). Or we could select
> > a better naming for s2_cfg too, since s1_cfg is gone.
> 
> By spec it should be called CD and STE value, but frankly I like CDTE
> and STE a little better (context descriptor table entry) as it evokes
> a sense of similarity. I don't care for the words 's1_cfg' and
> 's2_cfg' to represent the CD and STE, that is pretty confusing now
> that we have more uses for the CD and STE's than simple s1/s2 IO page
> tables.

We have STE and PTE, so CDTE sounds legit to me, though probably
it'd be safer by just following the spec? We can always add kdoc
and comments to make things clear.

@Michael,
Would it be possible for you to update a v5, following Jason's
suggestion overall? And I think we can have a smaller refactor
series first without set_dev_pasid, to have a common codeline
sooner for us to add new features, such as set_dev_pasid, the
use case of IDENTITY default substream, and the nesting series.
I will help testing with some pasid/non-pasid use cases too.

> > Yet two cases that would waste the preallocated CD table:
> > 1) If a master with ssid_bits=0 gets attached to an IDENITY S1
> >    domain, it sets CONFIG=BYPASS in the STE, which wastes the
> >    single-entry CD table, unlike currently the driver bypassing
> >    the allocation of a CD table at all.
> > 2) When enabling nested translation, we should replace all the
> >    S1 fields in the STE with guest/user values. So, the kernel-
> >    level CD table (either single-entry or multi-entry) in the
> >    master struct will not be used. Although this seems to be
> >    unchanged since currently the driver wastes this too in the
> >    default domain?
> 
> As we discussed in another thread dynamic resizing of the CD table
> makes some sense. Eg keep it at one entry until PASID mode is enabled
> then resize it to the max PASID bits.

I see.

> But I view that as an improvement beyond here. It seems fine for now
> to pre allocate the full CD table and waste the memory. PASID capable
> devices are rare.

Yea, that'd be easier for us to move forward other features :)

> > With that, I still feel it clear and flexible. And I can simply
> > add my use case of attaching an IDENITY domain to the default
> > substream when having a multi-entry CD table.
> 
> Yes, with the above note about dynamically resizing the CD table, I
> think we generally have the idea that programming the CD, eg by
> installing an entry, can cause the CD tables's STE to (atomically)
> change.
> 
> Eg to toggle the S1DSS bit if the PASID 0 is IDENTITY, or to resize
> the table if we exceed the PASID space.

Yea, we have enable_pasid() so probably can resize over there.

> Longer term we'd also need a way to setup IDENTITY domains for !0
> substreams as well too (eg for SIOV). It is too bad the CDTE doesn't
> have a bit for bypass. I suppose it will need dummy 1:1 IO page tables
> or something.

Oh. I haven't thought about that far. Noted it down.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> @Michael,
> Would it be possible for you to update a v5, following Jason's
> suggestion overall? And I think we can have a smaller refactor
> series first without set_dev_pasid, to have a common codeline
> sooner for us to add new features, such as set_dev_pasid, the
> use case of IDENTITY default substream, and the nesting series.
> I will help testing with some pasid/non-pasid use cases too.

Want to make sure I fully understand these last few messages first. At
a high level, we want:
1. arm_smmu_master is allowed to own a CD table, but not an
STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
and we can probably get there with minimal changes.
2. arm_smmu_master shouldn't point to the currently active CD table
(which may or may not be the one it owns) or STE-precursor as a
shortcut. All code should figure it out by looking at the master's
currently attached domain (functionality could be provided by helper).
3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
STE/CD for a domain should either be computed right when it is
written, or computed ahead of time and stored as a copy in the
smmu-domain.
Is that right?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Will Deacon]

On Fri, Jul 14, 2023 at 05:12:35PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > @Michael,
> > Would it be possible for you to update a v5, following Jason's
> > suggestion overall? And I think we can have a smaller refactor
> > series first without set_dev_pasid, to have a common codeline
> > sooner for us to add new features, such as set_dev_pasid, the
> > use case of IDENTITY default substream, and the nesting series.
> > I will help testing with some pasid/non-pasid use cases too.
> 
> Want to make sure I fully understand these last few messages first. At
> a high level, we want:
> 1. arm_smmu_master is allowed to own a CD table, but not an
> STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
> and we can probably get there with minimal changes.
> 2. arm_smmu_master shouldn't point to the currently active CD table
> (which may or may not be the one it owns) or STE-precursor as a
> shortcut. All code should figure it out by looking at the master's
> currently attached domain (functionality could be provided by helper).
> 3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
> STE/CD for a domain should either be computed right when it is
> written, or computed ahead of time and stored as a copy in the
> smmu-domain.
> Is that right?

To be honest, I'm having a hard time following the thread. I'd like to
avoid renaming things "for fun", so let's try to focus on the actual
restructuring initially. I'd also like to see what this "computing" of
the SMMU data structures actually looks like -- the information stored
in the 'arm_smmu_master' structure isn't redundant, so what's the problem?

If you all grok this, then I'm happy to wait for the patches, but I don't
want you to go away and spend lots of effort hacking something which doesn't
end up being what folks are asking for.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Fri, Jul 14, 2023 at 05:12:35PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > @Michael,
> > Would it be possible for you to update a v5, following Jason's
> > suggestion overall? And I think we can have a smaller refactor
> > series first without set_dev_pasid, to have a common codeline
> > sooner for us to add new features, such as set_dev_pasid, the
> > use case of IDENTITY default substream, and the nesting series.
> > I will help testing with some pasid/non-pasid use cases too.
> 
> Want to make sure I fully understand these last few messages first. At
> a high level, we want:
> 1. arm_smmu_master is allowed to own a CD table, but not an
> STE-precursor (s1_cfg/s2_cfg). The s1_cfg is practically already that,
> and we can probably get there with minimal changes.

Yah

> 2. arm_smmu_master shouldn't point to the currently active CD table
> (which may or may not be the one it owns) or STE-precursor as a
> shortcut. All code should figure it out by looking at the master's
> currently attached domain (functionality could be provided by
> helper).

I think that is close.

Most likely the master should have a pointer to an STE owning
iommu_domain. If that is !NULL then the master's CD table is not used
and the STE is computed from that iommu_domain. Essentially it says
that iommu_domain owns the entire RID.

Otherwise the STE comes from the master's CD table, and this means the
master is in SSID mode.

> 3. arm_smmu_domain shouldn't pre-generate any STE-precursors. The
> STE/CD for a domain should either be computed right when it is
> written, or computed ahead of time and stored as a copy in the
> smmu-domain.

I think this is cleaner since alot of the STE/CD calculation is either
constant or copying data from other places in the domain struct, but
maybe you'll find this is wrong. At least it is not super important at
this point.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Fri, Jul 14, 2023 at 12:41 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> The master object owns an optional CD table. If it is exsists it is
> used by every domain that is attached to that master.
>
> In the code flow there are two entry points to attach a domain, attach
> to a PASID or attach to a RID.
>
> For attach to PASID the code should always force the master to have a
> CD table and then attach the domain to the CD table.
>
> For attach to RID the code should do a bunch of checks and decide if
> it should force the master to have a CD table and attach the domain to
> that, or directly attach the domain to the STE.

 Yes. This is the current flow (except that we fail instead of forcing
when a CD table isn't already attached in the PASID flow).
owned_s1_cfg is simply a pre-allocated version of your optional CD
table.

> When the master gains a CD table then the CD table object becomes
> attached to the STE. In all cases we should be able to point to the
> object the STE points at and don't need a cfg or pointer to cfg since
> the object itself can provide the cfg.

Ok, practically speaking, are we just talking about reverting patch 3
and keeping a handle to the primary domain in arm_smmu_master? Instead
of directly accessing the currently active CD table using
arm_smmu_master->s1_cfg, you'd like set_dev_pasid() to look for it by
investigating what kind of domain is attached, and reaching to the
pre-alllocated/optional CD table otherwise if necessary?

> > We want to write something (page-table pointer) to a common CD
> > table. Where should the s1_cfg which owns that common table live?
>
> I would suggest a 'cd table struct' that as all the stuff related to
> the CD table, including an API to cacluate the STE this CD table
> requires. If not in actual code with a real struct, then in a logical
> sense in that a chunk of the master struct is the "CD table".

Sure, that's almost exactly what s1_cfg is today (with these patches)....
  * s1_cfg.arm_smmu_ctx_desc_cfg describes the CD table
  * s1_cfg.s1fmt and s1_cfg.s1cdmax describes attributes of that CD
table. These could technically be deduced-back from
arm_smmu_ctx_desc_cfg's l1_desc and num_l1_ents
  * s1_cfg.stall_enabled was introduced by patch 4 and in retrospect
does not belong there at all. It should have gone in arm_smmu_ctx_desc
instead (ignoring the whole should arm_smmu_ctx_desc even be
precomputed in the first place topic for a second).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Fri, Jul 14, 2023 at 04:02:50PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 12:41 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > The master object owns an optional CD table. If it is exsists it is
> > used by every domain that is attached to that master.
> >
> > In the code flow there are two entry points to attach a domain, attach
> > to a PASID or attach to a RID.
> >
> > For attach to PASID the code should always force the master to have a
> > CD table and then attach the domain to the CD table.
> >
> > For attach to RID the code should do a bunch of checks and decide if
> > it should force the master to have a CD table and attach the domain to
> > that, or directly attach the domain to the STE.
> 
>  Yes. This is the current flow (except that we fail instead of forcing
> when a CD table isn't already attached in the PASID flow).
> owned_s1_cfg is simply a pre-allocated version of your optional CD
> table.

Really? That seems like a terrible name for the CD table.

> > When the master gains a CD table then the CD table object becomes
> > attached to the STE. In all cases we should be able to point to the
> > object the STE points at and don't need a cfg or pointer to cfg since
> > the object itself can provide the cfg.
> 
> Ok, practically speaking, are we just talking about reverting patch 3
> and keeping a handle to the primary domain in arm_smmu_master?

I think the master should have a pointer to the iommu_domain that owns
the STE or if NULL the master should assign its internal CD table to
the STE.

The iommu_domain structs should not have any references to a CD table.

> > I would suggest a 'cd table struct' that as all the stuff related to
> > the CD table, including an API to cacluate the STE this CD table
> > requires. If not in actual code with a real struct, then in a logical
> > sense in that a chunk of the master struct is the "CD table".
> 
> Sure, that's almost exactly what s1_cfg is today (with these patches)....
>   * s1_cfg.arm_smmu_ctx_desc_cfg describes the CD table
>   * s1_cfg.s1fmt and s1_cfg.s1cdmax describes attributes of that CD
> table. These could technically be deduced-back from
> arm_smmu_ctx_desc_cfg's l1_desc and num_l1_ents

Yes, this makes sense, there is some redundancy here for sure.

patch 1 makes sense, arm_smmu_ctx_desc is effectively the Context
Descriptor Entry, and it belongs in the domain

patch 2 should delete arm_smmu_s1_cfg and just put
arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
a weird name for the contex descriptor table, but it is much less
weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.

patch 3 I don't understand, we should not add something called
s1_cfg/s2_cfg to the master. The master should have
'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'

patch 4 should have everything working with the cd table accept a
'struct arm_smmu_ctx_desc_cfg *', eg arm_smmu_get_cd_ptr (get a
pointer to a CD entry in the CD table).

patch 5 makes sense, but something seems odd about the order as we
somehow half moved it in patch 2?

My suggestion for patch structure is to start by cleaning up the CD
table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
and then as the last step just move the arm_smmu_ctx_desc_cfg from the
iommu_domain to the master.

And that is a nice little series on its own - you end up with a shared
CD table in the master, and no CD table in any domains.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

On Fri, Jul 14, 2023 at 9:21 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> patch 2 should delete arm_smmu_s1_cfg and just put
> arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
> a weird name for the contex descriptor table, but it is much less
> weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.

s1fmt is fairly trivial to replace but s1cdmax requires inversing
previous computations. I don't really buy that getting rid of it
simplifies anything, even if it's technically redundant.

> patch 3 I don't understand, we should not add something called
> s1_cfg/s2_cfg to the master. The master should have
> 'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'

This was simply meant to be a more convenient way of finding the
currently active cdtable from the
arm_smmu_write_ctx_desc/write_strtab_ent functions without having to
inspect the currently attached domain. But sure, that's easy enough to
revert.

> patch 5 makes sense, but something seems odd about the order as we
> somehow half moved it in patch 2?
Ack; patch 2 can be reordered to come after patch 4, or even squashed
with 5 if you prefer.

> My suggestion for patch structure is to start by cleaning up the CD
> table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
> redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
> 'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
> and then as the last step just move the arm_smmu_ctx_desc_cfg from the
> iommu_domain to the master.
>
> And that is a nice little series on its own - you end up with a shared
> CD table in the master, and no CD table in any domains.

I don't entirely buy that refactoring s1_cfg is worth the extra
effort, nor that it should be tied to this patch series. This series
already makes s1_cfg behave as the CD table; whether we want to
entirely get rid of pre-computed data useful for writing an STE sounds
like a separate cleanup.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Mon, Jul 17, 2023 at 06:06:19PM +0800, Michael Shavit wrote:
> On Fri, Jul 14, 2023 at 9:21 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > patch 2 should delete arm_smmu_s1_cfg and just put
> > arm_smmu_ctx_desc_cfg directly in the master. arm_smmu_ctx_desc_cfg is
> > a weird name for the contex descriptor table, but it is much less
> > weird than s1_cfg. As you say s1fmt/s1cdmax are redundant.
> 
> s1fmt is fairly trivial to replace but s1cdmax requires inversing
> previous computations. I don't really buy that getting rid of it
> simplifies anything, even if it's technically redundant.

Then store s1cdmax in the arm_smmu_ctx_desc_cfg and still get rid of
arm_smmu_s1_cfg

The point is to get rid of "s1_cfg" entirely as language in the driver
*because it makes zero sense now*. Prior to your restructuring it was
sort of the STE to use for the S1 page table which had an embedded
CD. After all this change it isn't realated to a S1 page table anymore
at all.

> > patch 3 I don't understand, we should not add something called
> > s1_cfg/s2_cfg to the master. The master should have
> > 'arm_smmu_ctx_desc_cfg cd_table' and 'arm_smmu_domain ste_domain'
> 
> This was simply meant to be a more convenient way of finding the
> currently active cdtable from the

There is only one (meaningful) choice though, the cdtable is either
the master's default CD table or it isn't. You detect that by checking
for NULL ste_domain

> > My suggestion for patch structure is to start by cleaning up the CD
> > table object. Make arm_smmu_ctx_desc_cfg the CD table, remove the
> > redudencies, remove arm_s1_cfg, clean the CD table APIs to only use
> > 'struct arm_smmu_ctx_desc_cfg *', add the 'ste_domain' to the master,
> > and then as the last step just move the arm_smmu_ctx_desc_cfg from the
> > iommu_domain to the master.
> >
> > And that is a nice little series on its own - you end up with a shared
> > CD table in the master, and no CD table in any domains.
> 
> I don't entirely buy that refactoring s1_cfg is worth the extra
> effort, nor that it should be tied to this patch series. This series
> already makes s1_cfg behave as the CD table; whether we want to
> entirely get rid of pre-computed data useful for writing an STE sounds
> like a separate cleanup.

s1_cfg is a terrible name to describe the cd table. Please do the
tiny bit extra to get rid of it for clarity. 

Jason


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

Haven't had time to address this yet; but Ack on last message. Will
address in a v5 series.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

Sorry for the delay; I'm trying to refactor these patches now.

> I think the master should have a pointer to the iommu_domain that owns
> the STE or if NULL the master should assign its internal CD table to
> the STE.
Just to clarify, does the nested domain patch series require writing
CDs into the user-space's CD table using arm_smmu_write_ctx_desc()? Or
is there any other requirement for writing a CD into a domain-owned CD
table from arm_smmu_write_ctx_desc?


Writing a CD entry to the CD table involves some dependencies on the
master(s) that the table is attached to:

1. The CD entries STALL bit value in arm_smmu_write_ctx_desc depends
on the master (e.g. if STALL_FORCE is set on the smmu device). This
could potentially be encoded in arm_smmu_ctx_desc_cfg, at which point
that CD table is only attachable to masters with the same
stall_enabled value.
2. arm_smmu_write_ctx_desc must sync the CD for the attached master(s)
in the middle of writing CD entry.

This is all easier to handle in arm_smmu_write_ctx_desc if the table
is always owned by the master.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Thu, Jul 27, 2023 at 07:22:05PM +0800, Michael Shavit wrote:
> Sorry for the delay; I'm trying to refactor these patches now.
> 
> > I think the master should have a pointer to the iommu_domain that owns
> > the STE or if NULL the master should assign its internal CD table to
> > the STE.
> Just to clarify, does the nested domain patch series require writing
> CDs into the user-space's CD table using arm_smmu_write_ctx_desc()?

No, CD entries in nested CD tables are written by userspace only.

> Or is there any other requirement for writing a CD into a
> domain-owned CD table from arm_smmu_write_ctx_desc?

Not that I know of.

The only time the kernel writes a CD entry is to the shared CD table
stored in the master.

> 1. The CD entries STALL bit value in arm_smmu_write_ctx_desc depends
> on the master (e.g. if STALL_FORCE is set on the smmu device). This
> could potentially be encoded in arm_smmu_ctx_desc_cfg, at which point
> that CD table is only attachable to masters with the same
> stall_enabled value.

For cleanness I would orgnize it like this.

> 2. arm_smmu_write_ctx_desc must sync the CD for the attached master(s)
> in the middle of writing CD entry.
>
> This is all easier to handle in arm_smmu_write_ctx_desc if the table
> is always owned by the master.

I think it is fine if you start with a shared CD table being 1:1 with
a single master.

Making the CD table shared between masters (eg for multi-device-group)
is a micro-optimization, and I'm not sure we have workloads where it
is worthwhile. We already block PASID support for multi-device-group.

Though resizable CD table is probably a better place to put efforts.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Michael Shavit]

Awesome that helps a lot. These are the patches I have in mind:

1. Move ctx_desc out of s1_cfg
2. Replace domain->s1_cfg with `struct arm_smmu_ctx_desc_cfg cd_table`
3. Move stall_enabled from domain to arm_smmu_ctx_desc_cfg
4. Accept an arm_smmu_master instead of an arm_smmu_domain as the
parameter for arm_smmu_write_ctx_desc
--- arm_smmu_write_ctx_desc can simply get the cd_table it writes to
from master->domain->cd_table; this get's replaced by master->cd_table
in subsequent commits.
--- SVA is weird if we cut changes here: it iterates over all masters
a domain is attached to in order to call arm_smmu_write_ctx_desc(),
which ends up writing to the shared cd_table since in master->domain.
This becomes more sensible once masters stop sharing the cd _table in
subsequent commits.
--- arm_smmu_write_ctx_desc is used to sync the cd for all masters the
domain was attached to. Now that SVA is calling it in a loop, and to
break the dependency on arm_smmu_domain, we should only sync for the
master passed in as the parameter
5. Introduce a skip_cd_sync parameter to arm_smmu_write_ctx_desc so
that arm_smmu_domain_finalise_s1 can skip the sync_cd before the
cd_table is attached to the master. Before the previous commit,
arm_smmu_domain_finalise_s1 was calling arm_smmu_write_ctx_desc()
before adding the master to the domain->devices list, which was used
by arm_sync_smmu() to issue sync commands to masters. This
optimization was broken by the previous commit since we stopped
depending on domain->devices.
6. Move cd_table from domain to arm_smmu_master->cd_table.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/13] iommu/arm-smmu-v3: Add smmu_s1_cfg to smmu_master

[Jason Gunthorpe]

On Thu, Jul 27, 2023 at 10:04:04PM +0800, Michael Shavit wrote:
> Awesome that helps a lot. These are the patches I have in mind:
> 
> 1. Move ctx_desc out of s1_cfg
> 2. Replace domain->s1_cfg with `struct arm_smmu_ctx_desc_cfg cd_table`
> 3. Move stall_enabled from domain to arm_smmu_ctx_desc_cfg
> 4. Accept an arm_smmu_master instead of an arm_smmu_domain as the
> parameter for arm_smmu_write_ctx_desc
> --- arm_smmu_write_ctx_desc can simply get the cd_table it writes to
> from master->domain->cd_table; this get's replaced by master->cd_table
> in subsequent commits.

Makes sense

> --- SVA is weird if we cut changes here: it iterates over all masters
> a domain is attached to in order to call arm_smmu_write_ctx_desc(),
> which ends up writing to the shared cd_table since in master->domain.
> This becomes more sensible once masters stop sharing the cd _table in
> subsequent commits.

Seems like it is OK inside the series

> --- arm_smmu_write_ctx_desc is used to sync the cd for all masters the
> domain was attached to. Now that SVA is calling it in a loop, and to
> break the dependency on arm_smmu_domain, we should only sync for the
> master passed in as the parameter

Sounds correct

> 5. Introduce a skip_cd_sync parameter to arm_smmu_write_ctx_desc so
> that arm_smmu_domain_finalise_s1 can skip the sync_cd before the
> cd_table is attached to the master. Before the previous commit,
> arm_smmu_domain_finalise_s1 was calling arm_smmu_write_ctx_desc()
> before adding the master to the domain->devices list, which was used
> by arm_sync_smmu() to issue sync commands to masters. This
> optimization was broken by the previous commit since we stopped
> depending on domain->devices.

Tracking if the cd table is not yet installed in a STE might be a
cleaner approach than a flag?

> 6. Move cd_table from domain to arm_smmu_master->cd_table.

Yep

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 03/13] iommu/arm-smmu-v3: Refactor write_strtab_ent

[Michael Shavit]

Explicity keep track of the s1_cfg and s2_cfg that are attached to a
master in arm_smmu_master, regardless of whether they are owned by
arm_smmu_master, arm_smmu_domain or userspace.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 37 +++++++++------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
 2 files changed, 17 insertions(+), 22 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 023769f5ca79a..d79c6ef5d6ed4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1269,10 +1269,9 @@ 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_device *smmu = master->smmu;
 	struct arm_smmu_s1_cfg *s1_cfg = NULL;
 	struct arm_smmu_s2_cfg *s2_cfg = NULL;
-	struct arm_smmu_domain *smmu_domain = NULL;
 	struct arm_smmu_cmdq_ent prefetch_cmd = {
 		.opcode		= CMDQ_OP_PREFETCH_CFG,
 		.prefetch	= {
@@ -1280,24 +1279,10 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
 		},
 	};
 
-	if (master) {
-		smmu_domain = master->domain;
-		smmu = master->smmu;
-	}
-
-	if (smmu_domain) {
-		switch (smmu_domain->stage) {
-		case ARM_SMMU_DOMAIN_S1:
-			s1_cfg = &smmu_domain->s1_cfg;
-			break;
-		case ARM_SMMU_DOMAIN_S2:
-		case ARM_SMMU_DOMAIN_NESTED:
-			s2_cfg = &smmu_domain->s2_cfg;
-			break;
-		default:
-			break;
-		}
-	}
+	if (master->s1_cfg)
+		s1_cfg = master->s1_cfg;
+	else if (master->s2_cfg)
+		s2_cfg = master->s2_cfg;
 
 	if (val & STRTAB_STE_0_V) {
 		switch (FIELD_GET(STRTAB_STE_0_CFG, val)) {
@@ -1319,8 +1304,8 @@ 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 && disable_bypass)
+	if (!(s1_cfg || s2_cfg)) {
+		if (disable_bypass)
 			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_ABORT);
 		else
 			val |= FIELD_PREP(STRTAB_STE_0_CFG, STRTAB_STE_0_CFG_BYPASS);
@@ -2401,6 +2386,8 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 
 	master->domain = NULL;
 	master->ats_enabled = false;
+	master->s1_cfg = NULL;
+	master->s2_cfg = NULL;
 	arm_smmu_install_ste_for_dev(master);
 }
 
@@ -2454,6 +2441,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	master->domain = smmu_domain;
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+		master->s1_cfg = &smmu_domain->s1_cfg;
+	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
+		   smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED) {
+		master->s2_cfg = &smmu_domain->s2_cfg;
+	}
 
 	/*
 	 * The SMMU does not support enabling ATS with bypass. When the STE is
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 053cc14c23969..3c614fbe2b8b9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -689,6 +689,8 @@ struct arm_smmu_master {
 	struct list_head		domain_head;
 	struct arm_smmu_stream		*streams;
 	struct arm_smmu_s1_cfg		owned_s1_cfg;
+	struct arm_smmu_s1_cfg		*s1_cfg;
+	struct arm_smmu_s2_cfg		*s2_cfg;
 	unsigned int			num_streams;
 	bool				ats_enabled;
 	bool				stall_enabled;
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/13] iommu/arm-smmu-v3: Refactor write_strtab_ent

[Nicolin Chen]

On Wed, Jun 21, 2023 at 02:37:15PM +0800, Michael Shavit wrote:

> Explicity keep track of the s1_cfg and s2_cfg that are attached to a
> master in arm_smmu_master, regardless of whether they are owned by
> arm_smmu_master, arm_smmu_domain or userspace.

An s1_cfg is in a master while an s2_cfg is in a domain. So we
we know where they are. I kinda get that having these two ptrs
could ease the cleanup, especially in arm_smmu_write_strtab_ent.

> 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 053cc14c23969..3c614fbe2b8b9 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -689,6 +689,8 @@ struct arm_smmu_master {
>         struct list_head                domain_head;
>         struct arm_smmu_stream          *streams;
>         struct arm_smmu_s1_cfg          owned_s1_cfg;
> +       struct arm_smmu_s1_cfg          *s1_cfg;
> +       struct arm_smmu_s2_cfg          *s2_cfg;

Yet, this looks a bit overcomplicated to me by having an s1_cfg
that points to master->owned_s1_cfg?

I am wondering if it'd be neater to have a new struct for STE,
replacing the owned_s1_cfg, s1_cfg and s2_cfg above. It could
be something like struct arm_smmu_cmdq_ent, which contains all
the STE fields so that in arm_smmu_write_strtab_ent we can just
plainly copy to the dst[]. Also, whenever a device attaches to
a domain having the necessary info needed by the STE, we update
the STE struct owned by the master.

Thanks
Nic

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 04/13] iommu/arm-smmu-v3: Refactor write_ctx_desc

[Michael Shavit]

Update arm_smmu_write_ctx_desc and downstream functions to be agnostic
of the CD table's ownership. Note that in practice, it will only be
called in cases where the table is owned by arm_smmu_master. Whether
arm_smmu_write_ctx_desc will trigger a sync is also made part of the
API.

Note that this change isn't a nop refactor since SVA will call
arm_smmu_write_ctx_desc in a loop for every master the domain is
attached to despite the fact that they all share the same CD table. Note
that the next commit ceases this sharing of the s1_cfg which makes that
loop necessary.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 36 ++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 80 +++++++++++--------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  6 +-
 3 files changed, 81 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 968559d625c40..48fa8eb271a45 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -45,10 +45,12 @@ static struct arm_smmu_ctx_desc *
 arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 {
 	int ret;
+	unsigned long flags;
 	u32 new_asid;
 	struct arm_smmu_ctx_desc *cd;
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain;
+	struct arm_smmu_master *master;
 
 	cd = xa_load(&arm_smmu_asid_xa, asid);
 	if (!cd)
@@ -80,7 +82,11 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	 * be some overlap between use of both ASIDs, until we invalidate the
 	 * TLB.
 	 */
-	arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
+	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+		arm_smmu_write_ctx_desc(smmu, master->s1_cfg, master, 0, cd);
+	}
+	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
 	/* Invalidate TLB entries previously associated with that context */
 	arm_smmu_tlb_inv_asid(smmu, asid);
@@ -211,6 +217,8 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
 	struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn);
 	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
+	struct arm_smmu_master *master;
+	unsigned long flags;
 
 	mutex_lock(&sva_lock);
 	if (smmu_mn->cleared) {
@@ -222,7 +230,12 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events,
 	 * but disable translation.
 	 */
-	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, &quiet_cd);
+	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+		arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
+					mm->pasid, &quiet_cd);
+	}
+	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
 	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
 	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
@@ -248,8 +261,10 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 			  struct mm_struct *mm)
 {
 	int ret;
+	unsigned long flags;
 	struct arm_smmu_ctx_desc *cd;
 	struct arm_smmu_mmu_notifier *smmu_mn;
+	struct arm_smmu_master *master;
 
 	list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) {
 		if (smmu_mn->mn.mm == mm) {
@@ -279,7 +294,12 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 		goto err_free_cd;
 	}
 
-	ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd);
+	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+		ret = arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg,
+					      master, mm->pasid, cd);
+	}
+	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 	if (ret)
 		goto err_put_notifier;
 
@@ -296,15 +316,23 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 
 static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 {
+	unsigned long flags;
 	struct mm_struct *mm = smmu_mn->mn.mm;
 	struct arm_smmu_ctx_desc *cd = smmu_mn->cd;
+	struct arm_smmu_master *master;
 	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
 
 	if (!refcount_dec_and_test(&smmu_mn->refs))
 		return;
 
 	list_del(&smmu_mn->list);
-	arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, NULL);
+
+	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
+	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+		arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
+					mm->pasid, NULL);
+	}
+	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
 	/*
 	 * If we went through clear(), we've already invalidated, and no
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 d79c6ef5d6ed4..b6f7cf60f8f3d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -965,14 +965,13 @@ void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid)
 	arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
 }
 
-static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
+/* master may be null */
+static void arm_smmu_sync_cd(struct arm_smmu_master *master,
 			     int ssid, bool leaf)
 {
 	size_t i;
-	unsigned long flags;
-	struct arm_smmu_master *master;
 	struct arm_smmu_cmdq_batch cmds;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_device *smmu;
 	struct arm_smmu_cmdq_ent cmd = {
 		.opcode	= CMDQ_OP_CFGI_CD,
 		.cfgi	= {
@@ -981,16 +980,15 @@ static void arm_smmu_sync_cd(struct arm_smmu_domain *smmu_domain,
 		},
 	};
 
-	cmds.num = 0;
+	if (!master)
+		return;
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-		for (i = 0; i < master->num_streams; i++) {
-			cmd.cfgi.sid = master->streams[i].id;
-			arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
-		}
+	smmu = master->smmu;
+	cmds.num = 0;
+	for (i = 0; i < master->num_streams; i++) {
+		cmd.cfgi.sid = master->streams[i].id;
+		arm_smmu_cmdq_batch_add(smmu, &cmds, &cmd);
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
 	arm_smmu_cmdq_batch_submit(smmu, &cmds);
 }
@@ -1020,16 +1018,18 @@ static void arm_smmu_write_cd_l1_desc(__le64 *dst,
 	WRITE_ONCE(*dst, cpu_to_le64(val));
 }
 
-static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
+/* master may be null */
+static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_device *smmu,
+				   struct arm_smmu_s1_cfg *s1_cfg,
+				   struct arm_smmu_master *master,
 				   u32 ssid)
 {
 	__le64 *l1ptr;
 	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 = &s1_cfg->cdcfg;
 
-	if (smmu_domain->s1_cfg.s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
+	if (s1_cfg->s1fmt == STRTAB_STE_0_S1FMT_LINEAR)
 		return cdcfg->cdtab + ssid * CTXDESC_CD_DWORDS;
 
 	idx = ssid >> CTXDESC_SPLIT;
@@ -1041,13 +1041,21 @@ static __le64 *arm_smmu_get_cd_ptr(struct arm_smmu_domain *smmu_domain,
 		l1ptr = cdcfg->cdtab + idx * CTXDESC_L1_DESC_DWORDS;
 		arm_smmu_write_cd_l1_desc(l1ptr, l1_desc);
 		/* An invalid L1CD can be cached */
-		arm_smmu_sync_cd(smmu_domain, ssid, false);
+		arm_smmu_sync_cd(master, ssid, false);
 	}
 	idx = ssid & (CTXDESC_L2_ENTRIES - 1);
 	return l1_desc->l2ptr + idx * CTXDESC_CD_DWORDS;
 }
 
-int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
+/*
+ * master must be provided if a CD sync is required but may be null otherwise
+ * (such as when the CD table isn't inserted into the STE yet, or is about to
+ * be detached.
+ */
+int arm_smmu_write_ctx_desc(struct arm_smmu_device *smmu,
+			    struct arm_smmu_s1_cfg *s1_cfg,
+			    struct arm_smmu_master *master,
+			    int ssid,
 			    struct arm_smmu_ctx_desc *cd)
 {
 	/*
@@ -1065,10 +1073,10 @@ 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 << s1_cfg->s1cdmax)))
 		return -E2BIG;
 
-	cdptr = arm_smmu_get_cd_ptr(smmu_domain, ssid);
+	cdptr = arm_smmu_get_cd_ptr(smmu, s1_cfg, master, ssid);
 	if (!cdptr)
 		return -ENOMEM;
 
@@ -1092,11 +1100,11 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
 		cdptr[3] = cpu_to_le64(cd->mair);
 
 		/*
-		 * STE is live, and the SMMU might read dwords of this CD in any
-		 * order. Ensure that it observes valid values before reading
-		 * V=1.
+		 * STE may be live, and the SMMU might read dwords of this CD
+		 * in any order. Ensure that it observes valid values before
+		 * reading V=1.
 		 */
-		arm_smmu_sync_cd(smmu_domain, ssid, true);
+		arm_smmu_sync_cd(master, ssid, true);
 
 		val = cd->tcr |
 #ifdef __BIG_ENDIAN
@@ -1108,7 +1116,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
 			FIELD_PREP(CTXDESC_CD_0_ASID, cd->asid) |
 			CTXDESC_CD_0_V;
 
-		if (smmu_domain->stall_enabled)
+		if (s1_cfg->stall_enabled)
 			val |= CTXDESC_CD_0_S;
 	}
 
@@ -1122,7 +1130,7 @@ int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
 	 *   without first making the structure invalid.
 	 */
 	WRITE_ONCE(cdptr[0], cpu_to_le64(val));
-	arm_smmu_sync_cd(smmu_domain, ssid, true);
+	arm_smmu_sync_cd(master, ssid, true);
 	return 0;
 }
 
@@ -1136,6 +1144,7 @@ static int arm_smmu_init_s1_cfg(struct arm_smmu_master *master,
 	struct arm_smmu_ctx_desc_cfg *cdcfg = &cfg->cdcfg;
 
 	cfg->s1cdmax = master->ssid_bits;
+	cfg->stall_enabled = master->stall_enabled;
 	max_contexts = 1 << cfg->s1cdmax;
 
 	if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
@@ -2093,8 +2102,6 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		goto out_unlock;
 
-	smmu_domain->stall_enabled = master->stall_enabled;
-
 	ret = arm_smmu_init_s1_cfg(master, cfg);
 	if (ret)
 		goto out_free_asid;
@@ -2110,12 +2117,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;
 
-	/*
-	 * Note that this will end up calling arm_smmu_sync_cd() before
-	 * the master has been added to the devices list for this domain.
-	 * This isn't an issue because the STE hasn't been installed yet.
-	 */
-	ret = arm_smmu_write_ctx_desc(smmu_domain, 0, cd);
+	ret = arm_smmu_write_ctx_desc(smmu, cfg,
+				      NULL /*Not attached to a master yet */,
+				      0, cd);
 	if (ret)
 		goto out_free_cd_tables;
 
@@ -2386,6 +2390,11 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 
 	master->domain = NULL;
 	master->ats_enabled = false;
+	if (master->s1_cfg)
+		arm_smmu_write_ctx_desc(
+			master->smmu, master->s1_cfg,
+			NULL /* Skip sync since we detach the CD table next*/,
+			0, NULL);
 	master->s1_cfg = NULL;
 	master->s2_cfg = NULL;
 	arm_smmu_install_ste_for_dev(master);
@@ -2435,7 +2444,8 @@ 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 &&
-		   smmu_domain->stall_enabled != master->stall_enabled) {
+		   smmu_domain->s1_cfg.stall_enabled !=
+			   master->stall_enabled) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
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 3c614fbe2b8b9..00a493442d6f9 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -595,6 +595,7 @@ struct arm_smmu_s1_cfg {
 	struct arm_smmu_ctx_desc_cfg	cdcfg;
 	u8				s1fmt;
 	u8				s1cdmax;
+	bool				stall_enabled;
 };
 
 struct arm_smmu_s2_cfg {
@@ -713,7 +714,6 @@ struct arm_smmu_domain {
 	struct mutex				init_mutex; /* Protects smmu pointer */
 
 	struct io_pgtable_ops			*pgtbl_ops;
-	bool					stall_enabled;
 	atomic_t				nr_ats_masters;
 
 	enum arm_smmu_domain_stage		stage;
@@ -742,7 +742,9 @@ extern struct xarray arm_smmu_asid_xa;
 extern struct mutex arm_smmu_asid_lock;
 extern struct arm_smmu_ctx_desc quiet_cd;
 
-int arm_smmu_write_ctx_desc(struct arm_smmu_domain *smmu_domain, int ssid,
+int arm_smmu_write_ctx_desc(struct arm_smmu_device *smmu,
+			    struct arm_smmu_s1_cfg *s1_cfg,
+			    struct arm_smmu_master *smmu_master, int ssid,
 			    struct arm_smmu_ctx_desc *cd);
 void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
 void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 05/13] iommu/arm-smmu-v3: Use the master-owned s1_cfg

[Michael Shavit]

Insert CDs for STAGE_1 domains into a CD table owned by the
arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 43 ++++++---------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 --
 2 files changed, 12 insertions(+), 34 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 b6f7cf60f8f3d..08f440fe1da6d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2065,12 +2065,8 @@ 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;
-
 		/* Prevent SVA from touching the CD while we're freeing it */
 		mutex_lock(&arm_smmu_asid_lock);
-		if (cfg->cdcfg.cdtab)
-			arm_smmu_free_cd_tables(smmu, &cfg->cdcfg);
 		arm_smmu_free_asid(&smmu_domain->cd);
 		mutex_unlock(&arm_smmu_asid_lock);
 	} else {
@@ -2082,14 +2078,13 @@ 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)
 {
 	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 *cd = &smmu_domain->cd;
 	typeof(&pgtbl_cfg->arm_lpae_s1_cfg.tcr) tcr = &pgtbl_cfg->arm_lpae_s1_cfg.tcr;
 
@@ -2102,10 +2097,6 @@ static int arm_smmu_domain_finalise_s1(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		goto out_unlock;
 
-	ret = arm_smmu_init_s1_cfg(master, cfg);
-	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) |
@@ -2117,19 +2108,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(smmu, cfg,
-				      NULL /*Not attached to a master yet */,
-				      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, &cfg->cdcfg);
-out_free_asid:
-	arm_smmu_free_asid(cd);
 out_unlock:
 	mutex_unlock(&arm_smmu_asid_lock);
 	return ret;
@@ -2192,7 +2173,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:
@@ -2439,20 +2420,20 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	} 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->s1_cfg.s1cdmax) {
-		ret = -EINVAL;
-		goto out_unlock;
-	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
-		   smmu_domain->s1_cfg.stall_enabled !=
-			   master->stall_enabled) {
-		ret = -EINVAL;
-		goto out_unlock;
 	}
 
 	master->domain = smmu_domain;
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
-		master->s1_cfg = &smmu_domain->s1_cfg;
+		master->s1_cfg = &master->owned_s1_cfg;
+		ret = arm_smmu_write_ctx_desc(
+			smmu,
+			master->s1_cfg, NULL /*Not attached to a master yet */,
+			0, &smmu_domain->cd);
+		if (ret) {
+			master->s1_cfg = NULL;
+			master->domain = NULL;
+			goto out_unlock;
+		}
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
 		   smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED) {
 		master->s2_cfg = &smmu_domain->s2_cfg;
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 00a493442d6f9..dff0fa8345462 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -718,10 +718,7 @@ struct arm_smmu_domain {
 
 	enum arm_smmu_domain_stage		stage;
 	union {
-		struct {
 		struct arm_smmu_ctx_desc	cd;
-		struct arm_smmu_s1_cfg		s1_cfg;
-		};
 		struct arm_smmu_s2_cfg		s2_cfg;
 	};
 
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/13] iommu/arm-smmu-v3: Use the master-owned s1_cfg

[Nicolin Chen]

On Wed, Jun 21, 2023 at 02:37:17PM +0800, Michael Shavit wrote:
 
> Insert CDs for STAGE_1 domains into a CD table owned by the
> arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.

> @@ -718,10 +718,7 @@ struct arm_smmu_domain {
> 
>         enum arm_smmu_domain_stage              stage;
>         union {
> -               struct {
>                 struct arm_smmu_ctx_desc        cd;
> -               struct arm_smmu_s1_cfg          s1_cfg;
> -               };
>                 struct arm_smmu_s2_cfg          s2_cfg;
>         };

So the arm_smmu_domain looks like an object representing either:
1) a CD table + (if !IDENTITY) an S1 IOPT for default substream
2) an S2 IOPT

I wonder if we need something like struct arm_smmu_subdomain to
represent a substream, because now an IOMMU_DOMAIN_SVA domain is
represented by this struct arm_smmu_domain too, which is neither
of the two cases in the list above.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/13] iommu/arm-smmu-v3: Use the master-owned s1_cfg

[Nicolin Chen]

On Wed, Jul 12, 2023 at 06:57:33PM -0700, Nicolin Chen wrote:
> On Wed, Jun 21, 2023 at 02:37:17PM +0800, Michael Shavit wrote:
>  
> > Insert CDs for STAGE_1 domains into a CD table owned by the
> > arm_smmu_master. Remove the CD table that was owned by arm_smmu_domain.
> 
> > @@ -718,10 +718,7 @@ struct arm_smmu_domain {
> > 
> >         enum arm_smmu_domain_stage              stage;
> >         union {
> > -               struct {
> >                 struct arm_smmu_ctx_desc        cd;
> > -               struct arm_smmu_s1_cfg          s1_cfg;
> > -               };
> >                 struct arm_smmu_s2_cfg          s2_cfg;
> >         };
> 
> So the arm_smmu_domain looks like an object representing either:
> 1) a CD table + (if !IDENTITY) an S1 IOPT for default substream
> 2) an S2 IOPT

["]
> I wonder if we need something like struct arm_smmu_subdomain to
> represent a substream, because now an IOMMU_DOMAIN_SVA domain is
> represented by this struct arm_smmu_domain too, which is neither
> of the two cases in the list above.
["]

I got this part incorrect: an SVA domain is actually represented
by an arm_smmu_sva_domain, so there's no nested arm_smmu_domain
situation. And we use arm_smmu_domain for other purpose, where I
got confused.

Please ignore this part, though I am still wondering if we could
name it in a way having a clear topology.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 06/13] iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats

[Michael Shavit]

arm_smmu_enable_ats's call to inv_domain would trigger an invalidation
for all masters that a domain is attached to everytime it's attached to
another ATS-enabled master. It doesn't seem like those invalidations are
necessary, and it's easier to reason about arm_smmu_enable_ats if it
only issues invalidation commands for the current master.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
v1->v2: Fix commit message wrapping
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 08f440fe1da6d..dc7a59e87a2b4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2286,7 +2286,7 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 	pdev = to_pci_dev(master->dev);
 
 	atomic_inc(&smmu_domain->nr_ats_masters);
-	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
+	arm_smmu_atc_inv_master(master);
 	if (pci_enable_ats(pdev, stu))
 		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
 }
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

The arm-smmu-v3 driver keeps track of all masters that a domain is
attached to so that it can re-write their STEs when the domain's ASID is
upated by SVA. This tracking is also used to invalidate ATCs on all
masters that a domain is attached to.

This change introduces a new data structures to track all the CD entries
that a domain is attached to. This change is a pre-requisite to allow
domain attachment on non 0 SSIDs.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
v3->v4: Remove reference to the master's domain accidentally re-introduced
        during a rebase.
	Make arm_smmu_atc_inv_domain static.
v1->v2: Fix arm_smmu_atc_inv_cmd_set_ssid and other cosmetic changes
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 53 +++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 89 ++++++++++++-------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 18 ++--
 3 files changed, 105 insertions(+), 55 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 48fa8eb271a45..d07c08b53c5cf 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -51,6 +51,7 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_domain *smmu_domain;
 	struct arm_smmu_master *master;
+	struct arm_smmu_attached_domain *attached_domain;
 
 	cd = xa_load(&arm_smmu_asid_xa, asid);
 	if (!cd)
@@ -82,11 +83,14 @@ arm_smmu_share_asid(struct mm_struct *mm, u16 asid)
 	 * be some overlap between use of both ASIDs, until we invalidate the
 	 * TLB.
 	 */
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-		arm_smmu_write_ctx_desc(smmu, master->s1_cfg, master, 0, cd);
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
+			    domain_head) {
+		master = attached_domain->master;
+		arm_smmu_write_ctx_desc(smmu, master->s1_cfg, master,
+					attached_domain->ssid, cd);
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
 	/* Invalidate TLB entries previously associated with that context */
 	arm_smmu_tlb_inv_asid(smmu, asid);
@@ -210,7 +214,7 @@ static void arm_smmu_mm_invalidate_range(struct mmu_notifier *mn,
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_BTM))
 		arm_smmu_tlb_inv_range_asid(start, size, smmu_mn->cd->asid,
 					    PAGE_SIZE, false, smmu_domain);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, start, size);
+	arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, start, size);
 }
 
 static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
@@ -218,6 +222,7 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	struct arm_smmu_mmu_notifier *smmu_mn = mn_to_smmu(mn);
 	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
 	struct arm_smmu_master *master;
+	struct arm_smmu_attached_domain *attached_domain;
 	unsigned long flags;
 
 	mutex_lock(&sva_lock);
@@ -230,15 +235,21 @@ static void arm_smmu_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * DMA may still be running. Keep the cd valid to avoid C_BAD_CD events,
 	 * but disable translation.
 	 */
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
-		arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
-					mm->pasid, &quiet_cd);
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
+			    domain_head) {
+		master = attached_domain->master;
+		/*
+		 * SVA domains piggyback on the attached_domain with SSID 0.
+		 */
+		if (attached_domain->ssid == 0)
+			arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg,
+						master, mm->pasid, &quiet_cd);
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
 	arm_smmu_tlb_inv_asid(smmu_domain->smmu, smmu_mn->cd->asid);
-	arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+	arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, 0, 0);
 
 	smmu_mn->cleared = true;
 	mutex_unlock(&sva_lock);
@@ -265,6 +276,7 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 	struct arm_smmu_ctx_desc *cd;
 	struct arm_smmu_mmu_notifier *smmu_mn;
 	struct arm_smmu_master *master;
+	struct arm_smmu_attached_domain *attached_domain;
 
 	list_for_each_entry(smmu_mn, &smmu_domain->mmu_notifiers, list) {
 		if (smmu_mn->mn.mm == mm) {
@@ -294,12 +306,14 @@ arm_smmu_mmu_notifier_get(struct arm_smmu_domain *smmu_domain,
 		goto err_free_cd;
 	}
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
+			    domain_head) {
+		master = attached_domain->master;
 		ret = arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg,
 					      master, mm->pasid, cd);
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 	if (ret)
 		goto err_put_notifier;
 
@@ -319,6 +333,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	unsigned long flags;
 	struct mm_struct *mm = smmu_mn->mn.mm;
 	struct arm_smmu_ctx_desc *cd = smmu_mn->cd;
+	struct arm_smmu_attached_domain *attached_domain;
 	struct arm_smmu_master *master;
 	struct arm_smmu_domain *smmu_domain = smmu_mn->domain;
 
@@ -327,12 +342,14 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 
 	list_del(&smmu_mn->list);
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
+			    domain_head) {
+		master = attached_domain->master;
 		arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
 					mm->pasid, NULL);
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
 	/*
 	 * If we went through clear(), we've already invalidated, and no
@@ -340,7 +357,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	 */
 	if (!smmu_mn->cleared) {
 		arm_smmu_tlb_inv_asid(smmu_domain->smmu, cd->asid);
-		arm_smmu_atc_inv_domain(smmu_domain, mm->pasid, 0, 0);
+		arm_smmu_atc_inv_domain_ssid(smmu_domain, mm->pasid, 0, 0);
 	}
 
 	/* Frees smmu_mn */
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 dc7a59e87a2b4..65e2dfd28b7d8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1711,7 +1711,14 @@ static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
 }
 
 static void
-arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
+arm_smmu_atc_inv_cmd_set_ssid(int ssid, struct arm_smmu_cmdq_ent *cmd)
+{
+	cmd->substream_valid = !!ssid;
+	cmd->atc.ssid = ssid;
+}
+
+static void
+arm_smmu_atc_inv_to_cmd(unsigned long iova, size_t size,
 			struct arm_smmu_cmdq_ent *cmd)
 {
 	size_t log2_span;
@@ -1736,8 +1743,8 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size,
 	 */
 	*cmd = (struct arm_smmu_cmdq_ent) {
 		.opcode			= CMDQ_OP_ATC_INV,
-		.substream_valid	= !!ssid,
-		.atc.ssid		= ssid,
+		.substream_valid	= false,
+		.atc.ssid		= 0,
 	};
 
 	if (!size) {
@@ -1783,8 +1790,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_cmdq_batch cmds;
 
-	arm_smmu_atc_inv_to_cmd(0, 0, 0, &cmd);
-
+	arm_smmu_atc_inv_to_cmd(0, 0, &cmd);
 	cmds.num = 0;
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
@@ -1794,13 +1800,19 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
 }
 
-int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
-			    unsigned long iova, size_t size)
+/*
+ * If ssid is non-zero, issue atc invalidations with the given ssid instead of
+ * the one the domain is attached to. This is used by SVA since it's pasid
+ * attachments aren't recorded in smmu_domain yet.
+ */
+int arm_smmu_atc_inv_domain_ssid(struct arm_smmu_domain *smmu_domain, int ssid,
+				 unsigned long iova, size_t size)
 {
 	int i;
 	unsigned long flags;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_master *master;
+	struct arm_smmu_attached_domain *attached_domain;
 	struct arm_smmu_cmdq_batch cmds;
 
 	if (!(smmu_domain->smmu->features & ARM_SMMU_FEAT_ATS))
@@ -1823,25 +1835,37 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
 	if (!atomic_read(&smmu_domain->nr_ats_masters))
 		return 0;
 
-	arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
+	arm_smmu_atc_inv_to_cmd(iova, size, &cmd);
 
 	cmds.num = 0;
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
+			    domain_head) {
+		master = attached_domain->master;
 		if (!master->ats_enabled)
 			continue;
+		if (ssid != 0)
+			arm_smmu_atc_inv_cmd_set_ssid(ssid, &cmd);
+		else
+			arm_smmu_atc_inv_cmd_set_ssid(attached_domain->ssid, &cmd);
 
 		for (i = 0; i < master->num_streams; i++) {
 			cmd.atc.sid = master->streams[i].id;
 			arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
 		}
 	}
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
 	return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds);
 }
 
+static int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,
+				   unsigned long iova, size_t size)
+{
+	return arm_smmu_atc_inv_domain_ssid(smmu_domain, 0, iova, size);
+}
+
 /* IO_PGTABLE API */
 static void arm_smmu_tlb_inv_context(void *cookie)
 {
@@ -1863,7 +1887,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 		cmd.tlbi.vmid	= smmu_domain->s2_cfg.vmid;
 		arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
 	}
-	arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0);
+	arm_smmu_atc_inv_domain(smmu_domain, 0, 0);
 }
 
 static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
@@ -1951,7 +1975,7 @@ static void arm_smmu_tlb_inv_range_domain(unsigned long iova, size_t size,
 	 * Unfortunately, this can't be leaf-only since we may have
 	 * zapped an entire table.
 	 */
-	arm_smmu_atc_inv_domain(smmu_domain, 0, iova, size);
+	arm_smmu_atc_inv_domain(smmu_domain, iova, size);
 }
 
 void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
@@ -2031,8 +2055,8 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 		return NULL;
 
 	mutex_init(&smmu_domain->init_mutex);
-	INIT_LIST_HEAD(&smmu_domain->devices);
-	spin_lock_init(&smmu_domain->devices_lock);
+	INIT_LIST_HEAD(&smmu_domain->attached_domains);
+	spin_lock_init(&smmu_domain->attached_domains_lock);
 	INIT_LIST_HEAD(&smmu_domain->mmu_notifiers);
 
 	return &smmu_domain->domain;
@@ -2270,12 +2294,12 @@ static bool arm_smmu_ats_supported(struct arm_smmu_master *master)
 	return dev_is_pci(dev) && pci_ats_supported(to_pci_dev(dev));
 }
 
-static void arm_smmu_enable_ats(struct arm_smmu_master *master)
+static void arm_smmu_enable_ats(struct arm_smmu_master *master,
+				struct arm_smmu_domain *smmu_domain)
 {
 	size_t stu;
 	struct pci_dev *pdev;
 	struct arm_smmu_device *smmu = master->smmu;
-	struct arm_smmu_domain *smmu_domain = master->domain;
 
 	/* Don't enable ATS at the endpoint if it's not enabled in the STE */
 	if (!master->ats_enabled)
@@ -2291,10 +2315,9 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master)
 		dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu);
 }
 
-static void arm_smmu_disable_ats(struct arm_smmu_master *master)
+static void arm_smmu_disable_ats(struct arm_smmu_master *master,
+				 struct arm_smmu_domain *smmu_domain)
 {
-	struct arm_smmu_domain *smmu_domain = master->domain;
-
 	if (!master->ats_enabled)
 		return;
 
@@ -2358,18 +2381,17 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 {
 	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = master->domain;
+	struct arm_smmu_domain *smmu_domain = master->non_pasid_domain.domain;
 
 	if (!smmu_domain)
 		return;
 
-	arm_smmu_disable_ats(master);
+	arm_smmu_disable_ats(master, smmu_domain);
 
-	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
-	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_del(&master->non_pasid_domain.domain_head);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
-	master->domain = NULL;
 	master->ats_enabled = false;
 	if (master->s1_cfg)
 		arm_smmu_write_ctx_desc(
@@ -2378,6 +2400,7 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 			0, NULL);
 	master->s1_cfg = NULL;
 	master->s2_cfg = NULL;
+	master->non_pasid_domain.domain = NULL;
 	arm_smmu_install_ste_for_dev(master);
 }
 
@@ -2422,7 +2445,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		goto out_unlock;
 	}
 
-	master->domain = smmu_domain;
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		master->s1_cfg = &master->owned_s1_cfg;
 		ret = arm_smmu_write_ctx_desc(
@@ -2431,7 +2453,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			0, &smmu_domain->cd);
 		if (ret) {
 			master->s1_cfg = NULL;
-			master->domain = NULL;
 			goto out_unlock;
 		}
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
@@ -2449,13 +2470,17 @@ 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);
 
+	master->non_pasid_domain.master = master;
+	master->non_pasid_domain.domain = smmu_domain;
+	master->non_pasid_domain.ssid = 0;
 	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);
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_add(&master->non_pasid_domain.domain_head,
+		 &smmu_domain->attached_domains);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
 
-	arm_smmu_enable_ats(master);
+	arm_smmu_enable_ats(master, smmu_domain);
 
 out_unlock:
 	mutex_unlock(&smmu_domain->init_mutex);
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 dff0fa8345462..6929590530367 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -682,11 +682,19 @@ struct arm_smmu_stream {
 	struct rb_node			node;
 };
 
+/* List of {masters, ssid} that a domain is attached to */
+struct arm_smmu_attached_domain {
+	struct list_head	domain_head;
+	struct arm_smmu_domain  *domain;
+	struct arm_smmu_master  *master;
+	int			ssid;
+};
+
 /* SMMU private data for each master */
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
 	struct device			*dev;
-	struct arm_smmu_domain		*domain;
+	struct arm_smmu_attached_domain	non_pasid_domain;
 	struct list_head		domain_head;
 	struct arm_smmu_stream		*streams;
 	struct arm_smmu_s1_cfg		owned_s1_cfg;
@@ -724,8 +732,8 @@ struct arm_smmu_domain {
 
 	struct iommu_domain			domain;
 
-	struct list_head			devices;
-	spinlock_t				devices_lock;
+	struct list_head			attached_domains;
+	spinlock_t				attached_domains_lock;
 
 	struct list_head			mmu_notifiers;
 };
@@ -748,8 +756,8 @@ void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
 				 size_t granule, bool leaf,
 				 struct arm_smmu_domain *smmu_domain);
 bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd);
-int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
-			    unsigned long iova, size_t size);
+int arm_smmu_atc_inv_domain_ssid(struct arm_smmu_domain *smmu_domain, int ssid,
+				 unsigned long iova, size_t size);
 
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Nicolin Chen]

On Wed, Jun 21, 2023 at 02:37:19PM +0800, Michael Shavit wrote:
 
> +/* List of {masters, ssid} that a domain is attached to */
> +struct arm_smmu_attached_domain {
> +       struct list_head        domain_head;
> +       struct arm_smmu_domain  *domain;
> +       struct arm_smmu_master  *master;
> +       int                     ssid;
> +};
> +
>  /* SMMU private data for each master */
>  struct arm_smmu_master {
>         struct arm_smmu_device          *smmu;
>         struct device                   *dev;
> -       struct arm_smmu_domain          *domain;
> +       struct arm_smmu_attached_domain non_pasid_domain;
>         struct list_head                domain_head;
>         struct arm_smmu_stream          *streams;
>         struct arm_smmu_s1_cfg          owned_s1_cfg;
> @@ -724,8 +732,8 @@ struct arm_smmu_domain {
> 
>         struct iommu_domain                     domain;
> 
> -       struct list_head                        devices;
> -       spinlock_t                              devices_lock;
> +       struct list_head                        attached_domains;
> +       spinlock_t                              attached_domains_lock;

Yea, I think that this arm_smmu_attached_domain is similar to
the "subdomain" that I was talking about in the previous reply,
though having a list of attached domains under a domain doesn't
feel very clear.

Perhaps it would be good to have some renaming and kdoc too.

And since we have a group of subdomains that are simply indexed
by ssids, perhaps we can add an xarray to store a subdomain ptr
along with an ssid, replacing the list?

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

> And since we have a group of subdomains that are simply indexed
> by ssids, perhaps we can add an xarray to store a subdomain ptr
> along with an ssid, replacing the list?

Hmmmm, I think the only place where we search through the list for a
specific SSID is during remove_dev_pasid. We mostly use the list to
iterate over  all the masters/ssids that this domain is attached to.
I'm not sure if moving to an xarray to optimize the remove_dev_pasid
call is worth it (at the cost of iterations which IIUC would become N
log(N))

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Nicolin Chen]

On Fri, Jul 21, 2023 at 02:48:23PM +0800, Michael Shavit wrote:
> 
> > And since we have a group of subdomains that are simply indexed
> > by ssids, perhaps we can add an xarray to store a subdomain ptr
> > along with an ssid, replacing the list?
> 
> Hmmmm, I think the only place where we search through the list for a
> specific SSID is during remove_dev_pasid. We mostly use the list to
> iterate over  all the masters/ssids that this domain is attached to.
> I'm not sure if moving to an xarray to optimize the remove_dev_pasid
> call is worth it (at the cost of iterations which IIUC would become N
> log(N))

OK. That's fine.

Can we have a smaller rework series first? Let's have all data
structures defined. And then we can add features on top of it.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Nicolin Chen]

On Wed, Jun 21, 2023 at 02:37:19PM +0800, Michael Shavit wrote:

> -int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
> -                           unsigned long iova, size_t size)
> +/*
> + * If ssid is non-zero, issue atc invalidations with the given ssid instead of
> + * the one the domain is attached to. This is used by SVA since it's pasid
> + * attachments aren't recorded in smmu_domain yet.
> + */
> +int arm_smmu_atc_inv_domain_ssid(struct arm_smmu_domain *smmu_domain, int ssid,
> +                                unsigned long iova, size_t size)
[..]
> @@ -1823,25 +1835,37 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,
>         if (!atomic_read(&smmu_domain->nr_ats_masters))
>                 return 0;
> 
> -       arm_smmu_atc_inv_to_cmd(ssid, iova, size, &cmd);
> +       arm_smmu_atc_inv_to_cmd(iova, size, &cmd);
> 
>         cmds.num = 0;
> 
> -       spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -       list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> +       spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
> +       list_for_each_entry(attached_domain, &smmu_domain->attached_domains,
> +                           domain_head) {
> +               master = attached_domain->master;
>                 if (!master->ats_enabled)
>                         continue;
> +               if (ssid != 0)
> +                       arm_smmu_atc_inv_cmd_set_ssid(ssid, &cmd);
> +               else
> +                       arm_smmu_atc_inv_cmd_set_ssid(attached_domain->ssid, &cmd);
> 
>                 for (i = 0; i < master->num_streams; i++) {
>                         cmd.atc.sid = master->streams[i].id;
>                         arm_smmu_cmdq_batch_add(smmu_domain->smmu, &cmds, &cmd);
>                 }
>         }

And I don't quite get this part. Prior to this change, it issues
one ATC_INV command covering all ATC entries per comments inside
arm_smmu_atc_inv_to_cmd(). But now we replace that single command
with all attached subdomains in the list? Any reason for such a
change here?

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

> And I don't quite get this part. Prior to this change, it issues
> one ATC_INV command covering all ATC entries per comments inside
> arm_smmu_atc_inv_to_cmd(). But now we replace that single command
> with all attached subdomains in the list? Any reason for such a
> change here?

Because we don't necessarily want to invalidate all PASID-domains
attached to a master. If arm_smmu_atc_inv_domain() is called on a
domain that is only attached with Pasid, we can restrict the
invalidations to those specific PASID by looping over them. But yeah,
you're right that we could potentially optimize this?
* Skip the per-pasid invalidations if the domain is also attached to
this master without PASID as we have to invalidate all its pasids in
that case anyways. It's hard to imagine clients attaching a domain
both with pasid and without pasid to the same device but could be
possible.
* Always invalidate all pasids by issuing atc invalidations on SSID 0.
This sounds like the wrong trade-off??

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Nicolin Chen]

On Fri, Jul 14, 2023 at 05:30:42PM +0800, Michael Shavit wrote:
 
> > And I don't quite get this part. Prior to this change, it issues
> > one ATC_INV command covering all ATC entries per comments inside
> > arm_smmu_atc_inv_to_cmd(). But now we replace that single command
> > with all attached subdomains in the list? Any reason for such a
> > change here?
> 
> Because we don't necessarily want to invalidate all PASID-domains
> attached to a master. If arm_smmu_atc_inv_domain() is called on a
> domain that is only attached with Pasid, we can restrict the
> invalidations to those specific PASID by looping over them.
>
> But yeah,
> you're right that we could potentially optimize this?
> * Skip the per-pasid invalidations if the domain is also attached to
> this master without PASID as we have to invalidate all its pasids in
> that case anyways. It's hard to imagine clients attaching a domain
> both with pasid and without pasid to the same device but could be
> possible.
> * Always invalidate all pasids by issuing atc invalidations on SSID 0.
> This sounds like the wrong trade-off??

Well, firstly it's kinda odd to have this optimization hidden in
a big rework patch. And I am not sure if it alone would work for
all use cases, as it impacts the arm_smmu_atc_inv_domain() that
passes in a zero ssid, in which case the affected function is not
used by a pasid case all the time:
	/*
	 * ATS and PASID:
...
	 * When using STRTAB_STE_1_S1DSS_SSID0 (reserving CD 0 for non-PASID
	 * traffic), translation requests without PASID create ATC entries
	 * without PASID, which must be invalidated with substream_valid clear.
	 * This has the unpleasant side-effect of invalidating all PASID-tagged
	 * ATC entries within the address range.
	 */

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Nicolin Chen]

On Fri, Jul 14, 2023 at 05:30:42PM +0800, Michael Shavit wrote:
 
> > And I don't quite get this part. Prior to this change, it issues
> > one ATC_INV command covering all ATC entries per comments inside
> > arm_smmu_atc_inv_to_cmd(). But now we replace that single command
> > with all attached subdomains in the list? Any reason for such a
> > change here?
> 
> Because we don't necessarily want to invalidate all PASID-domains
> attached to a master. If arm_smmu_atc_inv_domain() is called on a
> domain that is only attached with Pasid, we can restrict the
> invalidations to those specific PASID by looping over them.
>
> But yeah,
> you're right that we could potentially optimize this?
> * Skip the per-pasid invalidations if the domain is also attached to
> this master without PASID as we have to invalidate all its pasids in
> that case anyways. It's hard to imagine clients attaching a domain
> both with pasid and without pasid to the same device but could be
> possible.
> * Always invalidate all pasids by issuing atc invalidations on SSID 0.
> This sounds like the wrong trade-off??

Well, firstly it's kinda odd to have this optimization hidden in
a big rework patch. And I am not sure if it alone would work for
all use cases, as it impacts the arm_smmu_atc_inv_domain() that
passes in a zero ssid, in which case the affected function is not
used by a pasid case all the time:
	/*
	 * ATS and PASID:
...
	 * When using STRTAB_STE_1_S1DSS_SSID0 (reserving CD 0 for non-PASID
	 * traffic), translation requests without PASID create ATC entries
	 * without PASID, which must be invalidated with substream_valid clear.
	 * This has the unpleasant side-effect of invalidating all PASID-tagged
	 * ATC entries within the address range.
	 */

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/13] iommu/arm-smmu-v3: Keep track of attached ssids

[Michael Shavit]

On Sat, Jul 15, 2023 at 8:35 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> Well, firstly it's kinda odd to have this optimization hidden in
> a big rework patch. And I am not sure if it alone would work for
> all use cases, as it impacts the arm_smmu_atc_inv_domain() that
> passes in a zero ssid, in which case the affected function is not
> used by a pasid case all the time:

To clarify, we haven't changed anything for the existing flow. Before
the latter set_dev_pasid patch is introduced, there's only ever a
single {master,domain} pair in the attached_domain list. When
arm_smmu_atc_inv_domain() is called on a non-pasid domain, it issues a
single atc inv command with substream_valid=false, same as before.
When SVA wants to invalidate a domain, it calls
arm_smmu_atc_inv_domain_ssid to issue a single atc inv command with
that specific PASID, same as before. At no point does the driver ever
rely on the fact that invalidating with substream_valid=false would
invalidate all PASIDs.

This patch series simply follows the precedent set by SVA: when a
domain is attached with a pasid, invalidates those specific pasids.
It'd feel kinda hacky to rely on the fact that invalidating with
substream_valid=false would invalidate all PASIDs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 08/13] iommu/arm-smmu-v3: Add helper for atc invalidation

[Michael Shavit]

This will be used to invalidate ATC entries made on an SSID for a master
when detaching a domain with pasid.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
v1->v2: Make use of arm_smmu_atc_inv_cmd_set_ssid
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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 65e2dfd28b7d8..0a5e875abda86 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1784,13 +1784,15 @@ arm_smmu_atc_inv_to_cmd(unsigned long iova, size_t size,
 	cmd->atc.size	= log2_span;
 }
 
-static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
+static int arm_smmu_atc_inv_master_ssid(struct arm_smmu_master *master,
+					int ssid)
 {
 	int i;
 	struct arm_smmu_cmdq_ent cmd;
 	struct arm_smmu_cmdq_batch cmds;
 
 	arm_smmu_atc_inv_to_cmd(0, 0, &cmd);
+	arm_smmu_atc_inv_cmd_set_ssid(ssid, &cmd);
 	cmds.num = 0;
 	for (i = 0; i < master->num_streams; i++) {
 		cmd.atc.sid = master->streams[i].id;
@@ -1800,6 +1802,11 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
 	return arm_smmu_cmdq_batch_submit(master->smmu, &cmds);
 }
 
+static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)
+{
+	return arm_smmu_atc_inv_master_ssid(master, 0);
+}
+
 /*
  * If ssid is non-zero, issue atc invalidations with the given ssid instead of
  * the one the domain is attached to. This is used by SVA since it's pasid
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Michael Shavit]

This change enables the use of the iommu_attach_dev_pasid API for
UNMANAGED domains. The primary use-case is to allow in-kernel users of
the iommu API to manage domains with PASID. This change also allows for
future support of pasid in the DMA api.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
v1->v2: Add missing atc invalidation when detaching with pasid
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 167 +++++++++++++++++---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   1 +
 2 files changed, 149 insertions(+), 19 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 0a5e875abda86..b928997d35ed3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2173,6 +2173,10 @@ static int arm_smmu_domain_finalise_s2(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
+/*
+ * master may be null for domain types that are finalized before being attached
+ * to a master.
+ */
 static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 				    struct arm_smmu_master *master)
 {
@@ -2369,6 +2373,11 @@ static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
 	return 0;
 }
 
+static bool arm_smmu_master_has_pasid_domains(struct arm_smmu_master *master)
+{
+	return master->nr_attached_pasid_domains > 0;
+}
+
 static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 {
 	struct pci_dev *pdev;
@@ -2411,6 +2420,28 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 	arm_smmu_install_ste_for_dev(master);
 }
 
+/*
+ * Once attached for the first time, a domain can no longer be attached to any
+ * master with a distinct upstream SMMU.
+ */
+static int arm_smmu_prepare_domain_for_smmu(struct arm_smmu_device *smmu,
+					    struct arm_smmu_domain *smmu_domain)
+{
+	int ret = 0;
+
+	mutex_lock(&smmu_domain->init_mutex);
+	if (!smmu_domain->smmu) {
+		smmu_domain->smmu = smmu;
+		ret = arm_smmu_domain_finalise(&smmu_domain->domain, NULL);
+		if (ret)
+			smmu_domain->smmu = NULL;
+	} else if (smmu_domain->smmu != smmu) {
+		ret = -EINVAL;
+	}
+	mutex_unlock(&smmu_domain->init_mutex);
+	return ret;
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret = 0;
@@ -2426,6 +2457,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	master = dev_iommu_priv_get(dev);
 	smmu = master->smmu;
 
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain);
+	if (ret)
+		return ret;
+
 	/*
 	 * Checking that SVA is disabled ensures that this device isn't bound to
 	 * any mm, and can be safely detached from its old domain. Bonds cannot
@@ -2436,22 +2471,18 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return -EBUSY;
 	}
 
-	arm_smmu_detach_dev(master);
-
-	mutex_lock(&smmu_domain->init_mutex);
-
-	if (!smmu_domain->smmu) {
-		smmu_domain->smmu = smmu;
-		ret = arm_smmu_domain_finalise(domain, master);
-		if (ret) {
-			smmu_domain->smmu = NULL;
-			goto out_unlock;
-		}
-	} else if (smmu_domain->smmu != smmu) {
-		ret = -EINVAL;
-		goto out_unlock;
+	/*
+	 * Attaching a bypass or stage 2 domain would break any domains attached
+	 * with pasid. Attaching an S1 domain should be feasible but requires
+	 * more complicated logic to handle.
+	 */
+	if (arm_smmu_master_has_pasid_domains(master)) {
+		dev_err(dev, "cannot attach - domain attached with pasid\n");
+		return -EBUSY;
 	}
 
+	arm_smmu_detach_dev(master);
+
 	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
 		master->s1_cfg = &master->owned_s1_cfg;
 		ret = arm_smmu_write_ctx_desc(
@@ -2460,7 +2491,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			0, &smmu_domain->cd);
 		if (ret) {
 			master->s1_cfg = NULL;
-			goto out_unlock;
+			return ret;
 		}
 	} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
 		   smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED) {
@@ -2489,11 +2520,75 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	arm_smmu_enable_ats(master, smmu_domain);
 
-out_unlock:
-	mutex_unlock(&smmu_domain->init_mutex);
 	return ret;
 }
 
+static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
+				  struct device *dev, ioasid_t pasid)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu;
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_attached_domain *attached_domain;
+	struct arm_smmu_master *master;
+
+	if (!fwspec)
+		return -ENOENT;
+
+	master = dev_iommu_priv_get(dev);
+	smmu = master->smmu;
+
+	ret = arm_smmu_prepare_domain_for_smmu(smmu, smmu_domain);
+	if (ret)
+		return ret;
+
+	if (pasid == 0) {
+		dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
+		return -ENODEV;
+	}
+
+	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
+		dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");
+		return -EINVAL;
+	}
+
+	if (!master->s1_cfg || master->s2_cfg)
+		return -EBUSY;
+
+	attached_domain = kzalloc(sizeof(*attached_domain), GFP_KERNEL);
+	if (!attached_domain)
+		return -ENOMEM;
+
+	attached_domain->master = master;
+	attached_domain->domain = smmu_domain;
+	attached_domain->ssid = pasid;
+
+	master->nr_attached_pasid_domains += 1;
+	/*
+	 * arm_smmu_share_asid may update the cd's asid value and write the
+	 * ctx_desc for every attached_domains in the list. There's a potential
+	 * race here regardless of whether we first write the ctx_desc or
+	 * first insert into the domain's list. Grabbing the asic_lock prevents
+	 * SVA from changing the cd's ASID while the cd is being attached.
+	 */
+	mutex_lock(&arm_smmu_asid_lock);
+	ret = arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
+				      pasid, &smmu_domain->cd);
+	if (ret) {
+		mutex_unlock(&arm_smmu_asid_lock);
+		kfree(attached_domain);
+	}
+
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_add(&attached_domain->domain_head, &smmu_domain->attached_domains);
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
+	mutex_unlock(&arm_smmu_asid_lock);
+
+	return 0;
+}
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -2739,6 +2834,15 @@ static void arm_smmu_release_device(struct device *dev)
 
 	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+	if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
+		/*
+		 * TODO: Do we need to handle this case?
+		 * This requires a mechanism to obtain all the pasid domains
+		 * that this master is attached to so that we can clean up the
+		 * domain's attached_domain list.
+		 */
+	}
+
 	arm_smmu_detach_dev(master);
 	arm_smmu_free_cd_tables(master->smmu, &master->owned_s1_cfg.cdcfg);
 	arm_smmu_disable_pasid(master);
@@ -2874,12 +2978,36 @@ static int arm_smmu_def_domain_type(struct device *dev)
 static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 {
 	struct iommu_domain *domain;
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_domain *smmu_domain;
+	struct arm_smmu_attached_domain *attached_domain;
+	unsigned long flags;
 
-	domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
+	if (!master || pasid == 0)
+		return;
+
+	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
 	if (WARN_ON(IS_ERR(domain)) || !domain)
 		return;
+	if (domain->type == IOMMU_DOMAIN_SVA)
+		return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
 
-	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
+	smmu_domain = to_smmu_domain(domain);
+	mutex_lock(&arm_smmu_asid_lock);
+	spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+	list_for_each_entry(attached_domain, &smmu_domain->attached_domains, domain_head) {
+		if (attached_domain->master != master ||
+		    attached_domain->ssid != pasid)
+			continue;
+		list_del(&attached_domain->domain_head);
+		break;
+	}
+	spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
+	arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master, pasid,
+				NULL);
+	arm_smmu_atc_inv_master_ssid(master, pasid);
+	master->nr_attached_pasid_domains -= 1;
+	mutex_unlock(&arm_smmu_asid_lock);
 }
 
 static struct iommu_ops arm_smmu_ops = {
@@ -2899,6 +3027,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.owner			= THIS_MODULE,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev		= arm_smmu_attach_dev,
+		.set_dev_pasid		= arm_smmu_set_dev_pasid,
 		.map_pages		= arm_smmu_map_pages,
 		.unmap_pages		= arm_smmu_unmap_pages,
 		.flush_iotlb_all	= arm_smmu_flush_iotlb_all,
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 6929590530367..48795a7287b69 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -707,6 +707,7 @@ struct arm_smmu_master {
 	bool				iopf_enabled;
 	struct list_head		bonds;
 	unsigned int			ssid_bits;
+	unsigned int			nr_attached_pasid_domains;
 };
 
 /* SMMU private data for an IOMMU domain */
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Nicolin Chen]

Hi Michael,

On Wed, Jun 21, 2023 at 02:37:21PM +0800, Michael Shavit wrote:
> 
> This change enables the use of the iommu_attach_dev_pasid API for
> UNMANAGED domains. The primary use-case is to allow in-kernel users of
> the iommu API to manage domains with PASID. This change also allows for
> future support of pasid in the DMA api.

Would you please elaborate a bit more for the use case of
allowing that? And which test configuration do you cover
using smmute? Would you mind sharing your test commands?

I have run a sanity with this series using an SVA domain
with a real master. It seems to be fine.

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Michael Shavit]

> Would you please elaborate a bit more for the use case of
allowing that?

In short; to support devices that use SSID to isolate different
(device) contexts. Those contexts (and the memory available to them)
is partially managed by the device's Kernel driver.

> And which test configuration do you cover
using smmute? Would you mind sharing your test commands?

I used the setup suggested by Jean in
https://lore.kernel.org/all/20230511195928.GA288490@myrica/ , with the
following commands:
>>>
# Basic test
./smmute;
./smmute -u mmap;

# Test invalid access to not-mapped address
./smmute -u mmap -f drv;

# Test invalid access after unmap
./smmute -u mmap -f write -d;

# Check smmu_mn released when killed
mount -t tracefs nodev /sys/kernel/tracing;
echo 1 > /sys/kernel/tracing/events/iommu/enable;
echo 1 > /sys/kernel/tracing/events/smmu/enable;
./smmute -u mmap -k bind;
cat /sys/kernel/tracing/trace;
<<<

This only covers existing SVA functionality. To test the functionality
introduced by this patch, I used a device capable of generating DMA
transactions w/ SSID and a test driver with following tests:
1. Successful dma Read/Write to buffer mapped on domain attached with pasid
2. Unsuccessful dma Read/Write to same buffer after domain detached
from pasid, or after buffer unmapped from domain
3. Variations of the above with a domain attached to multiple pasids

I've been considering migrating those tests to the smmute driver if
that would be valuable.



On Fri, Jun 23, 2023 at 8:32 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
>
> Hi Michael,
>
> On Wed, Jun 21, 2023 at 02:37:21PM +0800, Michael Shavit wrote:
> >
> > This change enables the use of the iommu_attach_dev_pasid API for
> > UNMANAGED domains. The primary use-case is to allow in-kernel users of
> > the iommu API to manage domains with PASID. This change also allows for
> > future support of pasid in the DMA api.
>
> Would you please elaborate a bit more for the use case of
> allowing that? And which test configuration do you cover
> using smmute? Would you mind sharing your test commands?
>
> I have run a sanity with this series using an SVA domain
> with a real master. It seems to be fine.
>
> Thanks
> Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Nicolin Chen]

On Mon, Jun 26, 2023 at 10:33:58AM +0800, Michael Shavit wrote:
 
> > Would you please elaborate a bit more for the use case of
> allowing that?
> 
> In short; to support devices that use SSID to isolate different
> (device) contexts. Those contexts (and the memory available to them)
> is partially managed by the device's Kernel driver.

What can be a real world use case for that?

> > And which test configuration do you cover
> using smmute? Would you mind sharing your test commands?
> 
> I used the setup suggested by Jean in
> https://lore.kernel.org/all/20230511195928.GA288490@myrica/ , with the
> following commands:
> >>>
> # Basic test
> ./smmute;
> ./smmute -u mmap;
> 
> # Test invalid access to not-mapped address
> ./smmute -u mmap -f drv;
> 
> # Test invalid access after unmap
> ./smmute -u mmap -f write -d;
> 
> # Check smmu_mn released when killed
> mount -t tracefs nodev /sys/kernel/tracing;
> echo 1 > /sys/kernel/tracing/events/iommu/enable;
> echo 1 > /sys/kernel/tracing/events/smmu/enable;
> ./smmute -u mmap -k bind;
> cat /sys/kernel/tracing/trace;
> <<<

OK. Thanks for sharing.

> This only covers existing SVA functionality. To test the functionality
> introduced by this patch, I used a device capable of generating DMA
> transactions w/ SSID and a test driver with following tests:
> 1. Successful dma Read/Write to buffer mapped on domain attached with pasid
> 2. Unsuccessful dma Read/Write to same buffer after domain detached
> from pasid, or after buffer unmapped from domain
> 3. Variations of the above with a domain attached to multiple pasids
> 
> I've been considering migrating those tests to the smmute driver if
> that would be valuable.

Is this on Gerrit too?

Thanks
Nicolin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Michael Shavit]

On Tue, Jun 27, 2023 at 2:14 AM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > I've been considering migrating those tests to the smmute driver if
> > that would be valuable.
>
> Is this on Gerrit too?

It's not pretty but I've rewritten the tests into the smmute kernel
driver. Pushed to Gerrit here:
https://linux-review.googlesource.com/id/Ibb33ba6f9c6d069324f21b9ad98e29c94e15374a
.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/13] iommu/arm-smmu-v3: Implement set_dev_pasid

[Michael Shavit]

On Wed, Jun 21, 2023 at 2:44 PM Michael Shavit <mshavit@google.com> wrote:
> +       mutex_lock(&arm_smmu_asid_lock);
> +       ret = arm_smmu_write_ctx_desc(master->smmu, master->s1_cfg, master,
> +                                     pasid, &smmu_domain->cd);
> +       if (ret) {
> +               mutex_unlock(&arm_smmu_asid_lock);
> +               kfree(attached_domain);
> +       }
> +
> +       spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
> +       list_add(&attached_domain->domain_head, &smmu_domain->attached_domains);
> +       spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
> +       mutex_unlock(&arm_smmu_asid_lock);
> +
> +       return 0;

A small bug in this patch as well: we return 0 when
arm_smmu_write_ctx_desc() fails instead of the error. Will upload fix
in v5.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/13] iommu/arm-smmu-v3-sva: Remove bond refcount

[Michael Shavit]

The iommu-sva framework checks if a bond between a device and mm already
exists and handles refcounting at the iommu_domain level.
__arm_smmu_sva_bind is therefore only called once for a device/mm pair.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index d07c08b53c5cf..20301d0a2c0b0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -29,7 +29,6 @@ struct arm_smmu_bond {
 	struct mm_struct		*mm;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
 	struct list_head		list;
-	refcount_t			refs;
 };
 
 #define sva_to_bond(handle) \
@@ -377,21 +376,12 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	if (!master || !master->sva_enabled)
 		return ERR_PTR(-ENODEV);
 
-	/* If bind() was already called for this {dev, mm} pair, reuse it. */
-	list_for_each_entry(bond, &master->bonds, list) {
-		if (bond->mm == mm) {
-			refcount_inc(&bond->refs);
-			return &bond->sva;
-		}
-	}
-
 	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
 	if (!bond)
 		return ERR_PTR(-ENOMEM);
 
 	bond->mm = mm;
 	bond->sva.dev = dev;
-	refcount_set(&bond->refs, 1);
 
 	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
 	if (IS_ERR(bond->smmu_mn)) {
@@ -570,7 +560,7 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
+	if (!WARN_ON(!bond)) {
 		list_del(&bond->list);
 		arm_smmu_mmu_notifier_put(bond->smmu_mn);
 		kfree(bond);
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/13] iommu/arm-smmu-v3-sva: Clean unused iommu_sva

[Michael Shavit]

The __arm_smmu_sva_bind function returned an unused iommu_sva handle
that can be removed.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 20301d0a2c0b0..650c9c9ad52f1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -25,7 +25,6 @@ struct arm_smmu_mmu_notifier {
 #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
 
 struct arm_smmu_bond {
-	struct iommu_sva		sva;
 	struct mm_struct		*mm;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
 	struct list_head		list;
@@ -364,8 +363,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	arm_smmu_free_shared_cd(cd);
 }
 
-static struct iommu_sva *
-__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
+static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 {
 	int ret;
 	struct arm_smmu_bond *bond;
@@ -374,14 +372,13 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
 	if (!master || !master->sva_enabled)
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 
 	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
 	if (!bond)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	bond->mm = mm;
-	bond->sva.dev = dev;
 
 	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
 	if (IS_ERR(bond->smmu_mn)) {
@@ -390,11 +387,11 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	}
 
 	list_add(&bond->list, &master->bonds);
-	return &bond->sva;
+	return 0;
 
 err_free_bond:
 	kfree(bond);
-	return ERR_PTR(ret);
+	return ret;
 }
 
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
@@ -572,13 +569,10 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 				      struct device *dev, ioasid_t id)
 {
 	int ret = 0;
-	struct iommu_sva *handle;
 	struct mm_struct *mm = domain->mm;
 
 	mutex_lock(&sva_lock);
-	handle = __arm_smmu_sva_bind(dev, mm);
-	if (IS_ERR(handle))
-		ret = PTR_ERR(handle);
+	ret = __arm_smmu_sva_bind(dev, mm);
 	mutex_unlock(&sva_lock);
 
 	return ret;
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 12/13] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Michael Shavit]

There's a 1:1 relationship between arm_smmu_bond and the iommu_domain
used in set_dev_pasid/remove_dev_pasid. arm_smmu_bond has become an
unnecessary complication. It's more natural to store any needed
information at the iommu_domain container level.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 69 +++++++------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
 3 files changed, 24 insertions(+), 48 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 650c9c9ad52f1..b615a85e6a54e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
 
 #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
 
-struct arm_smmu_bond {
-	struct mm_struct		*mm;
+struct arm_smmu_sva_domain {
+	struct iommu_domain		iommu_domain;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
-	struct list_head		list;
 };
 
-#define sva_to_bond(handle) \
-	container_of(handle, struct arm_smmu_bond, sva)
+#define to_sva_domain(domain) \
+	container_of(domain, struct arm_smmu_sva_domain, iommu_domain)
 
 static DEFINE_MUTEX(sva_lock);
 
@@ -363,10 +362,10 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	arm_smmu_free_shared_cd(cd);
 }
 
-static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
+static int __arm_smmu_sva_bind(struct device *dev,
+			       struct arm_smmu_sva_domain *sva_domain,
+			       struct mm_struct *mm)
 {
-	int ret;
-	struct arm_smmu_bond *bond;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -374,24 +373,14 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	if (!master || !master->sva_enabled)
 		return -ENODEV;
 
-	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
-	if (!bond)
-		return -ENOMEM;
-
-	bond->mm = mm;
-
-	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
-	if (IS_ERR(bond->smmu_mn)) {
-		ret = PTR_ERR(bond->smmu_mn);
-		goto err_free_bond;
+	sva_domain->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain,
+							mm);
+	if (IS_ERR(sva_domain->smmu_mn)) {
+		sva_domain->smmu_mn = NULL;
+		return PTR_ERR(sva_domain->smmu_mn);
 	}
-
-	list_add(&bond->list, &master->bonds);
+	master->nr_attached_sva_domains += 1;
 	return 0;
-
-err_free_bond:
-	kfree(bond);
-	return ret;
 }
 
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
@@ -521,7 +510,7 @@ int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
 int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
 {
 	mutex_lock(&sva_lock);
-	if (!list_empty(&master->bonds)) {
+	if (master->nr_attached_sva_domains != 0) {
 		dev_err(master->dev, "cannot disable SVA, device is bound\n");
 		mutex_unlock(&sva_lock);
 		return -EBUSY;
@@ -545,23 +534,12 @@ void arm_smmu_sva_notifier_synchronize(void)
 void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t id)
 {
-	struct mm_struct *mm = domain->mm;
-	struct arm_smmu_bond *bond = NULL, *t;
+	struct arm_smmu_sva_domain *sva_domain = to_sva_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
 	mutex_lock(&sva_lock);
-	list_for_each_entry(t, &master->bonds, list) {
-		if (t->mm == mm) {
-			bond = t;
-			break;
-		}
-	}
-
-	if (!WARN_ON(!bond)) {
-		list_del(&bond->list);
-		arm_smmu_mmu_notifier_put(bond->smmu_mn);
-		kfree(bond);
-	}
+	master->nr_attached_sva_domains -= 1;
+	arm_smmu_mmu_notifier_put(sva_domain->smmu_mn);
 	mutex_unlock(&sva_lock);
 }
 
@@ -572,7 +550,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	struct mm_struct *mm = domain->mm;
 
 	mutex_lock(&sva_lock);
-	ret = __arm_smmu_sva_bind(dev, mm);
+	ret = __arm_smmu_sva_bind(dev, to_sva_domain(domain), mm);
 	mutex_unlock(&sva_lock);
 
 	return ret;
@@ -590,12 +568,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
 
 struct iommu_domain *arm_smmu_sva_domain_alloc(void)
 {
-	struct iommu_domain *domain;
+	struct arm_smmu_sva_domain *sva_domain;
 
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
+	sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
+	if (!sva_domain)
 		return NULL;
-	domain->ops = &arm_smmu_sva_domain_ops;
-
-	return domain;
+	sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;
+	return &sva_domain->iommu_domain;
 }
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 b928997d35ed3..2ed6c9e0df241 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2784,7 +2784,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 
 	master->dev = dev;
 	master->smmu = smmu;
-	INIT_LIST_HEAD(&master->bonds);
 	dev_iommu_priv_set(dev, master);
 
 	ret = arm_smmu_insert_master(smmu, 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 48795a7287b69..3525d60668c23 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -705,7 +705,7 @@ struct arm_smmu_master {
 	bool				stall_enabled;
 	bool				sva_enabled;
 	bool				iopf_enabled;
-	struct list_head		bonds;
+	unsigned int			nr_attached_sva_domains;
 	unsigned int			ssid_bits;
 	unsigned int			nr_attached_pasid_domains;
 };
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 12/13] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Michael Shavit]

> On Wed, Jun 21, 2023 at 2:44 PM Michael Shavit <mshavit@google.com> wrote:
> @@ -545,23 +534,12 @@ void arm_smmu_sva_notifier_synchronize(void)
>  void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>                                    struct device *dev, ioasid_t id)
>  {
> -       struct mm_struct *mm = domain->mm;
> -       struct arm_smmu_bond *bond = NULL, *t;
> +       struct arm_smmu_sva_domain *sva_domain = to_sva_domain(domain);
>         struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>
>         mutex_lock(&sva_lock);
> -       list_for_each_entry(t, &master->bonds, list) {
> -               if (t->mm == mm) {
> -                       bond = t;
> -                       break;
> -               }
> -       }
> -
> -       if (!WARN_ON(!bond)) {
> -               list_del(&bond->list);
> -               arm_smmu_mmu_notifier_put(bond->smmu_mn);
> -               kfree(bond);
> -       }
> +       master->nr_attached_sva_domains -= 1;
> +       arm_smmu_mmu_notifier_put(sva_domain->smmu_mn);
>         mutex_unlock(&sva_lock);
>  }

I've encountered a bug with this change while working on a follow-up
series. arm_smmu_sva_remove_dev_pasid() is called on the domain if
arm_smmu_sva_set_dev_pasid failed. Removing the WARN_ON(!bond)
condition removed a protection against that scenario. I have a fix for
this change that I'll push into a v5 of the series.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 13/13] iommu/arm-smmu-v3-sva: Add check when enabling sva

[Michael Shavit]

SVA domains can only be attached when the master's STEs have a stage 1
domain.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index b615a85e6a54e..e2a91f20f0906 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -499,9 +499,15 @@ int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
 	int ret;
 
 	mutex_lock(&sva_lock);
+
+	if (!master->s1_cfg) {
+		ret = -EBUSY;
+		goto unlock;
+	}
 	ret = arm_smmu_master_sva_enable_iopf(master);
 	if (!ret)
 		master->sva_enabled = true;
+unlock:
 	mutex_unlock(&sva_lock);
 
 	return ret;
-- 
2.41.0.162.gfafddb0af9-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel