Re: [RFC PATCH 4/4] vhost: register and change IOMMU flag depending on ATS state

[Jason Wang <jasowang@redhat.com>]

在 2023/4/24 19:21, Viktor Prutyanov 写道:
> The guest can disable or never enable ATS. In these cases, Device-TLB
> can't be used even if enabled in QEMU. So, check ATS state before
> registering IOMMU notifier and select flag depending on that. Also,
> change IOMMU notifier flag if ATS state is changed.
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>   hw/virtio/vhost.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index eb8c4c378c..1b14937020 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -796,7 +796,9 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>       iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>                                                      MEMTXATTRS_UNSPECIFIED);
>       iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
> -                        IOMMU_NOTIFIER_DEVIOTLB_UNMAP,
> +                        dev->vdev->ats_enabled ?
> +                            IOMMU_NOTIFIER_DEVIOTLB_UNMAP :
> +                            IOMMU_NOTIFIER_UNMAP,
>                           section->offset_within_region,
>                           int128_get64(end),
>                           iommu_idx);
> @@ -804,7 +806,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>       iommu->iommu_offset = section->offset_within_address_space -
>                             section->offset_within_region;
>       iommu->hdev = dev;
> -    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, NULL);
> +    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n,
> +            dev->vdev->ats_enabled ? NULL : &error_fatal);
>       if (ret) {
>           /*
>            * Some vIOMMUs do not support dev-iotlb yet.  If so, try to use the
> @@ -818,6 +821,19 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>       /* TODO: can replay help performance here? */
>   }
>   
> +static void vhost_deviotlb_ctrl_trigger(bool enable, struct VirtIODevice *vdev)
> +{
> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> +    struct vhost_dev *hdev = vdc->get_vhost(vdev);
> +    struct vhost_iommu *iommu;
> +
> +    QLIST_FOREACH(iommu, &hdev->iommu_list, iommu_next) {
> +        iommu->n.notifier_flags = enable ?
> +            IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
> +        memory_region_iommu_notify_flags_changed(iommu->mr);
> +    }
> +}
> +
>   static void vhost_iommu_region_del(MemoryListener *listener,
>                                      MemoryRegionSection *section)
>   {
> @@ -2000,6 +2016,8 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>               struct vhost_virtqueue *vq = hdev->vqs + i;
>               vhost_device_iotlb_miss(hdev, vq->used_phys, true);
>           }
> +
> +        vdev->ats_ctrl_trigger = vhost_deviotlb_ctrl_trigger;


Changing virtio method in the vhost seems a layer violation.

I wonder if the following is better

1) have a virtio-net method in VirtioDeviceClass as ats trigger

2) call vhost_ops to enable/disable device IOTLB in this ats trigger 
(and do nothing if there's no vhost attached)

Thanks


>       }
>       vhost_start_config_intr(hdev);
>       return 0;
> @@ -2055,6 +2073,7 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>               hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
>           }
>           memory_listener_unregister(&hdev->iommu_listener);
> +        vdev->ats_ctrl_trigger = NULL;
>       }
>       vhost_stop_config_intr(hdev);
>       vhost_log_put(hdev, true);