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

[Tom Lendacky]

On 2/27/20 7:02 AM, 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?

Adding Brijesh for this, too.

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