On Sun, Jul 27, 2025 at 01:25:01PM -0300, Jason Gunthorpe wrote: > On Tue, Jul 22, 2025 at 02:58:21PM -0700, Nicolin Chen wrote: > > > /* > > > * This is called on the dma mapping fast path so avoid locking. This > > > * is racy, but we have an expectation that the driver will setup its > > > * DMAs inside probe while still single threaded to avoid racing. > > > */ > > > if (dev->iommu && !READ_ONCE(dev->iommu->attach_deferred)) > > > > This triggers a build error as attach_deferred is a bit-field. So I > > am changing it from "u32 attach_deferred:1" to "bool" for this. > > Bleck, that seems undesirable. But inevitable for READ_ONCE :( > > And, to keep the original logic, I think it should be: > > if (!dev->iommu || !READ_ONCE(dev->iommu->attach_deferred)) > > That doesn't seem right, if there is no iommu by the time a driver is > probed there never will be an iommu and this device should be running > in direct mode only. Well, the current function does: if (dev->iommu && dev->iommu->attach_deferred) return __iommu_attach_device(domain, dev); return 0; So, matching to that logic, it would be: if (!dev->iommu || !dev->iommu->attach_deferred) return 0; return __iommu_attach_device(domain, dev); then add guard(mutex). I do see your point. Yet, given that it is an exported function, I think it'd be safer to have a check. Perhaps it should give a WARN_ON(!dev->iommu). > > > And of course it is already quite crazy to be doing FLR during a > > > device probe so this is not a realistic scenario. > > > > Hmm, I am not sure about that, as I see iommu_deferred_attach() get > > mostly invoked by a dma_alloc() or even a dma_map(). So, this might > > not be confined to a device probe? > > Once you do deferred_attach the first time it is done and won't have > any further impact. So long as the dev->iommu->attach_deferred guards > any changes to domains it is unlikely to be racing with FLR. I see. The existing callers are all in dma-iommu.c. So, we can assume that iommu_deferred_attach() is already done, when a PCI driver calls any function from dma-iommu.c. > > > Either ignore this condition with the rational that we are about to > > > reset it so it doesn't matter, or we need to establish a new paging > > > domain for isolation purposes that has the RMR setup. > > > > Ah, you are right. ARM MSI in a VM uses RMR and sets this. > > > > But does it also raise a question that a VM having RMR can't use > > the blocked_domain, as __iommu_device_set_domain() has the exact > > same check rejecting blocked_domain? Not sure if there would be > > some unintended consequnce though... > > Sounds like it needs some sorting out.. For the purposes of FLR I > think the blocked domain is OK, so maybe just move some of those > checks around? These two new APIs call the lower-level __iommu_attach_device() that does not check require_direct. So, we are fine, so long as we don't check it in the new API as you previously pointed out. I'm worried about using blocked domains in general. Thanks Nicolin
