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

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

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