On Fri, Feb 05, 2021 at 09:33:29AM +0100, Auger Eric wrote: > Hi, > > On 2/5/21 4:16 AM, Jason Wang wrote: > > > > On 2021/2/5 上午3:12, Peter Xu wrote: > >> Previous work on dev-iotlb message broke vhost on either SMMU > > > > > > Have a quick git grep and it looks to me v3 support ATS and have command > > for device iotlb (ATC) invalidation. > > > Yes I will do that. Should not be a big deal.
Great, thanks. > > > > > >> or virtio-iommu > >> since dev-iotlb (or PCIe ATS) > > > > > > We may need to add this in the future. > added Jean-Philippe in CC So that's the part I'm unsure about.. Since everybody is cced so maybe good time to ask. :) The thing is I'm still not clear on whether dev-iotlb is useful for a full emulation environment and how that should differ from a normal iotlb, since after all normal iotlb will be attached with device information too. For real hardwares, they make sense because they ask for two things: iotlb is for IOMMU, but dev-iotlb is for the device cache. For emulation environment (virtio-iommu is the case) do we really need that complexity? Note that even if there're assigned devices under virtio-iommu in the future, we can still isolate that and iiuc we can easily convert an iotlb (from virtio-iommu) into a hardware IOMMU dev-iotlb no matter what type of IOMMU is underneath the vIOMMU. > > > > > >> is not yet supported for those archs. > > > > > > Rethink about this, it looks to me the point is that we should make > > vhost work when ATS is disabled like what ATS spec defined: > > > > """ > > > > ATS is enabled through a new Capability and associated configuration > > structure. To enable 15 ATS, software must detect this Capability and > > enable the Function to issue ATS TLP. If a Function is not enabled, the > > Function is required not to issue ATS Translation Requests and is > > required to issue all DMA Read and Write Requests with the TLP AT field > > set to “untranslated.” > > > > """ > > > > Maybe we can add this in the commit log. I saw Michael was super fast on handling this patch and already got it in a pull, so I may not directly post a new version. But I'll add it if I'll post a new version. [...] > > Patch looks good. I wonder whether we should fix intel when ATS is > > disabled. > good point I'm not sure I remember it right, but we seem to have similar discussion previously on "what if the user didn't specify ats=on" - I think at that time the conclusion was that we ignore the failure since that's not a valid configuration for qemu. But I agree it would be nicer to be able to fallback. The other issue I'm worried is (I think I mentioned it somewhere, but just to double confirm): I'd like to make sure SMMU and virtio-iommu are the only IOMMU platform that will use vhost. Otherwise IIUC we need to fix those vIOMMUs too. Thanks, -- Peter Xu
