| Message ID | 9-v5-cd1be8dd9c71+3fa-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers | expand |
Hi Jason, On Tue, Feb 06, 2024 at 11:12:46AM -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. > > Reviewed-by: Michael Shavit <mshavit@google.com> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Moritz Fischer <moritzf@google.com> > 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(-) > > 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 340f3dc82c9ce0..2a6ac0af932c54 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2575,14 +2575,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) > @@ -2659,6 +2651,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; > + } Instead of having duplicate if (ret) { master->domain = NULL; goto out_list_del; } In the if and the else, we can just move it outside. > } > > ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); > @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > > arm_smmu_make_cdtable_ste(&target, master); > + 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; > } This invalidates the CD while the STE is in bypass/S2 which is a new behavior to the driver, I don’t see anything from the user manual about this, so I believe that is fine. Reviewed-by: Mostafa Saleh <smostafa@google.com> > - arm_smmu_install_ste_for_dev(master, &target); > > arm_smmu_enable_ats(master); > goto out_unlock; > -- > 2.43.0 > Thanks, Mostafa
On Tue, Feb 13, 2024 at 03:42:53PM +0000, Mostafa Saleh wrote: > > @@ -2659,6 +2651,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; > > + } > > Instead of having duplicate > if (ret) { > master->domain = NULL; > goto out_list_del; > } > > In the if and the else, we can just move it outside. Stylistically I often try to avoid shifting the error path from its statement, but it is OK either way.. However, part 2 removes the need for error handling here entirely, so let's leave it. > > @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > > } > > > > arm_smmu_make_cdtable_ste(&target, master); > > + 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; > > } > This invalidates the CD while the STE is in bypass/S2 which is a new behavior > to the driver, Yes > I don’t see anything from the user manual about this, so I > believe that is fine. Nor do I. Nor can I see any reason why HW would care. We also invalidate ASID's and VMID's after their tables have been removed from the STE/CD too. There are other options here if this is found out to be a trouble but they are convoluted enough to not do them without a concrete reason. Thanks, 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 340f3dc82c9ce0..2a6ac0af932c54 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2575,14 +2575,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) @@ -2659,6 +2651,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); @@ -2668,15 +2671,23 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } arm_smmu_make_cdtable_ste(&target, master); + 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;