Re: [PATCH] vhost: correctly turn on VIRTIO_F_IOMMU_PLATFORM

[Jason Wang <jasowang@redhat.com>]

On 2020/2/26 下午4:12, Michael S. Tsirkin wrote:
> On Wed, Feb 26, 2020 at 02:20:36AM -0500, Jason Wang wrote:
>>
>> ----- Original Message -----
>>> On Wed, Feb 26, 2020 at 03:06:47PM +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")
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>   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..6e12c3d2de 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_has_feature(dev->acked_features,
>>>> VIRTIO_F_IOMMU_PLATFORM);
>>>>   }
>>>>   
>>>>   static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>>> Why check acked_features and not host features here?
>>> I'd worry that if we do it like this, userspace driver
>>> within guest can clear the feature and make device access
>>> memory directly.
>> Right, host_features should be more than enough.
>>
>>>> @@ -765,6 +772,9 @@ static int vhost_dev_set_features(struct vhost_dev
>>>> *dev,
>>>>       if (enable_log) {
>>>>           features |= 0x1ULL << VHOST_F_LOG_ALL;
>>>>       }
>>>> +    if (dev->vdev->dma_as == &address_space_memory) {
>>>> +        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
>>>> +    }
>>>
>>> That's a guest visible change. Which seems at best unnecessary.
>>>
>> I don't get how this can be visible from guest? It works as F_LOG_ALL.
>>
>> Thanks
> Oh you are right.
> So just call vhost_dev_has_iommu here too?


That should work.

Thanks


>
>>>>       r = dev->vhost_ops->vhost_set_features(dev, features);
>>>>       if (r < 0) {
>>>>           VHOST_OPS_DEBUG("vhost_set_features failed");
>>>> --
>>>> 2.19.1
>>>
>