> From: Jason Gunthorpe <jgg@xxxxxxxxxx> > Sent: Wednesday, May 22, 2024 2:38 AM > > On Tue, May 21, 2024 at 12:19:45PM -0600, Alex Williamson wrote: > > > I'm OK with this. If devices are insecure then they need quirks in > > > vfio to disclose their problems, we shouldn't punish everyone who > > > followed the spec because of some bad actors. > > > > > > But more broadly in a security engineered environment we can trust the > > > no-snoop bit to work properly. > > > > The spec has an interesting requirement on devices sending no-snoop > > transactions anyway (regarding PCI_EXP_DEVCTL_NOSNOOP_EN): > > > > "Even when this bit is Set, a Function is only permitted to Set the No > > Snoop attribute on a transaction when it can guarantee that the > > address of the transaction is not stored in any cache in the system." > > > > I wouldn't think the function itself has such visibility and it would > > leave the problem of reestablishing coherency to the driver, but am I > > overlooking something that implicitly makes this safe? > > I think it is just bad spec language! People are clearly using > no-snoop on cachable memory today. The authors must have had some > other usage in mind than what the industry actually did. sure no-snoop can be used on cacheable memory but then the driver needs to flush the cache before triggering the no-snoop DMA so it still meets the spec "the address of the transaction is not stored in any cache in the system". but as Alex said the function itself has no such visibility so it's really a guarantee made by the driver. > > > That is where this series is, it assumes a no-snoop transaction took > > > place even if that is impossible, because of config space, and then > > > does pessimistic flushes. > > > > So are you proposing that we can trust devices to honor the > > PCI_EXP_DEVCTL_NOSNOOP_EN bit and virtualize it to be hardwired to > zero > > on IOMMUs that do not enforce coherency as the entire solution? > > Maybe not entire, but as an additional step to reduce the cost of > this. ARM would like this for instance. I searched PCI_EXP_DEVCTL_NOSNOOP_EN but surprisingly it's not touched by i915 driver. sort of suggesting that Intel GPU doesn't follow the spec to honor that bit... > > > Or maybe we trap on setting the bit to make the flushing less > > pessimistic? > > Also a good idea. The VMM could then decide on policy. > On Intel platform there is no pessimistic flush. Only Intel GPUs are exempted from IOMMU force snoop (either being lacking of the capability on the IOMMU dedicated to the GPU or having a special flag bit < REQ_WO_PASID_PGSNP_NOTALLOWED> in the ACPI structure for the IOMMU hosting many devices) to require the additional flushes in this series. We just need to avoid such flushes on other platforms e.g. ARM. I'm fine to do a special check in the attach path to enable the flush only for Intel GPU. or alternatively could ARM SMMU driver implement @enforce_cache_coherency by disabling PCI nosnoop cap when the SMMU itself cannot force snoop? Then VFIO/IOMMUFD could still check enforce_cache_coherency generally to apply the cache flush trick... 😊
