On Wed, Mar 13, 2024 at 12:55 PM Jonah Palmer <jonah.pal...@oracle.com> wrote: > > Prevent the realization of a virtio device that attempts to use the > VIRTIO_F_NOTIFICATION_DATA transport feature without disabling > ioeventfd. > > Due to ioeventfd not being able to carry the extra data associated with > this feature, having both enabled is a functional mismatch and therefore > Qemu should not continue the device's realization process. > > Although the device does not yet know if the feature will be > successfully negotiated, many devices using this feature wont actually > work without this extra data and would fail FEATURES_OK anyway. > > If ioeventfd is able to work with the extra notification data in the > future, this compatibility check can be removed. > > Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com> > --- > hw/virtio/virtio.c | 22 ++++++++++++++++++++++ > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 24 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index bcb9e09df0..d0a433b465 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2971,6 +2971,20 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t > val) > return ret; > } > > +void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > + Error **errp) > +{ > + VirtioBusState *bus = VIRTIO_BUS(qdev_get_parent_bus(DEVICE(vdev))); > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + > + if (virtio_host_has_feature(vdev, VIRTIO_F_NOTIFICATION_DATA) && > + k->ioeventfd_enabled(proxy)) { > + error_setg(errp, > + "notification_data=on without ioeventfd=off is not > supported"); > + } > +} > + > size_t virtio_get_config_size(const VirtIOConfigSizeParams *params, > uint64_t host_features) > { > @@ -3731,6 +3745,14 @@ static void virtio_device_realize(DeviceState *dev, > Error **errp) > } > } > > + /* Devices should not use both ioeventfd and notification data feature */ > + virtio_device_check_notification_compatibility(vdev, &err); > + if (err != NULL) { > + error_propagate(errp, err); > + vdc->unrealize(dev); > + return; > + } > + > virtio_bus_device_plugged(vdev, &err); > if (err != NULL) { > error_propagate(errp, err); > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 53915947a7..e0325d84d0 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -346,6 +346,8 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t > queue_index); > void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index); > void virtio_update_irq(VirtIODevice *vdev); > int virtio_set_features(VirtIODevice *vdev, uint64_t val); > +void virtio_device_check_notification_compatibility(VirtIODevice *vdev, > + Error **errp);
Why not make it static? > > /* Base devices. */ > typedef struct VirtIOBlkConf VirtIOBlkConf; > -- > 2.39.3 >