| Message ID | 7-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:15PM -0300, Jason Gunthorpe wrote: > Allow creating and managing arm_smmu_mater_domain's with a non-zero SSID > through the arm_smmu_attach_*() family of functions. This triggers ATC > invalidation for the correct SSID in PASID cases and tracks the > per-attachment SSID in the struct arm_smmu_master_domain. > > Generalize arm_smmu_attach_remove() to be able to remove SSID's as well by > ensuring the ATC for the PASID is flushed properly. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > @@ -2689,11 +2692,14 @@ static void arm_smmu_attach_commit(struct arm_smmu_master *master, > * SMMU is translating for the new domain and both the old&new > * domain will issue invalidations. > */ > - arm_smmu_atc_inv_master(master); > + if (state->want_ats) > + arm_smmu_atc_inv_master(master, state->ssid); Just trying to confirm my understanding here: In the arm_smmu_set_pasid pathway, ssid is a valid pasid. So, here we invalidate both new and old master_domains using the same ssid/pasid, if either of them occupies the same cd entry. And... > - arm_smmu_remove_master_domain(master, state->old_domain); > + arm_smmu_remove_master_domain(master, state->old_domain, state->ssid); then we remove the old one here. Though the flow makes sense to me, yet I wonder if there can be such a use case like this where an old master_domain is holding the same cd entry as the one wanted by the new master_domain? Thanks Nicolin
On Sun, May 12, 2024 at 11:32:53PM -0700, Nicolin Chen wrote: > On Wed, May 08, 2024 at 03:57:15PM -0300, Jason Gunthorpe wrote: > > Allow creating and managing arm_smmu_mater_domain's with a non-zero SSID > > through the arm_smmu_attach_*() family of functions. This triggers ATC > > invalidation for the correct SSID in PASID cases and tracks the > > per-attachment SSID in the struct arm_smmu_master_domain. > > > > Generalize arm_smmu_attach_remove() to be able to remove SSID's as well by > > ensuring the ATC for the PASID is flushed properly. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > > > @@ -2689,11 +2692,14 @@ static void arm_smmu_attach_commit(struct arm_smmu_master *master, > > * SMMU is translating for the new domain and both the old&new > > * domain will issue invalidations. > > */ > > - arm_smmu_atc_inv_master(master); > > + if (state->want_ats) > > + arm_smmu_atc_inv_master(master, state->ssid); > > Just trying to confirm my understanding here: > > In the arm_smmu_set_pasid pathway, ssid is a valid pasid. So, > here we invalidate both new and old master_domains using the > same ssid/pasid, if either of them occupies the same cd entry. Yes. > And... > > > - arm_smmu_remove_master_domain(master, state->old_domain); > > + arm_smmu_remove_master_domain(master, state->old_domain, state->ssid); > > then we remove the old one here. > > Though the flow makes sense to me, yet I wonder if there can be > such a use case like this where an old master_domain is holding > the same cd entry as the one wanted by the new master_domain? It could happen, the API allows it, and the short lived double invalidation is a trade off to allow loose locking on the invalidation side. For this kind of thing we want to make the common case of invalidation to run fast. 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 5f418b96679c88..b2d8b7a1df343a 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2005,13 +2005,14 @@ arm_smmu_atc_inv_to_cmd(int ssid, unsigned long iova, size_t size, cmd->atc.size = log2_span; } -static int arm_smmu_atc_inv_master(struct arm_smmu_master *master) +static int arm_smmu_atc_inv_master(struct arm_smmu_master *master, + ioasid_t ssid) { int i; struct arm_smmu_cmdq_ent cmd; struct arm_smmu_cmdq_batch cmds; - arm_smmu_atc_inv_to_cmd(IOMMU_NO_PASID, 0, 0, &cmd); + arm_smmu_atc_inv_to_cmd(ssid, 0, 0, &cmd); cmds.num = 0; for (i = 0; i < master->num_streams; i++) { @@ -2494,7 +2495,7 @@ static void arm_smmu_enable_ats(struct arm_smmu_master *master) /* * ATC invalidation of PASID 0 causes the entire ATC to be flushed. */ - arm_smmu_atc_inv_master(master); + arm_smmu_atc_inv_master(master, IOMMU_NO_PASID); if (pci_enable_ats(pdev, stu)) dev_err(master->dev, "Failed to enable ATS (STU %zu)\n", stu); } @@ -2581,7 +2582,8 @@ to_smmu_domain_devices(struct iommu_domain *domain) } static void arm_smmu_remove_master_domain(struct arm_smmu_master *master, - struct iommu_domain *domain) + struct iommu_domain *domain, + ioasid_t ssid) { struct arm_smmu_domain *smmu_domain = to_smmu_domain_devices(domain); struct arm_smmu_master_domain *master_domain; @@ -2591,8 +2593,7 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master, return; spin_lock_irqsave(&smmu_domain->devices_lock, flags); - master_domain = arm_smmu_find_master_domain(smmu_domain, master, - IOMMU_NO_PASID); + master_domain = arm_smmu_find_master_domain(smmu_domain, master, ssid); if (master_domain) { list_del(&master_domain->devices_elm); kfree(master_domain); @@ -2606,6 +2607,7 @@ struct attach_state { bool want_ats; bool disable_ats; struct iommu_domain *old_domain; + ioasid_t ssid; }; /* @@ -2637,6 +2639,7 @@ static int arm_smmu_attach_prepare(struct arm_smmu_master *master, if (!master_domain) return -ENOMEM; master_domain->master = master; + master_domain->ssid = state->ssid; /* * During prepare we want the current smmu_domain and new @@ -2689,11 +2692,14 @@ static void arm_smmu_attach_commit(struct arm_smmu_master *master, * SMMU is translating for the new domain and both the old&new * domain will issue invalidations. */ - arm_smmu_atc_inv_master(master); + if (state->want_ats) + arm_smmu_atc_inv_master(master, state->ssid); + else + arm_smmu_atc_inv_master(master, IOMMU_NO_PASID); } master->ats_enabled = state->want_ats; - arm_smmu_remove_master_domain(master, state->old_domain); + arm_smmu_remove_master_domain(master, state->old_domain, state->ssid); } static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) @@ -2705,6 +2711,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct attach_state state = { .old_domain = iommu_get_domain_for_dev(dev), + .ssid = IOMMU_NO_PASID, }; struct arm_smmu_master *master; struct arm_smmu_cd *cdptr; @@ -2801,6 +2808,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 = { .old_domain = iommu_get_domain_for_dev(dev), + .ssid = IOMMU_NO_PASID, }; if (arm_smmu_ssids_in_use(&master->cd_table))