vhost-user-snd: correct the calculation of config_size

[PATCH v2] vhost-user-snd: correct the calculation of config_size

Posted by Matias Ezequiel Vara Larsen 11 months, 3 weeks ago

Use virtio_get_config_size() rather than sizeof(struct
virtio_snd_config) for the config_size in the vhost-user-snd frontend.
The frontend shall rely on device features for the size of the device
configuration space. The presence of `controls` in the config space
depends on VIRTIO_SND_F_CTLS according to the specification (v1.3):
`
5.14.4 Device Configuration Layout
...

controls
(driver-read-only) indicates a total number of all available control
elements if VIRTIO_SND_F_CTLS has been negotiated.
`
This fixes an issue introduced by commit ab0c7fb2 ("linux-headers:
update to current kvm/next") in which the optional field `controls` is
added to the virtio_snd_config structure. This breaks vhost-user-device
backends that do not implement the `controls` field.

Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
---
Changes in v2: 
 - Addressed comments from Stefano Garzarella about commit msg and the
   property name. 
---
 hw/virtio/vhost-user-snd.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user-snd.c b/hw/virtio/vhost-user-snd.c
index 8610370af8..b414c75c06 100644
--- a/hw/virtio/vhost-user-snd.c
+++ b/hw/virtio/vhost-user-snd.c
@@ -16,6 +16,18 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_snd.h"
 
+static const VirtIOFeature feature_sizes[] = {
+    {.flags = 1ULL << VIRTIO_SND_F_CTLS,
+    .end = endof(struct virtio_snd_config, controls)},
+    {}
+};
+
+static const VirtIOConfigSizeParams cfg_size_params = {
+    .min_size = endof(struct virtio_snd_config, chmaps),
+    .max_size = sizeof(struct virtio_snd_config),
+    .feature_sizes = feature_sizes
+};
+
 static const VMStateDescription vu_snd_vmstate = {
     .name = "vhost-user-snd",
     .unmigratable = 1,
@@ -23,16 +35,20 @@ static const VMStateDescription vu_snd_vmstate = {
 
 static const Property vsnd_properties[] = {
     DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
+    DEFINE_PROP_BIT64("controls", VHostUserBase,
+                      parent_obj.host_features, VIRTIO_SND_F_CTLS, false),
 };
 
 static void vu_snd_base_realize(DeviceState *dev, Error **errp)
 {
     VHostUserBase *vub = VHOST_USER_BASE(dev);
     VHostUserBaseClass *vubs = VHOST_USER_BASE_GET_CLASS(dev);
+    VirtIODevice *vdev = &vub->parent_obj;
 
     vub->virtio_id = VIRTIO_ID_SOUND;
     vub->num_vqs = 4;
-    vub->config_size = sizeof(struct virtio_snd_config);
+    vub->config_size = virtio_get_config_size(&cfg_size_params,
+                                              vdev->host_features);
     vub->vq_size = 64;
 
     vubs->parent_realize(dev, errp);
-- 
2.42.0

Re: [PATCH v2] vhost-user-snd: correct the calculation of config_size

Posted by Stefano Garzarella 11 months, 3 weeks ago

On Mon, Feb 17, 2025 at 02:12:55PM +0100, Matias Ezequiel Vara Larsen wrote:
>Use virtio_get_config_size() rather than sizeof(struct
>virtio_snd_config) for the config_size in the vhost-user-snd frontend.
>The frontend shall rely on device features for the size of the device
>configuration space. The presence of `controls` in the config space
>depends on VIRTIO_SND_F_CTLS according to the specification (v1.3):
>`
>5.14.4 Device Configuration Layout
>...
>
>controls
>(driver-read-only) indicates a total number of all available control
>elements if VIRTIO_SND_F_CTLS has been negotiated.
>`
>This fixes an issue introduced by commit ab0c7fb2 ("linux-headers:
>update to current kvm/next") in which the optional field `controls` is
>added to the virtio_snd_config structure. This breaks vhost-user-device
>backends that do not implement the `controls` field.
>
>Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next")

If we want to backport this on stable branches, better to add:

Cc: qemu-stable@nongnu.org

>Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
>---
>Changes in v2:
> - Addressed comments from Stefano Garzarella about commit msg and the
>   property name.

Thanks for that! LGTM, I also tested with old vhost-device-sound and it 
works well!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>---
> hw/virtio/vhost-user-snd.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
>diff --git a/hw/virtio/vhost-user-snd.c b/hw/virtio/vhost-user-snd.c
>index 8610370af8..b414c75c06 100644
>--- a/hw/virtio/vhost-user-snd.c
>+++ b/hw/virtio/vhost-user-snd.c
>@@ -16,6 +16,18 @@
> #include "standard-headers/linux/virtio_ids.h"
> #include "standard-headers/linux/virtio_snd.h"
>
>+static const VirtIOFeature feature_sizes[] = {
>+    {.flags = 1ULL << VIRTIO_SND_F_CTLS,
>+    .end = endof(struct virtio_snd_config, controls)},
>+    {}
>+};
>+
>+static const VirtIOConfigSizeParams cfg_size_params = {
>+    .min_size = endof(struct virtio_snd_config, chmaps),
>+    .max_size = sizeof(struct virtio_snd_config),
>+    .feature_sizes = feature_sizes
>+};
>+
> static const VMStateDescription vu_snd_vmstate = {
>     .name = "vhost-user-snd",
>     .unmigratable = 1,
>@@ -23,16 +35,20 @@ static const VMStateDescription vu_snd_vmstate = {
>
> static const Property vsnd_properties[] = {
>     DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
>+    DEFINE_PROP_BIT64("controls", VHostUserBase,
>+                      parent_obj.host_features, VIRTIO_SND_F_CTLS, false),
> };
>
> static void vu_snd_base_realize(DeviceState *dev, Error **errp)
> {
>     VHostUserBase *vub = VHOST_USER_BASE(dev);
>     VHostUserBaseClass *vubs = VHOST_USER_BASE_GET_CLASS(dev);
>+    VirtIODevice *vdev = &vub->parent_obj;
>
>     vub->virtio_id = VIRTIO_ID_SOUND;
>     vub->num_vqs = 4;
>-    vub->config_size = sizeof(struct virtio_snd_config);
>+    vub->config_size = virtio_get_config_size(&cfg_size_params,
>+                                              vdev->host_features);
>     vub->vq_size = 64;
>
>     vubs->parent_realize(dev, errp);
>-- 
>2.42.0
>

Re: [PATCH v2] vhost-user-snd: correct the calculation of config_size

Posted by Michael Tokarev 11 months, 2 weeks ago

17.02.2025 17:07, Stefano Garzarella wrote:
> On Mon, Feb 17, 2025 at 02:12:55PM +0100, Matias Ezequiel Vara Larsen wrote:
>> Use virtio_get_config_size() rather than sizeof(struct
>> virtio_snd_config) for the config_size in the vhost-user-snd frontend.
>> The frontend shall rely on device features for the size of the device
>> configuration space. The presence of `controls` in the config space
>> depends on VIRTIO_SND_F_CTLS according to the specification (v1.3):
>> `
>> 5.14.4 Device Configuration Layout
>> ...
>>
>> controls
>> (driver-read-only) indicates a total number of all available control
>> elements if VIRTIO_SND_F_CTLS has been negotiated.
>> `
>> This fixes an issue introduced by commit ab0c7fb2 ("linux-headers:
>> update to current kvm/next") in which the optional field `controls` is
>> added to the virtio_snd_config structure. This breaks vhost-user-device
>> backends that do not implement the `controls` field.
>>
>> Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next")
> 
> If we want to backport this on stable branches, better to add:
> 
> Cc: qemu-stable@nongnu.org

There's a problem with stable branches though:

>> static const Property vsnd_properties[] = {
>>     DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
>> +    DEFINE_PROP_BIT64("controls", VHostUserBase,
>> +                      parent_obj.host_features, VIRTIO_SND_F_CTLS, false),
>> };

This change introduces an incompatible change in migration stream,
it looks like.  I'm not sure how to handle this for stable branches,
is there a way?

Thanks,

/mjt

Re: [PATCH v2] vhost-user-snd: correct the calculation of config_size

Posted by Stefano Garzarella 11 months, 2 weeks ago

On Tue, 25 Feb 2025 at 10:40, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 17.02.2025 17:07, Stefano Garzarella wrote:
> > On Mon, Feb 17, 2025 at 02:12:55PM +0100, Matias Ezequiel Vara Larsen wrote:
> >> Use virtio_get_config_size() rather than sizeof(struct
> >> virtio_snd_config) for the config_size in the vhost-user-snd frontend.
> >> The frontend shall rely on device features for the size of the device
> >> configuration space. The presence of `controls` in the config space
> >> depends on VIRTIO_SND_F_CTLS according to the specification (v1.3):
> >> `
> >> 5.14.4 Device Configuration Layout
> >> ...
> >>
> >> controls
> >> (driver-read-only) indicates a total number of all available control
> >> elements if VIRTIO_SND_F_CTLS has been negotiated.
> >> `
> >> This fixes an issue introduced by commit ab0c7fb2 ("linux-headers:
> >> update to current kvm/next") in which the optional field `controls` is
> >> added to the virtio_snd_config structure. This breaks vhost-user-device
> >> backends that do not implement the `controls` field.
> >>
> >> Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next")
> >
> > If we want to backport this on stable branches, better to add:
> >
> > Cc: qemu-stable@nongnu.org
>
> There's a problem with stable branches though:
>
> >> static const Property vsnd_properties[] = {
> >>     DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
> >> +    DEFINE_PROP_BIT64("controls", VHostUserBase,
> >> +                      parent_obj.host_features, VIRTIO_SND_F_CTLS, false),
> >> };
>
> This change introduces an incompatible change in migration stream,
> it looks like.  I'm not sure how to handle this for stable branches,
> is there a way?

Good point, but in hw/virtio/vhost-user-snd.c IIUC the device is
marked unmigratable:

static const VMStateDescription vu_snd_vmstate = {
    .name = "vhost-user-snd",
    .unmigratable = 1,
};

So should we care about migration stream for this change?

Thanks,
Stefano