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

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

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

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

Tested-by: Lei Yang <leiyang@redhat.com>
Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
---
 hw/virtio/virtio-pci.c     | 10 +++++++---
 hw/virtio/virtio.c         | 18 ++++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 3 files changed, 26 insertions(+), 3 deletions(-)

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

On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
> Add support to virtio-pci devices for handling the extra data sent
> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> transport feature has been negotiated.
>
> The extra data that's passed to the virtio-pci device when this
> feature is enabled varies depending on the device's virtqueue
> layout.
>
> In a split virtqueue layout, this data includes:
>  - upper 16 bits: shadow_avail_idx
>  - lower 16 bits: virtqueue index
>
> In a packed virtqueue layout, this data includes:
>  - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>  - lower 16 bits: virtqueue index
>
> Tested-by: Lei Yang <leiyang@redhat.com>
> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
>  hw/virtio/virtio-pci.c     | 10 +++++++---
>  hw/virtio/virtio.c         | 18 ++++++++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  3 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index cb6940fc0e..0f5c3c3b2f 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VirtIOPCIProxy *proxy = opaque;
>      VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> -    uint16_t vector;
> +    uint16_t vector, vq_idx;
>      hwaddr pa;
>
>      switch (addr) {
> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>              vdev->queue_sel = val;
>          break;
>      case VIRTIO_PCI_QUEUE_NOTIFY:
> -        if (val < VIRTIO_QUEUE_MAX) {
> -            virtio_queue_notify(vdev, val);
> +        vq_idx = val;
> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> +                virtio_queue_set_shadow_avail_data(vdev, val);
> +            }
> +            virtio_queue_notify(vdev, vq_idx);
>          }
>          break;
>      case VIRTIO_PCI_STATUS:
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index d229755eae..bcb9e09df0 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>      }
>  }
>
> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
> +{
> +    /* Lower 16 bits is the virtqueue index */
> +    uint16_t i = data;
> +    VirtQueue *vq = &vdev->vq[i];
> +
> +    if (!vq->vring.desc) {
> +        return;
> +    }
> +
> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
> +    } else {
> +        vq->shadow_avail_idx = (data >> 16);

Do we need to do a sanity check for this value?

Thanks

> +    }
> +}
> +
>  static void virtio_queue_notify_vq(VirtQueue *vq)
>  {
>      if (vq->vring.desc && vq->handle_output) {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c8f72850bc..53915947a7 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>  void virtio_init_region_cache(VirtIODevice *vdev, int n);
>  void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>  void virtio_queue_notify(VirtIODevice *vdev, int n);
> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>  void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>  int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> --
> 2.39.3
>

On 3/13/24 11:01 PM, Jason Wang wrote:
> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>> Add support to virtio-pci devices for handling the extra data sent
>> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
>> transport feature has been negotiated.
>>
>> The extra data that's passed to the virtio-pci device when this
>> feature is enabled varies depending on the device's virtqueue
>> layout.
>>
>> In a split virtqueue layout, this data includes:
>>   - upper 16 bits: shadow_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> In a packed virtqueue layout, this data includes:
>>   - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>>   - lower 16 bits: virtqueue index
>>
>> Tested-by: Lei Yang <leiyang@redhat.com>
>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>> ---
>>   hw/virtio/virtio-pci.c     | 10 +++++++---
>>   hw/virtio/virtio.c         | 18 ++++++++++++++++++
>>   include/hw/virtio/virtio.h |  1 +
>>   3 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index cb6940fc0e..0f5c3c3b2f 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>   {
>>       VirtIOPCIProxy *proxy = opaque;
>>       VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>> -    uint16_t vector;
>> +    uint16_t vector, vq_idx;
>>       hwaddr pa;
>>
>>       switch (addr) {
>> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>               vdev->queue_sel = val;
>>           break;
>>       case VIRTIO_PCI_QUEUE_NOTIFY:
>> -        if (val < VIRTIO_QUEUE_MAX) {
>> -            virtio_queue_notify(vdev, val);
>> +        vq_idx = val;
>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>> +                virtio_queue_set_shadow_avail_data(vdev, val);
>> +            }
>> +            virtio_queue_notify(vdev, vq_idx);
>>           }
>>           break;
>>       case VIRTIO_PCI_STATUS:
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index d229755eae..bcb9e09df0 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>>       }
>>   }
>>
>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
>> +{
>> +    /* Lower 16 bits is the virtqueue index */
>> +    uint16_t i = data;
>> +    VirtQueue *vq = &vdev->vq[i];
>> +
>> +    if (!vq->vring.desc) {
>> +        return;
>> +    }
>> +
>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
>> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
>> +    } else {
>> +        vq->shadow_avail_idx = (data >> 16);
> 
> Do we need to do a sanity check for this value?
> 
> Thanks
> 

It can't hurt, right? What kind of check did you have in mind?

if (vq->shadow_avail_idx >= vq->vring.num)

Or something else?

>> +    }
>> +}
>> +
>>   static void virtio_queue_notify_vq(VirtQueue *vq)
>>   {
>>       if (vq->vring.desc && vq->handle_output) {
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index c8f72850bc..53915947a7 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>>   void virtio_init_region_cache(VirtIODevice *vdev, int n);
>>   void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>>   void virtio_queue_notify(VirtIODevice *vdev, int n);
>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
>>   uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>>   void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>>   int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>> --
>> 2.39.3
>>
> 

On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
>
>
> On 3/13/24 11:01 PM, Jason Wang wrote:
> > On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>
> >> Add support to virtio-pci devices for handling the extra data sent
> >> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> >> transport feature has been negotiated.
> >>
> >> The extra data that's passed to the virtio-pci device when this
> >> feature is enabled varies depending on the device's virtqueue
> >> layout.
> >>
> >> In a split virtqueue layout, this data includes:
> >>   - upper 16 bits: shadow_avail_idx
> >>   - lower 16 bits: virtqueue index
> >>
> >> In a packed virtqueue layout, this data includes:
> >>   - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> >>   - lower 16 bits: virtqueue index
> >>
> >> Tested-by: Lei Yang <leiyang@redhat.com>
> >> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >> ---
> >>   hw/virtio/virtio-pci.c     | 10 +++++++---
> >>   hw/virtio/virtio.c         | 18 ++++++++++++++++++
> >>   include/hw/virtio/virtio.h |  1 +
> >>   3 files changed, 26 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index cb6940fc0e..0f5c3c3b2f 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>   {
> >>       VirtIOPCIProxy *proxy = opaque;
> >>       VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >> -    uint16_t vector;
> >> +    uint16_t vector, vq_idx;
> >>       hwaddr pa;
> >>
> >>       switch (addr) {
> >> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>               vdev->queue_sel = val;
> >>           break;
> >>       case VIRTIO_PCI_QUEUE_NOTIFY:
> >> -        if (val < VIRTIO_QUEUE_MAX) {
> >> -            virtio_queue_notify(vdev, val);
> >> +        vq_idx = val;
> >> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> >> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >> +                virtio_queue_set_shadow_avail_data(vdev, val);
> >> +            }
> >> +            virtio_queue_notify(vdev, vq_idx);
> >>           }
> >>           break;
> >>       case VIRTIO_PCI_STATUS:
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index d229755eae..bcb9e09df0 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
> >>       }
> >>   }
> >>
> >> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)

Maybe I didn't explain well, but I think it is better to pass directly
idx to a VirtQueue *. That way only the caller needs to check for a
valid vq idx, and (my understanding is) the virtio.c interface is
migrating to VirtQueue * use anyway.

> >> +{
> >> +    /* Lower 16 bits is the virtqueue index */
> >> +    uint16_t i = data;
> >> +    VirtQueue *vq = &vdev->vq[i];
> >> +
> >> +    if (!vq->vring.desc) {
> >> +        return;
> >> +    }
> >> +
> >> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
> >> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
> >> +    } else {
> >> +        vq->shadow_avail_idx = (data >> 16);
> >
> > Do we need to do a sanity check for this value?
> >
> > Thanks
> >
>
> It can't hurt, right? What kind of check did you have in mind?
>
> if (vq->shadow_avail_idx >= vq->vring.num)
>

I'm a little bit lost too. shadow_avail_idx can take all uint16_t
values. Maybe you meant checking for a valid vq index, Jason?

Thanks!

> Or something else?
>
> >> +    }
> >> +}
> >> +
> >>   static void virtio_queue_notify_vq(VirtQueue *vq)
> >>   {
> >>       if (vq->vring.desc && vq->handle_output) {
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index c8f72850bc..53915947a7 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
> >>   void virtio_init_region_cache(VirtIODevice *vdev, int n);
> >>   void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
> >>   void virtio_queue_notify(VirtIODevice *vdev, int n);
> >> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
> >>   uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
> >>   void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
> >>   int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> >> --
> >> 2.39.3
> >>
> >
>

On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
> On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>>
>>
>> On 3/13/24 11:01 PM, Jason Wang wrote:
>>> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>>>
>>>> Add support to virtio-pci devices for handling the extra data sent
>>>> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
>>>> transport feature has been negotiated.
>>>>
>>>> The extra data that's passed to the virtio-pci device when this
>>>> feature is enabled varies depending on the device's virtqueue
>>>> layout.
>>>>
>>>> In a split virtqueue layout, this data includes:
>>>>    - upper 16 bits: shadow_avail_idx
>>>>    - lower 16 bits: virtqueue index
>>>>
>>>> In a packed virtqueue layout, this data includes:
>>>>    - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>>>>    - lower 16 bits: virtqueue index
>>>>
>>>> Tested-by: Lei Yang <leiyang@redhat.com>
>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>> ---
>>>>    hw/virtio/virtio-pci.c     | 10 +++++++---
>>>>    hw/virtio/virtio.c         | 18 ++++++++++++++++++
>>>>    include/hw/virtio/virtio.h |  1 +
>>>>    3 files changed, 26 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>> index cb6940fc0e..0f5c3c3b2f 100644
>>>> --- a/hw/virtio/virtio-pci.c
>>>> +++ b/hw/virtio/virtio-pci.c
>>>> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>    {
>>>>        VirtIOPCIProxy *proxy = opaque;
>>>>        VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>>> -    uint16_t vector;
>>>> +    uint16_t vector, vq_idx;
>>>>        hwaddr pa;
>>>>
>>>>        switch (addr) {
>>>> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>                vdev->queue_sel = val;
>>>>            break;
>>>>        case VIRTIO_PCI_QUEUE_NOTIFY:
>>>> -        if (val < VIRTIO_QUEUE_MAX) {
>>>> -            virtio_queue_notify(vdev, val);
>>>> +        vq_idx = val;
>>>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>>>> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>> +                virtio_queue_set_shadow_avail_data(vdev, val);
>>>> +            }
>>>> +            virtio_queue_notify(vdev, vq_idx);
>>>>            }
>>>>            break;
>>>>        case VIRTIO_PCI_STATUS:
>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>> index d229755eae..bcb9e09df0 100644
>>>> --- a/hw/virtio/virtio.c
>>>> +++ b/hw/virtio/virtio.c
>>>> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>>>>        }
>>>>    }
>>>>
>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
> 
> Maybe I didn't explain well, but I think it is better to pass directly
> idx to a VirtQueue *. That way only the caller needs to check for a
> valid vq idx, and (my understanding is) the virtio.c interface is
> migrating to VirtQueue * use anyway.
> 

Oh, are you saying to just pass in a VirtQueue *vq instead of 
VirtIODevice *vdev and get rid of the vq->vring.desc check in the function?

>>>> +{
>>>> +    /* Lower 16 bits is the virtqueue index */
>>>> +    uint16_t i = data;
>>>> +    VirtQueue *vq = &vdev->vq[i];
>>>> +
>>>> +    if (!vq->vring.desc) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>>>> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
>>>> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
>>>> +    } else {
>>>> +        vq->shadow_avail_idx = (data >> 16);
>>>
>>> Do we need to do a sanity check for this value?
>>>
>>> Thanks
>>>
>>
>> It can't hurt, right? What kind of check did you have in mind?
>>
>> if (vq->shadow_avail_idx >= vq->vring.num)
>>
> 
> I'm a little bit lost too. shadow_avail_idx can take all uint16_t
> values. Maybe you meant checking for a valid vq index, Jason?
> 
> Thanks!
> 
>> Or something else?
>>
>>>> +    }
>>>> +}
>>>> +
>>>>    static void virtio_queue_notify_vq(VirtQueue *vq)
>>>>    {
>>>>        if (vq->vring.desc && vq->handle_output) {
>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>> index c8f72850bc..53915947a7 100644
>>>> --- a/include/hw/virtio/virtio.h
>>>> +++ b/include/hw/virtio/virtio.h
>>>> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>>>>    void virtio_init_region_cache(VirtIODevice *vdev, int n);
>>>>    void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>>>>    void virtio_queue_notify(VirtIODevice *vdev, int n);
>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
>>>>    uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>>>>    void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>>>>    int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>>>> --
>>>> 2.39.3
>>>>
>>>
>>
> 

On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
>
>
> On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
> > On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>
> >>
> >>
> >> On 3/13/24 11:01 PM, Jason Wang wrote:
> >>> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>>>
> >>>> Add support to virtio-pci devices for handling the extra data sent
> >>>> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> >>>> transport feature has been negotiated.
> >>>>
> >>>> The extra data that's passed to the virtio-pci device when this
> >>>> feature is enabled varies depending on the device's virtqueue
> >>>> layout.
> >>>>
> >>>> In a split virtqueue layout, this data includes:
> >>>>    - upper 16 bits: shadow_avail_idx
> >>>>    - lower 16 bits: virtqueue index
> >>>>
> >>>> In a packed virtqueue layout, this data includes:
> >>>>    - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> >>>>    - lower 16 bits: virtqueue index
> >>>>
> >>>> Tested-by: Lei Yang <leiyang@redhat.com>
> >>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >>>> ---
> >>>>    hw/virtio/virtio-pci.c     | 10 +++++++---
> >>>>    hw/virtio/virtio.c         | 18 ++++++++++++++++++
> >>>>    include/hw/virtio/virtio.h |  1 +
> >>>>    3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>> index cb6940fc0e..0f5c3c3b2f 100644
> >>>> --- a/hw/virtio/virtio-pci.c
> >>>> +++ b/hw/virtio/virtio-pci.c
> >>>> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>>>    {
> >>>>        VirtIOPCIProxy *proxy = opaque;
> >>>>        VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >>>> -    uint16_t vector;
> >>>> +    uint16_t vector, vq_idx;
> >>>>        hwaddr pa;
> >>>>
> >>>>        switch (addr) {
> >>>> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>>>                vdev->queue_sel = val;
> >>>>            break;
> >>>>        case VIRTIO_PCI_QUEUE_NOTIFY:
> >>>> -        if (val < VIRTIO_QUEUE_MAX) {
> >>>> -            virtio_queue_notify(vdev, val);
> >>>> +        vq_idx = val;
> >>>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> >>>> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >>>> +                virtio_queue_set_shadow_avail_data(vdev, val);
> >>>> +            }
> >>>> +            virtio_queue_notify(vdev, vq_idx);
> >>>>            }
> >>>>            break;
> >>>>        case VIRTIO_PCI_STATUS:
> >>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>> index d229755eae..bcb9e09df0 100644
> >>>> --- a/hw/virtio/virtio.c
> >>>> +++ b/hw/virtio/virtio.c
> >>>> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
> >>>>        }
> >>>>    }
> >>>>
> >>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
> >
> > Maybe I didn't explain well, but I think it is better to pass directly
> > idx to a VirtQueue *. That way only the caller needs to check for a
> > valid vq idx, and (my understanding is) the virtio.c interface is
> > migrating to VirtQueue * use anyway.
> >
>
> Oh, are you saying to just pass in a VirtQueue *vq instead of
> VirtIODevice *vdev and get rid of the vq->vring.desc check in the function?
>

No, that needs to be kept. I meant the access to vdev->vq[i] without
checking for a valid i.

You can get the VirtQueue in the caller with virtio_get_queue. Which
also does not check for a valid index, but that way is clearer the
caller needs to check it.

As a side note, the check for desc != 0 is widespread in QEMU but the
driver may use 0 address for desc, so it's not 100% valid. But to
change that now requires a deeper change out of the scope of this
series, so let's keep it for now :).

Thanks!

> >>>> +{
> >>>> +    /* Lower 16 bits is the virtqueue index */
> >>>> +    uint16_t i = data;
> >>>> +    VirtQueue *vq = &vdev->vq[i];
> >>>> +
> >>>> +    if (!vq->vring.desc) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >>>> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
> >>>> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
> >>>> +    } else {
> >>>> +        vq->shadow_avail_idx = (data >> 16);
> >>>
> >>> Do we need to do a sanity check for this value?
> >>>
> >>> Thanks
> >>>
> >>
> >> It can't hurt, right? What kind of check did you have in mind?
> >>
> >> if (vq->shadow_avail_idx >= vq->vring.num)
> >>
> >
> > I'm a little bit lost too. shadow_avail_idx can take all uint16_t
> > values. Maybe you meant checking for a valid vq index, Jason?
> >
> > Thanks!
> >
> >> Or something else?
> >>
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>    static void virtio_queue_notify_vq(VirtQueue *vq)
> >>>>    {
> >>>>        if (vq->vring.desc && vq->handle_output) {
> >>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>> index c8f72850bc..53915947a7 100644
> >>>> --- a/include/hw/virtio/virtio.h
> >>>> +++ b/include/hw/virtio/virtio.h
> >>>> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
> >>>>    void virtio_init_region_cache(VirtIODevice *vdev, int n);
> >>>>    void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
> >>>>    void virtio_queue_notify(VirtIODevice *vdev, int n);
> >>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
> >>>>    uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
> >>>>    void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
> >>>>    int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> >>>> --
> >>>> 2.39.3
> >>>>
> >>>
> >>
> >
>

On 3/14/24 3:05 PM, Eugenio Perez Martin wrote:
> On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>
>>
>>
>> On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
>>> On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/13/24 11:01 PM, Jason Wang wrote:
>>>>> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>>>>>>
>>>>>> Add support to virtio-pci devices for handling the extra data sent
>>>>>> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
>>>>>> transport feature has been negotiated.
>>>>>>
>>>>>> The extra data that's passed to the virtio-pci device when this
>>>>>> feature is enabled varies depending on the device's virtqueue
>>>>>> layout.
>>>>>>
>>>>>> In a split virtqueue layout, this data includes:
>>>>>>     - upper 16 bits: shadow_avail_idx
>>>>>>     - lower 16 bits: virtqueue index
>>>>>>
>>>>>> In a packed virtqueue layout, this data includes:
>>>>>>     - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
>>>>>>     - lower 16 bits: virtqueue index
>>>>>>
>>>>>> Tested-by: Lei Yang <leiyang@redhat.com>
>>>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
>>>>>> ---
>>>>>>     hw/virtio/virtio-pci.c     | 10 +++++++---
>>>>>>     hw/virtio/virtio.c         | 18 ++++++++++++++++++
>>>>>>     include/hw/virtio/virtio.h |  1 +
>>>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>>>> index cb6940fc0e..0f5c3c3b2f 100644
>>>>>> --- a/hw/virtio/virtio-pci.c
>>>>>> +++ b/hw/virtio/virtio-pci.c
>>>>>> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>>     {
>>>>>>         VirtIOPCIProxy *proxy = opaque;
>>>>>>         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
>>>>>> -    uint16_t vector;
>>>>>> +    uint16_t vector, vq_idx;
>>>>>>         hwaddr pa;
>>>>>>
>>>>>>         switch (addr) {
>>>>>> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>>>>>>                 vdev->queue_sel = val;
>>>>>>             break;
>>>>>>         case VIRTIO_PCI_QUEUE_NOTIFY:
>>>>>> -        if (val < VIRTIO_QUEUE_MAX) {
>>>>>> -            virtio_queue_notify(vdev, val);
>>>>>> +        vq_idx = val;
>>>>>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
>>>>>> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
>>>>>> +                virtio_queue_set_shadow_avail_data(vdev, val);
>>>>>> +            }
>>>>>> +            virtio_queue_notify(vdev, vq_idx);
>>>>>>             }
>>>>>>             break;
>>>>>>         case VIRTIO_PCI_STATUS:
>>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>>>>> index d229755eae..bcb9e09df0 100644
>>>>>> --- a/hw/virtio/virtio.c
>>>>>> +++ b/hw/virtio/virtio.c
>>>>>> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
>>>>>>         }
>>>>>>     }
>>>>>>
>>>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
>>>
>>> Maybe I didn't explain well, but I think it is better to pass directly
>>> idx to a VirtQueue *. That way only the caller needs to check for a
>>> valid vq idx, and (my understanding is) the virtio.c interface is
>>> migrating to VirtQueue * use anyway.
>>>
>>
>> Oh, are you saying to just pass in a VirtQueue *vq instead of
>> VirtIODevice *vdev and get rid of the vq->vring.desc check in the function?
>>
> 
> No, that needs to be kept. I meant the access to vdev->vq[i] without
> checking for a valid i.
> 

Ahh okay I see what you mean. But I thought the following was checking 
for a valid VQ index:

if (vq_idx < VIRTIO_QUEUE_MAX)

Of course the virtio device may not have up to VIRTIO_QUEUE_MAX 
virtqueues, so maybe we should be checking for validity like this?

if (vdev->vq[i].vring.num == 0)

Or was there something else you had in mind? Apologies for the confusion.

> You can get the VirtQueue in the caller with virtio_get_queue. Which
> also does not check for a valid index, but that way is clearer the
> caller needs to check it.
> 

Roger, I'll use this instead for clarity.

> As a side note, the check for desc != 0 is widespread in QEMU but the
> driver may use 0 address for desc, so it's not 100% valid. But to
> change that now requires a deeper change out of the scope of this
> series, so let's keep it for now :).
> 
> Thanks! >

I'll add it to the todo list =]

>>>>>> +{
>>>>>> +    /* Lower 16 bits is the virtqueue index */
>>>>>> +    uint16_t i = data;
>>>>>> +    VirtQueue *vq = &vdev->vq[i];
>>>>>> +
>>>>>> +    if (!vq->vring.desc) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
>>>>>> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
>>>>>> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
>>>>>> +    } else {
>>>>>> +        vq->shadow_avail_idx = (data >> 16);
>>>>>
>>>>> Do we need to do a sanity check for this value?
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>> It can't hurt, right? What kind of check did you have in mind?
>>>>
>>>> if (vq->shadow_avail_idx >= vq->vring.num)
>>>>
>>>
>>> I'm a little bit lost too. shadow_avail_idx can take all uint16_t
>>> values. Maybe you meant checking for a valid vq index, Jason?
>>>
>>> Thanks!
>>>
>>>> Or something else?
>>>>
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>     static void virtio_queue_notify_vq(VirtQueue *vq)
>>>>>>     {
>>>>>>         if (vq->vring.desc && vq->handle_output) {
>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>> index c8f72850bc..53915947a7 100644
>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
>>>>>>     void virtio_init_region_cache(VirtIODevice *vdev, int n);
>>>>>>     void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
>>>>>>     void virtio_queue_notify(VirtIODevice *vdev, int n);
>>>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
>>>>>>     uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
>>>>>>     void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
>>>>>>     int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>>>>>> --
>>>>>> 2.39.3
>>>>>>
>>>>>
>>>>
>>>
>>
> 

On Thu, Mar 14, 2024 at 9:24 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
>
>
>
> On 3/14/24 3:05 PM, Eugenio Perez Martin wrote:
> > On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>
> >>
> >>
> >> On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
> >>> On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 3/13/24 11:01 PM, Jason Wang wrote:
> >>>>> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> >>>>>>
> >>>>>> Add support to virtio-pci devices for handling the extra data sent
> >>>>>> from the driver to the device when the VIRTIO_F_NOTIFICATION_DATA
> >>>>>> transport feature has been negotiated.
> >>>>>>
> >>>>>> The extra data that's passed to the virtio-pci device when this
> >>>>>> feature is enabled varies depending on the device's virtqueue
> >>>>>> layout.
> >>>>>>
> >>>>>> In a split virtqueue layout, this data includes:
> >>>>>>     - upper 16 bits: shadow_avail_idx
> >>>>>>     - lower 16 bits: virtqueue index
> >>>>>>
> >>>>>> In a packed virtqueue layout, this data includes:
> >>>>>>     - upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> >>>>>>     - lower 16 bits: virtqueue index
> >>>>>>
> >>>>>> Tested-by: Lei Yang <leiyang@redhat.com>
> >>>>>> Reviewed-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> >>>>>> ---
> >>>>>>     hw/virtio/virtio-pci.c     | 10 +++++++---
> >>>>>>     hw/virtio/virtio.c         | 18 ++++++++++++++++++
> >>>>>>     include/hw/virtio/virtio.h |  1 +
> >>>>>>     3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>>>> index cb6940fc0e..0f5c3c3b2f 100644
> >>>>>> --- a/hw/virtio/virtio-pci.c
> >>>>>> +++ b/hw/virtio/virtio-pci.c
> >>>>>> @@ -384,7 +384,7 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>>>>>     {
> >>>>>>         VirtIOPCIProxy *proxy = opaque;
> >>>>>>         VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> >>>>>> -    uint16_t vector;
> >>>>>> +    uint16_t vector, vq_idx;
> >>>>>>         hwaddr pa;
> >>>>>>
> >>>>>>         switch (addr) {
> >>>>>> @@ -408,8 +408,12 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >>>>>>                 vdev->queue_sel = val;
> >>>>>>             break;
> >>>>>>         case VIRTIO_PCI_QUEUE_NOTIFY:
> >>>>>> -        if (val < VIRTIO_QUEUE_MAX) {
> >>>>>> -            virtio_queue_notify(vdev, val);
> >>>>>> +        vq_idx = val;
> >>>>>> +        if (vq_idx < VIRTIO_QUEUE_MAX) {
> >>>>>> +            if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >>>>>> +                virtio_queue_set_shadow_avail_data(vdev, val);
> >>>>>> +            }
> >>>>>> +            virtio_queue_notify(vdev, vq_idx);
> >>>>>>             }
> >>>>>>             break;
> >>>>>>         case VIRTIO_PCI_STATUS:
> >>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >>>>>> index d229755eae..bcb9e09df0 100644
> >>>>>> --- a/hw/virtio/virtio.c
> >>>>>> +++ b/hw/virtio/virtio.c
> >>>>>> @@ -2255,6 +2255,24 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align)
> >>>>>>         }
> >>>>>>     }
> >>>>>>
> >>>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data)
> >>>
> >>> Maybe I didn't explain well, but I think it is better to pass directly
> >>> idx to a VirtQueue *. That way only the caller needs to check for a
> >>> valid vq idx, and (my understanding is) the virtio.c interface is
> >>> migrating to VirtQueue * use anyway.
> >>>
> >>
> >> Oh, are you saying to just pass in a VirtQueue *vq instead of
> >> VirtIODevice *vdev and get rid of the vq->vring.desc check in the function?
> >>
> >
> > No, that needs to be kept. I meant the access to vdev->vq[i] without
> > checking for a valid i.
> >
>
> Ahh okay I see what you mean. But I thought the following was checking
> for a valid VQ index:
>
> if (vq_idx < VIRTIO_QUEUE_MAX)
>

Right, but then the (potentially multiple) callers are responsible to
check for that. If we accept a VirtQueue *, it is assumed it is valid
already.

> Of course the virtio device may not have up to VIRTIO_QUEUE_MAX
> virtqueues, so maybe we should be checking for validity like this?
>
> if (vdev->vq[i].vring.num == 0)
>

Actually yes, if you're going to send a new version I think checking
against num is better. Good find!

> Or was there something else you had in mind? Apologies for the confusion.
>

No worries, virtio.c is full of checks like that :).

Thanks!

> > You can get the VirtQueue in the caller with virtio_get_queue. Which
> > also does not check for a valid index, but that way is clearer the
> > caller needs to check it.
> >
>
> Roger, I'll use this instead for clarity.
>
> > As a side note, the check for desc != 0 is widespread in QEMU but the
> > driver may use 0 address for desc, so it's not 100% valid. But to
> > change that now requires a deeper change out of the scope of this
> > series, so let's keep it for now :).
> >
> > Thanks! >
>
> I'll add it to the todo list =]
>
> >>>>>> +{
> >>>>>> +    /* Lower 16 bits is the virtqueue index */
> >>>>>> +    uint16_t i = data;
> >>>>>> +    VirtQueue *vq = &vdev->vq[i];
> >>>>>> +
> >>>>>> +    if (!vq->vring.desc) {
> >>>>>> +        return;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    if (virtio_vdev_has_feature(vdev, VIRTIO_F_RING_PACKED)) {
> >>>>>> +        vq->shadow_avail_wrap_counter = (data >> 31) & 0x1;
> >>>>>> +        vq->shadow_avail_idx = (data >> 16) & 0x7FFF;
> >>>>>> +    } else {
> >>>>>> +        vq->shadow_avail_idx = (data >> 16);
> >>>>>
> >>>>> Do we need to do a sanity check for this value?
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>
> >>>> It can't hurt, right? What kind of check did you have in mind?
> >>>>
> >>>> if (vq->shadow_avail_idx >= vq->vring.num)
> >>>>
> >>>
> >>> I'm a little bit lost too. shadow_avail_idx can take all uint16_t
> >>> values. Maybe you meant checking for a valid vq index, Jason?
> >>>
> >>> Thanks!
> >>>
> >>>> Or something else?
> >>>>
> >>>>>> +    }
> >>>>>> +}
> >>>>>> +
> >>>>>>     static void virtio_queue_notify_vq(VirtQueue *vq)
> >>>>>>     {
> >>>>>>         if (vq->vring.desc && vq->handle_output) {
> >>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >>>>>> index c8f72850bc..53915947a7 100644
> >>>>>> --- a/include/hw/virtio/virtio.h
> >>>>>> +++ b/include/hw/virtio/virtio.h
> >>>>>> @@ -335,6 +335,7 @@ void virtio_queue_update_rings(VirtIODevice *vdev, int n);
> >>>>>>     void virtio_init_region_cache(VirtIODevice *vdev, int n);
> >>>>>>     void virtio_queue_set_align(VirtIODevice *vdev, int n, int align);
> >>>>>>     void virtio_queue_notify(VirtIODevice *vdev, int n);
> >>>>>> +void virtio_queue_set_shadow_avail_data(VirtIODevice *vdev, uint32_t data);
> >>>>>>     uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
> >>>>>>     void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
> >>>>>>     int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
> >>>>>> --
> >>>>>> 2.39.3
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>