diff mbox series

[v2,4/8] iommu/arm-smmu-v3: move stall_enabled to the cd table

Message ID 20230731184817.v2.4.I5aa89c849228794a64146cfe86df21fb71629384@changeid
State New
Headers
Series Refactor the SMMU's CD table ownership |

Commit Message

Michael Shavit July 31, 2023, 10:48 a.m. UTC 
  This controls whether CD entries will have the stall bit set when
writing entries into the table.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

Changes in v2:
- Use a bitfield instead of a bool for stall_enabled

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 ++-
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

Nicolin Chen Aug. 1, 2023, 4:28 a.m. UTC | #1 
On Mon, Jul 31, 2023 at 06:48:14PM +0800, Michael Shavit wrote:
> This controls whether CD entries will have the stall bit set when
> writing entries into the table.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
> Changes in v2:
> - Use a bitfield instead of a bool for stall_enabled
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 ++-
>  2 files changed, 6 insertions(+), 5 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 8a286e3838d70..654acf6002bf3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1114,7 +1114,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 (smmu_domain->cd_table.stall_enabled)
>                         val |= CTXDESC_CD_0_S;

This cd_table->stall_enabled comes from master->stall_enabled, and
cd_table will be in master structure. Also, struct arm_smmu_master
pointer will be passed in to arm_smmu_write_ctx_desc(). So, there
seems to be no need of master->cd_table.stall_enabled in the end;
just use master->stall_enabled directly?

Thanks
Nicolin
Nicolin Chen Aug. 1, 2023, 4:52 a.m. UTC | #2 
On Mon, Jul 31, 2023 at 09:28:40PM -0700, Nicolin Chen wrote:
> On Mon, Jul 31, 2023 at 06:48:14PM +0800, Michael Shavit wrote:
> > This controls whether CD entries will have the stall bit set when
> > writing entries into the table.
> > 
> > Signed-off-by: Michael Shavit <mshavit@google.com>
> > ---
> > 
> > Changes in v2:
> > - Use a bitfield instead of a bool for stall_enabled
> > 
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 ++-
> >  2 files changed, 6 insertions(+), 5 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 8a286e3838d70..654acf6002bf3 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1114,7 +1114,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 (smmu_domain->cd_table.stall_enabled)
> >                         val |= CTXDESC_CD_0_S;
> 
> This cd_table->stall_enabled comes from master->stall_enabled, and
> cd_table will be in master structure. Also, struct arm_smmu_master
> pointer will be passed in to arm_smmu_write_ctx_desc(). So, there
> seems to be no need of master->cd_table.stall_enabled in the end;
> just use master->stall_enabled directly?

Actually the stall_enabled might still need to be per-CD/domain.
If a domain is attached by two masters. The domain->stall_enabled
is initialized with the first master->stall_enabled. Then, the
second master->stall_enabled would be required to match with the
domain->stall_enabled. arm_smmu_attach_dev() has such a sanity.

So, I think we might not need this patch.

Nicolin
Michael Shavit Aug. 1, 2023, 8:09 a.m. UTC | #3 
> On Mon, Jul 31, 2023 at 09:28:40PM -0700, Nicolin Chen wrote:
> > This cd_table->stall_enabled comes from master->stall_enabled, and
> > cd_table will be in master structure. Also, struct arm_smmu_master
> > pointer will be passed in to arm_smmu_write_ctx_desc(). So, there
> > seems to be no need of master->cd_table.stall_enabled in the end;
> > just use master->stall_enabled directly?

Yes it's correct that this change isn't strictly necessary. Thoughts jgg@ ?

On Tue, Aug 1, 2023 at 12:52 PM Nicolin Chen <nicolinc@nvidia.com> wrote:
> Actually the stall_enabled might still need to be per-CD/domain.
> If a domain is attached by two masters. The domain->stall_enabled
> is initialized with the first master->stall_enabled. Then, the
> second master->stall_enabled would be required to match with the
> domain->stall_enabled. arm_smmu_attach_dev() has such a sanity.
>
> So, I think we might not need this patch.

But why force domains attached to different masters to have the same
stall_enabled setting? Whether stall is enabled is strictly a property
of the master, not the domain. IMO the fact that it was stored in
domain and checked in attach_dev was only because the previous design
required it, not because it's more appropriate.
Jason Gunthorpe Aug. 1, 2023, 1:31 p.m. UTC | #4 
On Tue, Aug 01, 2023 at 04:09:52PM +0800, Michael Shavit wrote:
> > On Mon, Jul 31, 2023 at 09:28:40PM -0700, Nicolin Chen wrote:
> > > This cd_table->stall_enabled comes from master->stall_enabled, and
> > > cd_table will be in master structure. Also, struct arm_smmu_master
> > > pointer will be passed in to arm_smmu_write_ctx_desc(). So, there
> > > seems to be no need of master->cd_table.stall_enabled in the end;
> > > just use master->stall_enabled directly?
> 
> Yes it's correct that this change isn't strictly necessary. Thoughts jgg@ ?

I don't have a strong feeling here

The stall bits in the STE/CDTE should be set only for masters that
operate in stall mode.

I would hope a a single domain should be mixable between stall and PRI
masters?

If the cd_table is 1:1 with a master then keeping it in the master is
logical enough. If we ever imagine a CD table with multiple masters
then we'd need to have a bit in the cd_table too.


> On Tue, Aug 1, 2023 at 12:52 PM Nicolin Chen <nicolinc@nvidia.com> wrote:
> > Actually the stall_enabled might still need to be per-CD/domain.
> > If a domain is attached by two masters. The domain->stall_enabled
> > is initialized with the first master->stall_enabled. Then, the
> > second master->stall_enabled would be required to match with the
> > domain->stall_enabled. arm_smmu_attach_dev() has such a sanity.
> >
> > So, I think we might not need this patch.
> 
> But why force domains attached to different masters to have the same
> stall_enabled setting? Whether stall is enabled is strictly a property
> of the master, not the domain. IMO the fact that it was stored in
> domain and checked in attach_dev was only because the previous design
> required it, not because it's more appropriate.

Yes, definately remove stall from the domains, the faulting flow
should learn the faulting mode based on the master that triggered the
fault, not the domain that received it.

Jason
Jason Gunthorpe Aug. 1, 2023, 2:12 p.m. UTC | #5 
On Mon, Jul 31, 2023 at 06:48:14PM +0800, Michael Shavit wrote:
> This controls whether CD entries will have the stall bit set when
> writing entries into the table.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
> Changes in v2:
> - Use a bitfield instead of a bool for stall_enabled
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 ++++----
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 ++-
>  2 files changed, 6 insertions(+), 5 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 8a286e3838d70..654acf6002bf3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1114,7 +1114,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 (smmu_domain->cd_table.stall_enabled)
>  			val |= CTXDESC_CD_0_S;
>  	}

Since patch 6 makes arm_smmu_write_ctx_desc() take in the master
parameter, it does make sense to just refer to the stall in the master
at this point. Can you defer this until after patch 6?

> 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 35a93e8858872..05b1f0ee60808 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -597,6 +597,8 @@ struct arm_smmu_ctx_desc_cfg {
>  	unsigned int			num_l1_ents;
>  	/* log2 of the maximum number of CDs supported by this table */
>  	u8				max_cds_bits;
> +	/* Whether CD entries in this table have the stall bit set. */
> +	u8				stall_enabled:1;
>  };
>  
>  struct arm_smmu_s2_cfg {
> @@ -714,7 +716,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;

But this also makes sense, and removing stall_enabled from the domain
is important, so

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

If you keep it this way

Jason
diff mbox series

Patch

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 8a286e3838d70..654acf6002bf3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1114,7 +1114,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 (smmu_domain->cd_table.stall_enabled)
 			val |= CTXDESC_CD_0_S;
 	}
 
@@ -1141,6 +1141,7 @@  static int arm_smmu_alloc_cd_tables(struct arm_smmu_domain *smmu_domain,
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_ctx_desc_cfg *cdcfg = &smmu_domain->cd_table;
 
+	cdcfg->stall_enabled = master->stall_enabled;
 	cdcfg->max_cds_bits = master->ssid_bits;
 	max_contexts = 1 << cdcfg->max_cds_bits;
 
@@ -2121,8 +2122,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_alloc_cd_tables(smmu_domain, master);
 	if (ret)
 		goto out_free_asid;
@@ -2461,7 +2460,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->cd_table.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 35a93e8858872..05b1f0ee60808 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -597,6 +597,8 @@  struct arm_smmu_ctx_desc_cfg {
 	unsigned int			num_l1_ents;
 	/* log2 of the maximum number of CDs supported by this table */
 	u8				max_cds_bits;
+	/* Whether CD entries in this table have the stall bit set. */
+	u8				stall_enabled:1;
 };
 
 struct arm_smmu_s2_cfg {
@@ -714,7 +716,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;