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

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
>

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

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
>

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

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

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