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 <sgarz...@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-sta...@nongnu.org
>
> >Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2805
> >Suggested-by: Stefano Garzarella <sgarz...@redhat.com>
> >Signed-off-by: Matias Ezequiel Vara Larsen <mvara...@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
> >
