Re: [PATCH v1 2/8] virtio-pci: Lock ioeventfd state with VIRTIO_F_NOTIFICATION_DATA

On Fri, Mar 8, 2024 at 6:01 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Mon, Mar 04, 2024 at 02:46:06PM -0500, Jonah Palmer wrote:
> > Prevent ioeventfd from being enabled/disabled when a virtio-pci
> > device has negotiated the VIRTIO_F_NOTIFICATION_DATA transport
> > feature.
> >
> > Due to ioeventfd not being able to carry the extra data associated with
> > this feature, the ioeventfd should be left in a disabled state for
> > emulated virtio-pci devices using this feature.
> >
> > Reviewed-by: Eugenio Pérez <[email protected]>
> > Signed-off-by: Jonah Palmer <[email protected]>
>
> I thought hard about this. I propose that for now,
> instead of disabling ioevetfd silently we error out unless
> user disabled it for us.
> WDYT?
>

Yes, error is a better plan than silently disabling it. In the
(unlikely?) case we are able to make notification data work with
eventfd in the future, it makes the change more evident.

>
> > ---
> >  hw/virtio/virtio-pci.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index d12edc567f..287b8f7720 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -417,13 +417,15 @@ static void virtio_ioport_write(void *opaque, uint32_t addr, uint32_t val)
> >          }
> >          break;
> >      case VIRTIO_PCI_STATUS:
> > -        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > +        if (!(val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >              virtio_pci_stop_ioeventfd(proxy);
> >          }
> >
> >          virtio_set_status(vdev, val & 0xFF);
> >
> > -        if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
> > +        if ((val & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > +            !virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA)) {
> >              virtio_pci_start_ioeventfd(proxy);
> >          }
> >
> > --
> > 2.39.3
>