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

[Lei Yang]

Hi Jonah

QE tested this series v1 with a tap device with vhost=off with
regression tests, everything works fine. And QE also add
"notification_data=true" to the qemu command line then got "1" when
performing the command [1] inside the guest.
[1] cut -c39 /sys/devices/pci0000:00/0000:00:01.3/0000:05:00.0/virtio1/features

Tested-by: Lei Yang <leiyang@redhat.com>

On Thu, Mar 7, 2024 at 7:18 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Wed, Mar 6, 2024 at 8:34 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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...
>
> I agree.
>
> > But vhost does not really need the value at all.
> > So why mask out ioeventfd with vhost?
>
> The interface should not be able to start with vhost-kernel because
> the feature is not offered by the vhost-kernel device. So ioeventfd is
> always enabled with vhost-kernel.
>
> Or do you mean we should allow it and let vhost-kernel fetch data from
> the avail ring as usual? I'm ok with that but then the guest can place
> any value to it, so the driver cannot be properly "validated by
> software" that way.
>
> > vhost-vdpa is probably the only one that might need it...
>
> Right, but vhost-vdpa already supports doorbell memory regions so I
> guess it has little use, isn't it?
>
> Thanks!
>
> >
> >
> >
> > > > 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
> > > > >
> > > >
> >
>
>