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

[Michael S. Tsirkin]

On Wed, Mar 06, 2024 at 08:07:31AM +0100, Eugenio Perez Martin wrote:
> On Wed, Mar 6, 2024 at 6:34 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Mar 5, 2024 at 3:46 AM Jonah Palmer <jonah.palmer@oracle.com> wrote:
> > >
> > > The goal of these patches are to add support to a variety of virtio and
> > > vhost devices for the VIRTIO_F_NOTIFICATION_DATA transport feature. This
> > > feature indicates that a driver will pass extra data (instead of just a
> > > virtqueue's index) when notifying the corresponding device.
> > >
> > > The data passed in by the driver when this feature is enabled varies in
> > > format depending on if the device is using a split or packed virtqueue
> > > layout:
> > >
> > >  Split VQ
> > >   - Upper 16 bits: shadow_avail_idx
> > >   - Lower 16 bits: virtqueue index
> > >
> > >  Packed VQ
> > >   - Upper 16 bits: 1-bit wrap counter & 15-bit shadow_avail_idx
> > >   - Lower 16 bits: virtqueue index
> > >
> > > Also, due to the limitations of ioeventfd not being able to carry the
> > > extra provided by the driver, ioeventfd is left disabled for any devices
> > > using this feature.
> >
> > Is there any method to overcome this? This might help for vhost.
> >
> 
> As a half-baked idea, read(2)ing an eventfd descriptor returns an
> 8-byte integer already. The returned value of read depends on eventfd
> flags, but both have to do with the number of writes of the other end.
> 
> My proposal is to replace this value with the last value written by
> the guest, so we can extract the virtio notification data from there.
> The behavior of read is similar to not-EFD_SEMAPHORE, reading a value
> and then blocking if read again without writes. The behavior of KVM
> writes is different, as it is not a counter anymore.
> 
> Thanks!


I doubt you will be able to support this in ioeventfd...
But vhost does not really need the value at all.
So why mask out ioeventfd with vhost?
vhost-vdpa is probably the only one that might need it...



> > Thanks
> >
> > >
> > > A significant aspect of this effort has been to maintain compatibility
> > > across different backends. As such, the feature is offered by backend
> > > devices only when supported, with fallback mechanisms where backend
> > > support is absent.
> > >
> > > Jonah Palmer (8):
> > >   virtio/virtio-pci: Handle extra notification data
> > >   virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   virtio-mmio: Handle extra notification data
> > >   virtio-mmio: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   virtio-ccw: Handle extra notification data
> > >   virtio-ccw: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA
> > >   vhost/vhost-user: Add VIRTIO_F_NOTIFICATION_DATA to vhost feature bits
> > >   virtio: Add VIRTIO_F_NOTIFICATION_DATA property definition
> > >
> > >  hw/block/vhost-user-blk.c    |  1 +
> > >  hw/net/vhost_net.c           |  2 ++
> > >  hw/s390x/s390-virtio-ccw.c   | 16 ++++++++++++----
> > >  hw/s390x/virtio-ccw.c        |  6 ++++--
> > >  hw/scsi/vhost-scsi.c         |  1 +
> > >  hw/scsi/vhost-user-scsi.c    |  1 +
> > >  hw/virtio/vhost-user-fs.c    |  2 +-
> > >  hw/virtio/vhost-user-vsock.c |  1 +
> > >  hw/virtio/virtio-mmio.c      | 15 +++++++++++----
> > >  hw/virtio/virtio-pci.c       | 16 +++++++++++-----
> > >  hw/virtio/virtio.c           | 18 ++++++++++++++++++
> > >  include/hw/virtio/virtio.h   |  5 ++++-
> > >  net/vhost-vdpa.c             |  1 +
> > >  13 files changed, 68 insertions(+), 17 deletions(-)
> > >
> > > --
> > > 2.39.3
> > >
> >