| Message ID | 20250217131255.829892-1-mvaralar@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] vhost-user-snd: correct the calculation of config_size | expand |
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 >
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
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
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);
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(-)