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

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

[Viktor Prutyanov]

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

Viktor Prutyanov (4):
  pci: add handling of Enable bit in ATS Control Register
  virtio-pci: add handling of ATS and Device-TLB enable
  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               |  8 ++++++++
 hw/pci/pcie.c                     | 22 ++++++++++++++++++++++
 hw/virtio/vhost-backend.c         |  6 ++++++
 hw/virtio/vhost.c                 | 26 ++++++++++++++++++++++++--
 hw/virtio/virtio-pci.c            | 17 +++++++++++++++++
 include/hw/pci/pcie.h             |  5 +++++
 include/hw/virtio/vhost-backend.h |  4 ++++
 include/hw/virtio/vhost.h         |  1 +
 include/hw/virtio/virtio.h        |  2 ++
 include/net/vhost_net.h           |  2 ++
 11 files changed, 102 insertions(+), 2 deletions(-)

-- 
2.35.1

[RFC PATCH v2 1/4] pci: add handling of Enable bit in ATS Control Register

[Viktor Prutyanov]

According to PCIe Address Translation Services specification 5.1.3.,
ATS Control Register has Enable bit to enable/disable ATS.
A trigger function is called at the Enable bit change, so that PCIe
devices can handle ATS enable/disable.

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

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index b8c24cf45f..14ac1b0fb9 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1063,6 +1063,28 @@ void pcie_ats_init(PCIDevice *dev, uint16_t offset, bool aligned)
     pci_set_word(dev->wmask + dev->exp.ats_cap + PCI_ATS_CTRL, 0x800f);
 }
 
+void pcie_ats_config_write(PCIDevice *dev, uint32_t address,
+                           uint32_t val, int len,
+                           void (*trigger_func)(PCIDevice *dev, bool enable))
+{
+    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)) {
+        if (trigger_func) {
+            trigger_func(dev, !!(val & PCI_ATS_CTRL_ENABLE));
+        }
+    }
+}
+
 /* ACS (Access Control Services) */
 void pcie_acs_init(PCIDevice *dev, uint16_t offset)
 {
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 3cc2b15957..f5571527d3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -146,4 +146,9 @@ void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                              Error **errp);
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp);
+
+void pcie_ats_config_write(PCIDevice *dev, uint32_t address,
+                           uint32_t val, int len,
+                           void (*trigger_func)(PCIDevice *dev, bool enable));
+
 #endif /* QEMU_PCIE_H */
-- 
2.35.1

[RFC PATCH v2 2/4] virtio-pci: add handling of ATS and Device-TLB enable

[Viktor Prutyanov]

Guest may enable or disable PCI ATS and, accordingly, Device-TLB for
the device. Add a flag and a trigger function to handle Device-TLB
enable/disable in VirtIO devices and hook it to ATS enable/disable for
PCI transport.

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

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 247325c193..ccd8c4efa1 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -716,6 +716,18 @@ 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, enable);
+}
+
 static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
                                 uint32_t val, int len)
 {
@@ -729,6 +741,11 @@ 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,
+                virtio_pci_ats_ctrl_trigger);
+    }
+
     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 f236e94ca6..83d07bb6b7 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, bool enable);
 };
 
 void virtio_instance_init_common(Object *proxy_obj, void *data,
-- 
2.35.1

Re: [RFC PATCH v2 2/4] virtio-pci: add handling of ATS and Device-TLB enable

[Jason Wang]

On Mon, May 1, 2023 at 10:02 AM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> Guest may enable or disable PCI ATS and, accordingly, Device-TLB for
> the device. Add a flag and a trigger function to handle Device-TLB
> enable/disable in VirtIO devices and hook it to ATS enable/disable for
> PCI transport.
>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>  hw/virtio/virtio-pci.c     | 17 +++++++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 19 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 247325c193..ccd8c4efa1 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -716,6 +716,18 @@ 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, enable);
> +}
> +
>  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>                                  uint32_t val, int len)
>  {
> @@ -729,6 +741,11 @@ 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,
> +                virtio_pci_ats_ctrl_trigger);

I think we can directly call virtio_pci_ats_ctrl_trigger instead of
using an indirection like pcie_ats_config_write?

Thanks

> +    }
> +
>      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 f236e94ca6..83d07bb6b7 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, bool enable);
>  };
>
>  void virtio_instance_init_common(Object *proxy_obj, void *data,
> --
> 2.35.1
>

[RFC PATCH v2 3/4] vhost: register and change IOMMU flag depending on Device-TLB state

[Viktor Prutyanov]

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                 | 26 ++++++++++++++++++++++++--
 include/hw/virtio/vhost-backend.h |  4 ++++
 include/hw/virtio/vhost.h         |  1 +
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 8e581575c9..30eb71fb83 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, int enable)
+{
+    vhost_toggle_device_iotlb(dev, enable);
+}
+
 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 a266396576..1bfcc6d263 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->device_iotlb_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->device_iotlb_enabled ? NULL : &error_fatal);
     if (ret) {
         /*
          * Some vIOMMUs do not support dev-iotlb yet.  If so, try to use the
@@ -841,6 +844,25 @@ static void vhost_iommu_region_del(MemoryListener *listener,
     }
 }
 
+void vhost_toggle_device_iotlb(struct vhost_dev *dev, bool enable)
+{
+    struct vhost_iommu *iommu;
+    int ret;
+
+    QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
+        memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n);
+        iommu->n.notifier_flags = enable ?
+                IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
+        ret = memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
+                enable ? NULL : &error_fatal);
+        if (ret) {
+            iommu->n.notifier_flags = 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..f8e9660a96 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -133,6 +133,9 @@ 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,
+                                             int enabled);
+
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_backend_init vhost_backend_init;
@@ -181,6 +184,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..b3f585c6cd 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, bool enable);
 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

Re: [RFC PATCH v2 3/4] vhost: register and change IOMMU flag depending on Device-TLB state

[Jason Wang]

On Mon, May 1, 2023 at 10:02 AM 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                 | 26 ++++++++++++++++++++++++--
>  include/hw/virtio/vhost-backend.h |  4 ++++
>  include/hw/virtio/vhost.h         |  1 +
>  4 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 8e581575c9..30eb71fb83 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, int enable)
> +{
> +    vhost_toggle_device_iotlb(dev, enable);
> +}
> +
>  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 a266396576..1bfcc6d263 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->device_iotlb_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->device_iotlb_enabled ? NULL : &error_fatal);
>      if (ret) {
>          /*
>           * Some vIOMMUs do not support dev-iotlb yet.  If so, try to use the
> @@ -841,6 +844,25 @@ static void vhost_iommu_region_del(MemoryListener *listener,
>      }
>  }
>
> +void vhost_toggle_device_iotlb(struct vhost_dev *dev, bool enable)
> +{
> +    struct vhost_iommu *iommu;
> +    int ret;
> +
> +    QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
> +        memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n);
> +        iommu->n.notifier_flags = enable ?
> +                IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
> +        ret = memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> +                enable ? NULL : &error_fatal);
> +        if (ret) {
> +            iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP;
> +            memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> +                                                  &error_fatal);

I think it's better to tweak the code to avoid doing IOMMU_NOTIFIER_UNMAP twice.

The rest looks good.

Thanks

> +        }
> +    }
> +}
> +
>  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..f8e9660a96 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -133,6 +133,9 @@ 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,
> +                                             int enabled);
> +
>  typedef struct VhostOps {
>      VhostBackendType backend_type;
>      vhost_backend_init vhost_backend_init;
> @@ -181,6 +184,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..b3f585c6cd 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, bool enable);
>  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
>

Re: [RFC PATCH v2 3/4] vhost: register and change IOMMU flag depending on Device-TLB state

[Jason Wang]

On Mon, May 8, 2023 at 1:25 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, May 1, 2023 at 10:02 AM 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                 | 26 ++++++++++++++++++++++++--
> >  include/hw/virtio/vhost-backend.h |  4 ++++
> >  include/hw/virtio/vhost.h         |  1 +
> >  4 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 8e581575c9..30eb71fb83 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, int enable)
> > +{
> > +    vhost_toggle_device_iotlb(dev, enable);
> > +}
> > +
> >  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 a266396576..1bfcc6d263 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->device_iotlb_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->device_iotlb_enabled ? NULL : &error_fatal);
> >      if (ret) {
> >          /*
> >           * Some vIOMMUs do not support dev-iotlb yet.  If so, try to use the
> > @@ -841,6 +844,25 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> >      }
> >  }
> >
> > +void vhost_toggle_device_iotlb(struct vhost_dev *dev, bool enable)
> > +{
> > +    struct vhost_iommu *iommu;
> > +    int ret;
> > +
> > +    QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
> > +        memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n);
> > +        iommu->n.notifier_flags = enable ?
> > +                IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
> > +        ret = memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> > +                enable ? NULL : &error_fatal);
> > +        if (ret) {
> > +            iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP;
> > +            memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> > +                                                  &error_fatal);
>
> I think it's better to tweak the code to avoid doing IOMMU_NOTIFIER_UNMAP twice.
>
> The rest looks good.

Btw, it might worth to add comment to explain why we need this fallback.

Actually, I'm not sure I understand the logic.

E.g if guest tries to enable ATS it means it knows there's a vIOMMU
that support device IOTLB. If we use UNMAP notifier, we will lose the
device device IOTLB event here?

Thanks

>
> Thanks
>
> > +        }
> > +    }
> > +}
> > +
> >  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..f8e9660a96 100644
> > --- a/include/hw/virtio/vhost-backend.h
> > +++ b/include/hw/virtio/vhost-backend.h
> > @@ -133,6 +133,9 @@ 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,
> > +                                             int enabled);
> > +
> >  typedef struct VhostOps {
> >      VhostBackendType backend_type;
> >      vhost_backend_init vhost_backend_init;
> > @@ -181,6 +184,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..b3f585c6cd 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, bool enable);
> >  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
> >

Re: [RFC PATCH v2 3/4] vhost: register and change IOMMU flag depending on Device-TLB state

[Viktor Prutyanov]

On Mon, May 8, 2023 at 8:28 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, May 8, 2023 at 1:25 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, May 1, 2023 at 10:02 AM 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                 | 26 ++++++++++++++++++++++++--
> > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > >  include/hw/virtio/vhost.h         |  1 +
> > >  4 files changed, 35 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > > index 8e581575c9..30eb71fb83 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, int enable)
> > > +{
> > > +    vhost_toggle_device_iotlb(dev, enable);
> > > +}
> > > +
> > >  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 a266396576..1bfcc6d263 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->device_iotlb_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->device_iotlb_enabled ? NULL : &error_fatal);
> > >      if (ret) {
> > >          /*
> > >           * Some vIOMMUs do not support dev-iotlb yet.  If so, try to use the
> > > @@ -841,6 +844,25 @@ static void vhost_iommu_region_del(MemoryListener *listener,
> > >      }
> > >  }
> > >
> > > +void vhost_toggle_device_iotlb(struct vhost_dev *dev, bool enable)
> > > +{
> > > +    struct vhost_iommu *iommu;
> > > +    int ret;
> > > +
> > > +    QLIST_FOREACH(iommu, &dev->iommu_list, iommu_next) {
> > > +        memory_region_unregister_iommu_notifier(iommu->mr, &iommu->n);
> > > +        iommu->n.notifier_flags = enable ?
> > > +                IOMMU_NOTIFIER_DEVIOTLB_UNMAP : IOMMU_NOTIFIER_UNMAP;
> > > +        ret = memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> > > +                enable ? NULL : &error_fatal);
> > > +        if (ret) {
> > > +            iommu->n.notifier_flags = IOMMU_NOTIFIER_UNMAP;
> > > +            memory_region_register_iommu_notifier(iommu->mr, &iommu->n,
> > > +                                                  &error_fatal);
> >
> > I think it's better to tweak the code to avoid doing IOMMU_NOTIFIER_UNMAP twice.
> >
> > The rest looks good.
>
> Btw, it might worth to add comment to explain why we need this fallback.
>
> Actually, I'm not sure I understand the logic.
>
> E.g if guest tries to enable ATS it means it knows there's a vIOMMU
> that support device IOTLB. If we use UNMAP notifier, we will lose the
> device device IOTLB event here?

Yes. So, the fallback is not really needed anymore. It can't help if
the guest is going to use Device-TLB (by enabling ATS) but it isn't
available in emulated IOMMU.

Thanks,
Viktor Prutyanov

>
> Thanks
>
> >
> > Thanks
> >
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  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..f8e9660a96 100644
> > > --- a/include/hw/virtio/vhost-backend.h
> > > +++ b/include/hw/virtio/vhost-backend.h
> > > @@ -133,6 +133,9 @@ 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,
> > > +                                             int enabled);
> > > +
> > >  typedef struct VhostOps {
> > >      VhostBackendType backend_type;
> > >      vhost_backend_init vhost_backend_init;
> > > @@ -181,6 +184,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..b3f585c6cd 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, bool enable);
> > >  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
> > >
>

[RFC PATCH v2 4/4] virtio-net: pass Device-TLB enable/disable events to vhost

[Viktor Prutyanov]

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     |  8 ++++++++
 include/net/vhost_net.h |  2 ++
 3 files changed, 21 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c4eecc6f36..2364c8de99 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, bool enable)
+{
+    const VhostOps *vhost_ops = dev->vhost_ops;
+
+    if (!vhost_ops->vhost_toggle_device_iotlb) {
+        return;
+    }
+
+    vhost_ops->vhost_toggle_device_iotlb(dev, enable);
+}
+
 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 53e1c32643..e6851b885c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3843,6 +3843,13 @@ static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
     return &net->dev;
 }
 
+static void virtio_net_toggle_device_iotlb(VirtIODevice *vdev,
+                                           bool enable)
+{
+    if (vdev->vhost_started)
+        vhost_net_toggle_device_iotlb(virtio_net_get_vhost(vdev), enable);
+}
+
 static const VMStateDescription vmstate_virtio_net = {
     .name = "virtio-net",
     .minimum_version_id = VIRTIO_NET_VM_VERSION,
@@ -3948,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..36d527f321 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, bool enable);
 #endif
-- 
2.35.1

Re: [RFC PATCH v2 4/4] virtio-net: pass Device-TLB enable/disable events to vhost

[Jason Wang]

On Mon, May 1, 2023 at 10:02 AM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> If vhost is enabled for virtio-net, Device-TLB enable/disable events
> must be passed to vhost for proper IOMMU unmap flag selection.

The patch looks good, just wonder if you have tested it with vhost-user?

(It looks to me like it should work there).

Thanks

>
> Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> ---
>  hw/net/vhost_net.c      | 11 +++++++++++
>  hw/net/virtio-net.c     |  8 ++++++++
>  include/net/vhost_net.h |  2 ++
>  3 files changed, 21 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index c4eecc6f36..2364c8de99 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, bool enable)
> +{
> +    const VhostOps *vhost_ops = dev->vhost_ops;
> +
> +    if (!vhost_ops->vhost_toggle_device_iotlb) {
> +        return;
> +    }
> +
> +    vhost_ops->vhost_toggle_device_iotlb(dev, enable);
> +}
> +
>  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 53e1c32643..e6851b885c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3843,6 +3843,13 @@ static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
>      return &net->dev;
>  }
>
> +static void virtio_net_toggle_device_iotlb(VirtIODevice *vdev,
> +                                           bool enable)
> +{
> +    if (vdev->vhost_started)
> +        vhost_net_toggle_device_iotlb(virtio_net_get_vhost(vdev), enable);
> +}
> +
>  static const VMStateDescription vmstate_virtio_net = {
>      .name = "virtio-net",
>      .minimum_version_id = VIRTIO_NET_VM_VERSION,
> @@ -3948,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..36d527f321 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, bool enable);
>  #endif
> --
> 2.35.1
>

Re: [RFC PATCH v2 4/4] virtio-net: pass Device-TLB enable/disable events to vhost

[Viktor Prutyanov]

On Mon, May 8, 2023 at 8:26 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, May 1, 2023 at 10:02 AM Viktor Prutyanov <viktor@daynix.com> wrote:
> >
> > If vhost is enabled for virtio-net, Device-TLB enable/disable events
> > must be passed to vhost for proper IOMMU unmap flag selection.
>
> The patch looks good, just wonder if you have tested it with vhost-user?

Not yet, because I don't know which use case to test. Actually, the
device_iotlb_enabled flag logic doesn't rely on a backend. And that's
really enough to fix the problem for all the guests I know at the
moment. The trigger logic which turned out to be backend-aware is
needed only if ATS is enabled/disabled in runtime and I create such a
situation manually and test that IOMMU flag re-registration is done.

Thanks,
Viktor Prutyanov

>
> (It looks to me like it should work there).
>
> Thanks
>
> >
> > Signed-off-by: Viktor Prutyanov <viktor@daynix.com>
> > ---
> >  hw/net/vhost_net.c      | 11 +++++++++++
> >  hw/net/virtio-net.c     |  8 ++++++++
> >  include/net/vhost_net.h |  2 ++
> >  3 files changed, 21 insertions(+)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index c4eecc6f36..2364c8de99 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, bool enable)
> > +{
> > +    const VhostOps *vhost_ops = dev->vhost_ops;
> > +
> > +    if (!vhost_ops->vhost_toggle_device_iotlb) {
> > +        return;
> > +    }
> > +
> > +    vhost_ops->vhost_toggle_device_iotlb(dev, enable);
> > +}
> > +
> >  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 53e1c32643..e6851b885c 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3843,6 +3843,13 @@ static struct vhost_dev *virtio_net_get_vhost(VirtIODevice *vdev)
> >      return &net->dev;
> >  }
> >
> > +static void virtio_net_toggle_device_iotlb(VirtIODevice *vdev,
> > +                                           bool enable)
> > +{
> > +    if (vdev->vhost_started)
> > +        vhost_net_toggle_device_iotlb(virtio_net_get_vhost(vdev), enable);
> > +}
> > +
> >  static const VMStateDescription vmstate_virtio_net = {
> >      .name = "virtio-net",
> >      .minimum_version_id = VIRTIO_NET_VM_VERSION,
> > @@ -3948,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..36d527f321 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, bool enable);
> >  #endif
> > --
> > 2.35.1
> >
>

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

[Jason Wang]

On Mon, May 1, 2023 at 10:02 AM Viktor Prutyanov <viktor@daynix.com> wrote:
>
> 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

It would be better if we can have a change log here.

Thanks

>
> Viktor Prutyanov (4):
>   pci: add handling of Enable bit in ATS Control Register
>   virtio-pci: add handling of ATS and Device-TLB enable
>   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               |  8 ++++++++
>  hw/pci/pcie.c                     | 22 ++++++++++++++++++++++
>  hw/virtio/vhost-backend.c         |  6 ++++++
>  hw/virtio/vhost.c                 | 26 ++++++++++++++++++++++++--
>  hw/virtio/virtio-pci.c            | 17 +++++++++++++++++
>  include/hw/pci/pcie.h             |  5 +++++
>  include/hw/virtio/vhost-backend.h |  4 ++++
>  include/hw/virtio/vhost.h         |  1 +
>  include/hw/virtio/virtio.h        |  2 ++
>  include/net/vhost_net.h           |  2 ++
>  11 files changed, 102 insertions(+), 2 deletions(-)
>
> --
> 2.35.1
>