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

[Jonah Palmer <jonah.palmer@oracle.com>]

On 3/8/24 8:28 AM, Lei Yang wrote:
> 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>
> 

Thank you for double-checking this series for me Lei! I appreciate it :)

Jonah

> 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
>>>>>>
>>>>>
>>>
>>
>>
>