[PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Jonah Palmer]

The goal of these patches are to add support to a variety of virtio and
vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
feature indicates that a driver will pass extra data (instead of just a
virtqueue's index) when notifying the corresponding device.

The data passed in by the driver when this feature is enabled varies in
format depending on if the device is using a split or packed virtqueue
layout:

 Split VQ
  - Upper 16 bits: shadow_avail_idx
  - Lower 16 bits: virtqueue index

 Packed VQ
  - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
  - Lower 16 bits: virtqueue index

Also, due to the limitations of ioeventfd not being able to carry the
extra provided by the driver, ioeventfd is left disabled for any devices
using this feature.

A significant aspect of this effort has been to maintain compatibility
across different backends. As such, the feature is offered by backend
devices only when supported, with fallback mechanisms where backend
support is absent.

Jonah Palmer (8):
  virtio/virtio-pci: Handle extra notification data
  virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
  virtio-mmio: Handle extra notification data
  virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
  virtio-ccw: Handle extra notification data
  virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
  vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
  virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition

 hw/block/vhost-user-blk.c    |  1 +
 hw/net/vhost_net.c           |  2 ++
 hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
 hw/s390x/virtio-ccw.c        |  6 ++++--
 hw/scsi/vhost-scsi.c         |  1 +
 hw/scsi/vhost-user-scsi.c    |  1 +
 hw/virtio/vhost-user-fs.c    |  2 +-
 hw/virtio/vhost-user-vsock.c |  1 +
 hw/virtio/virtio-mmio.c      | 15 +++++++++++----
 hw/virtio/virtio-pci.c       | 16 +++++++++++-----
 hw/virtio/virtio.c           | 18 ++++++++++++++++++
 include/hw/virtio/virtio.h   |  5 ++++-
 net/vhost-vdpa.c             |  1 +
 13 files changed, 68 insertions(+), 17 deletions(-)

-- 
2.39.3

[PATCH v1 1/8] virtio/virtio-pci: Handle extra notification data

[Jonah Palmer]

Add support to virtio-pci devices for handling the extra data sent
from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
transport feature has been negotiated.

The extra data that's passed to the virtio-pci device when this
feature is enabled varies depending on the device's virtqueue
layout.

In a split virtqueue layout, this data includes:
 - upper 16 bits: shadow_avail_idx
 - lower 16 bits: virtqueue index

In a packed virtqueue layout, this data includes:
 - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
 - lower 16 bits: virtqueue index

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio-pci.c     | 10 +++++++---
 hw/virtio/virtio.c         | 18 ++++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1a7039fb0c..d12edc567f 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
     VirtIOPCIProxy *proxy = opaque;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
-    uint16_t vector;
+    uint16_t vector, vq_idx;
     hwaddr pa;
 
     switch (addr) {
@@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
             vdev->queue_sel = val;
         break;
     case VIRTIO_PCI_QUEUE_NOTIFY:
-        if (val < VIRTIO_QUEUE_MAX) {
-            virtio_queue_notify(vdev, val);
+        vq_idx = val;
+        if (vq_idx < VIRTIO_QUEUE_MAX) {
+            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+                virtio_queue_set_shadow_avail_data(vdev, val);
+            }
+            virtio_queue_notify(vdev, vq_idx);
         }
         break;
     case VIRTIO_PCI_STATUS:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index d229755eae..bcb9e09df0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
     }
 }
 
+void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
+{
+    /* Lower 16 bits is the virtqueue index */
+    uint16_t i = data;
+    VirtQueue *vq = &vdev->vq[i];
+
+    if (!vq->vring.desc) {
+        return;
+    }
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
+        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
+    } else {
+        vq->shadow_avail_idx = (data >> 16);
+    }
+}
+
 static void virtio_queue_notify_vq(VirtQueue *vq)
 {
     if (vq->vring.desc && vq->handle_output) {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c8f72850bc..53915947a7 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
 void virtio_init_region_cache(VirtIODevice *vdev, int n);
 void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
+void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
 int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
-- 
2.39.3

[PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

Prevent ioeventfd from being enabled/disabled when a virtio-pci
device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
feature.

Due to ioeventfd not being able to carry the extra data associated with
this feature, the ioeventfd should be left in a disabled state for
emulated virtio-pci devices using this feature.

Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio-pci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index d12edc567f..287b8f7720 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
         }
         break;
     case VIRTIO_PCI_STATUS:
-        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
+            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
             virtio_pci_stop_ioeventfd(proxy);
         }
 
         virtio_set_status(vdev, val & 0xFF);
 
-        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
+        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
+            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
             virtio_pci_start_ioeventfd(proxy);
         }
 
-- 
2.39.3

[PATCH v1 3/8] virtio-mmio: Handle extra notification data

[Jonah Palmer]

Add support to virtio-mmio devices for handling the extra data sent from
the driver to the device when the VIRTIO_F_NOTIFICATION_DATA transport
feature has been negotiated.

The extra data that's passed to the virtio-mmio device when this feature
is enabled varies depending on the device's virtqueue layout.

The data passed to the virtio-mmio device is in the same format as the
data passed to virtio-pci devices.

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio-mmio.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 22f9fbcf5a..f99d5851a2 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -248,6 +248,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
 {
     VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
     VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+    uint16_t vq_idx;
 
     trace_virtio_mmio_write_offset(offset, value);
 
@@ -407,8 +408,12 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         }
         break;
     case VIRTIO_MMIO_QUEUE_NOTIFY:
-        if (value < VIRTIO_QUEUE_MAX) {
-            virtio_queue_notify(vdev, value);
+        vq_idx = value;
+        if (vq_idx < VIRTIO_QUEUE_MAX) {
+            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+                virtio_queue_set_shadow_avail_data(vdev, value);
+            }
+            virtio_queue_notify(vdev, vq_idx);
         }
         break;
     case VIRTIO_MMIO_INTERRUPT_ACK:
-- 
2.39.3

[PATCH v1 4/8] virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

Prevent ioeventfd from being enabled/disabled when a virtio-mmio device
has negotiated the VIRTIO_F_NOTIFICATION_DATA transport feature.

Due to ioeventfd not being able to carry the extra data associated with
this feature, the ioeventfd should be left in a disabled state for
emulated virtio-mmio devices using this feature.

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio-mmio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index f99d5851a2..f42ed5c512 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -421,7 +421,8 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
         virtio_update_irq(vdev);
         break;
     case VIRTIO_MMIO_STATUS:
-        if (!(value & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        if (!(value & VIRTIO_CONFIG_S_DRIVER_OK) &&
+            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
             virtio_mmio_stop_ioeventfd(proxy);
         }
 
@@ -433,7 +434,8 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
 
         virtio_set_status(vdev, value & 0xff);
 
-        if (value & VIRTIO_CONFIG_S_DRIVER_OK) {
+        if ((value & VIRTIO_CONFIG_S_DRIVER_OK) &&
+            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
             virtio_mmio_start_ioeventfd(proxy);
         }
 
-- 
2.39.3

[PATCH v1 5/8] virtio-ccw: Handle extra notification data

[Jonah Palmer]

Add support to virtio-ccw devices for handling the extra data sent from
the driver to the device when the VIRTIO_F_NOTIFICATION_DATA transport
feature has been negotiated.

The extra data that's passed to the virtio-ccw device when this feature
is enabled varies depending on the device's virtqueue layout.

That data passed to the virtio-ccw device is in the same format as the
data passed to virtio-pci devices.

Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62804cc228..828052046b 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -140,9 +140,11 @@ static void subsystem_reset(void)
 static int virtio_ccw_hcall_notify(const uint64_t *args)
 {
     uint64_t subch_id = args[0];
-    uint64_t queue = args[1];
+    uint64_t data = args[1];
     SubchDev *sch;
+    VirtIODevice *vdev;
     int cssid, ssid, schid, m;
+    uint16_t vq_idx = data;
 
     if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
         return -EINVAL;
@@ -151,12 +153,18 @@ static int virtio_ccw_hcall_notify(const uint64_t *args)
     if (!sch || !css_subch_visible(sch)) {
         return -EINVAL;
     }
-    if (queue >= VIRTIO_QUEUE_MAX) {
+
+    if (vq_idx >= VIRTIO_QUEUE_MAX) {
         return -EINVAL;
     }
-    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
-    return 0;
 
+    vdev = virtio_ccw_get_vdev(sch);
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+        virtio_queue_set_shadow_avail_data(vdev, data);
+    }
+
+    virtio_queue_notify(vdev, vq_idx);
+    return 0;
 }
 
 static int virtio_ccw_hcall_early_printk(const uint64_t *args)
-- 
2.39.3

[PATCH v1 6/8] virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

Prevent ioeventfd from being enabled/disabled when a virtio-ccw device
has negotiated the VIRTIO_F_NOTIFICATION_DATA transport feature.

Due to the ioeventfd not being able to carry the extra data associated
with this feature, the ioeventfd should be left in a disabled state for
emulated virtio-ccw devices using this feature.

Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/s390x/virtio-ccw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index b4676909dd..936ba78fda 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -530,14 +530,16 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
             if (ret) {
                 break;
             }
-            if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+            if (!(status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+                !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
                 virtio_ccw_stop_ioeventfd(dev);
             }
             if (virtio_set_status(vdev, status) == 0) {
                 if (vdev->status == 0) {
                     virtio_ccw_reset_virtio(dev);
                 }
-                if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
+                if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
+                    !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
                     virtio_ccw_start_ioeventfd(dev);
                 }
                 sch->curr_status.scsw.count = ccw.count - sizeof(status);
-- 
2.39.3

[PATCH v1 7/8] vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits

[Jonah Palmer]

Add support for the VIRTIO_F_NOTIFICATION_DATA feature across a variety
of vhost devices.

The inclusion of VIRTIO_F_NOTIFICATION_DATA in the feature bits arrays
for these devices ensures that the backend is capable of offering and
providing support for this feature, and that it can be disabled if the
backend does not support it.

Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/block/vhost-user-blk.c    | 1 +
 hw/net/vhost_net.c           | 2 ++
 hw/scsi/vhost-scsi.c         | 1 +
 hw/scsi/vhost-user-scsi.c    | 1 +
 hw/virtio/vhost-user-fs.c    | 2 +-
 hw/virtio/vhost-user-vsock.c | 1 +
 net/vhost-vdpa.c             | 1 +
 7 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 6a856ad51a..983c0657da 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -51,6 +51,7 @@ static const int user_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_NOTIFICATION_DATA,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index e8e1661646..bb1f975b39 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -48,6 +48,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_NOTIFICATION_DATA,
     VIRTIO_NET_F_HASH_REPORT,
     VHOST_INVALID_FEATURE_BIT
 };
@@ -55,6 +56,7 @@ static const int kernel_feature_bits[] = {
 /* Features supported by others. */
 static const int user_feature_bits[] = {
     VIRTIO_F_NOTIFY_ON_EMPTY,
+    VIRTIO_F_NOTIFICATION_DATA,
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
 
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 58a00336c2..b8048f18e9 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -38,6 +38,7 @@ static const int kernel_feature_bits[] = {
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_SCSI_F_HOTPLUG,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_NOTIFICATION_DATA,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a63b1f4948..0b050805a8 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -36,6 +36,7 @@ static const int user_feature_bits[] = {
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_SCSI_F_HOTPLUG,
     VIRTIO_F_RING_RESET,
+    VIRTIO_F_NOTIFICATION_DATA,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index cca2cd41be..ae48cc1c96 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -33,7 +33,7 @@ static const int user_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_RESET,
-
+    VIRTIO_F_NOTIFICATION_DATA,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index 9431b9792c..802b44a07d 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -21,6 +21,7 @@ static const int user_feature_bits[] = {
     VIRTIO_RING_F_INDIRECT_DESC,
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_F_NOTIFY_ON_EMPTY,
+    VIRTIO_F_NOTIFICATION_DATA,
     VHOST_INVALID_FEATURE_BIT
 };
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index e6bdb4562d..08b822e6ed 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -62,6 +62,7 @@ const int vdpa_feature_bits[] = {
     VIRTIO_F_RING_PACKED,
     VIRTIO_F_RING_RESET,
     VIRTIO_F_VERSION_1,
+    VIRTIO_F_NOTIFICATION_DATA,
     VIRTIO_NET_F_CSUM,
     VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
     VIRTIO_NET_F_CTRL_MAC_ADDR,
-- 
2.39.3

[PATCH v1 8/8] virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition

[Jonah Palmer]

Extend the virtio device property definitions to include the
VIRTIO_F_NOTIFICATION_DATA feature.

The default state of this feature is disabled, allowing it to be
explicitly enabled where it's supported.

Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 include/hw/virtio/virtio.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 53915947a7..41ef3c4aef 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -369,7 +369,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("packed", _state, _field, \
                       VIRTIO_F_RING_PACKED, false), \
     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
-                      VIRTIO_F_RING_RESET, true)
+                      VIRTIO_F_RING_RESET, true), \
+    DEFINE_PROP_BIT64("notification_data", _state, _field, \
+                      VIRTIO_F_NOTIFICATION_DATA, false)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
-- 
2.39.3

Re: [PATCH v1 1/8] virtio/virtio-pci: Handle extra notification data

[Eugenio Perez Martin]

On Mon, Mar 4, 2024 at 8:46 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add support to virtio-pci devices for handling the extra data sent
> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> transport feature has been negotiated.
>
> The extra data that's passed to the virtio-pci device when this
> feature is enabled varies depending on the device's virtqueue
> layout.
>
> In a split virtqueue layout, this data includes:
>  - upper 16 bits: shadow_avail_idx
>  - lower 16 bits: virtqueue index
>
> In a packed virtqueue layout, this data includes:
>  - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>  - lower 16 bits: virtqueue index
>

Reviewed-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-pci.c     | 10 +++++++---
>  hw/virtio/virtio.c         | 18 ++++++++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..d12edc567f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -    uint16_t vector;
> +    uint16_t vector, vq_idx;
>      hwaddr pa;
>
>      switch (addr) {
> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>              vdev->queue_sel = val;
>          break;
>      case VIRTIO_PCI_QUEUE_NOTIFY:
> -        if (val < VIRTIO_QUEUE_MAX) {
> -            virtio_queue_notify(vdev, val);
> +        vq_idx = val;
> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +                virtio_queue_set_shadow_avail_data(vdev, val);
> +            }
> +            virtio_queue_notify(vdev, vq_idx);
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d229755eae..bcb9e09df0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>      }
>  }
>
> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
> +{
> +    /* Lower 16 bits is the virtqueue index */
> +    uint16_t i = data;
> +    VirtQueue *vq = &vdev->vq[i];
> +
> +    if (!vq->vring.desc) {
> +        return;
> +    }
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
> +    } else {
> +        vq->shadow_avail_idx = (data >> 16);
> +    }
> +}
> +
>  static void virtio_queue_notify_vq(VirtQueue *vq)
>  {
>      if (vq->vring.desc && vq->handle_output) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c8f72850bc..53915947a7 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>  void virtio_init_region_cache(VirtIODevice *vdev, int n);
>  void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>  void virtio_queue_notify(VirtIODevice *vdev, int n);
> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>  void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>  int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> --
> 2.39.3
>

Re: [PATCH v1 3/8] virtio-mmio: Handle extra notification data

[Eugenio Perez Martin]

On Mon, Mar 4, 2024 at 8:46 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add support to virtio-mmio devices for handling the extra data sent from
> the driver to the device when the VIRTIO_F_NOTIFICATION_DATA transport
> feature has been negotiated.
>
> The extra data that's passed to the virtio-mmio device when this feature
> is enabled varies depending on the device's virtqueue layout.
>
> The data passed to the virtio-mmio device is in the same format as the
> data passed to virtio-pci devices.
>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-mmio.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 22f9fbcf5a..f99d5851a2 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -248,6 +248,7 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>  {
>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +    uint16_t vq_idx;
>
>      trace_virtio_mmio_write_offset(offset, value);
>
> @@ -407,8 +408,12 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>          }
>          break;
>      case VIRTIO_MMIO_QUEUE_NOTIFY:
> -        if (value < VIRTIO_QUEUE_MAX) {
> -            virtio_queue_notify(vdev, value);
> +        vq_idx = value;
> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +                virtio_queue_set_shadow_avail_data(vdev, value);
> +            }
> +            virtio_queue_notify(vdev, vq_idx);
>          }
>          break;
>      case VIRTIO_MMIO_INTERRUPT_ACK:
> --
> 2.39.3
>

Re: [PATCH v1 4/8] virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Eugenio Perez Martin]

On Mon, Mar 4, 2024 at 8:46 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Prevent ioeventfd from being enabled/disabled when a virtio-mmio device
> has negotiated the VIRTIO_F_NOTIFICATION_DATA transport feature.
>
> Due to ioeventfd not being able to carry the extra data associated with
> this feature, the ioeventfd should be left in a disabled state for
> emulated virtio-mmio devices using this feature.
>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-mmio.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index f99d5851a2..f42ed5c512 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -421,7 +421,8 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>          virtio_update_irq(vdev);
>          break;
>      case VIRTIO_MMIO_STATUS:
> -        if (!(value & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +        if (!(value & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>              virtio_mmio_stop_ioeventfd(proxy);
>          }
>
> @@ -433,7 +434,8 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>
>          virtio_set_status(vdev, value & 0xff);
>
> -        if (value & VIRTIO_CONFIG_S_DRIVER_OK) {
> +        if ((value & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>              virtio_mmio_start_ioeventfd(proxy);
>          }
>
> --
> 2.39.3
>

Re: [PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Jason Wang]

On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> The goal of these patches are to add support to a variety of virtio and
> vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> feature indicates that a driver will pass extra data (instead of just a
> virtqueue's index) when notifying the corresponding device.
>
> The data passed in by the driver when this feature is enabled varies in
> format depending on if the device is using a split or packed virtqueue
> layout:
>
>  Split VQ
>   - Upper 16 bits: shadow_avail_idx
>   - Lower 16 bits: virtqueue index
>
>  Packed VQ
>   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>   - Lower 16 bits: virtqueue index
>
> Also, due to the limitations of ioeventfd not being able to carry the
> extra provided by the driver, ioeventfd is left disabled for any devices
> using this feature.

Is there any method to overcome this? This might help for vhost.

Thanks

>
> A significant aspect of this effort has been to maintain compatibility
> across different backends. As such, the feature is offered by backend
> devices only when supported, with fallback mechanisms where backend
> support is absent.
>
> Jonah Palmer (8):
>   virtio/virtio-pci: Handle extra notification data
>   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>   virtio-mmio: Handle extra notification data
>   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>   virtio-ccw: Handle extra notification data
>   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
>   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
>
>  hw/block/vhost-user-blk.c    |  1 +
>  hw/net/vhost_net.c           |  2 ++
>  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
>  hw/s390x/virtio-ccw.c        |  6 ++++--
>  hw/scsi/vhost-scsi.c         |  1 +
>  hw/scsi/vhost-user-scsi.c    |  1 +
>  hw/virtio/vhost-user-fs.c    |  2 +-
>  hw/virtio/vhost-user-vsock.c |  1 +
>  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
>  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
>  hw/virtio/virtio.c           | 18 ++++++++++++++++++
>  include/hw/virtio/virtio.h   |  5 ++++-
>  net/vhost-vdpa.c             |  1 +
>  13 files changed, 68 insertions(+), 17 deletions(-)
>
> --
> 2.39.3
>

Re: [PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Eugenio Perez Martin]

On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >
> > The goal of these patches are to add support to a variety of virtio and
> > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> > feature indicates that a driver will pass extra data (instead of just a
> > virtqueue's index) when notifying the corresponding device.
> >
> > The data passed in by the driver when this feature is enabled varies in
> > format depending on if the device is using a split or packed virtqueue
> > layout:
> >
> >  Split VQ
> >   - Upper 16 bits: shadow_avail_idx
> >   - Lower 16 bits: virtqueue index
> >
> >  Packed VQ
> >   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> >   - Lower 16 bits: virtqueue index
> >
> > Also, due to the limitations of ioeventfd not being able to carry the
> > extra provided by the driver, ioeventfd is left disabled for any devices
> > using this feature.
>
> Is there any method to overcome this? This might help for vhost.
>

As a half-baked idea, read(2)ing an eventfd descriptor returns an
8-byte integer already. The returned value of read depends on eventfd
flags, but both have to do with the number of writes of the other end.

My proposal is to replace this value with the last value written by
the guest, so we can extract the virtio notification data from there.
The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
and then blocking if read again without writes. The behavior of KVM
writes is different, as it is not a counter anymore.

Thanks!

> Thanks
>
> >
> > A significant aspect of this effort has been to maintain compatibility
> > across different backends. As such, the feature is offered by backend
> > devices only when supported, with fallback mechanisms where backend
> > support is absent.
> >
> > Jonah Palmer (8):
> >   virtio/virtio-pci: Handle extra notification data
> >   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> >   virtio-mmio: Handle extra notification data
> >   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> >   virtio-ccw: Handle extra notification data
> >   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> >   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
> >   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
> >
> >  hw/block/vhost-user-blk.c    |  1 +
> >  hw/net/vhost_net.c           |  2 ++
> >  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
> >  hw/s390x/virtio-ccw.c        |  6 ++++--
> >  hw/scsi/vhost-scsi.c         |  1 +
> >  hw/scsi/vhost-user-scsi.c    |  1 +
> >  hw/virtio/vhost-user-fs.c    |  2 +-
> >  hw/virtio/vhost-user-vsock.c |  1 +
> >  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
> >  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
> >  hw/virtio/virtio.c           | 18 ++++++++++++++++++
> >  include/hw/virtio/virtio.h   |  5 ++++-
> >  net/vhost-vdpa.c             |  1 +
> >  13 files changed, 68 insertions(+), 17 deletions(-)
> >
> > --
> > 2.39.3
> >
>

Re: [PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Michael S. Tsirkin]

On Wed, Mar 06, 2024 at 08:07:31AM +0100, Eugenio Perez Martin wrote:
> On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> > >
> > > The goal of these patches are to add support to a variety of virtio and
> > > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> > > feature indicates that a driver will pass extra data (instead of just a
> > > virtqueue's index) when notifying the corresponding device.
> > >
> > > The data passed in by the driver when this feature is enabled varies in
> > > format depending on if the device is using a split or packed virtqueue
> > > layout:
> > >
> > >  Split VQ
> > >   - Upper 16 bits: shadow_avail_idx
> > >   - Lower 16 bits: virtqueue index
> > >
> > >  Packed VQ
> > >   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> > >   - Lower 16 bits: virtqueue index
> > >
> > > Also, due to the limitations of ioeventfd not being able to carry the
> > > extra provided by the driver, ioeventfd is left disabled for any devices
> > > using this feature.
> >
> > Is there any method to overcome this? This might help for vhost.
> >
> 
> As a half-baked idea, read(2)ing an eventfd descriptor returns an
> 8-byte integer already. The returned value of read depends on eventfd
> flags, but both have to do with the number of writes of the other end.
> 
> My proposal is to replace this value with the last value written by
> the guest, so we can extract the virtio notification data from there.
> The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
> and then blocking if read again without writes. The behavior of KVM
> writes is different, as it is not a counter anymore.
> 
> Thanks!


I doubt you will be able to support this in ioeventfd...
But vhost does not really need the value at all.
So why mask out ioeventfd with vhost?
vhost-vdpa is probably the only one that might need it...



> > Thanks
> >
> > >
> > > A significant aspect of this effort has been to maintain compatibility
> > > across different backends. As such, the feature is offered by backend
> > > devices only when supported, with fallback mechanisms where backend
> > > support is absent.
> > >
> > > Jonah Palmer (8):
> > >   virtio/virtio-pci: Handle extra notification data
> > >   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   virtio-mmio: Handle extra notification data
> > >   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   virtio-ccw: Handle extra notification data
> > >   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
> > >   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
> > >
> > >  hw/block/vhost-user-blk.c    |  1 +
> > >  hw/net/vhost_net.c           |  2 ++
> > >  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
> > >  hw/s390x/virtio-ccw.c        |  6 ++++--
> > >  hw/scsi/vhost-scsi.c         |  1 +
> > >  hw/scsi/vhost-user-scsi.c    |  1 +
> > >  hw/virtio/vhost-user-fs.c    |  2 +-
> > >  hw/virtio/vhost-user-vsock.c |  1 +
> > >  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
> > >  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
> > >  hw/virtio/virtio.c           | 18 ++++++++++++++++++
> > >  include/hw/virtio/virtio.h   |  5 ++++-
> > >  net/vhost-vdpa.c             |  1 +
> > >  13 files changed, 68 insertions(+), 17 deletions(-)
> > >
> > > --
> > > 2.39.3
> > >
> >

Re: [PATCH v1 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Eugenio Perez Martin]

On Wed, Mar 6, 2024 at 8:34 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Mar 06, 2024 at 08:07:31AM +0100, Eugenio Perez Martin wrote:
> > On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> > > >
> > > > The goal of these patches are to add support to a variety of virtio and
> > > > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> > > > feature indicates that a driver will pass extra data (instead of just a
> > > > virtqueue's index) when notifying the corresponding device.
> > > >
> > > > The data passed in by the driver when this feature is enabled varies in
> > > > format depending on if the device is using a split or packed virtqueue
> > > > layout:
> > > >
> > > >  Split VQ
> > > >   - Upper 16 bits: shadow_avail_idx
> > > >   - Lower 16 bits: virtqueue index
> > > >
> > > >  Packed VQ
> > > >   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> > > >   - Lower 16 bits: virtqueue index
> > > >
> > > > Also, due to the limitations of ioeventfd not being able to carry the
> > > > extra provided by the driver, ioeventfd is left disabled for any devices
> > > > using this feature.
> > >
> > > Is there any method to overcome this? This might help for vhost.
> > >
> >
> > As a half-baked idea, read(2)ing an eventfd descriptor returns an
> > 8-byte integer already. The returned value of read depends on eventfd
> > flags, but both have to do with the number of writes of the other end.
> >
> > My proposal is to replace this value with the last value written by
> > the guest, so we can extract the virtio notification data from there.
> > The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
> > and then blocking if read again without writes. The behavior of KVM
> > writes is different, as it is not a counter anymore.
> >
> > Thanks!
>
>
> I doubt you will be able to support this in ioeventfd...

I agree.

> But vhost does not really need the value at all.
> So why mask out ioeventfd with vhost?

The interface should not be able to start with vhost-kernel because
the feature is not offered by the vhost-kernel device. So ioeventfd is
always enabled with vhost-kernel.

Or do you mean we should allow it and let vhost-kernel fetch data from
the avail ring as usual? I'm ok with that but then the guest can place
any value to it, so the driver cannot be properly "validated by
software" that way.

> vhost-vdpa is probably the only one that might need it...

Right, but vhost-vdpa already supports doorbell memory regions so I
guess it has little use, isn't it?

Thanks!

>
>
>
> > > Thanks
> > >
> > > >
> > > > A significant aspect of this effort has been to maintain compatibility
> > > > across different backends. As such, the feature is offered by backend
> > > > devices only when supported, with fallback mechanisms where backend
> > > > support is absent.
> > > >
> > > > Jonah Palmer (8):
> > > >   virtio/virtio-pci: Handle extra notification data
> > > >   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > >   virtio-mmio: Handle extra notification data
> > > >   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > >   virtio-ccw: Handle extra notification data
> > > >   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > >   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
> > > >   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
> > > >
> > > >  hw/block/vhost-user-blk.c    |  1 +
> > > >  hw/net/vhost_net.c           |  2 ++
> > > >  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
> > > >  hw/s390x/virtio-ccw.c        |  6 ++++--
> > > >  hw/scsi/vhost-scsi.c         |  1 +
> > > >  hw/scsi/vhost-user-scsi.c    |  1 +
> > > >  hw/virtio/vhost-user-fs.c    |  2 +-
> > > >  hw/virtio/vhost-user-vsock.c |  1 +
> > > >  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
> > > >  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
> > > >  hw/virtio/virtio.c           | 18 ++++++++++++++++++
> > > >  include/hw/virtio/virtio.h   |  5 ++++-
> > > >  net/vhost-vdpa.c             |  1 +
> > > >  13 files changed, 68 insertions(+), 17 deletions(-)
> > > >
> > > > --
> > > > 2.39.3
> > > >
> > >
>

Re: [PATCH v1 0/8] virtio, vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Lei Yang]

Hi Jonah

QE tested this series v1 with a tap device with vhost=off with
regression tests, everything works fine. And QE also add
"notification_data=true" to the qemu command line then got "1" when
performing the command [1] inside the guest.
[1] cut -c39 /sys/devices/pci0000:00/0000:00:01.3/0000:05:00.0/virtio1/features

Tested-by: Lei Yang <leiyang@redhat.com>

On Thu, Mar 7, 2024 at 7:18 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Mar 6, 2024 at 8:34 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 08:07:31AM +0100, Eugenio Perez Martin wrote:
> > > On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> > > > >
> > > > > The goal of these patches are to add support to a variety of virtio and
> > > > > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> > > > > feature indicates that a driver will pass extra data (instead of just a
> > > > > virtqueue's index) when notifying the corresponding device.
> > > > >
> > > > > The data passed in by the driver when this feature is enabled varies in
> > > > > format depending on if the device is using a split or packed virtqueue
> > > > > layout:
> > > > >
> > > > >  Split VQ
> > > > >   - Upper 16 bits: shadow_avail_idx
> > > > >   - Lower 16 bits: virtqueue index
> > > > >
> > > > >  Packed VQ
> > > > >   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> > > > >   - Lower 16 bits: virtqueue index
> > > > >
> > > > > Also, due to the limitations of ioeventfd not being able to carry the
> > > > > extra provided by the driver, ioeventfd is left disabled for any devices
> > > > > using this feature.
> > > >
> > > > Is there any method to overcome this? This might help for vhost.
> > > >
> > >
> > > As a half-baked idea, read(2)ing an eventfd descriptor returns an
> > > 8-byte integer already. The returned value of read depends on eventfd
> > > flags, but both have to do with the number of writes of the other end.
> > >
> > > My proposal is to replace this value with the last value written by
> > > the guest, so we can extract the virtio notification data from there.
> > > The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
> > > and then blocking if read again without writes. The behavior of KVM
> > > writes is different, as it is not a counter anymore.
> > >
> > > Thanks!
> >
> >
> > I doubt you will be able to support this in ioeventfd...
>
> I agree.
>
> > But vhost does not really need the value at all.
> > So why mask out ioeventfd with vhost?
>
> The interface should not be able to start with vhost-kernel because
> the feature is not offered by the vhost-kernel device. So ioeventfd is
> always enabled with vhost-kernel.
>
> Or do you mean we should allow it and let vhost-kernel fetch data from
> the avail ring as usual? I'm ok with that but then the guest can place
> any value to it, so the driver cannot be properly "validated by
> software" that way.
>
> > vhost-vdpa is probably the only one that might need it...
>
> Right, but vhost-vdpa already supports doorbell memory regions so I
> guess it has little use, isn't it?
>
> Thanks!
>
> >
> >
> >
> > > > Thanks
> > > >
> > > > >
> > > > > A significant aspect of this effort has been to maintain compatibility
> > > > > across different backends. As such, the feature is offered by backend
> > > > > devices only when supported, with fallback mechanisms where backend
> > > > > support is absent.
> > > > >
> > > > > Jonah Palmer (8):
> > > > >   virtio/virtio-pci: Handle extra notification data
> > > > >   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > > >   virtio-mmio: Handle extra notification data
> > > > >   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > > >   virtio-ccw: Handle extra notification data
> > > > >   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > > > >   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
> > > > >   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
> > > > >
> > > > >  hw/block/vhost-user-blk.c    |  1 +
> > > > >  hw/net/vhost_net.c           |  2 ++
> > > > >  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
> > > > >  hw/s390x/virtio-ccw.c        |  6 ++++--
> > > > >  hw/scsi/vhost-scsi.c         |  1 +
> > > > >  hw/scsi/vhost-user-scsi.c    |  1 +
> > > > >  hw/virtio/vhost-user-fs.c    |  2 +-
> > > > >  hw/virtio/vhost-user-vsock.c |  1 +
> > > > >  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
> > > > >  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
> > > > >  hw/virtio/virtio.c           | 18 ++++++++++++++++++
> > > > >  include/hw/virtio/virtio.h   |  5 ++++-
> > > > >  net/vhost-vdpa.c             |  1 +
> > > > >  13 files changed, 68 insertions(+), 17 deletions(-)
> > > > >
> > > > > --
> > > > > 2.39.3
> > > > >
> > > >
> >
>
>

Re: [PATCH v1 0/8] virtio, vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Jonah Palmer]

On 3/8/24 8:28 AM, Lei Yang wrote:
> Hi Jonah
> 
> QE tested this series v1 with a tap device with vhost=off with
> regression tests, everything works fine. And QE also add
> "notification_data=true" to the qemu command line then got "1" when
> performing the command [1] inside the guest.
> [1] cut -c39 /sys/devices/pci0000:00/0000:00:01.3/0000:05:00.0/virtio1/features
> 
> Tested-by: Lei Yang <leiyang@redhat.com>
> 

Thank you for double-checking this series for me Lei! I appreciate it :)

Jonah

> On Thu, Mar 7, 2024 at 7:18 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>>
>> On Wed, Mar 6, 2024 at 8:34 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Wed, Mar 06, 2024 at 08:07:31AM +0100, Eugenio Perez Martin wrote:
>>>> On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>
>>>>> On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>>>>>
>>>>>> The goal of these patches are to add support to a variety of virtio and
>>>>>> vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
>>>>>> feature indicates that a driver will pass extra data (instead of just a
>>>>>> virtqueue's index) when notifying the corresponding device.
>>>>>>
>>>>>> The data passed in by the driver when this feature is enabled varies in
>>>>>> format depending on if the device is using a split or packed virtqueue
>>>>>> layout:
>>>>>>
>>>>>>   Split VQ
>>>>>>    - Upper 16 bits: shadow_avail_idx
>>>>>>    - Lower 16 bits: virtqueue index
>>>>>>
>>>>>>   Packed VQ
>>>>>>    - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>>>>>>    - Lower 16 bits: virtqueue index
>>>>>>
>>>>>> Also, due to the limitations of ioeventfd not being able to carry the
>>>>>> extra provided by the driver, ioeventfd is left disabled for any devices
>>>>>> using this feature.
>>>>>
>>>>> Is there any method to overcome this? This might help for vhost.
>>>>>
>>>>
>>>> As a half-baked idea, read(2)ing an eventfd descriptor returns an
>>>> 8-byte integer already. The returned value of read depends on eventfd
>>>> flags, but both have to do with the number of writes of the other end.
>>>>
>>>> My proposal is to replace this value with the last value written by
>>>> the guest, so we can extract the virtio notification data from there.
>>>> The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
>>>> and then blocking if read again without writes. The behavior of KVM
>>>> writes is different, as it is not a counter anymore.
>>>>
>>>> Thanks!
>>>
>>>
>>> I doubt you will be able to support this in ioeventfd...
>>
>> I agree.
>>
>>> But vhost does not really need the value at all.
>>> So why mask out ioeventfd with vhost?
>>
>> The interface should not be able to start with vhost-kernel because
>> the feature is not offered by the vhost-kernel device. So ioeventfd is
>> always enabled with vhost-kernel.
>>
>> Or do you mean we should allow it and let vhost-kernel fetch data from
>> the avail ring as usual? I'm ok with that but then the guest can place
>> any value to it, so the driver cannot be properly "validated by
>> software" that way.
>>
>>> vhost-vdpa is probably the only one that might need it...
>>
>> Right, but vhost-vdpa already supports doorbell memory regions so I
>> guess it has little use, isn't it?
>>
>> Thanks!
>>
>>>
>>>
>>>
>>>>> Thanks
>>>>>
>>>>>>
>>>>>> A significant aspect of this effort has been to maintain compatibility
>>>>>> across different backends. As such, the feature is offered by backend
>>>>>> devices only when supported, with fallback mechanisms where backend
>>>>>> support is absent.
>>>>>>
>>>>>> Jonah Palmer (8):
>>>>>>    virtio/virtio-pci: Handle extra notification data
>>>>>>    virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>>>>>>    virtio-mmio: Handle extra notification data
>>>>>>    virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>>>>>>    virtio-ccw: Handle extra notification data
>>>>>>    virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
>>>>>>    vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
>>>>>>    virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
>>>>>>
>>>>>>   hw/block/vhost-user-blk.c    |  1 +
>>>>>>   hw/net/vhost_net.c           |  2 ++
>>>>>>   hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
>>>>>>   hw/s390x/virtio-ccw.c        |  6 ++++--
>>>>>>   hw/scsi/vhost-scsi.c         |  1 +
>>>>>>   hw/scsi/vhost-user-scsi.c    |  1 +
>>>>>>   hw/virtio/vhost-user-fs.c    |  2 +-
>>>>>>   hw/virtio/vhost-user-vsock.c |  1 +
>>>>>>   hw/virtio/virtio-mmio.c      | 15 +++++++++++----
>>>>>>   hw/virtio/virtio-pci.c       | 16 +++++++++++-----
>>>>>>   hw/virtio/virtio.c           | 18 ++++++++++++++++++
>>>>>>   include/hw/virtio/virtio.h   |  5 ++++-
>>>>>>   net/vhost-vdpa.c             |  1 +
>>>>>>   13 files changed, 68 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> --
>>>>>> 2.39.3
>>>>>>
>>>>>
>>>
>>
>>
> 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Michael S. Tsirkin]

On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> Prevent ioeventfd from being enabled/disabled when a virtio-pci
> device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> feature.
> 
> Due to ioeventfd not being able to carry the extra data associated with
> this feature, the ioeventfd should be left in a disabled state for
> emulated virtio-pci devices using this feature.
> 
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

I thought hard about this. I propose that for now,
instead of disabling ioevetfd silently we error out unless
user disabled it for us.
WDYT?


> ---
>  hw/virtio/virtio-pci.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index d12edc567f..287b8f7720 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>              virtio_pci_stop_ioeventfd(proxy);
>          }
>  
>          virtio_set_status(vdev, val & 0xFF);
>  
> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>              virtio_pci_start_ioeventfd(proxy);
>          }
>  
> -- 
> 2.39.3

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Eugenio Perez Martin]

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > feature.
> >
> > Due to ioeventfd not being able to carry the extra data associated with
> > this feature, the ioeventfd should be left in a disabled state for
> > emulated virtio-pci devices using this feature.
> >
> > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>
> I thought hard about this. I propose that for now,
> instead of disabling ioevetfd silently we error out unless
> user disabled it for us.
> WDYT?
>

Yes, error is a better plan than silently disabling it. In the
(unlikely?) case we are able to make notification data work with
eventfd in the future, it makes the change more evident.

>
> > ---
> >  hw/virtio/virtio-pci.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index d12edc567f..287b8f7720 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          }
> >          break;
> >      case VIRTIO_PCI_STATUS:
> > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >              virtio_pci_stop_ioeventfd(proxy);
> >          }
> >
> >          virtio_set_status(vdev, val & 0xFF);
> >
> > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >              virtio_pci_start_ioeventfd(proxy);
> >          }
> >
> > --
> > 2.39.3
>

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
> On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com> 
> wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer 
> wrote: > > Prevent ioeventfd from being enabled/disabled when a 
> virtio-pci > > device
> ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> This message came from outside your organization.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
> ZjQcmQRYFpfptBannerEnd
> 
> On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
>> > Prevent ioeventfd from being enabled/disabled when a virtio-pci
>> > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
>> > feature.
>> >
>> > Due to ioeventfd not being able to carry the extra data associated with
>> > this feature, the ioeventfd should be left in a disabled state for
>> > emulated virtio-pci devices using this feature.
>> >
>> > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>
>> I thought hard about this. I propose that for now,
>> instead of disabling ioevetfd silently we error out unless
>> user disabled it for us.
>> WDYT?
>>
> 
> Yes, error is a better plan than silently disabling it. In the
> (unlikely?) case we are able to make notification data work with
> eventfd in the future, it makes the change more evident.
> 

Will do in v2. I assume we'll also make this the case for virtio-mmio 
and virtio-ccw?

>>
>> > ---
>> >  hw/virtio/virtio-pci.c | 6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> > index d12edc567f..287b8f7720 100644
>> > --- a/hw/virtio/virtio-pci.c
>> > +++ b/hw/virtio/virtio-pci.c
>> > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>> >          }
>> >          break;
>> >      case VIRTIO_PCI_STATUS:
>> > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>> >              virtio_pci_stop_ioeventfd(proxy);
>> >          }
>> >
>> >          virtio_set_status(vdev, val & 0xFF);
>> >
>> > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>> > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>> >              virtio_pci_start_ioeventfd(proxy);
>> >          }
>> >
>> > --
>> > 2.39.3
>>
> 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Michael S. Tsirkin]

On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
> 
> 
> On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
> > On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
> > wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
> > wrote: > > Prevent ioeventfd from being enabled/disabled when a
> > virtio-pci > > device
> > ZjQcmQRYFpfptBannerStart
> > This Message Is From an External Sender
> > This message came from outside your organization.
> > Report Suspicious
> > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
> > ZjQcmQRYFpfptBannerEnd
> > 
> > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > 
> > > On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > > > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > > > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > > > feature.
> > > >
> > > > Due to ioeventfd not being able to carry the extra data associated with
> > > > this feature, the ioeventfd should be left in a disabled state for
> > > > emulated virtio-pci devices using this feature.
> > > >
> > > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> > > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > > 
> > > I thought hard about this. I propose that for now,
> > > instead of disabling ioevetfd silently we error out unless
> > > user disabled it for us.
> > > WDYT?
> > > 
> > 
> > Yes, error is a better plan than silently disabling it. In the
> > (unlikely?) case we are able to make notification data work with
> > eventfd in the future, it makes the change more evident.
> > 
> 
> Will do in v2. I assume we'll also make this the case for virtio-mmio and
> virtio-ccw?

Guess so. Pls note freeze is imminent.
> > > 
> > > > ---
> > > >  hw/virtio/virtio-pci.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > index d12edc567f..287b8f7720 100644
> > > > --- a/hw/virtio/virtio-pci.c
> > > > +++ b/hw/virtio/virtio-pci.c
> > > > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> > > >          }
> > > >          break;
> > > >      case VIRTIO_PCI_STATUS:
> > > > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > >              virtio_pci_stop_ioeventfd(proxy);
> > > >          }
> > > >
> > > >          virtio_set_status(vdev, val & 0xFF);
> > > >
> > > > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > >              virtio_pci_start_ioeventfd(proxy);
> > > >          }
> > > >
> > > > --
> > > > 2.39.3
> > > 
> > 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
> On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
>>
>>
>> On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
>>> On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
>>> wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
>>> wrote: > > Prevent ioeventfd from being enabled/disabled when a
>>> virtio-pci > > device
>>> ZjQcmQRYFpfptBannerStart
>>> This Message Is From an External Sender
>>> This message came from outside your organization.
>>> Report Suspicious
>>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
>>> ZjQcmQRYFpfptBannerEnd
>>>
>>> On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
>>>>> Prevent ioeventfd from being enabled/disabled when a virtio-pci
>>>>> device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
>>>>> feature.
>>>>>
>>>>> Due to ioeventfd not being able to carry the extra data associated with
>>>>> this feature, the ioeventfd should be left in a disabled state for
>>>>> emulated virtio-pci devices using this feature.
>>>>>
>>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>>
>>>> I thought hard about this. I propose that for now,
>>>> instead of disabling ioevetfd silently we error out unless
>>>> user disabled it for us.
>>>> WDYT?
>>>>
>>>
>>> Yes, error is a better plan than silently disabling it. In the
>>> (unlikely?) case we are able to make notification data work with
>>> eventfd in the future, it makes the change more evident.
>>>
>>
>> Will do in v2. I assume we'll also make this the case for virtio-mmio and
>> virtio-ccw?
> 
> Guess so. Pls note freeze is imminent.

Got it. Also, would you mind elaborating a bit more on "error out"? E.g. 
do we want to prevent the Qemu from starting at all if a device is 
attempting to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do 
you mean something like still keep ioeventfd disabled but also log an 
error message unless it was explicitly disabled by the user?

>>>>
>>>>> ---
>>>>>   hw/virtio/virtio-pci.c | 6 ++++--
>>>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>>> index d12edc567f..287b8f7720 100644
>>>>> --- a/hw/virtio/virtio-pci.c
>>>>> +++ b/hw/virtio/virtio-pci.c
>>>>> @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>           }
>>>>>           break;
>>>>>       case VIRTIO_PCI_STATUS:
>>>>> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>>> +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>               virtio_pci_stop_ioeventfd(proxy);
>>>>>           }
>>>>>
>>>>>           virtio_set_status(vdev, val & 0xFF);
>>>>>
>>>>> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>> +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>               virtio_pci_start_ioeventfd(proxy);
>>>>>           }
>>>>>
>>>>> --
>>>>> 2.39.3
>>>>
>>>
> 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Michael S. Tsirkin]

On Mon, Mar 11, 2024 at 10:53:25AM -0400, Jonah Palmer wrote:
> 
> 
> On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
> > On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
> > > 
> > > 
> > > On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
> > > > On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
> > > > wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
> > > > wrote: > > Prevent ioeventfd from being enabled/disabled when a
> > > > virtio-pci > > device
> > > > ZjQcmQRYFpfptBannerStart
> > > > This Message Is From an External Sender
> > > > This message came from outside your organization.
> > > > Report Suspicious
> > > > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
> > > > ZjQcmQRYFpfptBannerEnd
> > > > 
> > > > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > 
> > > > > On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > > > > > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > > > > > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > > > > > feature.
> > > > > > 
> > > > > > Due to ioeventfd not being able to carry the extra data associated with
> > > > > > this feature, the ioeventfd should be left in a disabled state for
> > > > > > emulated virtio-pci devices using this feature.
> > > > > > 
> > > > > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > > > > 
> > > > > I thought hard about this. I propose that for now,
> > > > > instead of disabling ioevetfd silently we error out unless
> > > > > user disabled it for us.
> > > > > WDYT?
> > > > > 
> > > > 
> > > > Yes, error is a better plan than silently disabling it. In the
> > > > (unlikely?) case we are able to make notification data work with
> > > > eventfd in the future, it makes the change more evident.
> > > > 
> > > 
> > > Will do in v2. I assume we'll also make this the case for virtio-mmio and
> > > virtio-ccw?
> > 
> > Guess so. Pls note freeze is imminent.
> 
> Got it. Also, would you mind elaborating a bit more on "error out"? E.g. do
> we want to prevent the Qemu from starting at all if a device is attempting
> to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do you mean
> something like still keep ioeventfd disabled but also log an error message
> unless it was explicitly disabled by the user?


my preference would be to block device instance from being created.

> > > > > 
> > > > > > ---
> > > > > >   hw/virtio/virtio-pci.c | 6 ++++--
> > > > > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > > index d12edc567f..287b8f7720 100644
> > > > > > --- a/hw/virtio/virtio-pci.c
> > > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> > > > > >           }
> > > > > >           break;
> > > > > >       case VIRTIO_PCI_STATUS:
> > > > > > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > > > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > >               virtio_pci_stop_ioeventfd(proxy);
> > > > > >           }
> > > > > > 
> > > > > >           virtio_set_status(vdev, val & 0xFF);
> > > > > > 
> > > > > > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > > > > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > >               virtio_pci_start_ioeventfd(proxy);
> > > > > >           }
> > > > > > 
> > > > > > --
> > > > > > 2.39.3
> > > > > 
> > > > 
> > 

Re: [PATCH v1 5/8] virtio-ccw: Handle extra notification data

[Eric Farman]

On Mon, 2024-03-04 at 14:46 -0500, Jonah Palmer wrote:
> > Add support to virtio-ccw devices for handling the extra data sent
> > from
> > the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> > transport
> > feature has been negotiated.
> > 
> > The extra data that's passed to the virtio-ccw device when this
> > feature
> > is enabled varies depending on the device's virtqueue layout.
> > 
> > That data passed to the virtio-ccw device is in the same format as
> > the
> > data passed to virtio-pci devices.
> > 
> > Acked-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)

Acked-by: Eric Farman <farman@linux.ibm.com>

(I see a v2 is coming for the ioeventfd side, but I was going through
this series today and thought that would affect the next patch rather
than this one.)

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:
> On Mon, Mar 11, 2024 at 10:53:25AM -0400, Jonah Palmer wrote:
>>
>>
>> On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
>>> On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
>>>>
>>>>
>>>> On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
>>>>> On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
>>>>> wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
>>>>> wrote: > > Prevent ioeventfd from being enabled/disabled when a
>>>>> virtio-pci > > device
>>>>> ZjQcmQRYFpfptBannerStart
>>>>> This Message Is From an External Sender
>>>>> This message came from outside your organization.
>>>>> Report Suspicious
>>>>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
>>>>> ZjQcmQRYFpfptBannerEnd
>>>>>
>>>>> On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
>>>>>>> Prevent ioeventfd from being enabled/disabled when a virtio-pci
>>>>>>> device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
>>>>>>> feature.
>>>>>>>
>>>>>>> Due to ioeventfd not being able to carry the extra data associated with
>>>>>>> this feature, the ioeventfd should be left in a disabled state for
>>>>>>> emulated virtio-pci devices using this feature.
>>>>>>>
>>>>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>>>>
>>>>>> I thought hard about this. I propose that for now,
>>>>>> instead of disabling ioevetfd silently we error out unless
>>>>>> user disabled it for us.
>>>>>> WDYT?
>>>>>>
>>>>>
>>>>> Yes, error is a better plan than silently disabling it. In the
>>>>> (unlikely?) case we are able to make notification data work with
>>>>> eventfd in the future, it makes the change more evident.
>>>>>
>>>>
>>>> Will do in v2. I assume we'll also make this the case for virtio-mmio and
>>>> virtio-ccw?
>>>
>>> Guess so. Pls note freeze is imminent.
>>
>> Got it. Also, would you mind elaborating a bit more on "error out"? E.g. do
>> we want to prevent the Qemu from starting at all if a device is attempting
>> to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do you mean
>> something like still keep ioeventfd disabled but also log an error message
>> unless it was explicitly disabled by the user?
> 
> 
> my preference would be to block device instance from being created.
> 

I could very well be missing something here, but I was looking to see 
how I could block the device from being created (realized) given the 
functional mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA 
feature and ioeventfd being enabled.

However, I realized that feature negotiation only happens after the 
virtio device has been realized and it's one of the last steps before 
the device becomes fully operational. In other words, we don't know if 
the guest (driver) also supports this feature until the feature 
negotiation phase, which is after realization.

So, during realization (e.g. virtio_device_realize), we know if the 
virtio device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA 
feature and (2) has enabled ioeventfd, however, we don't know if the 
driver will actually support this notification data feature.

Given this, we could block the device from being created if the device 
is *intending* to use the notification data feature along with 
ioeventfd, but this seems premature since we don't know if the feature 
will actually be successfully negotiated.

Another option might be check this during/immediately after feature 
negotiation, and then unrealize the device. However, I'm not sure if by 
this point it's "too late" to unrealize it.

There's also other options like defaulting to using notification data 
over ioeventfd (since a user would need to explicitly enable it, showing 
intent to actually use the feature), which is what we're doing now, 
except we could add some kind of warning message for the user. Another 
option could be setting the device to broken. However, these options 
don't align with your suggestion of removing the device completely.

Let me know how you'd like me to proceed with this. Thanks!

>>>>>>
>>>>>>> ---
>>>>>>>    hw/virtio/virtio-pci.c | 6 ++++--
>>>>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>>>>> index d12edc567f..287b8f7720 100644
>>>>>>> --- a/hw/virtio/virtio-pci.c
>>>>>>> +++ b/hw/virtio/virtio-pci.c
>>>>>>> @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>>>            }
>>>>>>>            break;
>>>>>>>        case VIRTIO_PCI_STATUS:
>>>>>>> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>>>>> +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>>>                virtio_pci_stop_ioeventfd(proxy);
>>>>>>>            }
>>>>>>>
>>>>>>>            virtio_set_status(vdev, val & 0xFF);
>>>>>>>
>>>>>>> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>>>> +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>>>                virtio_pci_start_ioeventfd(proxy);
>>>>>>>            }
>>>>>>>
>>>>>>> --
>>>>>>> 2.39.3
>>>>>>
>>>>>
>>>
> 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Michael S. Tsirkin]

On Tue, Mar 12, 2024 at 10:33:51AM -0400, Jonah Palmer wrote:
> 
> 
> On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:
> > On Mon, Mar 11, 2024 at 10:53:25AM -0400, Jonah Palmer wrote:
> > > 
> > > 
> > > On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
> > > > On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
> > > > > 
> > > > > 
> > > > > On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
> > > > > > On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
> > > > > > wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
> > > > > > wrote: > > Prevent ioeventfd from being enabled/disabled when a
> > > > > > virtio-pci > > device
> > > > > > ZjQcmQRYFpfptBannerStart
> > > > > > This Message Is From an External Sender
> > > > > > This message came from outside your organization.
> > > > > > Report Suspicious
> > > > > > <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
> > > > > > ZjQcmQRYFpfptBannerEnd
> > > > > > 
> > > > > > On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > 
> > > > > > > On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > > > > > > > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > > > > > > > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > > > > > > > feature.
> > > > > > > > 
> > > > > > > > Due to ioeventfd not being able to carry the extra data associated with
> > > > > > > > this feature, the ioeventfd should be left in a disabled state for
> > > > > > > > emulated virtio-pci devices using this feature.
> > > > > > > > 
> > > > > > > > Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> > > > > > > 
> > > > > > > I thought hard about this. I propose that for now,
> > > > > > > instead of disabling ioevetfd silently we error out unless
> > > > > > > user disabled it for us.
> > > > > > > WDYT?
> > > > > > > 
> > > > > > 
> > > > > > Yes, error is a better plan than silently disabling it. In the
> > > > > > (unlikely?) case we are able to make notification data work with
> > > > > > eventfd in the future, it makes the change more evident.
> > > > > > 
> > > > > 
> > > > > Will do in v2. I assume we'll also make this the case for virtio-mmio and
> > > > > virtio-ccw?
> > > > 
> > > > Guess so. Pls note freeze is imminent.
> > > 
> > > Got it. Also, would you mind elaborating a bit more on "error out"? E.g. do
> > > we want to prevent the Qemu from starting at all if a device is attempting
> > > to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do you mean
> > > something like still keep ioeventfd disabled but also log an error message
> > > unless it was explicitly disabled by the user?
> > 
> > 
> > my preference would be to block device instance from being created.
> > 
> 
> I could very well be missing something here, but I was looking to see how I
> could block the device from being created (realized) given the functional
> mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA feature and
> ioeventfd being enabled.
> 
> However, I realized that feature negotiation only happens after the virtio
> device has been realized and it's one of the last steps before the device
> becomes fully operational. In other words, we don't know if the guest
> (driver) also supports this feature until the feature negotiation phase,
> which is after realization.
> 
> So, during realization (e.g. virtio_device_realize), we know if the virtio
> device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA feature and
> (2) has enabled ioeventfd, however, we don't know if the driver will
> actually support this notification data feature.
> 
> Given this, we could block the device from being created if the device is
> *intending* to use the notification data feature along with ioeventfd, but
> this seems premature since we don't know if the feature will actually be
> successfully negotiated.

Yes this is the option I had in mind. Many devices with this feature
do not actually work if they do not get the extra data
so they fail FEATURES_OK, anyway.


> Another option might be check this during/immediately after feature
> negotiation, and then unrealize the device. However, I'm not sure if by this
> point it's "too late" to unrealize it.
> 
> There's also other options like defaulting to using notification data over
> ioeventfd (since a user would need to explicitly enable it, showing intent
> to actually use the feature), which is what we're doing now, except we could
> add some kind of warning message for the user. Another option could be
> setting the device to broken. However, these options don't align with your
> suggestion of removing the device completely.
> 
> Let me know how you'd like me to proceed with this. Thanks!
> 
> > > > > > > 
> > > > > > > > ---
> > > > > > > >    hw/virtio/virtio-pci.c | 6 ++++--
> > > > > > > >    1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > > > > index d12edc567f..287b8f7720 100644
> > > > > > > > --- a/hw/virtio/virtio-pci.c
> > > > > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > > > > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> > > > > > > >            }
> > > > > > > >            break;
> > > > > > > >        case VIRTIO_PCI_STATUS:
> > > > > > > > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > > > > > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > > > >                virtio_pci_stop_ioeventfd(proxy);
> > > > > > > >            }
> > > > > > > > 
> > > > > > > >            virtio_set_status(vdev, val & 0xFF);
> > > > > > > > 
> > > > > > > > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > > > > > > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > > > > > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> > > > > > > >                virtio_pci_start_ioeventfd(proxy);
> > > > > > > >            }
> > > > > > > > 
> > > > > > > > --
> > > > > > > > 2.39.3
> > > > > > > 
> > > > > > 
> > > > 
> > 

Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Jonah Palmer]

On 3/12/24 10:58 AM, Michael S. Tsirkin wrote:
> On Tue, Mar 12, 2024 at 10:33:51AM -0400, Jonah Palmer wrote:
>>
>>
>> On 3/11/24 11:47 AM, Michael S. Tsirkin wrote:
>>> On Mon, Mar 11, 2024 at 10:53:25AM -0400, Jonah Palmer wrote:
>>>>
>>>>
>>>> On 3/8/24 2:19 PM, Michael S. Tsirkin wrote:
>>>>> On Fri, Mar 08, 2024 at 12:45:13PM -0500, Jonah Palmer wrote:
>>>>>>
>>>>>>
>>>>>> On 3/8/24 12:36 PM, Eugenio Perez Martin wrote:
>>>>>>> On Fri, Mar 8, 2024 at 6: 01 PM Michael S. Tsirkin <mst@ redhat. com>
>>>>>>> wrote: > > On Mon, Mar 04, 2024 at 02: 46: 06PM -0500, Jonah Palmer
>>>>>>> wrote: > > Prevent ioeventfd from being enabled/disabled when a
>>>>>>> virtio-pci > > device
>>>>>>> ZjQcmQRYFpfptBannerStart
>>>>>>> This Message Is From an External Sender
>>>>>>> This message came from outside your organization.
>>>>>>> Report Suspicious
>>>>>>> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/ACWV5N9M2RV99hQ!Op20OCZE8kFi__wOXJ_Z0URZ2e_9fdaYz2tejZvKqiDgOm6ijq_imUptzxsrej_4riwCrBGeKmQ9VKXqnbV1ujbfiOV5-E2e1s3pKqpqUL-gRIuMQLDLygRD1hoX3Q$>
>>>>>>> ZjQcmQRYFpfptBannerEnd
>>>>>>>
>>>>>>> On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
>>>>>>>>> Prevent ioeventfd from being enabled/disabled when a virtio-pci
>>>>>>>>> device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
>>>>>>>>> feature.
>>>>>>>>>
>>>>>>>>> Due to ioeventfd not being able to carry the extra data associated with
>>>>>>>>> this feature, the ioeventfd should be left in a disabled state for
>>>>>>>>> emulated virtio-pci devices using this feature.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>>>>>>
>>>>>>>> I thought hard about this. I propose that for now,
>>>>>>>> instead of disabling ioevetfd silently we error out unless
>>>>>>>> user disabled it for us.
>>>>>>>> WDYT?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, error is a better plan than silently disabling it. In the
>>>>>>> (unlikely?) case we are able to make notification data work with
>>>>>>> eventfd in the future, it makes the change more evident.
>>>>>>>
>>>>>>
>>>>>> Will do in v2. I assume we'll also make this the case for virtio-mmio and
>>>>>> virtio-ccw?
>>>>>
>>>>> Guess so. Pls note freeze is imminent.
>>>>
>>>> Got it. Also, would you mind elaborating a bit more on "error out"? E.g. do
>>>> we want to prevent the Qemu from starting at all if a device is attempting
>>>> to use both VIRTIO_F_NOTIFICATION_DATA and ioeventfd? Or do you mean
>>>> something like still keep ioeventfd disabled but also log an error message
>>>> unless it was explicitly disabled by the user?
>>>
>>>
>>> my preference would be to block device instance from being created.
>>>
>>
>> I could very well be missing something here, but I was looking to see how I
>> could block the device from being created (realized) given the functional
>> mismatch between negotiating the VIRTIO_F_NOTIFICATION_DATA feature and
>> ioeventfd being enabled.
>>
>> However, I realized that feature negotiation only happens after the virtio
>> device has been realized and it's one of the last steps before the device
>> becomes fully operational. In other words, we don't know if the guest
>> (driver) also supports this feature until the feature negotiation phase,
>> which is after realization.
>>
>> So, during realization (e.g. virtio_device_realize), we know if the virtio
>> device (1) intends to negotiate the VIRTIO_F_NOTIFICATION_DATA feature and
>> (2) has enabled ioeventfd, however, we don't know if the driver will
>> actually support this notification data feature.
>>
>> Given this, we could block the device from being created if the device is
>> *intending* to use the notification data feature along with ioeventfd, but
>> this seems premature since we don't know if the feature will actually be
>> successfully negotiated.
> 
> Yes this is the option I had in mind. Many devices with this feature
> do not actually work if they do not get the extra data
> so they fail FEATURES_OK, anyway.
> 
> 

Ah, okay I see. This was the extra context I was missing.

Will do, thanks Michael!

>> Another option might be check this during/immediately after feature
>> negotiation, and then unrealize the device. However, I'm not sure if by this
>> point it's "too late" to unrealize it.
>>
>> There's also other options like defaulting to using notification data over
>> ioeventfd (since a user would need to explicitly enable it, showing intent
>> to actually use the feature), which is what we're doing now, except we could
>> add some kind of warning message for the user. Another option could be
>> setting the device to broken. However, these options don't align with your
>> suggestion of removing the device completely.
>>
>> Let me know how you'd like me to proceed with this. Thanks!
>>
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>     hw/virtio/virtio-pci.c | 6 ++++--
>>>>>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>>>>>>> index d12edc567f..287b8f7720 100644
>>>>>>>>> --- a/hw/virtio/virtio-pci.c
>>>>>>>>> +++ b/hw/virtio/virtio-pci.c
>>>>>>>>> @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>>>>>             }
>>>>>>>>>             break;
>>>>>>>>>         case VIRTIO_PCI_STATUS:
>>>>>>>>> -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>>>>>>> +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>>>>>                 virtio_pci_stop_ioeventfd(proxy);
>>>>>>>>>             }
>>>>>>>>>
>>>>>>>>>             virtio_set_status(vdev, val & 0xFF);
>>>>>>>>>
>>>>>>>>> -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>>>>>>> +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>>>>>>> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>>>>>                 virtio_pci_start_ioeventfd(proxy);
>>>>>>>>>             }
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.39.3
>>>>>>>>
>>>>>>>
>>>>>
>>>
>