[PATCH v1 0/4] virtio_snd_set_config: Fix #2296

[PATCH v1 0/4] virtio_snd_set_config: Fix #2296

[Manos Pitsidianakis]

Changing the number of streams via virtio_snd_set_config() did not
re-configure the audio card, leaving it in an invalid state.

Reported in https://gitlab.com/qemu-project/qemu/-/issues/2296

Manos Pitsidianakis (4):
  virtio-snd: add virtio_snd_is_config_valid()
  virtio-snd: factor card setup out of realize func
  virtio-snd: factor card removal out of unrealize()
  virtio_snd_set_config: validate and re-setup card

 hw/audio/virtio-snd.c | 174 +++++++++++++++++++++++++-----------------
 1 file changed, 105 insertions(+), 69 deletions(-)


base-commit: 62dbe54c24dbf77051bafe1039c31ddc8f37602d
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 1/4] virtio-snd: add virtio_snd_is_config_valid()

[Manos Pitsidianakis]

Factor out virtio_snd_config value validation in a separate function, in
order to re-use it in follow up commits.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 47 ++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..7ca9ed251c 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1045,6 +1045,34 @@ virtio_snd_vm_state_change(void *opaque, bool running,
     }
 }
 
+static bool
+virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
+{
+    if (snd_conf.jacks > 8) {
+        error_setg(errp,
+                   "Invalid number of jacks: %"PRIu32
+                   ": maximum value is 8", snd_conf.jacks);
+        return false;
+    }
+    if (snd_conf.streams < 1 || snd_conf.streams > 64) {
+        error_setg(errp,
+                   "Invalid number of streams: %"PRIu32
+                   ": minimum value is 1, maximum value is 64",
+                   snd_conf.streams);
+        return false;
+    }
+
+    if (snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
+        error_setg(errp,
+                  "Invalid number of channel maps: %"PRIu32
+                  ": VIRTIO v1.2 sets the maximum value at %"PRIu32,
+                  snd_conf.chmaps, VIRTIO_SND_CHMAP_MAX_SIZE);
+        return false;
+    }
+
+    return true;
+}
+
 static void virtio_snd_realize(DeviceState *dev, Error **errp)
 {
     ERRP_GUARD();
@@ -1055,24 +1083,7 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
 
     trace_virtio_snd_realize(vsnd);
 
-    /* check number of jacks and streams */
-    if (vsnd->snd_conf.jacks > 8) {
-        error_setg(errp,
-                   "Invalid number of jacks: %"PRIu32,
-                   vsnd->snd_conf.jacks);
-        return;
-    }
-    if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
-        error_setg(errp,
-                   "Invalid number of streams: %"PRIu32,
-                    vsnd->snd_conf.streams);
-        return;
-    }
-
-    if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
-        error_setg(errp,
-                   "Invalid number of channel maps: %"PRIu32,
-                   vsnd->snd_conf.chmaps);
+    if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
         return;
     }
 
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 1/4] virtio-snd: add virtio_snd_is_config_valid()

[Philippe Mathieu-Daudé]

Hi Manos,

On 22/4/24 14:52, Manos Pitsidianakis wrote:
> Factor out virtio_snd_config value validation in a separate function, in
> order to re-use it in follow up commits.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/audio/virtio-snd.c | 47 ++++++++++++++++++++++++++-----------------
>   1 file changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index c80b58bf5d..7ca9ed251c 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -1045,6 +1045,34 @@ virtio_snd_vm_state_change(void *opaque, bool running,
>       }
>   }
>   
> +static bool
> +virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
> +{
> +    if (snd_conf.jacks > 8) {
> +        error_setg(errp,
> +                   "Invalid number of jacks: %"PRIu32
> +                   ": maximum value is 8", snd_conf.jacks);
> +        return false;
> +    }
> +    if (snd_conf.streams < 1 || snd_conf.streams > 64) {

Why the undocumented 10 -> 64 change?

> +        error_setg(errp,
> +                   "Invalid number of streams: %"PRIu32
> +                   ": minimum value is 1, maximum value is 64",
> +                   snd_conf.streams);
> +        return false;
> +    }
> +
> +    if (snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
> +        error_setg(errp,
> +                  "Invalid number of channel maps: %"PRIu32
> +                  ": VIRTIO v1.2 sets the maximum value at %"PRIu32,
> +                  snd_conf.chmaps, VIRTIO_SND_CHMAP_MAX_SIZE);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>   static void virtio_snd_realize(DeviceState *dev, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -1055,24 +1083,7 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
>   
>       trace_virtio_snd_realize(vsnd);
>   
> -    /* check number of jacks and streams */
> -    if (vsnd->snd_conf.jacks > 8) {
> -        error_setg(errp,
> -                   "Invalid number of jacks: %"PRIu32,
> -                   vsnd->snd_conf.jacks);
> -        return;
> -    }
> -    if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
> -        error_setg(errp,
> -                   "Invalid number of streams: %"PRIu32,
> -                    vsnd->snd_conf.streams);
> -        return;
> -    }
> -
> -    if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
> -        error_setg(errp,
> -                   "Invalid number of channel maps: %"PRIu32,
> -                   vsnd->snd_conf.chmaps);
> +    if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
>           return;
>       }
>   

[PATCH v1 2/4] virtio-snd: factor card setup out of realize func

[Manos Pitsidianakis]

Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 72 ++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 7ca9ed251c..82dd320ebe 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1073,27 +1073,21 @@ virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
     return true;
 }
 
-static void virtio_snd_realize(DeviceState *dev, Error **errp)
+/* Registers card and sets up streams according to configuration. */
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp)
 {
     ERRP_GUARD();
-    VirtIOSound *vsnd = VIRTIO_SND(dev);
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     virtio_snd_pcm_set_params default_params = { 0 };
     uint32_t status;
 
-    trace_virtio_snd_realize(vsnd);
-
     if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
-        return;
+        return false;
     }
 
     if (!AUD_register_card("virtio-sound", &vsnd->card, errp)) {
-        return;
+        return false;
     }
 
-    vsnd->vmstate =
-        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
-
     vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
     vsnd->pcm->snd = vsnd;
     vsnd->pcm->streams =
@@ -1101,9 +1095,6 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
     vsnd->pcm->pcm_params =
         g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams);
 
-    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
-    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
-
     /* set default params for all streams */
     default_params.features = 0;
     default_params.buffer_bytes = cpu_to_le32(8192);
@@ -1111,6 +1102,41 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
     default_params.channels = 2;
     default_params.format = VIRTIO_SND_PCM_FMT_S16;
     default_params.rate = VIRTIO_SND_PCM_RATE_48000;
+
+    for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
+        status = virtio_snd_set_pcm_params(vsnd, i, &default_params);
+        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+            error_setg(errp,
+                       "Can't initialize stream params, device responded with %s.",
+                       print_code(status));
+            return false;
+        }
+        status = virtio_snd_pcm_prepare(vsnd, i);
+        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+            error_setg(errp,
+                       "Can't prepare streams, device responded with %s.",
+                       print_code(status));
+            return false;
+        }
+    }
+
+    return true;
+}
+
+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+    VirtIOSound *vsnd = VIRTIO_SND(dev);
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+    trace_virtio_snd_realize(vsnd);
+
+    vsnd->vmstate =
+        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
+
     vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
         virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
     vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
     QTAILQ_INIT(&vsnd->cmdq);
     QSIMPLEQ_INIT(&vsnd->invalid);
 
-    for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-        status = virtio_snd_set_pcm_params(vsnd, i, &default_params);
-        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-            error_setg(errp,
-                       "Can't initialize stream params, device responded with %s.",
-                       print_code(status));
-            goto error_cleanup;
-        }
-        status = virtio_snd_pcm_prepare(vsnd, i);
-        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-            error_setg(errp,
-                       "Can't prepare streams, device responded with %s.",
-                       print_code(status));
-            goto error_cleanup;
-        }
+    if (virtio_snd_setup(vsnd, errp)) {
+        return;
     }
 
-    return;
-
-error_cleanup:
     virtio_snd_unrealize(dev);
 }
 
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 2/4] virtio-snd: factor card setup out of realize func

[Philippe Mathieu-Daudé]

On 22/4/24 14:52, Manos Pitsidianakis wrote:
> Extract audio card setup logic out of the device realize callback so
> that it can be re-used in follow up commits.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/audio/virtio-snd.c | 72 ++++++++++++++++++++++++-------------------
>   1 file changed, 41 insertions(+), 31 deletions(-)


> +static void virtio_snd_realize(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    VirtIOSound *vsnd = VIRTIO_SND(dev);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> +    trace_virtio_snd_realize(vsnd);
> +
> +    vsnd->vmstate =
> +        qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
> +
> +    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
> +    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
> +
>       vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
>           virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
>       vsnd->queues[VIRTIO_SND_VQ_EVENT] =
> @@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
>       QTAILQ_INIT(&vsnd->cmdq);
>       QSIMPLEQ_INIT(&vsnd->invalid);
>   
> -    for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
> -        status = virtio_snd_set_pcm_params(vsnd, i, &default_params);
> -        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
> -            error_setg(errp,
> -                       "Can't initialize stream params, device responded with %s.",
> -                       print_code(status));
> -            goto error_cleanup;
> -        }
> -        status = virtio_snd_pcm_prepare(vsnd, i);
> -        if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
> -            error_setg(errp,
> -                       "Can't prepare streams, device responded with %s.",
> -                       print_code(status));
> -            goto error_cleanup;
> -        }
> +    if (virtio_snd_setup(vsnd, errp)) {
> +        return;
>       }
>   
> -    return;
> -
> -error_cleanup:
>       virtio_snd_unrealize(dev);

We usually handle failure as:

   if (!virtio_snd_setup(vsnd, errp)) {
     virtio_snd_unrealize(dev);
   }

>   }
>   

Otherwise LGTM.

[PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()

[Manos Pitsidianakis]

Extract audio card removal logic out of the device unrealize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 82dd320ebe..a9cfaea046 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1343,15 +1343,11 @@ static inline void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
     }
 }
 
-static void virtio_snd_unrealize(DeviceState *dev)
+/* Remove audio card and cleanup streams. */
+static void virtio_snd_unsetup(VirtIOSound *vsnd)
 {
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VirtIOSound *vsnd = VIRTIO_SND(dev);
     VirtIOSoundPCMStream *stream;
 
-    qemu_del_vm_change_state_handler(vsnd->vmstate);
-    trace_virtio_snd_unrealize(vsnd);
-
     if (vsnd->pcm) {
         if (vsnd->pcm->streams) {
             for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
@@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
         vsnd->pcm = NULL;
     }
     AUD_remove_card(&vsnd->card);
+}
+
+static void virtio_snd_unrealize(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VirtIOSound *vsnd = VIRTIO_SND(dev);
+
+    qemu_del_vm_change_state_handler(vsnd->vmstate);
+    trace_virtio_snd_unrealize(vsnd);
+
+    virtio_snd_unsetup(vsnd);
+
     qemu_mutex_destroy(&vsnd->cmdq_mutex);
     virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
     virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_EVENT]);
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()

[Philippe Mathieu-Daudé]

On 22/4/24 14:52, Manos Pitsidianakis wrote:
> Extract audio card removal logic out of the device unrealize callback so
> that it can be re-used in follow up commits.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>   hw/audio/virtio-snd.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)


> -static void virtio_snd_unrealize(DeviceState *dev)
> +/* Remove audio card and cleanup streams. */
> +static void virtio_snd_unsetup(VirtIOSound *vsnd)

Maybe s/unsetup/cleanup/?

>   {
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VirtIOSound *vsnd = VIRTIO_SND(dev);
>       VirtIOSoundPCMStream *stream;
>   
> -    qemu_del_vm_change_state_handler(vsnd->vmstate);
> -    trace_virtio_snd_unrealize(vsnd);
> -
>       if (vsnd->pcm) {
>           if (vsnd->pcm->streams) {
>               for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
> @@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
>           vsnd->pcm = NULL;
>       }
>       AUD_remove_card(&vsnd->card);
> +}

Re: [PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()

[Philippe Mathieu-Daudé]

On 22/4/24 15:27, Philippe Mathieu-Daudé wrote:
> On 22/4/24 14:52, Manos Pitsidianakis wrote:
>> Extract audio card removal logic out of the device unrealize callback so
>> that it can be re-used in follow up commits.
>>
>> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>> ---
>>   hw/audio/virtio-snd.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
> 
> 
>> -static void virtio_snd_unrealize(DeviceState *dev)
>> +/* Remove audio card and cleanup streams. */
>> +static void virtio_snd_unsetup(VirtIOSound *vsnd)
> 
> Maybe s/unsetup/cleanup/?

Or 'teardown'?

> 
>>   {
>> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> -    VirtIOSound *vsnd = VIRTIO_SND(dev);
>>       VirtIOSoundPCMStream *stream;
>> -    qemu_del_vm_change_state_handler(vsnd->vmstate);
>> -    trace_virtio_snd_unrealize(vsnd);
>> -
>>       if (vsnd->pcm) {
>>           if (vsnd->pcm->streams) {
>>               for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
>> @@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
>>           vsnd->pcm = NULL;
>>       }
>>       AUD_remove_card(&vsnd->card);
>> +}

[PATCH v1 4/4] virtio_snd_set_config: validate and re-setup card

[Manos Pitsidianakis]

Validate new configuration values and re-setup audio card.

Changing the number of streams via virtio_snd_set_config() did not
re-configure the audio card, leaving it in an invalid state. This can be
demonstrated by this heap buffer overflow:

```shell
cat << EOF | qemu-system-x86_64 -display none \
-machine accel=qtest -m 512M -machine q35 -device \
virtio-sound,audiodev=my_audiodev,streams=2 -audiodev \
alsa,id=my_audiodev -qtest stdio
outl 0xcf8 0x80001804
outw 0xcfc 0x06
outl 0xcf8 0x80001820
outl 0xcfc 0xe0008000
write 0xe0008016 0x1 0x03
write 0xe0008020 0x4 0x00901000
write 0xe0008028 0x4 0x00a01000
write 0xe000801c 0x1 0x01
write 0xe000a004 0x1 0x40
write 0x10c000 0x1 0x02
write 0x109001 0x1 0xc0
write 0x109002 0x1 0x10
write 0x109008 0x1 0x04
write 0x10a002 0x1 0x01
write 0xe000b00d 0x1 0x00
EOF
```

When built with `--enable-sanitizers`, QEMU prints this error:

  ERROR: AddressSanitizer: heap-buffer-overflow [..snip..] in
  virtio_snd_handle_rx_xfer().

Closes #2296.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2296
Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Suggested-by: Zheyu Ma <zheyuma97@gmail.com>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/audio/virtio-snd.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a9cfaea046..d47af2ed69 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -36,7 +36,11 @@ static void virtio_snd_pcm_out_cb(void *data, int available);
 static void virtio_snd_process_cmdq(VirtIOSound *s);
 static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
 static void virtio_snd_pcm_in_cb(void *data, int available);
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp);
+static void virtio_snd_unsetup(VirtIOSound *vsnd);
 static void virtio_snd_unrealize(DeviceState *dev);
+static bool virtio_snd_is_config_valid(virtio_snd_config snd_conf,
+                                       Error **errp);
 
 static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
                                   | BIT(VIRTIO_SND_PCM_FMT_U8)
@@ -111,23 +115,26 @@ static void
 virtio_snd_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
     VirtIOSound *s = VIRTIO_SND(vdev);
-    const virtio_snd_config *sndconfig =
-        (const virtio_snd_config *)config;
+    virtio_snd_config new_value =
+        *(const virtio_snd_config *)config;
 
+    le32_to_cpus(&new_value.jacks);
+    le32_to_cpus(&new_value.streams);
+    le32_to_cpus(&new_value.chmaps);
 
-   trace_virtio_snd_set_config(vdev,
-                               s->snd_conf.jacks,
-                               sndconfig->jacks,
-                               s->snd_conf.streams,
-                               sndconfig->streams,
-                               s->snd_conf.chmaps,
-                               sndconfig->chmaps);
-
-    memcpy(&s->snd_conf, sndconfig, sizeof(virtio_snd_config));
-    le32_to_cpus(&s->snd_conf.jacks);
-    le32_to_cpus(&s->snd_conf.streams);
-    le32_to_cpus(&s->snd_conf.chmaps);
+    trace_virtio_snd_set_config(vdev,
+                                s->snd_conf.jacks,
+                                new_value.jacks,
+                                s->snd_conf.streams,
+                                new_value.streams,
+                                s->snd_conf.chmaps,
+                                new_value.chmaps);
 
+    if (virtio_snd_is_config_valid(new_value, &error_warn)) {
+        virtio_snd_unsetup(s);
+        s->snd_conf = new_value;
+        virtio_snd_setup(s, &error_fatal);
+    }
 }
 
 static void
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v1 0/4] virtio_snd_set_config: Fix #2296

[Volker Rümelin]

Am 22.04.24 um 14:52 schrieb Manos Pitsidianakis:
> Changing the number of streams via virtio_snd_set_config() did not
> re-configure the audio card, leaving it in an invalid state.
>
> Reported in https://gitlab.com/qemu-project/qemu/-/issues/2296
>
> Manos Pitsidianakis (4):
>   virtio-snd: add virtio_snd_is_config_valid()
>   virtio-snd: factor card setup out of realize func
>   virtio-snd: factor card removal out of unrealize()
>   virtio_snd_set_config: validate and re-setup card
>
>  hw/audio/virtio-snd.c | 174 +++++++++++++++++++++++++-----------------
>  1 file changed, 105 insertions(+), 69 deletions(-)
>
>
> base-commit: 62dbe54c24dbf77051bafe1039c31ddc8f37602d

Hi Manos,

I don't think the virtio_snd_set_config function is necessary. The
virtio sound device specification in chapter 5.14.4 states that all
fields in the configuration space are driver-read-only. I wrote a patch
to remove the virtio_snd_set_config function. The patch can be found at
https://lists.nongnu.org/archive/html/qemu-devel/2024-09/msg00003.html.

With best regards,
Volker