qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] vhost: register and change IOMMU flag depending on ATS state
@ 2023-05-12 13:51 Viktor Prutyanov
  2023-05-12 13:51 ` [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable Viktor Prutyanov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-12 13:51 UTC (permalink / raw)
  To: mst, jasowang; +Cc: qemu-devel, viktor, yan, yuri.benditovich

When IOMMU and vhost are enabled together, QEMU tracks IOTLB or
Device-TLB unmap events depending on whether Device-TLB is enabled. But
even if Device-TLB and PCI ATS is enabled, the guest can reject to use
it. For example, this situation appears when Windows Server 2022 is
running with intel-iommu with device-iotlb=on and virtio-net-pci with
vhost=on. The guest implies that no address translation info cached in
device IOTLB and doesn't send device IOTLB invalidation commands. So,
it leads to irrelevant address translations in vhost-net in the host
kernel. Therefore network frames from the guest in host tap interface
contains wrong payload data.

This series adds checking of ATS state for proper unmap flag register
(IOMMU_NOTIFIER_UNMAP or IOMMU_NOTIFIER_DEVIOTLB_UNMAP).

Tested on Windows Server 2022, Windows 11 and Fedora guests with
 -device virtio-net-pci,bus=pci.3,netdev=nd0,iommu_platform=on,ats=on
 -netdev tap,id=nd0,ifname=tap1,script=no,downscript=no,vhost=on
 -device intel-iommu,intremap=on,eim=on,device-iotlb=on/off

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001312

v3: call virtio_pci_ats_ctrl_trigger directly, remove
    IOMMU_NOTIFIER_UNMAP fallbacks
v2: remove memory_region_iommu_notify_flags_changed, move trigger to
    VirtioDeviceClass, use vhost_ops, use device_iotlb name

Viktor Prutyanov (3):
  virtio-pci: add handling of PCI ATS and Device-TLB enable/disable
  vhost: register and change IOMMU flag depending on Device-TLB state
  virtio-net: pass Device-TLB enable/disable events to vhost

 hw/net/vhost_net.c                | 11 ++++++++++
 hw/net/virtio-net.c               |  7 +++++++
 hw/virtio/vhost-backend.c         |  6 ++++++
 hw/virtio/vhost.c                 | 30 +++++++++++++++-----------
 hw/virtio/virtio-pci.c            | 35 +++++++++++++++++++++++++++++++
 include/hw/virtio/vhost-backend.h |  3 +++
 include/hw/virtio/vhost.h         |  1 +
 include/hw/virtio/virtio.h        |  2 ++
 include/net/vhost_net.h           |  2 ++
 9 files changed, 85 insertions(+), 12 deletions(-)


-- 
2.35.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable
  2023-05-12 13:51 [PATCH v3 0/3] vhost: register and change IOMMU flag depending on ATS state Viktor Prutyanov
@ 2023-05-12 13:51 ` Viktor Prutyanov
  2023-05-18  6:10   ` Jason Wang
  2023-05-12 13:51 ` [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state Viktor Prutyanov
  2023-05-12 13:51 ` [PATCH v3 3/3] virtio-net: pass Device-TLB enable/disable events to vhost Viktor Prutyanov
  2 siblings, 1 reply; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-12 13:51 UTC (permalink / raw)
  To: mst, jasowang; +Cc: qemu-devel, viktor, yan, yuri.benditovich

According to PCIe Address Translation Services specification 5.1.3.,
ATS Control Register has Enable bit to enable/disable ATS. Guest may
enable/disable PCI ATS and, accordingly, Device-TLB for the VirtIO PCI
device. So, raise/lower a flag and call a trigger function to pass this
event to a device implementation.

Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
---
 hw/virtio/virtio-pci.c     | 36 ++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 38 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 02fb84a8fa..edbc0daa18 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -716,6 +716,38 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,
     }
 }
 
+static void virtio_pci_ats_ctrl_trigger(PCIDevice *pci_dev, bool enable)
+{
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+    vdev->device_iotlb_enabled = enable;
+
+    if (k->toggle_device_iotlb) {
+        k->toggle_device_iotlb(vdev);
+    }
+}
+
+static void pcie_ats_config_write(PCIDevice *dev, uint32_t address,
+                                  uint32_t val, int len)
+{
+    uint32_t off;
+    uint16_t ats_cap = dev->exp.ats_cap;
+
+    if (!ats_cap || address < ats_cap) {
+        return;
+    }
+    off = address - ats_cap;
+    if (off >= PCI_EXT_CAP_ATS_SIZEOF) {
+        return;
+    }
+
+    if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) {
+        virtio_pci_ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE));
+    }
+}
+
 static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
@@ -729,6 +761,10 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
         pcie_cap_flr_write_config(pci_dev, address, val, len);
     }
 
+    if (proxy->flags & VIRTIO_PCI_FLAG_ATS) {
+        pcie_ats_config_write(pci_dev, address, val, len);
+    }
+
     if (range_covers_byte(address, len, PCI_COMMAND)) {
         if (!(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
             virtio_set_disabled(vdev, true);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f6b38f7e9c..af86ed7249 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -155,6 +155,7 @@ struct VirtIODevice
     QLIST_HEAD(, VirtQueue) *vector_queues;
     QTAILQ_ENTRY(VirtIODevice) next;
     EventNotifier config_notifier;
+    bool device_iotlb_enabled;
 };
 
 struct VirtioDeviceClass {
@@ -212,6 +213,7 @@ struct VirtioDeviceClass {
     const VMStateDescription *vmsd;
     bool (*primary_unplug_pending)(void *opaque);
     struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
+    void (*toggle_device_iotlb)(VirtIODevice *vdev);
 };
 
 void virtio_instance_init_common(Object *proxy_obj, void *data,
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-12 13:51 [PATCH v3 0/3] vhost: register and change IOMMU flag depending on ATS state Viktor Prutyanov
  2023-05-12 13:51 ` [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable Viktor Prutyanov
@ 2023-05-12 13:51 ` Viktor Prutyanov
  2023-05-18  6:14   ` Jason Wang
  2023-05-12 13:51 ` [PATCH v3 3/3] virtio-net: pass Device-TLB enable/disable events to vhost Viktor Prutyanov
  2 siblings, 1 reply; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-12 13:51 UTC (permalink / raw)
  To: mst, jasowang; +Cc: qemu-devel, viktor, yan, yuri.benditovich

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 <viktor@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);
-    }
+    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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/3] virtio-net: pass Device-TLB enable/disable events to vhost
  2023-05-12 13:51 [PATCH v3 0/3] vhost: register and change IOMMU flag depending on ATS state Viktor Prutyanov
  2023-05-12 13:51 ` [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable Viktor Prutyanov
  2023-05-12 13:51 ` [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state Viktor Prutyanov
@ 2023-05-12 13:51 ` Viktor Prutyanov
  2 siblings, 0 replies; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-12 13:51 UTC (permalink / raw)
  To: mst, jasowang; +Cc: qemu-devel, viktor, yan, yuri.benditovich

If vhost is enabled for virtio-net, Device-TLB enable/disable events
must be passed to vhost for proper IOMMU unmap flag selection.

Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
---
 hw/net/vhost_net.c      | 11 +++++++++++
 hw/net/virtio-net.c     |  7 +++++++
 include/net/vhost_net.h |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c4eecc6f36..df5c312993 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -552,6 +552,17 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
     return vhost_ops->vhost_net_set_mtu(&net->dev, mtu);
 }
 
+void vhost_net_toggle_device_iotlb(struct vhost_dev *dev)
+{
+    const VhostOps *vhost_ops = dev->vhost_ops;
+
+    if (!vhost_ops->vhost_toggle_device_iotlb) {
+        return;
+    }
+
+    vhost_ops->vhost_toggle_device_iotlb(dev);
+}
+
 void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
                                int vq_index)
 {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 447f669921..faba810db1 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3844,6 +3844,12 @@ static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
     return &net->dev;
 }
 
+static void virtio_net_toggle_device_iotlb(VirtIODevice *vdev)
+{
+    if (vdev->vhost_started)
+        vhost_net_toggle_device_iotlb(virtio_net_get_vhost(vdev));
+}
+
 static const VMStateDescription vmstate_virtio_net = {
     .name = "virtio-net",
     .minimum_version_id = VIRTIO_NET_VM_VERSION,
@@ -3949,6 +3955,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->vmsd = &vmstate_virtio_net_device;
     vdc->primary_unplug_pending = primary_unplug_pending;
     vdc->get_vhost = virtio_net_get_vhost;
+    vdc->toggle_device_iotlb = virtio_net_toggle_device_iotlb;
 }
 
 static const TypeInfo virtio_net_info = {
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index c37aba35e6..28d93ea8c5 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -56,4 +56,6 @@ int vhost_net_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
                                 int vq_index);
 
 void vhost_net_save_acked_features(NetClientState *nc);
+
+void vhost_net_toggle_device_iotlb(struct vhost_dev *dev);
 #endif
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable
  2023-05-12 13:51 ` [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable Viktor Prutyanov
@ 2023-05-18  6:10   ` Jason Wang
  2023-07-18 11:14     ` Viktor Prutyanov
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2023-05-18  6:10 UTC (permalink / raw)
  To: Viktor Prutyanov; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> According to PCIe Address Translation Services specification 5.1.3.,
> ATS Control Register has Enable bit to enable/disable ATS. Guest may
> enable/disable PCI ATS and, accordingly, Device-TLB for the VirtIO PCI
> device. So, raise/lower a flag and call a trigger function to pass this
> event to a device implementation.
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

> ---
>  hw/virtio/virtio-pci.c     | 36 ++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 38 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 02fb84a8fa..edbc0daa18 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -716,6 +716,38 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,
>      }
>  }
>
> +static void virtio_pci_ats_ctrl_trigger(PCIDevice *pci_dev, bool enable)
> +{
> +    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +    vdev->device_iotlb_enabled = enable;
> +
> +    if (k->toggle_device_iotlb) {
> +        k->toggle_device_iotlb(vdev);
> +    }
> +}
> +
> +static void pcie_ats_config_write(PCIDevice *dev, uint32_t address,
> +                                  uint32_t val, int len)
> +{
> +    uint32_t off;
> +    uint16_t ats_cap = dev->exp.ats_cap;
> +
> +    if (!ats_cap || address < ats_cap) {
> +        return;
> +    }
> +    off = address - ats_cap;
> +    if (off >= PCI_EXT_CAP_ATS_SIZEOF) {
> +        return;
> +    }
> +
> +    if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) {
> +        virtio_pci_ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE));
> +    }
> +}
> +
>  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>                                  uint32_t val, int len)
>  {
> @@ -729,6 +761,10 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>          pcie_cap_flr_write_config(pci_dev, address, val, len);
>      }
>
> +    if (proxy->flags & VIRTIO_PCI_FLAG_ATS) {
> +        pcie_ats_config_write(pci_dev, address, val, len);
> +    }
> +
>      if (range_covers_byte(address, len, PCI_COMMAND)) {
>          if (!(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
>              virtio_set_disabled(vdev, true);
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index f6b38f7e9c..af86ed7249 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -155,6 +155,7 @@ struct VirtIODevice
>      QLIST_HEAD(, VirtQueue) *vector_queues;
>      QTAILQ_ENTRY(VirtIODevice) next;
>      EventNotifier config_notifier;
> +    bool device_iotlb_enabled;
>  };
>
>  struct VirtioDeviceClass {
> @@ -212,6 +213,7 @@ struct VirtioDeviceClass {
>      const VMStateDescription *vmsd;
>      bool (*primary_unplug_pending)(void *opaque);
>      struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
> +    void (*toggle_device_iotlb)(VirtIODevice *vdev);
>  };
>
>  void virtio_instance_init_common(Object *proxy_obj, void *data,
> --
> 2.35.1
>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-12 13:51 ` [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state Viktor Prutyanov
@ 2023-05-18  6:14   ` Jason Wang
  2023-05-19 17:49     ` Viktor Prutyanov
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2023-05-18  6:14 UTC (permalink / raw)
  To: Viktor Prutyanov; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@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 <viktor@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?

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
>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-18  6:14   ` Jason Wang
@ 2023-05-19 17:49     ` Viktor Prutyanov
  2023-05-24  8:25       ` Jason Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-19 17:49 UTC (permalink / raw)
  To: Jason Wang; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Thu, May 18, 2023 at 9:14 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@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 <viktor@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
> >
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-19 17:49     ` Viktor Prutyanov
@ 2023-05-24  8:25       ` Jason Wang
  2023-05-25 12:55         ` Viktor Prutyanov
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Wang @ 2023-05-24  8:25 UTC (permalink / raw)
  To: Viktor Prutyanov; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Sat, May 20, 2023 at 1:50 AM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> On Thu, May 18, 2023 at 9:14 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@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 <viktor@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.

Ok.

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

Yes, so I think it's better to fix others vhost backends. Actually I
wonder if we can have simply reuse vhost_toggle_device_iotlb() since
it's nothing specific to the vhost backend. It's just about the way to
receive IOTLB invalidation from vIOMMU.

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

I'm not sure how this can help? Or anything makes this fix different
from other fixes?

One thing I can think is to backport the fixes to -stable.

Thanks

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-24  8:25       ` Jason Wang
@ 2023-05-25 12:55         ` Viktor Prutyanov
  2023-05-26  1:33           ` Jason Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Viktor Prutyanov @ 2023-05-25 12:55 UTC (permalink / raw)
  To: Jason Wang; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Wed, May 24, 2023 at 11:25 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, May 20, 2023 at 1:50 AM Viktor Prutyanov <viktor@daynix.com> wrote:
> >
> > On Thu, May 18, 2023 at 9:14 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@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 <viktor@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.
>
> Ok.
>
> >
> > 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?
>
> Yes, so I think it's better to fix others vhost backends. Actually I
> wonder if we can have simply reuse vhost_toggle_device_iotlb() since
> it's nothing specific to the vhost backend. It's just about the way to
> receive IOTLB invalidation from vIOMMU.
>
> >
> > 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?
>
> I'm not sure how this can help? Or anything makes this fix different
> from other fixes?

Let's imagine a user who runs Windows on QEMU with virtio-net-pci and
intel-iommu with enabled vhost, ATS and Device-TLB.

At the moment, DMA remapping is disabled through INF in NetKVM driver,
and IOMMU is not working actually, and such a user doesn't observe the
packet corruption issue at all.

After DMA remapping in INF will be enabled, the issue will be observed
with old QEMU. So, if such a user will not update QEMU but update the
driver, he will encounter a problem he has never had before.

>
> One thing I can think is to backport the fixes to -stable.

I think, it would be nice. It doesn't solve the problem 100%, though.

Thanks

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state
  2023-05-25 12:55         ` Viktor Prutyanov
@ 2023-05-26  1:33           ` Jason Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2023-05-26  1:33 UTC (permalink / raw)
  To: Viktor Prutyanov; +Cc: mst, qemu-devel, yan, yuri.benditovich

On Thu, May 25, 2023 at 8:55 PM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> On Wed, May 24, 2023 at 11:25 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Sat, May 20, 2023 at 1:50 AM Viktor Prutyanov <viktor@daynix.com> wrote:
> > >
> > > On Thu, May 18, 2023 at 9:14 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@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 <viktor@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.
> >
> > Ok.
> >
> > >
> > > 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?
> >
> > Yes, so I think it's better to fix others vhost backends. Actually I
> > wonder if we can have simply reuse vhost_toggle_device_iotlb() since
> > it's nothing specific to the vhost backend. It's just about the way to
> > receive IOTLB invalidation from vIOMMU.
> >
> > >
> > > 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?
> >
> > I'm not sure how this can help? Or anything makes this fix different
> > from other fixes?
>
> Let's imagine a user who runs Windows on QEMU with virtio-net-pci and
> intel-iommu with enabled vhost, ATS and Device-TLB.
>
> At the moment, DMA remapping is disabled through INF in NetKVM driver,
> and IOMMU is not working actually, and such a user doesn't observe the
> packet corruption issue at all.
>
> After DMA remapping in INF will be enabled, the issue will be observed
> with old QEMU. So, if such a user will not update QEMU but update the
> driver, he will encounter a problem he has never had before.

Exactly, but this is not specific to this bug. If we don't backport
other fixes to -stable, the old Qemu will suffer from other issues for
sure.

Thanks

>
> >
> > One thing I can think is to backport the fixes to -stable.
>
> I think, it would be nice. It doesn't solve the problem 100%, though.
>
> Thanks
>
> >
> > Thanks
> >
> > >
> > > 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
> > > > >
> > > >
> > >
> >
>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable
  2023-05-18  6:10   ` Jason Wang
@ 2023-07-18 11:14     ` Viktor Prutyanov
  0 siblings, 0 replies; 11+ messages in thread
From: Viktor Prutyanov @ 2023-07-18 11:14 UTC (permalink / raw)
  To: mst, Jason Wang; +Cc: qemu-devel, yan, yuri.benditovich, qemu-stable

Add qemu-stable@nongnu.org

On Thu, May 18, 2023 at 9:10 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, May 12, 2023 at 9:51 PM Viktor Prutyanov <viktor@daynix.com> wrote:
> >
> > According to PCIe Address Translation Services specification 5.1.3.,
> > ATS Control Register has Enable bit to enable/disable ATS. Guest may
> > enable/disable PCI ATS and, accordingly, Device-TLB for the VirtIO PCI
> > device. So, raise/lower a flag and call a trigger function to pass this
> > event to a device implementation.
> >
> > Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>
> > ---
> >  hw/virtio/virtio-pci.c     | 36 ++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio.h |  2 ++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 02fb84a8fa..edbc0daa18 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -716,6 +716,38 @@ virtio_address_space_read(VirtIOPCIProxy *proxy, hwaddr addr,
> >      }
> >  }
> >
> > +static void virtio_pci_ats_ctrl_trigger(PCIDevice *pci_dev, bool enable)
> > +{
> > +    VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> > +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > +
> > +    vdev->device_iotlb_enabled = enable;
> > +
> > +    if (k->toggle_device_iotlb) {
> > +        k->toggle_device_iotlb(vdev);
> > +    }
> > +}
> > +
> > +static void pcie_ats_config_write(PCIDevice *dev, uint32_t address,
> > +                                  uint32_t val, int len)
> > +{
> > +    uint32_t off;
> > +    uint16_t ats_cap = dev->exp.ats_cap;
> > +
> > +    if (!ats_cap || address < ats_cap) {
> > +        return;
> > +    }
> > +    off = address - ats_cap;
> > +    if (off >= PCI_EXT_CAP_ATS_SIZEOF) {
> > +        return;
> > +    }
> > +
> > +    if (range_covers_byte(off, len, PCI_ATS_CTRL + 1)) {
> > +        virtio_pci_ats_ctrl_trigger(dev, !!(val & PCI_ATS_CTRL_ENABLE));
> > +    }
> > +}
> > +
> >  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >                                  uint32_t val, int len)
> >  {
> > @@ -729,6 +761,10 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
> >          pcie_cap_flr_write_config(pci_dev, address, val, len);
> >      }
> >
> > +    if (proxy->flags & VIRTIO_PCI_FLAG_ATS) {
> > +        pcie_ats_config_write(pci_dev, address, val, len);
> > +    }
> > +
> >      if (range_covers_byte(address, len, PCI_COMMAND)) {
> >          if (!(pci_dev->config[PCI_COMMAND] & PCI_COMMAND_MASTER)) {
> >              virtio_set_disabled(vdev, true);
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index f6b38f7e9c..af86ed7249 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -155,6 +155,7 @@ struct VirtIODevice
> >      QLIST_HEAD(, VirtQueue) *vector_queues;
> >      QTAILQ_ENTRY(VirtIODevice) next;
> >      EventNotifier config_notifier;
> > +    bool device_iotlb_enabled;
> >  };
> >
> >  struct VirtioDeviceClass {
> > @@ -212,6 +213,7 @@ struct VirtioDeviceClass {
> >      const VMStateDescription *vmsd;
> >      bool (*primary_unplug_pending)(void *opaque);
> >      struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
> > +    void (*toggle_device_iotlb)(VirtIODevice *vdev);
> >  };
> >
> >  void virtio_instance_init_common(Object *proxy_obj, void *data,
> > --
> > 2.35.1
> >
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-07-18 11:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-12 13:51 [PATCH v3 0/3] vhost: register and change IOMMU flag depending on ATS state Viktor Prutyanov
2023-05-12 13:51 ` [PATCH v3 1/3] virtio-pci: add handling of PCI ATS and Device-TLB enable/disable Viktor Prutyanov
2023-05-18  6:10   ` Jason Wang
2023-07-18 11:14     ` Viktor Prutyanov
2023-05-12 13:51 ` [PATCH v3 2/3] vhost: register and change IOMMU flag depending on Device-TLB state Viktor Prutyanov
2023-05-18  6:14   ` Jason Wang
2023-05-19 17:49     ` Viktor Prutyanov
2023-05-24  8:25       ` Jason Wang
2023-05-25 12:55         ` Viktor Prutyanov
2023-05-26  1:33           ` Jason Wang
2023-05-12 13:51 ` [PATCH v3 3/3] virtio-net: pass Device-TLB enable/disable events to vhost Viktor Prutyanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).