Re: [PATCH V2] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM

[Halil Pasic]

On Thu, 27 Feb 2020 10:47:22 -0500
"Michael S. Tsirkin" <address@hidden> wrote:

> On Thu, Feb 27, 2020 at 02:02:15PM +0100, Halil Pasic wrote:
> > On Wed, 26 Feb 2020 11:52:26 -0500
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Wed, Feb 26, 2020 at 04:36:18PM +0100, Halil Pasic wrote:
> > > > On Wed, 26 Feb 2020 08:37:13 -0500
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > > 
> > > > > On Wed, Feb 26, 2020 at 02:28:39PM +0100, Halil Pasic wrote:
> > > > > > On Wed, 26 Feb 2020 17:43:57 +0800
> > > > > > Jason Wang <address@hidden> wrote:
> > > > > > 
> > > > > > > We turn on device IOTLB via VIRTIO_F_IOMMU_PLATFORM 
> > > > > > > unconditionally on
> > > > > > > platform without IOMMU support. This can lead unnecessary IOTLB
> > > > > > > transactions which will damage the performance.
> > > > > > > 
> > > > > > > Fixing this by check whether the device is backed by IOMMU and 
> > > > > > > disable
> > > > > > > device IOTLB.
> > > > > > > 
> > > > > > > Reported-by: Halil Pasic <address@hidden>
> > > > > > > Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> > > > > > > Cc: address@hidden
> > > > > > > Signed-off-by: Jason Wang <address@hidden>
> > > > > > 
> > > > > > Tested-by: Halil Pasic <address@hidden>
> > > > > > Reviewed-by: Halil Pasic <address@hidden>
> > > > > > 
> > > > > > Thank you very much for fixing this! BTW as I mentioned before it
> > > > > > fixes vhost-vsock with iommu_platform=on as well.
> > > > > 
> > > > > Fixes as in improves performance?
> > > > 
> > > > No, fixes like one does not get something like:
> > > > qemu-system-s390x: vhost_set_features failed: Operation not supported 
> > > > (95)
> > > > qemu-system-s390x: Error starting vhost: 95
> > > > any more.
> > > > 
> > > > Regards,
> > > > Halil
> > > > 
> > > > [..]
> > > 
> > > But can commit c471ad0e9bd46 actually boot a secure guest
> > > where iommu_platform=on is required?
> > > 
> > 
> > No, of course it can not. But I'm not sure about AMD SEV. AFAIU without
> > Jason's patch it does not work for AMD SEV. Tom already stated that with
> > SEV they don't need the IOVA translation aspect of ACCESS_PLATFORM, but
> > I have no idea if the condition vdev->dma_as == &address_space_memory
> > catches them as well or not. They probably have !=.
> > 
> > CCing Tom. @Tom does vhost-vsock work for you with SEV and current qemu?
> > 
> > Also, one can specify iommu_platform=on on a device that ain't a part of
> > a secure-capable VM, just for the fun of it. And that breaks
> > vhost-vsock. Or is setting iommu_platform=on only valid if
> > qemu-system-s390x is protected virtualization capable?
> > 
> > BTW, I don't have a strong opinion on the fixes tag. We currently do not
> > recommend setting iommu_platform, and thus I don't think we care too
> > much about past qemus having problems with it.
> > 
> > Regards,
> > Halil
> 
> 
> Let's just say if we do have a Fixes: tag we want to set it correctly to
> the commit that needs this fix.
> 

You are absolutely correct. And c471ad0e9bd46 has nothing to do with
vsock. On s390x the situation with virtio-net + vhost + iommu_platform=on
seems rather complex. I did some digging, but I have no conclusive
results yet. And I don't think we care all that much for iommu_platform=on
for old qemus.

Regards,
Halil