Re: [PATCH] vhost-user-snd: Use virtio_get_config_size()

[Stefano Garzarella <sgarzare@redhat.com>]

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
> > >