| Message ID | 6-v7-9597c885796c+d2-smmuv3_newapi_p2b_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 2b/3) | expand |
On Wed, May 08, 2024 at 03:57:14PM -0300, Jason Gunthorpe wrote: > We no longer need a master->sva_enable to control what attaches are > allowed. Instead we can tell if the attach is legal based on the current > configuration of the master. > > Keep track of the number of valid CD entries for SSID's in the cd_table > and if the cd_table has been installed in the STE directly so we know what > the configuration is. > > The attach logic is then made into: > - SVA bind, check if the CD is installed > - RID attach of S2, block if SSIDs are used > - RID attach of IDENTITY/BLOCKING, block if SSIDs are used > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++---------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 ++++++ > 3 files changed, 20 insertions(+), 13 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 d31caceb584984..7627cb53da55b9 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 > @@ -628,7 +628,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > struct arm_smmu_cd target; > int ret; > > - if (mm_get_enqcmd_pasid(mm) != id) > + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.in_ste) > return -EINVAL; The in_ste is added in PATCH-2 and the new arm_smmu_set_pasid() in the same patch is also guarded by the in_ste. So, it feels a little more related by moving this check here into PATCH-2 too? Thanks Nicolin
On Sun, May 12, 2024 at 11:01:32PM -0700, Nicolin Chen wrote: > On Wed, May 08, 2024 at 03:57:14PM -0300, Jason Gunthorpe wrote: > > We no longer need a master->sva_enable to control what attaches are > > allowed. Instead we can tell if the attach is legal based on the current > > configuration of the master. > > > > Keep track of the number of valid CD entries for SSID's in the cd_table > > and if the cd_table has been installed in the STE directly so we know what > > the configuration is. > > > > The attach logic is then made into: > > - SVA bind, check if the CD is installed > > - RID attach of S2, block if SSIDs are used > > - RID attach of IDENTITY/BLOCKING, block if SSIDs are used > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++---------- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 ++++++ > > 3 files changed, 20 insertions(+), 13 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 d31caceb584984..7627cb53da55b9 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 > > @@ -628,7 +628,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > struct arm_smmu_cd target; > > int ret; > > > > - if (mm_get_enqcmd_pasid(mm) != id) > > + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.in_ste) > > return -EINVAL; > > The in_ste is added in PATCH-2 and the new arm_smmu_set_pasid() > in the same patch is also guarded by the in_ste. So, it feels a > little more related by moving this check here into PATCH-2 too? Actually, I found this in_ste sanity gets removed in PATCH-10. I assume we rely on the same sanity in the arm_smmu_set_pasid() instead? If so, this sanity exists already in the arm_smmu_set_pasid() in the code that this patch applies to. So, could we just start to rely on that here, instead of adding this additional in_ste sanity in this patch? Thanks Nicolin
On Sun, May 12, 2024 at 11:01:19PM -0700, Nicolin Chen wrote: > > 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 d31caceb584984..7627cb53da55b9 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 > > @@ -628,7 +628,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > > struct arm_smmu_cd target; > > int ret; > > > > - if (mm_get_enqcmd_pasid(mm) != id) > > + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.in_ste) > > return -EINVAL; > > The in_ste is added in PATCH-2 and the new arm_smmu_set_pasid() > in the same patch is also guarded by the in_ste. So, it feels a > little more related by moving this check here into PATCH-2 too? Hmm, actually I think we can just remove this hunk now. The arm_smmu_set_pasid() a few lines below will check this already. It was here because before I didn't have the error unwind for arm_smmu_set_pasid() failure. Now that we have it we can just delete this. Thanks, Jason
On Wed, May 08, 2024 at 03:57:14PM -0300, Jason Gunthorpe wrote: > We no longer need a master->sva_enable to control what attaches are > allowed. Instead we can tell if the attach is legal based on the current > configuration of the master. > > Keep track of the number of valid CD entries for SSID's in the cd_table > and if the cd_table has been installed in the STE directly so we know what > the configuration is. > > The attach logic is then made into: > - SVA bind, check if the CD is installed > - RID attach of S2, block if SSIDs are used > - RID attach of IDENTITY/BLOCKING, block if SSIDs are used > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> With that in_ste check removed, Reviewed-y: Nicolin Chen <nicolinc@nvidia.com>
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 d31caceb584984..7627cb53da55b9 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 @@ -628,7 +628,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, struct arm_smmu_cd target; int ret; - if (mm_get_enqcmd_pasid(mm) != id) + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.in_ste) return -EINVAL; mutex_lock(&sva_lock); 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 3a7ef73994b57b..5f418b96679c88 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1289,6 +1289,8 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, struct arm_smmu_cd *cdptr, const struct arm_smmu_cd *target) { + bool target_valid = target->data[0] & cpu_to_le64(CTXDESC_CD_0_V); + bool cur_valid = cdptr->data[0] & cpu_to_le64(CTXDESC_CD_0_V); struct arm_smmu_cd_writer cd_writer = { .writer = { .ops = &arm_smmu_cd_writer_ops, @@ -1297,6 +1299,13 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, .ssid = ssid, }; + if (ssid != IOMMU_NO_PASID && cur_valid != target_valid) { + if (cur_valid) + master->cd_table.used_ssids--; + else + master->cd_table.used_ssids++; + } + arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); } @@ -2706,16 +2715,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) master = dev_iommu_priv_get(dev); smmu = master->smmu; - /* - * 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 - * be removed concurrently since we're holding the group mutex. - */ - if (arm_smmu_master_sva_enabled(master)) { - dev_err(dev, "cannot attach - SVA enabled\n"); - return -EBUSY; - } - mutex_lock(&smmu_domain->init_mutex); if (!smmu_domain->smmu) { @@ -2731,7 +2730,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) cdptr = arm_smmu_alloc_cd_ptr(master, IOMMU_NO_PASID); if (!cdptr) return -ENOMEM; - } + } else if (arm_smmu_ssids_in_use(&master->cd_table)) + return -EBUSY; /* * Prevent arm_smmu_share_asid() from trying to change the ASID @@ -2803,7 +2803,7 @@ static int arm_smmu_attach_dev_ste(struct iommu_domain *domain, .old_domain = iommu_get_domain_for_dev(dev), }; - if (arm_smmu_master_sva_enabled(master)) + if (arm_smmu_ssids_in_use(&master->cd_table)) return -EBUSY; /* 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 23d8a5b7912069..e850b7913e67a1 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -602,12 +602,19 @@ struct arm_smmu_ctx_desc_cfg { dma_addr_t cdtab_dma; struct arm_smmu_l1_ctx_desc *l1_desc; unsigned int num_l1_ents; + unsigned int used_ssids; u8 in_ste; u8 s1fmt; /* log2 of the maximum number of CDs supported by this table */ u8 s1cdmax; }; +/* True if the cd table has SSIDS > 0 in use. */ +static inline bool arm_smmu_ssids_in_use(struct arm_smmu_ctx_desc_cfg *cd_table) +{ + return cd_table->used_ssids; +} + struct arm_smmu_s2_cfg { u16 vmid; };
We no longer need a master->sva_enable to control what attaches are allowed. Instead we can tell if the attach is legal based on the current configuration of the master. Keep track of the number of valid CD entries for SSID's in the cd_table and if the cd_table has been installed in the STE directly so we know what the configuration is. The attach logic is then made into: - SVA bind, check if the CD is installed - RID attach of S2, block if SSIDs are used - RID attach of IDENTITY/BLOCKING, block if SSIDs are used Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++---------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 7 ++++++ 3 files changed, 20 insertions(+), 13 deletions(-)