On Thu, May 18, 2023 at 9:14 AM Jason Wang <jasow...@redhat.com> wrote:
>
> On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <vik...@daynix.com> wrote:
> >
> > The guest can disable or never enable Device-TLB. In these cases,
> > it can't be used even if enabled in QEMU. So, check Device-TLB state
> > before registering IOMMU notifier and select unmap flag depending on
> > that. Also, implement a way to change IOMMU notifier flag if Device-TLB
> > state is changed.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001312
> > Signed-off-by: Viktor Prutyanov <vik...@daynix.com>
> > ---
> > hw/virtio/vhost-backend.c | 6 ++++++
> > hw/virtio/vhost.c | 30 ++++++++++++++++++------------
> > include/hw/virtio/vhost-backend.h | 3 +++
> > include/hw/virtio/vhost.h | 1 +
> > 4 files changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 8e581575c9..d39bfefd2d 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -297,6 +297,11 @@ static void vhost_kernel_set_iotlb_callback(struct
> > vhost_dev *dev,
> > qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
> > }
> >
> > +static void vhost_kernel_toggle_device_iotlb(struct vhost_dev *dev)
> > +{
> > + vhost_toggle_device_iotlb(dev);
> > +}
> > +
> > const VhostOps kernel_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_KERNEL,
> > .vhost_backend_init = vhost_kernel_init,
> > @@ -328,6 +333,7 @@ const VhostOps kernel_ops = {
> > .vhost_vsock_set_running = vhost_kernel_vsock_set_running,
> > .vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
> > .vhost_send_device_iotlb_msg = vhost_kernel_send_device_iotlb_msg,
> > + .vhost_toggle_device_iotlb = vhost_kernel_toggle_device_iotlb,
> > };
> > #endif
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 746d130c74..41c9fbf286 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -781,7 +781,6 @@ static void vhost_iommu_region_add(MemoryListener
> > *listener,
> > Int128 end;
> > int iommu_idx;
> > IOMMUMemoryRegion *iommu_mr;
> > - int ret;
> >
> > if (!memory_region_is_iommu(section->mr)) {
> > return;
> > @@ -796,7 +795,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->device_iotlb_enabled ?
> > + IOMMU_NOTIFIER_DEVIOTLB_UNMAP :
> > + IOMMU_NOTIFIER_UNMAP,
> > section->offset_within_region,
> > int128_get64(end),
> > iommu_idx);
> > @@ -804,16 +805,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);
> > - if (ret) {
> > - /*
> > - * Some vIOMMUs do not support dev-iotlb yet. If so, try to use
> > the
> > - * UNMAP legacy message
> > - */
> > - iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP;
> > - memory_region_register_iommu_notifier(section->mr, &iommu->n,
> > - &error_fatal);
> > - }
>
> So we lose this fallback. Is this really intended?
>
> E.g does it work if you are using virtio-iommu?
It works for virtio-iommu because Linux guest doesn't enable PCI ATS in
this situation. From my point of view, this fallback is not needed
anymore, because it triggers only if Device-TLB isn't available on the
host side but the guest misbehaves and tries to enable it.
Also, I would like to discuss two more points:
1. The patch series in its current form will fix the issue for
vhost+iommu setup for any VirtIO device with any vhost backend when
ATS is enabled at the beginning. But if ATS is enabled/disabled in the
runtime it will only work for virtio-net with vhost-kernel. All other
devices and backends are out of scope and will need to add almost the
same device_iotlb_toggle and vhost_device_iotlb_toggle handlers. Since
the issue is general for any device and any backend, is it normal from
architectural point of view?
2. When the series will be applied, we should enable DMA remapping for
new Windows guest drivers, such as NetKVM. But if the user with enabled
vhost+iommu updated the driver with old QEMU, the bug would reappear,
because the guest doesn't know that the fix isn't present. May be we
should discuss some mechanism to report that host is aware of guest's
accept/reject of ATS/Device-TLB?
Thanks,
Viktor Prutyanov
>
> Thanks
>
> > + memory_region_register_iommu_notifier(section->mr, &iommu->n,
> > + &error_fatal);
> > QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next);
> > /* TODO: can replay help performance here? */
> > }
> > @@ -841,6 +834,19 @@ static void vhost_iommu_region_del(MemoryListener
> > *listener,
> > }
> > }
> >
> > +void vhost_toggle_device_iotlb(struct vhost_dev *dev)
> > +{
> > + struct vhost_iommu *iommu;
> > +
> > + QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
> > + memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n);
> > + iommu->n.notifier_flags = dev->vdev->device_iotlb_enabled ?
> > + IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
> > + memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> > + &error_fatal);
> > + }
> > +}
> > +
> > static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> > struct vhost_virtqueue *vq,
> > unsigned idx, bool enable_log)
> > diff --git a/include/hw/virtio/vhost-backend.h
> > b/include/hw/virtio/vhost-backend.h
> > index ec3fbae58d..10a3c36b4b 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -133,6 +133,8 @@ typedef int (*vhost_set_config_call_op)(struct
> > vhost_dev *dev,
> >
> > typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
> >
> > +typedef void (*vhost_toggle_device_iotlb_op)(struct vhost_dev *dev);
> > +
> > typedef struct VhostOps {
> > VhostBackendType backend_type;
> > vhost_backend_init vhost_backend_init;
> > @@ -181,6 +183,7 @@ typedef struct VhostOps {
> > vhost_force_iommu_op vhost_force_iommu;
> > vhost_set_config_call_op vhost_set_config_call;
> > vhost_reset_status_op vhost_reset_status;
> > + vhost_toggle_device_iotlb_op vhost_toggle_device_iotlb;
> > } VhostOps;
> >
> > int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index a52f273347..785832ed46 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -320,6 +320,7 @@ bool vhost_has_free_slot(void);
> > int vhost_net_set_backend(struct vhost_dev *hdev,
> > struct vhost_vring_file *file);
> >
> > +void vhost_toggle_device_iotlb(struct vhost_dev *dev);
> > int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int
> > write);
> >
> > int vhost_virtqueue_start(struct vhost_dev *dev, struct VirtIODevice *vdev,
> > --
> > 2.35.1
> >
>
