Hi, On 2/9/21 4:12 AM, Jason Wang wrote: > > On 2021/2/9 上午2:37, Peter Xu wrote: >> On Mon, Feb 08, 2021 at 11:21:23AM +0800, Jason Wang wrote: >> >> [...] >> >>>> 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. >>> >>> Yes, but I think I was wrong at that time. >> I can't say you're wrong - I actually still agree with you that at least >> there's a priority of things we'd do, and this one is not extremely >> important >> if that's not a major use case (say, if you will 100% always suggest >> an user to >> use ats=on for a viommu enabled vhost). > > > Right, but it depends on e.g how libvirt use that. As far as I know, > they do enable ATS. But it would still an issue if libvirt want to > support vIOMMUs other than intel. > > >> >>>> 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. >>> >>> For upstream, it won't be easy :) >> Sorry I definitely didn't make myself clear... :) >> >> To be explicit, does ppc use vhost kernel too? > > > I think the answer is yes. > > >> Since I know at least ppc has >> its own translation unit and its iommu notifier in qemu, so I'm unsure >> whether >> the same patch would break ppc too, because vhost could also ignore >> all UNMAP >> sent by the ppc vIOMMU. > > > If this is true, we probably need to fix that. > > >> >>> >>>> Otherwise IIUC we need to fix those vIOMMUs too. >>> >>> Right, last time I check AMD IOMMU emulation, it simply trigger >>> device IOTLB >>> invalidation during IOTLB invalidation which looks wrong. >> I did quickly grep IOMMU_NOTIFIER_UNMAP in amd_iommu.c and saw >> nothing. It >> seems amd iommu is not ready for any kind of IOMMU notifiers yet. >> >> Thanks, > > > Right. > > Thanks > > >> > > I just noted that the vhost fix now breaks virtio-iommu/vfio integration because VFIO registers IOMMU_NOTIFIER_ALL which includes the DEV-IOTLB that is now rejected by virtio-iommu virtio_iommu_notify_flag_changed(). Is it safe to replace IOMMU_NOTIFIER_ALL by IOMMU_NOTIFIER_IOTLB_EVENTS in vfio_listener_region_add (hw/vfio/common.c) or shall we also do the 2-step registration? After your confirmation, I can send the patch.
Thanks Eric