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

[Jason Wang <jasowang@redhat.com>]

On 2021/2/5 下午11:31, Peter Xu wrote:
> 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.


I think vhost is a good example with device IOTLB? It solves the issue 
exactly the bottleneck of a centralized IOTLB when everything is cached 
in the vhost.


>
> 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?


I think the answer is yes it virtio-iommu is the only choice for some 
platform/archs.


>
> 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.


Looks like another topic (e.g if we need to expose ATS to guest for 
assigned device)?


>
>>>
>>>> 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.
>
> [...]


Right.


>
>>> 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.


Yes, but I think I was wrong at that time.


>
> But I agree it would be nicer to be able to fallback.


So see my reply quoted from ATS spec. My understanding is that the 
device should behave correctly if ATS is disabled.


>
> 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 :)


>    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.

Thanks


>
> Thanks,
>