| Message ID | 16-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 2/3) | expand |
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > We no longer need a master->sva_enable to control what attaches are > allowed. > > Instead keep track inside the cd_table how many valid CD entries exist, > and if the RID has a valid entry. > > Replace all the attach focused master->sva_enabled tests with a check if > the CD has valid entries (or not). If there are any valid entries then the > CD table must be currently programmed to the STE. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +--- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++++--------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 +++++++ > 3 files changed, 25 insertions(+), 16 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 ab9de8e36c45f5..82b9c4d4061c3d 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 > @@ -433,9 +433,6 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, > if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) > return -ENODEV; > I assume this doesn't matter because of subsequent patches, but the check above could also be removed since used_sid precisely means that the attached domain is an ARM_SMMU_DOMAIN_S1 domain. > - if (!master || !master->sva_enabled) > - return -ENODEV; > - > bond = kzalloc(sizeof(*bond), GFP_KERNEL); > if (!bond) > return -ENOMEM; > @@ -638,7 +635,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, > struct mm_struct *mm = domain->mm; > struct arm_smmu_cd target; > > - if (mm_get_enqcmd_pasid(mm) != id) > + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.used_sid) > return -EINVAL; > > if (!arm_smmu_get_cd_ptr(master, id)) > 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 26c6b9f6f34fd3..fc19585bf192c6 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1285,6 +1285,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, > @@ -1293,6 +1295,15 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, > .ssid = ssid, > }; > > + if (cur_valid != target_valid) { > + if (cur_valid) > + master->cd_table.used_ssids--; > + else > + master->cd_table.used_ssids++; > + } > + if (ssid == IOMMU_NO_PASID) > + master->cd_table.used_sid = target_valid; > + > arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); > } > > @@ -2725,16 +2736,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) { > @@ -2750,7 +2751,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > cdptr = arm_smmu_get_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 > @@ -2825,7 +2827,7 @@ static int arm_smmu_attach_dev_ste(struct iommu_domain *domain, > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > struct attach_state state = {}; > > - 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 98dc5885c48655..7e1f6af4ce4e79 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -602,11 +602,21 @@ 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; > + bool used_sid; This probably deserves a comment. There's plenty of places where the "rid" domain is handled as the CD with ssid 0; but we don't count it as a used_ssid here. I also don't find the meaning of used_sid obvious, especially if I didn't have the context from the commit description. > 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) > +{ > + if (cd_table->used_sid) > + return cd_table->used_ssids > 1; > + return cd_table->used_ssids; > +} > + > struct arm_smmu_s2_cfg { > u16 vmid; > }; > -- > 2.43.2 >
On Tue, Mar 19, 2024 at 09:55:17PM +0800, Michael Shavit wrote: > On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > We no longer need a master->sva_enable to control what attaches are > > allowed. > > > > Instead keep track inside the cd_table how many valid CD entries exist, > > and if the RID has a valid entry. > > > > Replace all the attach focused master->sva_enabled tests with a check if > > the CD has valid entries (or not). If there are any valid entries then the > > CD table must be currently programmed to the STE. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 5 +--- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++++--------- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 10 +++++++ > > 3 files changed, 25 insertions(+), 16 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 ab9de8e36c45f5..82b9c4d4061c3d 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 > > @@ -433,9 +433,6 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, > > if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) > > return -ENODEV; > > > I assume this doesn't matter because of subsequent patches, but the > check above could also be removed since used_sid precisely means that > the attached domain is an ARM_SMMU_DOMAIN_S1 domain. Right, but lets move the delete here for clarity. The same comment applies to some later patches too that do: if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev)) || !master->cd_table.used_sid) return -ENODEV; > > 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 98dc5885c48655..7e1f6af4ce4e79 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > > @@ -602,11 +602,21 @@ 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; > > + bool used_sid; > > This probably deserves a comment. There's plenty of places where the > "rid" domain is handled as the CD with ssid 0; but we don't count it > as a used_ssid here. As a page table? I didn't think so, the only way to get a CD page table installed is through arm_smmu_write_cd_entry() which will capture this.. Non paging domains don't get captured here, they are translating the RID but they are not using the CD table. > I also don't find the meaning of used_sid obvious, especially if I > didn't have the context from the commit description. Hum, okay, so looking over all of this again I think we can simplify. At the end there was only one place using used_sid and it can instead be calling arm_smmu_ssids_in_use() directly: @@ -2987,11 +2986,13 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) * When the last user of the CD table goes away downgrade the STE back * to a non-cd_table one. */ - if (last_ssid && !master->cd_table.used_sid) { + if (!arm_smmu_ssids_in_use(&master->cd_table)) { struct iommu_domain *sid_domain = iommu_get_domain_for_dev(master->dev); - sid_domain->ops->attach_dev(sid_domain, master->dev); + if (domain->type == IOMMU_DOMAIN_IDENTITY || + domain->type == IOMMU_DOMAIN_BLOCKED) + sid_domain->ops->attach_dev(sid_domain, dev); } Then we can get rid of used_sid and just have used_ssids count the !0 ssids directly. I reorganized a bunch of things in the in between patches so we go more directly to this final outcome. Thanks, Jason
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 ab9de8e36c45f5..82b9c4d4061c3d 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 @@ -433,9 +433,6 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) return -ENODEV; - if (!master || !master->sva_enabled) - return -ENODEV; - bond = kzalloc(sizeof(*bond), GFP_KERNEL); if (!bond) return -ENOMEM; @@ -638,7 +635,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, struct mm_struct *mm = domain->mm; struct arm_smmu_cd target; - if (mm_get_enqcmd_pasid(mm) != id) + if (mm_get_enqcmd_pasid(mm) != id || !master->cd_table.used_sid) return -EINVAL; if (!arm_smmu_get_cd_ptr(master, id)) 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 26c6b9f6f34fd3..fc19585bf192c6 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1285,6 +1285,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, @@ -1293,6 +1295,15 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, .ssid = ssid, }; + if (cur_valid != target_valid) { + if (cur_valid) + master->cd_table.used_ssids--; + else + master->cd_table.used_ssids++; + } + if (ssid == IOMMU_NO_PASID) + master->cd_table.used_sid = target_valid; + arm_smmu_write_entry(&cd_writer.writer, cdptr->data, target->data); } @@ -2725,16 +2736,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) { @@ -2750,7 +2751,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) cdptr = arm_smmu_get_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 @@ -2825,7 +2827,7 @@ static int arm_smmu_attach_dev_ste(struct iommu_domain *domain, struct arm_smmu_master *master = dev_iommu_priv_get(dev); struct attach_state state = {}; - 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 98dc5885c48655..7e1f6af4ce4e79 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -602,11 +602,21 @@ 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; + bool used_sid; 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) +{ + if (cd_table->used_sid) + return cd_table->used_ssids > 1; + return cd_table->used_ssids; +} + struct arm_smmu_s2_cfg { u16 vmid; };