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

[Michael S. Tsirkin]

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.

-- 
MST