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

[Jason Wang <jasowang@redhat.com>]

On 2020/2/26 下午5:53, Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 05:43:57PM +0800, Jason Wang 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 <pasic@linux.ibm.com>
>> Fixes: c471ad0e9bd46 ("vhost_net: device IOTLB support")
> Well it's just an optimization, isn't it?


Kind of, or a fix for the performance.


> I don't think it's justified to push this to everyone using
> vhost with IOTLB, is it?


My understanding is that the function should be equivalent to IOTLB in 
this case.

Since no IOMMU is used and device may only see GPA.

Another possible direction is to qemu to update memory table via device 
IOTLB API, this seems less straightforward.


> If you disagree, could you comment a bit on which configurations where tested?


I just do dirty shortcut to emulate the platform without get_dma_as wth 
IOMMU_PLATFORM set.

Thanks


>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Halil could you test this pls? Does this help your performance issue?
>
>> ---
>> Changes from V1:
>> - do not check acked_features
>> - reuse vhost_dev_has_iommu()
>> ---
>>   hw/virtio/vhost.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 9edfadc81d..9182a00495 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -290,7 +290,14 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
>>   {
>>       VirtIODevice *vdev = dev->vdev;
>>   
>> -    return virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>> +    /*
>> +     * For vhost, VIRTIO_F_IOMMU_PLATFORM means the backend support
>> +     * incremental memory mapping API via IOTLB API. For platform that
>> +     * does not have IOMMU, there's no need to enable this feature
>> +     * which may cause unnecessary IOTLB miss/update trnasactions.
>> +     */
>> +    return vdev->dma_as != &address_space_memory &&
>> +           virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
>>   }
>>   
>>   static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>> @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
>>       if (enable_log) {
>>           features |= 0x1ULL << VHOST_F_LOG_ALL;
>>       }
>> +    if (!vhost_dev_has_iommu(dev)) {
>> +        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
>> +    }
>>       r = dev->vhost_ops->vhost_set_features(dev, features);
>>       if (r < 0) {
>>           VHOST_OPS_DEBUG("vhost_set_features failed");
>> -- 
>> 2.19.1