| Message ID | 2-v9-5cd718286059+79186-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 Tue, Jun 25, 2024 at 09:37:33AM -0300, Jason Gunthorpe wrote: > Add arm_smmu_set_pasid()/arm_smmu_remove_pasid() which are to be used by > callers that already constructed the arm_smmu_cd they wish to program. > > These functions will encapsulate the shared logic to setup a CD entry that > will be shared by SVA and S1 domain cases. > > Prior fixes had already moved most of this logic up into > __arm_smmu_sva_bind(), move it to it's final home. > > Following patches will relieve some of the remaining SVA restrictions: > > - The RID domain is a S1 domain and has already setup the STE to point to > the CD table > - The programmed PASID is the mm_get_enqcmd_pasid() > - Nothing changes while SVA is running (sva_enable) > > SVA invalidation will still iterate over the S1 domain's master list, > later patches will resolve that. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 57 ++++++++++--------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++++++++- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 ++- > 3 files changed, 67 insertions(+), 31 deletions(-) [...] > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, > struct arm_smmu_bond *bond = NULL, *t; > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > + arm_smmu_remove_pasid(master, to_smmu_domain(domain), id); > + > mutex_lock(&sva_lock); > - > - arm_smmu_clear_cd(master, id); This looks a bit alarming, as you're effectively moving the CD modification outside of the critical section. I assume we're relying on the iommu group mutex to serialise this in the caller? I can't see any consistent locking in the driver for arm_smmu_clear_cd(). As an additional patch, perhaps we should consider documenting what each lock in the driver protects and the lock ordering requirements they have? We've got a few global locks with generic names and after a few rounds of refactoring it's really hard to know who's responsible for what, especially now that we have stale comments referring to arm_smmu_share_asid(). We've also grown a number of places where we drop a lock in the callee and immediately re-take it in the caller, which tends to be a source of bugs. Thanks, Will
On Tue, Jul 02, 2024 at 03:57:05PM +0100, Will Deacon wrote: > > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, > > struct arm_smmu_bond *bond = NULL, *t; > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > > > + arm_smmu_remove_pasid(master, to_smmu_domain(domain), id); > > + > > mutex_lock(&sva_lock); > > - > > - arm_smmu_clear_cd(master, id); > > This looks a bit alarming, as you're effectively moving the CD > modification outside of the critical section. I assume we're relying on > the iommu group mutex to serialise this in the caller? I can't see any > consistent locking in the driver for arm_smmu_clear_cd(). Hi Will, your assumption is correct. Jason had the same remark during the v7 review: https://lore.kernel.org/linux-iommu/ZkDAL+TX93QfTFMc@nvidia.com/ Thanks Nicolin
On Tue, Jul 02, 2024 at 03:57:05PM +0100, Will Deacon wrote: > > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, > > struct arm_smmu_bond *bond = NULL, *t; > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > > > + arm_smmu_remove_pasid(master, to_smmu_domain(domain), id); > > + > > mutex_lock(&sva_lock); > > - > > - arm_smmu_clear_cd(master, id); > > This looks a bit alarming, as you're effectively moving the CD > modification outside of the critical section. I assume we're relying on > the iommu group mutex to serialise this in the caller? I can't see any > consistent locking in the driver for arm_smmu_clear_cd(). I see Nicolin got this - but yes, sva_lock has nothing to do with CD. CD/STE has always been protected by the group_mutex. > As an additional patch, perhaps we should consider documenting what each > lock in the driver protects and the lock ordering requirements they > have? There is still a bunch of rework to do here, it may be better to complete the rework than to try to document it, but let me know which ones you are interested in and I'll write some thing. sva_lock is almost gone, it just locks the IOPF flow on the master because we are doing the IOPF flow in the wrong place. The IOPF enable/disbale should be done in attach under the group mutex. init_mutex will be deleted once the iommu_domain_alloc_paging() conversion is done. asid_lock.. Is a place holder for the nascent BTM support. It locks domain->asid only. The BTM patches make this per-instance instead of global. Unfortunately that locking scheme doesn't work 100%, but I have a notion how to fix it.. asid_lock is also going to need some reconsidering when we make the domain able to attach to multiple instances which is something iommufd wants. devices_lock protects the device list only, excluding the special nr_ats_masters thing. Then the hidden group mutex makes all the ops touching master single threaded, and the driver has always quietly relied on this. It protects the STE/CD and parts of the master. So I think we end up with only the group mutex, asid_lock and devices_lock it is OK. The order is group -> asid -> devices, which is layed out clearly in the attach functions. > We've got a few global locks with generic names and after a few > rounds of refactoring it's really hard to know who's responsible for > what, especially now that we have stale comments referring to > arm_smmu_share_asid(). > We've also grown a number of places where we > drop a lock in the callee and immediately re-take it in the caller, > which tends to be a source of bugs. Do we? Can you point to what you noticed? Thanks, Jason
Hi Jason, Sorry, it's taken me ages to get back to this after applying the series. On Tue, Jul 09, 2024 at 04:39:05PM -0300, Jason Gunthorpe wrote: > On Tue, Jul 02, 2024 at 03:57:05PM +0100, Will Deacon wrote: > > > > @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, > > > struct arm_smmu_bond *bond = NULL, *t; > > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > > > > > + arm_smmu_remove_pasid(master, to_smmu_domain(domain), id); > > > + > > > mutex_lock(&sva_lock); > > > - > > > - arm_smmu_clear_cd(master, id); > > > > This looks a bit alarming, as you're effectively moving the CD > > modification outside of the critical section. I assume we're relying on > > the iommu group mutex to serialise this in the caller? I can't see any > > consistent locking in the driver for arm_smmu_clear_cd(). > > I see Nicolin got this - but yes, sva_lock has nothing to do with > CD. CD/STE has always been protected by the group_mutex. > > > As an additional patch, perhaps we should consider documenting what each > > lock in the driver protects and the lock ordering requirements they > > have? > > There is still a bunch of rework to do here, it may be better to > complete the rework than to try to document it, but let me know which > ones you are interested in and I'll write some thing. I think listing the locks we have in the driver and describing both what they protect and the ordering between them would be really helpful. > sva_lock is almost gone, it just locks the IOPF flow on the master > because we are doing the IOPF flow in the wrong place. The IOPF > enable/disbale should be done in attach under the group mutex. > > init_mutex will be deleted once the iommu_domain_alloc_paging() > conversion is done. > > asid_lock.. Is a place holder for the nascent BTM support. It locks > domain->asid only. The BTM patches make this per-instance instead of > global. Unfortunately that locking scheme doesn't work 100%, but I > have a notion how to fix it.. > > asid_lock is also going to need some reconsidering when we make the > domain able to attach to multiple instances which is something iommufd > wants. > > devices_lock protects the device list only, excluding the special > nr_ats_masters thing. > > Then the hidden group mutex makes all the ops touching master single > threaded, and the driver has always quietly relied on this. It > protects the STE/CD and parts of the master. > > So I think we end up with only the group mutex, asid_lock and > devices_lock it is OK. > > The order is group -> asid -> devices, which is layed out clearly in > the attach functions. > > > We've got a few global locks with generic names and after a few > > rounds of refactoring it's really hard to know who's responsible for > > what, especially now that we have stale comments referring to > > arm_smmu_share_asid(). > > > We've also grown a number of places where we > > drop a lock in the callee and immediately re-take it in the caller, > > which tends to be a source of bugs. > > Do we? Can you point to what you noticed? As I recall, I just noticed that: - We have a bunch of comments around 'arm_smmu_asid_lock' that refer to arm_smmu_share_asid(), which no longer exists. - arm_smmu_remove_dev_pasid() drops the asid_lock only to have it retaken in the callee via ->attach_dev(). - arm_smmu_attach_dev() takes/drops/re-takes the devices_lock indirectly when it calls arm_smmu_attach_prepare() and arm_smmu_attach_commit(). - arm_smmu_attach_dev() takes/drops 'arm_smmu_asid_lock' via arm_smmu_domain_finalise()) and then re-takes it before the attach. Please note, I'm not saying that there's a bug here, just that it would be easier to work with if we had some documentation and lock ordering assertions. Will
On Fri, Sep 06, 2024 at 03:08:54PM +0100, Will Deacon wrote: > - We have a bunch of comments around 'arm_smmu_asid_lock' that refer > to arm_smmu_share_asid(), which no longer exists. So, we can go ahead and put back the BTM support in the pre-existing slightly racy form. I think we can turn it on for guest support by operating it only if there is no S2 support in HW which would make it justified. Or we could delete the 'arm_smmu_asid_lock' for now. My idea to fix the race goes through making the domains sharable across instances because that will change the invalidation in a way that lets double invalidation happen during the ASID change. I was planning to wait for that, but it looks like it will be more time. It is linked to the viommu work which needs to go first. > - arm_smmu_attach_dev() takes/drops/re-takes the devices_lock indirectly > when it calls arm_smmu_attach_prepare() and arm_smmu_attach_commit(). This isn't retaking a lock, the operation being locked ran to completion, we just need to run two operations.. > - arm_smmu_attach_dev() takes/drops 'arm_smmu_asid_lock' via > arm_smmu_domain_finalise()) and then re-takes it before the attach. finalize won't be called from arm_smmu_attach_dev() very soon, they are unrelated operations. > Please note, I'm not saying that there's a bug here, just that it would > be easier to work with if we had some documentation and lock ordering > assertions. There are more assertions than there was now, I think most of the new code has them already, excluding the group mutex. 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 28f8bf4327f69a..71ca87c2c5c3b6 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 @@ -417,29 +417,27 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn) arm_smmu_free_shared_cd(cd); } -static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid, - struct mm_struct *mm) +static struct arm_smmu_bond *__arm_smmu_sva_bind(struct device *dev, + struct mm_struct *mm) { int ret; - struct arm_smmu_cd target; - struct arm_smmu_cd *cdptr; struct arm_smmu_bond *bond; struct arm_smmu_master *master = dev_iommu_priv_get(dev); struct iommu_domain *domain = iommu_get_domain_for_dev(dev); struct arm_smmu_domain *smmu_domain; if (!(domain->type & __IOMMU_DOMAIN_PAGING)) - return -ENODEV; + return ERR_PTR(-ENODEV); smmu_domain = to_smmu_domain(domain); if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) - return -ENODEV; + return ERR_PTR(-ENODEV); if (!master || !master->sva_enabled) - return -ENODEV; + return ERR_PTR(-ENODEV); bond = kzalloc(sizeof(*bond), GFP_KERNEL); if (!bond) - return -ENOMEM; + return ERR_PTR(-ENOMEM); bond->mm = mm; @@ -449,22 +447,12 @@ static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid, goto err_free_bond; } - cdptr = arm_smmu_alloc_cd_ptr(master, mm_get_enqcmd_pasid(mm)); - if (!cdptr) { - ret = -ENOMEM; - goto err_put_notifier; - } - arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid); - arm_smmu_write_cd_entry(master, pasid, cdptr, &target); - list_add(&bond->list, &master->bonds); - return 0; + return bond; -err_put_notifier: - arm_smmu_mmu_notifier_put(bond->smmu_mn); err_free_bond: kfree(bond); - return ret; + return ERR_PTR(ret); } bool arm_smmu_sva_supported(struct arm_smmu_device *smmu) @@ -611,10 +599,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, struct arm_smmu_bond *bond = NULL, *t; struct arm_smmu_master *master = dev_iommu_priv_get(dev); + arm_smmu_remove_pasid(master, to_smmu_domain(domain), id); + mutex_lock(&sva_lock); - - arm_smmu_clear_cd(master, id); - list_for_each_entry(t, &master->bonds, list) { if (t->mm == mm) { bond = t; @@ -633,17 +620,33 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain, struct device *dev, ioasid_t id) { - int ret = 0; + struct arm_smmu_master *master = dev_iommu_priv_get(dev); struct mm_struct *mm = domain->mm; + struct arm_smmu_bond *bond; + struct arm_smmu_cd target; + int ret; if (mm_get_enqcmd_pasid(mm) != id) return -EINVAL; mutex_lock(&sva_lock); - ret = __arm_smmu_sva_bind(dev, id, mm); - mutex_unlock(&sva_lock); + bond = __arm_smmu_sva_bind(dev, mm); + if (IS_ERR(bond)) { + mutex_unlock(&sva_lock); + return PTR_ERR(bond); + } - return ret; + arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid); + ret = arm_smmu_set_pasid(master, NULL, id, &target); + if (ret) { + list_del(&bond->list); + arm_smmu_mmu_notifier_put(bond->smmu_mn); + kfree(bond); + mutex_unlock(&sva_lock); + return ret; + } + mutex_unlock(&sva_lock); + return 0; } static void arm_smmu_sva_domain_free(struct iommu_domain *domain) 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 bd79422f7b6f50..3f19436fe86a37 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1211,8 +1211,8 @@ struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, return &l1_desc->l2ptr[ssid % CTXDESC_L2_ENTRIES]; } -struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, - u32 ssid) +static struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, + u32 ssid) { struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table; struct arm_smmu_device *smmu = master->smmu; @@ -2412,6 +2412,10 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master, int i, j; struct arm_smmu_device *smmu = master->smmu; + master->cd_table.in_ste = + FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(target->data[0])) == + STRTAB_STE_0_CFG_S1_TRANS; + for (i = 0; i < master->num_streams; ++i) { u32 sid = master->streams[i].id; struct arm_smmu_ste *step = @@ -2632,6 +2636,30 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) return 0; } +int arm_smmu_set_pasid(struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain, ioasid_t pasid, + const struct arm_smmu_cd *cd) +{ + struct arm_smmu_cd *cdptr; + + /* The core code validates pasid */ + + if (!master->cd_table.in_ste) + return -ENODEV; + + cdptr = arm_smmu_alloc_cd_ptr(master, pasid); + if (!cdptr) + return -ENOMEM; + arm_smmu_write_cd_entry(master, pasid, cdptr, cd); + return 0; +} + +void arm_smmu_remove_pasid(struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain, ioasid_t pasid) +{ + arm_smmu_clear_cd(master, pasid); +} + static int arm_smmu_attach_dev_ste(struct device *dev, struct arm_smmu_ste *ste) { 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 b10712d3de66a9..6a74d3d884fe8d 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -602,6 +602,7 @@ struct arm_smmu_ctx_desc_cfg { dma_addr_t cdtab_dma; struct arm_smmu_l1_ctx_desc *l1_desc; unsigned int num_l1_ents; + u8 in_ste; u8 s1fmt; /* log2 of the maximum number of CDs supported by this table */ u8 s1cdmax; @@ -777,8 +778,6 @@ extern struct mutex arm_smmu_asid_lock; void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid); struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master, u32 ssid); -struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master, - u32 ssid); void arm_smmu_make_s1_cd(struct arm_smmu_cd *target, struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain); @@ -786,6 +785,12 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid, struct arm_smmu_cd *cdptr, const struct arm_smmu_cd *target); +int arm_smmu_set_pasid(struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain, ioasid_t pasid, + const struct arm_smmu_cd *cd); +void arm_smmu_remove_pasid(struct arm_smmu_master *master, + struct arm_smmu_domain *smmu_domain, ioasid_t pasid); + void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid); void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid, size_t granule, bool leaf,