| Message ID | 20240313115412.3334962-2-jonah.palmer@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio,vhost: Add VIRTIO_F_NOTIFICATION_DATA support | expand |
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 > >>>>>> > >>>>> > >>>> > >>> > >> > > >
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,