Messages in this thread | | | Date | Thu, 10 Aug 2023 15:40:52 +0100 | From | Will Deacon <> | Subject | Re: [PATCH v5 5/9] iommu/arm-smmu-v3: Refactor write_ctx_desc |
| |
On Thu, Aug 10, 2023 at 05:15:50PM +0800, Michael Shavit wrote: > On Wed, Aug 9, 2023 at 9:50 PM Will Deacon <will@kernel.org> wrote: > > > > > - ret = arm_smmu_write_ctx_desc(smmu_domain, mm->pasid, cd); > > > - if (ret) > > > + ret = arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, cd); > > > + if (ret) { > > > + arm_smmu_write_ctx_desc_devices(smmu_domain, mm->pasid, NULL); > > > > Why is it safe to drop the lock between these two calls? > > Hmmm this is a tricky question. > Tracing through the SVA flow, it seems like there's a scenario where > multiple masters (with the same upstream SMMU device) can be attached > to the same primary/non-sva domain, in which case calling > iommu_attach_device_pasid on one device will write the CD entry for > both masters. This is still the case even with this patch series, and > changing this behavior will be the subject of a separate follow-up. > This is weird, especially since the second master need not even have > the sva_enabled bit set. This also means that the list of attached > masters can indeed change between these two calls if that second > master (not the one used on the iommu_attach_device_pasid call leading > to this code) is detached/attached at the same time. It's hard for me > to reason about whether this is safe or not, since this is already > weird behavior...
I really think the writing of the context descriptors should look atomic; dropping the lock half way through a failed update and then coming back to NULL them out definitely isn't correct. So I think you've probably pushed the locking too far down the stack.
Will
| |