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

[RFC 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: last_avail_idx
  - Lower 16 bits: virtqueue index

 Packed VQ
  - Upper 16 bits: 1-bit wrap counter & 15-bit last_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   | 18 ++++++++++++++----
 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      | 18 ++++++++++++++----
 hw/virtio/virtio-pci.c       | 19 ++++++++++++++-----
 hw/virtio/virtio.c           | 13 +++++++++++++
 include/hw/virtio/virtio.h   |  5 ++++-
 net/vhost-vdpa.c             |  1 +
 13 files changed, 71 insertions(+), 17 deletions(-)

-- 
2.39.3

[RFC 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: last_avail_idx
 - lower 16 bits: virtqueue index

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

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

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 1a7039fb0c..c7c577b177 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,15 @@ 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);
+        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+            vq_idx = val & 0xFFFF;
+            virtio_set_notification_data(vdev, vq_idx, val);
+        } else {
+            vq_idx = val;
+        }
+
+        if (vq_idx < VIRTIO_QUEUE_MAX) {
+            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..a61f69b7fd 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
     return 0;
 }
 
+void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
+{
+    VirtQueue *vq = &vdev->vq[i];
+
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
+        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
+        vq->last_avail_idx = (data >> 16) & 0x7FFF;
+    } else {
+        vq->last_avail_idx = (data >> 16) & 0xFFFF;
+    }
+    vq->shadow_avail_idx = vq->last_avail_idx;
+}
+
 static enum virtio_device_endian virtio_default_endian(void)
 {
     if (target_words_bigendian()) {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c8f72850bc..c92d8afc42 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
 void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint64_t val);
+void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
 
 /* Base devices.  */
 typedef struct VirtIOBlkConf VirtIOBlkConf;
-- 
2.39.3

[RFC 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.

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 c7c577b177..fd9717a0f5 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -420,13 +420,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

[RFC 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 | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 22f9fbcf5a..2bac77460e 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,15 @@ 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);
+        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+            vq_idx = value & 0xFFFF;
+            virtio_set_notification_data(vdev, vq_idx, value);
+        } else {
+            vq_idx = value;
+        }
+
+        if (vq_idx < VIRTIO_QUEUE_MAX) {
+            virtio_queue_notify(vdev, vq_idx);
         }
         break;
     case VIRTIO_MMIO_INTERRUPT_ACK:
-- 
2.39.3

[RFC 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 2bac77460e..fc780a03b2 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -424,7 +424,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);
         }
 
@@ -436,7 +437,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

[RFC 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.

Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/s390x/s390-virtio-ccw.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 62804cc228..b8e193956c 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;
 
     if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
         return -EINVAL;
@@ -151,12 +153,20 @@ static int virtio_ccw_hcall_notify(const uint64_t *args)
     if (!sch || !css_subch_visible(sch)) {
         return -EINVAL;
     }
-    if (queue >= VIRTIO_QUEUE_MAX) {
+
+    vdev = virtio_ccw_get_vdev(sch);
+    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
+        vq_idx = data & 0xFFFF;
+        virtio_set_notification_data(vdev, vq_idx, data);
+    } else {
+        vq_idx = data;
+    }
+
+    if (vq_idx >= VIRTIO_QUEUE_MAX) {
         return -EINVAL;
     }
-    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
+    virtio_queue_notify(vdev, vq_idx);
     return 0;
-
 }
 
 static int virtio_ccw_hcall_early_printk(const uint64_t *args)
-- 
2.39.3

[RFC 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.

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

[RFC 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.

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 3726ee5d67..2827d29ce7 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

[RFC 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.

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 c92d8afc42..5772737dde 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: [RFC 1/8] virtio/virtio-pci: Handle extra notification data

[Eugenio Perez Martin]

On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
>  - lower 16 bits: virtqueue index
>
> In a packed virtqueue layout, this data includes:
>  - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
>  - lower 16 bits: virtqueue index
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-pci.c     | 13 ++++++++++---
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +            vq_idx = val & 0xFFFF;

Nitpick, but since vq_idx is already a uint16_t the & 0xFFFF is not
needed. I think it's cleaner just to call virtio_set_notification data
in the has_feature(...) condition, but I'm happy with this too.

> +            virtio_set_notification_data(vdev, vq_idx, val);
> +        } else {
> +            vq_idx = val;
> +        }
> +
> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> +            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..a61f69b7fd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      return 0;
>  }
>
> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
> +{
> +    VirtQueue *vq = &vdev->vq[i];
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
> +    } else {
> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
> +    }

It should not set last_avail_idx, only shadow_avail_idx. Otherwise,
QEMU can only see the descriptors placed after the notification.

Or am I missing something?

In that regard, I would call this function
"virtqueue_set_shadow_avail_idx". But I'm very bad at naming :).

The rest looks good to me.

Thanks!

> +    vq->shadow_avail_idx = vq->last_avail_idx;
> +}
> +
>  static enum virtio_device_endian virtio_default_endian(void)
>  {
>      if (target_words_bigendian()) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c8f72850bc..c92d8afc42 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>  void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
>
>  /* Base devices.  */
>  typedef struct VirtIOBlkConf VirtIOBlkConf;
> --
> 2.39.3
>

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

[Eugenio Perez Martin]

On Fri, Mar 1, 2024 at 2:44 PM Jonah Palmer <jonah.palmer@oracle.com> 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.
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>

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

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 c7c577b177..fd9717a0f5 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -420,13 +420,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: [RFC 1/8] virtio/virtio-pci: Handle extra notification data

[Eugenio Perez Martin]

On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
>  - lower 16 bits: virtqueue index
>
> In a packed virtqueue layout, this data includes:
>  - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
>  - lower 16 bits: virtqueue index
>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-pci.c     | 13 ++++++++++---
>  hw/virtio/virtio.c         | 13 +++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +            vq_idx = val & 0xFFFF;
> +            virtio_set_notification_data(vdev, vq_idx, val);
> +        } else {
> +            vq_idx = val;
> +        }
> +
> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> +            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..a61f69b7fd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      return 0;
>  }
>
> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
> +{
> +    VirtQueue *vq = &vdev->vq[i];

Sorry I sent the previous mail too fast :).

i should also be checked against VIRTIO_QUEUE_MAX and vq->vring.desc
before continuing this function. Otherwise is an out of bound access.

> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
> +    } else {
> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
> +    }
> +    vq->shadow_avail_idx = vq->last_avail_idx;
> +}
> +
>  static enum virtio_device_endian virtio_default_endian(void)
>  {
>      if (target_words_bigendian()) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c8f72850bc..c92d8afc42 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>  void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>  void virtio_update_irq(VirtIODevice *vdev);
>  int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
>
>  /* Base devices.  */
>  typedef struct VirtIOBlkConf VirtIOBlkConf;
> --
> 2.39.3
>

Re: [RFC 7/8] vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits

[Eugenio Perez Martin]

On Fri, Mar 1, 2024 at 2:44 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> 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.
>
> 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,
>

vdpa_feature_bits also needs this feature bit added.

Apart from that,

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

> 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 3726ee5d67..2827d29ce7 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
>

Re: [RFC 8/8] virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition

[Eugenio Perez Martin]

On Fri, Mar 1, 2024 at 2:44 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> 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>

Thanks!

> 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 c92d8afc42..5772737dde 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: [RFC 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Eugenio Perez Martin]

Hi Wentao / Rick / Xinying Yu,

Would it work for you to test this series on your use cases, so we
make sure everything works as expected?

Thanks!

On Fri, Mar 1, 2024 at 2:44 PM 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: last_avail_idx
>   - Lower 16 bits: virtqueue index
>
>  Packed VQ
>   - Upper 16 bits: 1-bit wrap counter & 15-bit last_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.
>

Hi Wentao,

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

[Thomas Huth]

On 01/03/2024 14.43, 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.
> 
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 62804cc228..b8e193956c 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;
>   
>       if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) {
>           return -EINVAL;
> @@ -151,12 +153,20 @@ static int virtio_ccw_hcall_notify(const uint64_t *args)
>       if (!sch || !css_subch_visible(sch)) {
>           return -EINVAL;
>       }
> -    if (queue >= VIRTIO_QUEUE_MAX) {
> +
> +    vdev = virtio_ccw_get_vdev(sch);
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +        vq_idx = data & 0xFFFF;
> +        virtio_set_notification_data(vdev, vq_idx, data);
> +    } else {
> +        vq_idx = data;
> +    }
> +
> +    if (vq_idx >= VIRTIO_QUEUE_MAX) {
>           return -EINVAL;
>       }
> -    virtio_queue_notify(virtio_ccw_get_vdev(sch), queue);
> +    virtio_queue_notify(vdev, vq_idx);
>       return 0;
> -
>   }

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [RFC 6/8] virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

[Thomas Huth]

On 01/03/2024 14.43, Jonah Palmer wrote:
> 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.
> 
> 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);

Acked-by: Thomas Huth <thuth@redhat.com>

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

[Jonah Palmer]

On 3/1/24 2:31 PM, Eugenio Perez Martin wrote:
> On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> In a packed virtqueue layout, this data includes:
>>   - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio-pci.c     | 13 ++++++++++---
>>   hw/virtio/virtio.c         | 13 +++++++++++++
>>   include/hw/virtio/virtio.h |  1 +
>>   3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
>> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>> +            vq_idx = val & 0xFFFF;
> 
> Nitpick, but since vq_idx is already a uint16_t the & 0xFFFF is not
> needed. 

Ah okay. I wasn't sure if it was worthwhile to keep the '& 0xFFFF' in or 
not for the sake of clarity and good practice. In that case I could just 
do away with vq_idx here and use explicit casting on 'val'.

> I think it's cleaner just to call virtio_set_notification data
> in the has_feature(...) condition, but I'm happy with this too.

Do you mean something like:

if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
     virtio_set_notification_data(vdev, vq_idx, val)) {
     ...
}

Though I'm not sure what would then go in the body of this conditional, 
especially if I did something like:

case VIRTIO_PCI_QUEUE_NOTIFY:
     if ((uint16_t)val < VIRTIO_QUEUE_MAX) {
         if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
             virtio_set_notification_data(vdev, val)) {
             // Not sure what to put here other than a no-op
         }

         virtio_queue_notify(vdev, (uint16_t)val);
     }
     break;

But I'm not sure if you'd prefer this explicit casting of 'val' over 
implicit casting like:

uint16_t vq_idx = val;

> 
>> +            virtio_set_notification_data(vdev, vq_idx, val);
>> +        } else {
>> +            vq_idx = val;
>> +        }
>> +
>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>> +            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..a61f69b7fd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>       return 0;
>>   }
>>
>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
>> +{
>> +    VirtQueue *vq = &vdev->vq[i];
>> +
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
>> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
>> +    } else {
>> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
>> +    }
> 
> It should not set last_avail_idx, only shadow_avail_idx. Otherwise,
> QEMU can only see the descriptors placed after the notification.
> 
> Or am I missing something?
> 
> In that regard, I would call this function
> "virtqueue_set_shadow_avail_idx". But I'm very bad at naming :).

Ah that's right. This would make Qemu skip processing descriptors that 
might've been made available before the notification but after the 
host's last check of last_avail_idx. In other words, ignoring available 
descriptors that were placed before the notification but not yet 
processed. Good catch, thank you!

So, for the packed VQ layout, we'll still want to save the wrap counter 
but for the shadow_avail_idx, right? E.g.

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);
}

> 
> The rest looks good to me.
> 
> Thanks!
> 
>> +    vq->shadow_avail_idx = vq->last_avail_idx;
>> +}
>> +
>>   static enum virtio_device_endian virtio_default_endian(void)
>>   {
>>       if (target_words_bigendian()) {
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c8f72850bc..c92d8afc42 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>>   void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>>   void virtio_update_irq(VirtIODevice *vdev);
>>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
>>
>>   /* Base devices.  */
>>   typedef struct VirtIOBlkConf VirtIOBlkConf;
>> --
>> 2.39.3
>>
> 

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

[Jonah Palmer]

On 3/1/24 2:55 PM, Eugenio Perez Martin wrote:
> On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> In a packed virtqueue layout, this data includes:
>>   - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio-pci.c     | 13 ++++++++++---
>>   hw/virtio/virtio.c         | 13 +++++++++++++
>>   include/hw/virtio/virtio.h |  1 +
>>   3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
>> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>> +            vq_idx = val & 0xFFFF;
>> +            virtio_set_notification_data(vdev, vq_idx, val);
>> +        } else {
>> +            vq_idx = val;
>> +        }
>> +
>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>> +            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..a61f69b7fd 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>       return 0;
>>   }
>>
>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
>> +{
>> +    VirtQueue *vq = &vdev->vq[i];
> 
> Sorry I sent the previous mail too fast :).
> 
> i should also be checked against VIRTIO_QUEUE_MAX and vq->vring.desc
> before continuing this function. Otherwise is an out of bound access.

Missed this, thank you. I will add these checks in!

> 
>> +
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
>> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
>> +    } else {
>> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
>> +    }
>> +    vq->shadow_avail_idx = vq->last_avail_idx;
>> +}
>> +
>>   static enum virtio_device_endian virtio_default_endian(void)
>>   {
>>       if (target_words_bigendian()) {
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c8f72850bc..c92d8afc42 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>>   void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>>   void virtio_update_irq(VirtIODevice *vdev);
>>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
>>
>>   /* Base devices.  */
>>   typedef struct VirtIOBlkConf VirtIOBlkConf;
>> --
>> 2.39.3
>>
> 

Re: [RFC 7/8] vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits

[Jonah Palmer]

On 3/1/24 3:04 PM, Eugenio Perez Martin wrote:
> On Fri, Mar 1, 2024 at 2:44 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> 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.
>>
>> 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,
>>
> 
> vdpa_feature_bits also needs this feature bit added.

The vdpa_feature_bits in /net/vhost-vdpa.c, right? I did add this 
feature bit to this list, unless you're referring to something else.

> 
> Apart from that,
> 
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> 
>> 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 3726ee5d67..2827d29ce7 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
>>
> 

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

[Eugenio Perez Martin]

On Mon, Mar 4, 2024 at 6:09 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
>
>
> On 3/1/24 2:31 PM, Eugenio Perez Martin wrote:
> > On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
> >>   - lower 16 bits: virtqueue index
> >>
> >> In a packed virtqueue layout, this data includes:
> >>   - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
> >>   - lower 16 bits: virtqueue index
> >>
> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >> ---
> >>   hw/virtio/virtio-pci.c     | 13 ++++++++++---
> >>   hw/virtio/virtio.c         | 13 +++++++++++++
> >>   include/hw/virtio/virtio.h |  1 +
> >>   3 files changed, 24 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
> >> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >> +            vq_idx = val & 0xFFFF;
> >
> > Nitpick, but since vq_idx is already a uint16_t the & 0xFFFF is not
> > needed.
>
> Ah okay. I wasn't sure if it was worthwhile to keep the '& 0xFFFF' in or
> not for the sake of clarity and good practice. In that case I could just
> do away with vq_idx here and use explicit casting on 'val'.
>
> > I think it's cleaner just to call virtio_set_notification data
> > in the has_feature(...) condition, but I'm happy with this too.
>
> Do you mean something like:
>
> if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
>      virtio_set_notification_data(vdev, vq_idx, val)) {
>      ...
> }
>

Sorry I was not clear, I meant just to take out the common code of the
conditionals:
vq_idx = val;
if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) {
    virtio_set_notification_data(vdev, val);
}

> Though I'm not sure what would then go in the body of this conditional,
> especially if I did something like:
>
> case VIRTIO_PCI_QUEUE_NOTIFY:
>      if ((uint16_t)val < VIRTIO_QUEUE_MAX) {
>          if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
>              virtio_set_notification_data(vdev, val)) {
>              // Not sure what to put here other than a no-op
>          }
>
>          virtio_queue_notify(vdev, (uint16_t)val);
>      }
>      break;
>
> But I'm not sure if you'd prefer this explicit casting of 'val' over
> implicit casting like:
>
> uint16_t vq_idx = val;
>
> >
> >> +            virtio_set_notification_data(vdev, vq_idx, val);
> >> +        } else {
> >> +            vq_idx = val;
> >> +        }
> >> +
> >> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> >> +            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..a61f69b7fd 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
> >>       return 0;
> >>   }
> >>
> >> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
> >> +{
> >> +    VirtQueue *vq = &vdev->vq[i];
> >> +
> >> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
> >> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
> >> +    } else {
> >> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
> >> +    }
> >
> > It should not set last_avail_idx, only shadow_avail_idx. Otherwise,
> > QEMU can only see the descriptors placed after the notification.
> >
> > Or am I missing something?
> >
> > In that regard, I would call this function
> > "virtqueue_set_shadow_avail_idx". But I'm very bad at naming :).
>
> Ah that's right. This would make Qemu skip processing descriptors that
> might've been made available before the notification but after the
> host's last check of last_avail_idx. In other words, ignoring available
> descriptors that were placed before the notification but not yet
> processed. Good catch, thank you!
>
> So, for the packed VQ layout, we'll still want to save the wrap counter
> but for the shadow_avail_idx, right? E.g.
>
> 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);
> }
>

Right, I was not clear enough again :). You probably have guessed
already but not modifying avail_wrap_counter would make QEMu to read
the wrong index too.

Thanks!

> >
> > The rest looks good to me.
> >
> > Thanks!
> >
> >> +    vq->shadow_avail_idx = vq->last_avail_idx;
> >> +}
> >> +
> >>   static enum virtio_device_endian virtio_default_endian(void)
> >>   {
> >>       if (target_words_bigendian()) {
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index c8f72850bc..c92d8afc42 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> >>   void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
> >>   void virtio_update_irq(VirtIODevice *vdev);
> >>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);
> >> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
> >>
> >>   /* Base devices.  */
> >>   typedef struct VirtIOBlkConf VirtIOBlkConf;
> >> --
> >> 2.39.3
> >>
> >
>

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

[Jonah Palmer]

On 3/4/24 12:24 PM, Eugenio Perez Martin wrote:
> On Mon, Mar 4, 2024 at 6:09 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>>
>>
>> On 3/1/24 2:31 PM, Eugenio Perez Martin wrote:
>>> On Fri, Mar 1, 2024 at 2:44 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: last_avail_idx
>>>>    - lower 16 bits: virtqueue index
>>>>
>>>> In a packed virtqueue layout, this data includes:
>>>>    - upper 16 bits: 1-bit wrap counter & 15-bit last_avail_idx
>>>>    - lower 16 bits: virtqueue index
>>>>
>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>> ---
>>>>    hw/virtio/virtio-pci.c     | 13 ++++++++++---
>>>>    hw/virtio/virtio.c         | 13 +++++++++++++
>>>>    include/hw/virtio/virtio.h |  1 +
>>>>    3 files changed, 24 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>> index 1a7039fb0c..c7c577b177 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,15 @@ 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);
>>>> +        if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>> +            vq_idx = val & 0xFFFF;
>>>
>>> Nitpick, but since vq_idx is already a uint16_t the & 0xFFFF is not
>>> needed.
>>
>> Ah okay. I wasn't sure if it was worthwhile to keep the '& 0xFFFF' in or
>> not for the sake of clarity and good practice. In that case I could just
>> do away with vq_idx here and use explicit casting on 'val'.
>>
>>> I think it's cleaner just to call virtio_set_notification data
>>> in the has_feature(...) condition, but I'm happy with this too.
>>
>> Do you mean something like:
>>
>> if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
>>       virtio_set_notification_data(vdev, vq_idx, val)) {
>>       ...
>> }
>>
> 
> Sorry I was not clear, I meant just to take out the common code of the
> conditionals:
> vq_idx = val;
> if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) {
>      virtio_set_notification_data(vdev, val);
> }
> 

Ah, no problem! Thank you for the clarification!

>> Though I'm not sure what would then go in the body of this conditional,
>> especially if I did something like:
>>
>> case VIRTIO_PCI_QUEUE_NOTIFY:
>>       if ((uint16_t)val < VIRTIO_QUEUE_MAX) {
>>           if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) &&
>>               virtio_set_notification_data(vdev, val)) {
>>               // Not sure what to put here other than a no-op
>>           }
>>
>>           virtio_queue_notify(vdev, (uint16_t)val);
>>       }
>>       break;
>>
>> But I'm not sure if you'd prefer this explicit casting of 'val' over
>> implicit casting like:
>>
>> uint16_t vq_idx = val;
>>
>>>
>>>> +            virtio_set_notification_data(vdev, vq_idx, val);
>>>> +        } else {
>>>> +            vq_idx = val;
>>>> +        }
>>>> +
>>>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>>>> +            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..a61f69b7fd 100644
>>>> --- a/hw/virtio/virtio.c
>>>> +++ b/hw/virtio/virtio.c
>>>> @@ -2052,6 +2052,19 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
>>>>        return 0;
>>>>    }
>>>>
>>>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data)
>>>> +{
>>>> +    VirtQueue *vq = &vdev->vq[i];
>>>> +
>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>>>> +        vq->last_avail_wrap_counter = (data >> 31) & 0x1;
>>>> +        vq->last_avail_idx = (data >> 16) & 0x7FFF;
>>>> +    } else {
>>>> +        vq->last_avail_idx = (data >> 16) & 0xFFFF;
>>>> +    }
>>>
>>> It should not set last_avail_idx, only shadow_avail_idx. Otherwise,
>>> QEMU can only see the descriptors placed after the notification.
>>>
>>> Or am I missing something?
>>>
>>> In that regard, I would call this function
>>> "virtqueue_set_shadow_avail_idx". But I'm very bad at naming :).
>>
>> Ah that's right. This would make Qemu skip processing descriptors that
>> might've been made available before the notification but after the
>> host's last check of last_avail_idx. In other words, ignoring available
>> descriptors that were placed before the notification but not yet
>> processed. Good catch, thank you!
>>
>> So, for the packed VQ layout, we'll still want to save the wrap counter
>> but for the shadow_avail_idx, right? E.g.
>>
>> 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);
>> }
>>
> 
> Right, I was not clear enough again :). You probably have guessed
> already but not modifying avail_wrap_counter would make QEMu to read
> the wrong index too.
> 
> Thanks!
> 

Got it, thanks!

>>>
>>> The rest looks good to me.
>>>
>>> Thanks!
>>>
>>>> +    vq->shadow_avail_idx = vq->last_avail_idx;
>>>> +}
>>>> +
>>>>    static enum virtio_device_endian virtio_default_endian(void)
>>>>    {
>>>>        if (target_words_bigendian()) {
>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>> index c8f72850bc..c92d8afc42 100644
>>>> --- a/include/hw/virtio/virtio.h
>>>> +++ b/include/hw/virtio/virtio.h
>>>> @@ -345,6 +345,7 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>>>>    void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>>>>    void virtio_update_irq(VirtIODevice *vdev);
>>>>    int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>>>> +void virtio_set_notification_data(VirtIODevice *vdev, uint16_t i, uint32_t data);
>>>>
>>>>    /* Base devices.  */
>>>>    typedef struct VirtIOBlkConf VirtIOBlkConf;
>>>> --
>>>> 2.39.3
>>>>
>>>
>>
> 

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

[Thomas Huth]

On 05/03/2024 04.21, Xinying Yu wrote:

> One more thing, I would ask how do  I get the full series patch? Do I copy 
> the RFC line by line from this link[1]?

For getting patches that you might have missed on the mailing list, I 
recommend lore.kernel.org :

 
https://lore.kernel.org/qemu-devel/20240301134330.4191007-1-jonah.palmer@oracle.com/

You can download mbox files there that you can apply locally with "git am".

  HTH,
   Thomas

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

[Eugenio Perez Martin]

On Tue, Mar 5, 2024 at 4:21 AM Xinying Yu <xinying.yu@nephogine.com> wrote:
>
> Of course,  I am glad to do.  And I need to clarify that our use case only support VIRTIO_F_NOTIFICATION_DATA  transport feature on DPDK vDPA framework which the backend type is NET_CLIENT_DRIVER_VHOST_USER and use user_feature_bits. So the new feature add on vdpa_feature_bits  will not under verified in our case.  Not sure this meets your expectations?

As long as the driver keeps using notification data it is not able to
tell the difference between one scenario or another, so yes.

> One more thing, I would ask how do  I get the full series patch? Do I copy the RFC line by line from this link[1]?
>

lore.kernel.org is a good alternative as Thomas mentioned. Another
good one IMO is b4, which allows you to download the series and apply
with "git am" too using "b4 mbox
<20240301134330.4191007-1-jonah.palmer@oracle.com>" (untested).

https://pypi.org/project/b4/

Thanks!

> Thanks,
> Xinying
>
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-03/msg00090.html
>
> ________________________________
> From: Eugenio Perez Martin <eperezma@redhat.com>
> Sent: Saturday, March 2, 2024 4:32 AM
> To: Wentao Jia <wentao.jia@nephogine.com>; Rick Zhong <zhaoyong.zhong@nephogine.com>; Xinying Yu <xinying.yu@nephogine.com>
> Cc: Jonah Palmer <jonah.palmer@oracle.com>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; mst@redhat.com <mst@redhat.com>; jasowang@redhat.com <jasowang@redhat.com>; si-wei.liu@oracle.com <si-wei.liu@oracle.com>; boris.ostrovsky@oracle.com <boris.ostrovsky@oracle.com>; raphael@enfabrica.net <raphael@enfabrica.net>; kwolf@redhat.com <kwolf@redhat.com>; hreitz@redhat.com <hreitz@redhat.com>; pasic@linux.ibm.com <pasic@linux.ibm.com>; borntraeger@linux.ibm.com <borntraeger@linux.ibm.com>; farman@linux.ibm.com <farman@linux.ibm.com>; thuth@redhat.com <thuth@redhat.com>; richard.henderson@linaro.org <richard.henderson@linaro.org>; david@redhat.com <david@redhat.com>; iii@linux.ibm.com <iii@linux.ibm.com>; cohuck@redhat.com <cohuck@redhat.com>; pbonzini@redhat.com <pbonzini@redhat.com>; fam@euphon.net <fam@euphon.net>; stefanha@redhat.com <stefanha@redhat.com>; qemu-block@nongnu.org <qemu-block@nongnu.org>; qemu-s390x@nongnu.org <qemu-s390x@nongnu.org>; virtio-fs@lists.linux.dev <virtio-fs@lists.linux.dev>
> Subject: Re: [RFC 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support
>
> Hi Wentao / Rick / Xinying Yu,
>
> Would it work for you to test this series on your use cases, so we
> make sure everything works as expected?
>
> Thanks!
>
> On Fri, Mar 1, 2024 at 2:44 PM 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: last_avail_idx
> >   - Lower 16 bits: virtqueue index
> >
> >  Packed VQ
> >   - Upper 16 bits: 1-bit wrap counter & 15-bit last_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.
> >
>
> Hi Wentao,
>

RE: [RFC 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support

[Xinying Yu]

> On Tue, Mar 5, 2024 at 4:21 AM Xinying Yu <xinying.yu@nephogine.com>
> wrote:
> >
> > Of course,  I am glad to do.  And I need to clarify that our use case only
> support VIRTIO_F_NOTIFICATION_DATA  transport feature on DPDK vDPA
> framework which the backend type is NET_CLIENT_DRIVER_VHOST_USER and
> use user_feature_bits. So the new feature add on vdpa_feature_bits  will not
> under verified in our case.  Not sure this meets your expectations?
> 
> As long as the driver keeps using notification data it is not able to tell the
> difference between one scenario or another, so yes.
> 

OK, I see.  And the test result is OK, the feature negotiation correctly and the use case passed.

> > One more thing, I would ask how do  I get the full series patch? Do I copy the
> RFC line by line from this link[1]?
> >
> 
> lore.kernel.org is a good alternative as Thomas mentioned. Another good one
> IMO is b4, which allows you to download the series and apply with "git am"
> too using "b4 mbox <20240301134330.4191007-1-
> jonah.palmer@oracle.com>" (untested).
> 
> https://pypi.org/project/b4/
> 
> Thanks!
> 

> For getting patches that you might have missed on the mailing list, I
> recommend lore.kernel.org :
> 
> 
> https://lore.kernel.org/qemu-devel/20240301134330.4191007-1-
> jonah.palmer@oracle.com/
> 
> You can download mbox files there that you can apply locally with "git am".
> 
>   HTH,
>    Thomas

Thanks to you and Thomas for the advice which works well.

> > Thanks,
> > Xinying
> >
> >
> > [1]
> > https://lists.nongnu.org/archive/html/qemu-devel/2024-
> 03/msg00090.html
> >
> > ________________________________
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Sent: Saturday, March 2, 2024 4:32 AM
> > To: Wentao Jia <wentao.jia@nephogine.com>; Rick Zhong
> > <zhaoyong.zhong@nephogine.com>; Xinying Yu
> <xinying.yu@nephogine.com>
> > Cc: Jonah Palmer <jonah.palmer@oracle.com>; qemu-devel@nongnu.org
> > <qemu-devel@nongnu.org>; mst@redhat.com <mst@redhat.com>;
> > jasowang@redhat.com <jasowang@redhat.com>; si-wei.liu@oracle.com
> > <si-wei.liu@oracle.com>; boris.ostrovsky@oracle.com
> > <boris.ostrovsky@oracle.com>; raphael@enfabrica.net
> > <raphael@enfabrica.net>; kwolf@redhat.com <kwolf@redhat.com>;
> > hreitz@redhat.com <hreitz@redhat.com>; pasic@linux.ibm.com
> > <pasic@linux.ibm.com>; borntraeger@linux.ibm.com
> > <borntraeger@linux.ibm.com>; farman@linux.ibm.com
> > <farman@linux.ibm.com>; thuth@redhat.com <thuth@redhat.com>;
> > richard.henderson@linaro.org <richard.henderson@linaro.org>;
> > david@redhat.com <david@redhat.com>; iii@linux.ibm.com
> > <iii@linux.ibm.com>; cohuck@redhat.com <cohuck@redhat.com>;
> > pbonzini@redhat.com <pbonzini@redhat.com>; fam@euphon.net
> > <fam@euphon.net>; stefanha@redhat.com <stefanha@redhat.com>;
> > qemu-block@nongnu.org <qemu-block@nongnu.org>; qemu-
> s390x@nongnu.org
> > <qemu-s390x@nongnu.org>; virtio-fs@lists.linux.dev
> > <virtio-fs@lists.linux.dev>
> > Subject: Re: [RFC 0/8] virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA
> > support
> >
> > Hi Wentao / Rick / Xinying Yu,
> >
> > Would it work for you to test this series on your use cases, so we
> > make sure everything works as expected?
> >
> > Thanks!
> >
> > On Fri, Mar 1, 2024 at 2:44 PM 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: last_avail_idx
> > >   - Lower 16 bits: virtqueue index
> > >
> > >  Packed VQ
> > >   - Upper 16 bits: 1-bit wrap counter & 15-bit last_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.
> > >
> >
> > Hi Wentao,
> >