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

[Stefano Garzarella]

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