| Message ID | 12-v2-de8b10590bf5+400-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update SMMUv3 to the modern iommu API (part 1/3) | expand |
On Tue, Nov 14, 2023 at 1:53 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > Get closer to the IOMMU API ideal that changes between domains can be > hitless. The ordering for the CD table entry is not entirely clean from > this perspective. > > When switching away from a STE with a CD table programmed in it we should > write the new STE first, then clear any old data in the CD entry. > > If we are programming a CD table for the first time to a STE then the CD > entry should be programmed before the STE is loaded. > > If we are replacing a CD table entry when the STE already points at the CD > entry then we just need to do the make/break sequence. > > Lift this code out of arm_smmu_detach_dev() so it can all be sequenced > properly. The only other caller is arm_smmu_release_device() and it is > going to free the cdtable anyhow, so it doesn't matter what is in it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Michael Shavit <mshavit@google.com> This patch might be a better fit before the previous one. When going from S1 to S2 or bypass: Pre-both patches, attach_dev() installs a NULL STE, then clears the now unused CDE, then installs a new STE. After the previous patch, attach_dev() clears the *still used* CDE, and then replaces the STE. After this patch, attach_dev() replaces the STE, and then clears the CDE Reordering the two patches removes the scenario where we could hit a NULL-ed CDE. > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29 ++++++++++++++------- > 1 file changed, 20 insertions(+), 9 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 f70862806211de..eb5dcd357a42b8 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2495,14 +2495,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master) > > master->domain = NULL; > master->ats_enabled = false; > - /* > - * Clearing the CD entry isn't strictly required to detach the domain > - * since the table is uninstalled anyway, but it helps avoid confusion > - * in the call to arm_smmu_write_ctx_desc on the next attach (which > - * expects the entry to be empty). > - */ > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->cd_table.cdtab) > - arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, NULL); > } > > static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > @@ -2579,6 +2571,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > master->domain = NULL; > goto out_list_del; > } > + } else { > + /* > + * arm_smmu_write_ctx_desc() relies on the entry being > + * invalid to work, clear any existing entry. > + */ > + ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, > + NULL); > + if (ret) { > + master->domain = NULL; > + goto out_list_del; > + } > } > > ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); > @@ -2588,15 +2591,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > > arm_smmu_make_cdtable_ste(&target, master, &master->cd_table); > + arm_smmu_install_ste_for_dev(master, &target); > break; > case ARM_SMMU_DOMAIN_S2: > arm_smmu_make_s2_domain_ste(&target, master, smmu_domain); > + arm_smmu_install_ste_for_dev(master, &target); > + if (master->cd_table.cdtab) > + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, > + NULL); > break; > case ARM_SMMU_DOMAIN_BYPASS: > arm_smmu_make_bypass_ste(&target); > + arm_smmu_install_ste_for_dev(master, &target); > + if (master->cd_table.cdtab) > + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, > + NULL); > break; > } > - arm_smmu_install_ste_for_dev(master, &target); > > arm_smmu_enable_ats(master); > goto out_unlock; > -- > 2.42.0 >
On Wed, Nov 15, 2023 at 11:32:28PM +0800, Michael Shavit wrote: > > Lift this code out of arm_smmu_detach_dev() so it can all be sequenced > > properly. The only other caller is arm_smmu_release_device() and it is > > going to free the cdtable anyhow, so it doesn't matter what is in it. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > Reviewed-by: Michael Shavit <mshavit@google.com> > > This patch might be a better fit before the previous one. When going > from S1 to S2 or bypass: > Pre-both patches, attach_dev() installs a NULL STE, then clears the > now unused CDE, then installs a new STE. > After the previous patch, attach_dev() clears the *still used* CDE, > and then replaces the STE. > After this patch, attach_dev() replaces the STE, and then clears the CDE > > Reordering the two patches removes the scenario where we could hit a > NULL-ed CDE. NULLed = non-valid I see what you mean, but I haven't thought carefully about a different order so I'd rather leave it.. Regardless of order the two prior patches will have cases that hit non-valid/abort STE/CDEs, each step removes a few cases. Thanks, Jason
On Fri, Nov 17, 2023 at 12:47 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Nov 15, 2023 at 11:32:28PM +0800, Michael Shavit wrote: > > > > Lift this code out of arm_smmu_detach_dev() so it can all be sequenced > > > properly. The only other caller is arm_smmu_release_device() and it is > > > going to free the cdtable anyhow, so it doesn't matter what is in it. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Reviewed-by: Michael Shavit <mshavit@google.com> > > > > This patch might be a better fit before the previous one. When going > > from S1 to S2 or bypass: > > Pre-both patches, attach_dev() installs a NULL STE, then clears the > > now unused CDE, then installs a new STE. > > After the previous patch, attach_dev() clears the *still used* CDE, > > and then replaces the STE. > > After this patch, attach_dev() replaces the STE, and then clears the CDE > > > > Reordering the two patches removes the scenario where we could hit a > > NULL-ed CDE. > > NULLed = non-valid > > I see what you mean, but I haven't thought carefully about a different > order so I'd rather leave it.. Ack; it's probably too subtle to matter much anyhow. > > Regardless of order the two prior patches will have cases that hit > non-valid/abort STE/CDEs, each step removes a few cases. > > Thanks, > Jason On Fri, Nov 17, 2023 at 12:47 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Nov 15, 2023 at 11:32:28PM +0800, Michael Shavit wrote: > > > > Lift this code out of arm_smmu_detach_dev() so it can all be sequenced > > > properly. The only other caller is arm_smmu_release_device() and it is > > > going to free the cdtable anyhow, so it doesn't matter what is in it. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Reviewed-by: Michael Shavit <mshavit@google.com> > > > > This patch might be a better fit before the previous one. When going > > from S1 to S2 or bypass: > > Pre-both patches, attach_dev() installs a NULL STE, then clears the > > now unused CDE, then installs a new STE. > > After the previous patch, attach_dev() clears the *still used* CDE, > > and then replaces the STE. > > After this patch, attach_dev() replaces the STE, and then clears the CDE > > > > Reordering the two patches removes the scenario where we could hit a > > NULL-ed CDE. > > NULLed = non-valid > > I see what you mean, but I haven't thought carefully about a different > order so I'd rather leave it.. > > Regardless of order the two prior patches will have cases that hit > non-valid/abort STE/CDEs, each step removes a few cases. > > Thanks, > Jason
On Mon, Nov 13, 2023 at 01:53:19PM -0400, Jason Gunthorpe wrote: > Get closer to the IOMMU API ideal that changes between domains can be > hitless. The ordering for the CD table entry is not entirely clean from > this perspective. > > When switching away from a STE with a CD table programmed in it we should > write the new STE first, then clear any old data in the CD entry. > > If we are programming a CD table for the first time to a STE then the CD > entry should be programmed before the STE is loaded. > > If we are replacing a CD table entry when the STE already points at the CD > entry then we just need to do the make/break sequence. > > Lift this code out of arm_smmu_detach_dev() so it can all be sequenced > properly. The only other caller is arm_smmu_release_device() and it is > going to free the cdtable anyhow, so it doesn't matter what is in it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
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 f70862806211de..eb5dcd357a42b8 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2495,14 +2495,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master) master->domain = NULL; master->ats_enabled = false; - /* - * Clearing the CD entry isn't strictly required to detach the domain - * since the table is uninstalled anyway, but it helps avoid confusion - * in the call to arm_smmu_write_ctx_desc on the next attach (which - * expects the entry to be empty). - */ - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->cd_table.cdtab) - arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, NULL); } static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) @@ -2579,6 +2571,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) master->domain = NULL; goto out_list_del; } + } else { + /* + * arm_smmu_write_ctx_desc() relies on the entry being + * invalid to work, clear any existing entry. + */ + ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, + NULL); + if (ret) { + master->domain = NULL; + goto out_list_del; + } } ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); @@ -2588,15 +2591,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } arm_smmu_make_cdtable_ste(&target, master, &master->cd_table); + arm_smmu_install_ste_for_dev(master, &target); break; case ARM_SMMU_DOMAIN_S2: arm_smmu_make_s2_domain_ste(&target, master, smmu_domain); + arm_smmu_install_ste_for_dev(master, &target); + if (master->cd_table.cdtab) + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, + NULL); break; case ARM_SMMU_DOMAIN_BYPASS: arm_smmu_make_bypass_ste(&target); + arm_smmu_install_ste_for_dev(master, &target); + if (master->cd_table.cdtab) + arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, + NULL); break; } - arm_smmu_install_ste_for_dev(master, &target); arm_smmu_enable_ats(master); goto out_unlock;
Get closer to the IOMMU API ideal that changes between domains can be hitless. The ordering for the CD table entry is not entirely clean from this perspective. When switching away from a STE with a CD table programmed in it we should write the new STE first, then clear any old data in the CD entry. If we are programming a CD table for the first time to a STE then the CD entry should be programmed before the STE is loaded. If we are replacing a CD table entry when the STE already points at the CD entry then we just need to do the make/break sequence. Lift this code out of arm_smmu_detach_dev() so it can all be sequenced properly. The only other caller is arm_smmu_release_device() and it is going to free the cdtable anyhow, so it doesn't matter what is in it. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 29 ++++++++++++++------- 1 file changed, 20 insertions(+), 9 deletions(-)