Re: [PATCH v2 2/6] virtio: Prevent creation of device using notification-data with ioeventfd

[Eugenio Perez Martin]

On Wed, Mar 13, 2024 at 12:55 PM Jonah Palmer <jonah.palmer@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.palmer@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
>