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

[Peter Xu]

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