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

On 3/13/24 10:35 AM, Eugenio Perez Martin wrote:
> On Wed, Mar 13, 2024 at 12:55 PM Jonah Palmer <[email protected]> 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 <[email protected]>
>> ---
>>   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?
> 

Great question with no good answer! Will fix this.

>>
>>   /* Base devices.  */
>>   typedef struct VirtIOBlkConf VirtIOBlkConf;
>> --
>> 2.39.3
>>
>