| Message ID | 20230731184817.v2.4.I5aa89c849228794a64146cfe86df21fb71629384@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor the SMMU's CD table ownership | expand |
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
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
> 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.
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
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 --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;
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(-)