Re: [PATCH] vhost: Unbreak SMMU and virtio-iommu on dev-iotlb support

[Auger Eric]

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