| Message ID | 20250213132513.767709-1-mvaralar@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-snd: Use virtio_get_config_size() | expand |
On 13/2/25 14:25, 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. This fixes an issue introduced by commit ab0c7fb2 > in which the optional field `control` is added to the virtio_snd_config > structure. This breaks vhost-user-device backends that do not implement > the `controls` field. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 > Suggested-by: Stefano Garzarella <sgarzare@redhat.com> > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com> > --- > hw/virtio/vhost-user-snd.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) ¡¡Bien ahí!! Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
For the title, what about this? vhost-user-snd: fix incorrect config_size computation Or something like that, just to make clear that we are fixing a real issue. On Thu, Feb 13, 2025 at 02:25:13PM +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. This fixes an issue introduced by commit ab0c7fb2 When we refer to a commit it's a good practice to put both the sha1, but also the title, like this: This fixes an issue introduced by commit ab0c7fb22b ("linux-headers: update to current kvm/next") ... >in which the optional field `control` is added to the virtio_snd_config s/control/controls I would also specify here that the presence of `controls` in the config space depends on VIRTIO_SND_F_CTLS, citing the specification: 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. >structure. This breaks vhost-user-device backends that do not implement >the `controls` field. > I'd suggest to add the fixes tag: Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next") And maybe also: Cc: qemu-stable@nongnu.org >Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 >Suggested-by: Stefano Garzarella <sgarzare@redhat.com> >Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@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..8da4309470 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("config-controls", VHostUserBase, In almost all other virtio/vhost-user devices, the property name does not have the prefix `config-`, but usually the thing after F_, in this case CTLS is cryptic, so IMO just `controls` should be fine. The only example I found is `config-wce` for vhost-user-blk, but in that case the feature is actually called VIRTIO_BLK_F_CONFIG_WCE. Thanks, Stefano >+ 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 >
Unrelated to this patch, but since we are talking about
VIRTIO_SND_F_CTLS, I think it would be good to send a patch to Linux to
make it clear that `controls` depends on VIRTIO_SND_F_CTLS.
For example in linux/include/uapi/linux/virtio_blk.h we have:
struct virtio_blk_config {
/* The capacity (in 512-byte sectors). */
__virtio64 capacity;
/* The maximum segment size (if VIRTIO_BLK_F_SIZE_MAX) */
__virtio32 size_max;
/* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */
__virtio32 seg_max;
....
So I suggest something like this:
diff --git a/include/uapi/linux/virtio_snd.h b/include/uapi/linux/virtio_snd.h
index 5f4100c2cf04..a4cfb9f6561a 100644
--- a/include/uapi/linux/virtio_snd.h
+++ b/include/uapi/linux/virtio_snd.h
@@ -25,7 +25,7 @@ struct virtio_snd_config {
__le32 streams;
/* # of available channel maps */
__le32 chmaps;
- /* # of available control elements */
+ /* # of available control elements (if VIRTIO_SND_F_CTLS) */
__le32 controls;
};
Thanks,
Stefano
On Thu, 13 Feb 2025 at 16:31, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> For the title, what about this?
>
> vhost-user-snd: fix incorrect config_size computation
>
> Or something like that, just to make clear that we are fixing a
> real issue.
>
> On Thu, Feb 13, 2025 at 02:25:13PM +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. This fixes an issue introduced by commit ab0c7fb2
>
> When we refer to a commit it's a good practice to put both the sha1, but
> also the title, like this:
> This fixes an issue introduced by commit ab0c7fb22b ("linux-headers:
> update to current kvm/next") ...
>
> >in which the optional field `control` is added to the virtio_snd_config
>
> s/control/controls
>
> I would also specify here that the presence of `controls` in the config
> space depends on VIRTIO_SND_F_CTLS, citing the specification:
>
> 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.
>
> >structure. This breaks vhost-user-device backends that do not implement
> >the `controls` field.
> >
>
> I'd suggest to add the fixes tag:
>
> Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next")
>
> And maybe also:
>
> Cc: qemu-stable@nongnu.org
>
> >Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805
> >Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
> >Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@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..8da4309470 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("config-controls", VHostUserBase,
>
> In almost all other virtio/vhost-user devices, the property name does
> not have the prefix `config-`, but usually the thing after F_, in this
> case CTLS is cryptic, so IMO just `controls` should be fine.
>
> The only example I found is `config-wce` for vhost-user-blk, but in that
> case the feature is actually called VIRTIO_BLK_F_CONFIG_WCE.
>
> Thanks,
> Stefano
>
> >+ 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
> >
On Thu, 13 Feb 2025 at 16:43, Stefano Garzarella <sgarzare@redhat.com> wrote: > > Unrelated to this patch, but since we are talking about > VIRTIO_SND_F_CTLS, I think it would be good to send a patch to Linux to > make it clear that `controls` depends on VIRTIO_SND_F_CTLS. Patch posted here: https://lore.kernel.org/virtualization/20250213161825.139952-1-sgarzare@redhat.com/T/#u Stefano > > For example in linux/include/uapi/linux/virtio_blk.h we have: > > struct virtio_blk_config { > /* The capacity (in 512-byte sectors). */ > __virtio64 capacity; > /* The maximum segment size (if VIRTIO_BLK_F_SIZE_MAX) */ > __virtio32 size_max; > /* The maximum number of segments (if VIRTIO_BLK_F_SEG_MAX) */ > __virtio32 seg_max; > .... > > So I suggest something like this: > > diff --git a/include/uapi/linux/virtio_snd.h b/include/uapi/linux/virtio_snd.h > index 5f4100c2cf04..a4cfb9f6561a 100644 > --- a/include/uapi/linux/virtio_snd.h > +++ b/include/uapi/linux/virtio_snd.h > @@ -25,7 +25,7 @@ struct virtio_snd_config { > __le32 streams; > /* # of available channel maps */ > __le32 chmaps; > - /* # of available control elements */ > + /* # of available control elements (if VIRTIO_SND_F_CTLS) */ > __le32 controls; > }; > > Thanks, > Stefano > > > On Thu, 13 Feb 2025 at 16:31, Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > For the title, what about this? > > > > vhost-user-snd: fix incorrect config_size computation > > > > Or something like that, just to make clear that we are fixing a > > real issue. > > > > On Thu, Feb 13, 2025 at 02:25:13PM +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. This fixes an issue introduced by commit ab0c7fb2 > > > > When we refer to a commit it's a good practice to put both the sha1, but > > also the title, like this: > > This fixes an issue introduced by commit ab0c7fb22b ("linux-headers: > > update to current kvm/next") ... > > > > >in which the optional field `control` is added to the virtio_snd_config > > > > s/control/controls > > > > I would also specify here that the presence of `controls` in the config > > space depends on VIRTIO_SND_F_CTLS, citing the specification: > > > > 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. > > > > >structure. This breaks vhost-user-device backends that do not implement > > >the `controls` field. > > > > > > > I'd suggest to add the fixes tag: > > > > Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next") > > > > And maybe also: > > > > Cc: qemu-stable@nongnu.org > > > > >Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 > > >Suggested-by: Stefano Garzarella <sgarzare@redhat.com> > > >Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@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..8da4309470 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("config-controls", VHostUserBase, > > > > In almost all other virtio/vhost-user devices, the property name does > > not have the prefix `config-`, but usually the thing after F_, in this > > case CTLS is cryptic, so IMO just `controls` should be fine. > > > > The only example I found is `config-wce` for vhost-user-blk, but in that > > case the feature is actually called VIRTIO_BLK_F_CONFIG_WCE. > > > > Thanks, > > Stefano > > > > >+ 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 > > >
On Thu, Feb 13, 2025 at 04:07:47PM +0100, Philippe Mathieu-Daudé wrote: > On 13/2/25 14:25, 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. This fixes an issue introduced by commit ab0c7fb2 > > in which the optional field `control` is added to the virtio_snd_config > > structure. This breaks vhost-user-device backends that do not implement > > the `controls` field. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 > > Suggested-by: Stefano Garzarella <sgarzare@redhat.com> > > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com> > > --- > > hw/virtio/vhost-user-snd.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > ¡¡Bien ahí!! > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Thanks! ;)
On Thu, Feb 13, 2025 at 05:22:22PM +0100, Stefano Garzarella wrote: > On Thu, 13 Feb 2025 at 16:43, Stefano Garzarella <sgarzare@redhat.com> wrote: > > > > Unrelated to this patch, but since we are talking about > > VIRTIO_SND_F_CTLS, I think it would be good to send a patch to Linux to > > make it clear that `controls` depends on VIRTIO_SND_F_CTLS. > > Patch posted here: > https://lore.kernel.org/virtualization/20250213161825.139952-1-sgarzare@redhat.com/T/#u > > Stefano Thanks!
On Thu, Feb 13, 2025 at 04:31:41PM +0100, Stefano Garzarella wrote: > For the title, what about this? > > vhost-user-snd: fix incorrect config_size computation > > Or something like that, just to make clear that we are fixing a > real issue. > > On Thu, Feb 13, 2025 at 02:25:13PM +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. This fixes an issue introduced by commit ab0c7fb2 > > When we refer to a commit it's a good practice to put both the sha1, but > also the title, like this: > This fixes an issue introduced by commit ab0c7fb22b ("linux-headers: > update to current kvm/next") ... > > > in which the optional field `control` is added to the virtio_snd_config > > s/control/controls > > I would also specify here that the presence of `controls` in the config > space depends on VIRTIO_SND_F_CTLS, citing the specification: > > 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. > > > structure. This breaks vhost-user-device backends that do not implement > > the `controls` field. > > > > I'd suggest to add the fixes tag: > > Fixes: ab0c7fb22b ("linux-headers: update to current kvm/next") > > And maybe also: > > Cc: qemu-stable@nongnu.org > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 > > Suggested-by: Stefano Garzarella <sgarzare@redhat.com> > > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@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..8da4309470 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("config-controls", VHostUserBase, > > In almost all other virtio/vhost-user devices, the property name does not > have the prefix `config-`, but usually the thing after F_, in this case CTLS > is cryptic, so IMO just `controls` should be fine. > > The only example I found is `config-wce` for vhost-user-blk, but in that > case the feature is actually called VIRTIO_BLK_F_CONFIG_WCE. > Thanks for the review. I will address all the comments in the next version. Matias
diff --git a/hw/virtio/vhost-user-snd.c b/hw/virtio/vhost-user-snd.c index 8610370af8..8da4309470 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("config-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. This fixes an issue introduced by commit ab0c7fb2 in which the optional field `control` is added to the virtio_snd_config structure. This breaks vhost-user-device backends that do not implement the `controls` field. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805 Suggested-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com> --- hw/virtio/vhost-user-snd.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)