Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-15 Thread Eugenio Perez Martin 
On Thu, Mar 14, 2024 at 9:24 PM Jonah Palmer  wrote:
>
>
>
> On 3/14/24 3:05 PM, Eugenio Perez Martin wrote:
> > On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer  
> > wrote:
> >>
> >>
> >>
> >> On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
> >>> On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer  
> >>> wrote:
> 
> 
> 
>  On 3/13/24 11:01 PM, Jason Wang wrote:
> > On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  
> > 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 
> >> Reviewed-by: Eugenio Pérez 
> >> Signed-off-by: Jonah Palmer 
> >> ---
> >> 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(>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
> >

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-14 Thread Jonah Palmer 




On 3/14/24 3:05 PM, Eugenio Perez Martin wrote:

On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer  wrote:




On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:

On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer  wrote:




On 3/13/24 11:01 PM, Jason Wang wrote:

On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  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 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Jonah Palmer 
---
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(>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 = >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

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-14 Thread Eugenio Perez Martin 
On Thu, Mar 14, 2024 at 5:06 PM Jonah Palmer  wrote:
>
>
>
> On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:
> > On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer  
> > wrote:
> >>
> >>
> >>
> >> On 3/13/24 11:01 PM, Jason Wang wrote:
> >>> On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  
> >>> 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 
>  Reviewed-by: Eugenio Pérez 
>  Signed-off-by: Jonah Palmer 
>  ---
> 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(>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 = >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

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-14 Thread Jonah Palmer 




On 3/14/24 10:55 AM, Eugenio Perez Martin wrote:

On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer  wrote:




On 3/13/24 11:01 PM, Jason Wang wrote:

On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  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 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Jonah Palmer 
---
   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(>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 = >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

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-14 Thread Eugenio Perez Martin 
On Thu, Mar 14, 2024 at 1:16 PM Jonah Palmer  wrote:
>
>
>
> On 3/13/24 11:01 PM, Jason Wang wrote:
> > On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  
> > 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 
> >> Reviewed-by: Eugenio Pérez 
> >> Signed-off-by: Jonah Palmer 
> >> ---
> >>   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(>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 = >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
> >>
> >
>

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-14 Thread Jonah Palmer 




On 3/13/24 11:01 PM, Jason Wang wrote:

On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  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 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Jonah Palmer 
---
  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(>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 = >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

Re: [PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

2024-03-13 Thread Jason Wang 
On Wed, Mar 13, 2024 at 7:55 PM Jonah Palmer  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 
> Reviewed-by: Eugenio Pérez 
> Signed-off-by: Jonah Palmer 
> ---
>  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(>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 = >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
>

[PATCH v2 1/6] virtio/virtio-pci: Handle extra notification data

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

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

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

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

Tested-by: Lei Yang 
Reviewed-by: Eugenio Pérez 
Signed-off-by: Jonah Palmer 
---
 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(>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 = >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