[PATCH 0/7] audio related fixes for 10.1

[PATCH 0/7] audio related fixes for 10.1

[Volker Rümelin]

A few audio related fixes for 10.1.

The virtio-sound device is the first QEMU audio front end that supports floating point samples. The audio subsystem is only partially prepared for this. The commit message of patch 7/7 "audio: add float sample endianness converters" has the details. The new code paths in patch 7/7 are only compile tested. I don't have a big endian host to test.

checkpatch.pl complains about missing space characters in the type punning macros in patch 7/7. I don't agree.

Volker Rümelin (7):
  tests/functional: use 'none' audio driver for q800 tests
  audio: fix SIGSEGV in AUD_get_buffer_size_out()
  audio: fix size calculation in AUD_get_buffer_size_out()
  hw/audio/asc: fix SIGSEGV in asc_realize()
  hw/audio/asc: replace g_malloc0() with g_malloc()
  audio/mixeng: remove unnecessary pointer type casts
  audio: add float sample endianness converters

 audio/audio.c                        | 11 +++-
 audio/audio_template.h               | 12 ++--
 audio/mixeng.c                       | 83 ++++++++++++++++++++++++----
 audio/mixeng.h                       |  6 +-
 hw/audio/asc.c                       |  9 ++-
 tests/functional/test_m68k_q800.py   |  3 +-
 tests/functional/test_m68k_replay.py |  3 +-
 7 files changed, 106 insertions(+), 21 deletions(-)

-- 
2.43.0

[PATCH 1/7] tests/functional: use 'none' audio driver for q800 tests

[Volker Rümelin]

Since commit ac13a6b3fd ("audio: add Apple Sound Chip (ASC)
emulation") the Quadra 800 machine has an audio device. It is
not guaranteed that the default audio driver of the audio
subsystem will work correctly on all host systems. Therefore,
the 'none' audio driver should be used in all q800 tests.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2812
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 tests/functional/test_m68k_q800.py   | 3 ++-
 tests/functional/test_m68k_replay.py | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/functional/test_m68k_q800.py b/tests/functional/test_m68k_q800.py
index 400b7aeb5d..b3e655346c 100755
--- a/tests/functional/test_m68k_q800.py
+++ b/tests/functional/test_m68k_q800.py
@@ -25,7 +25,8 @@ def test_m68k_q800(self):
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'console=ttyS0 vga=off')
         self.vm.add_args('-kernel', kernel_path,
-                         '-append', kernel_command_line)
+                         '-append', kernel_command_line,
+                         '-audio', 'none')
         self.vm.launch()
         console_pattern = 'Kernel command line: %s' % kernel_command_line
         self.wait_for_console_pattern(console_pattern)
diff --git a/tests/functional/test_m68k_replay.py b/tests/functional/test_m68k_replay.py
index 18c1db539c..213d6ae07e 100755
--- a/tests/functional/test_m68k_replay.py
+++ b/tests/functional/test_m68k_replay.py
@@ -24,7 +24,8 @@ def test_q800(self):
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
                                'console=ttyS0 vga=off')
         console_pattern = 'No filesystem could mount root'
-        self.run_rr(kernel_path, kernel_command_line, console_pattern)
+        self.run_rr(kernel_path, kernel_command_line, console_pattern,
+                    args=('-audio', 'none'))
 
     ASSET_MCF5208 = Asset(
        'https://qemu-advcal.gitlab.io/qac-best-of-multiarch/download/day07.tar.xz',
-- 
2.43.0

[PATCH 2/7] audio: fix SIGSEGV in AUD_get_buffer_size_out()

[Volker Rümelin]

As far as the emulated audio devices are concerned the pointer
returned by AUD_open_out() is an opaque handle. This includes
the NULL pointer. In this case, AUD_get_buffer_size_out() should
return a sensible buffer size instead of triggering a segmentation
fault. All other public AUD_*_out() and audio_*_out() functions
handle this case.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index 41ee11aaad..70ef22b1a4 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -905,6 +905,10 @@ size_t AUD_read(SWVoiceIn *sw, void *buf, size_t size)
 
 int AUD_get_buffer_size_out(SWVoiceOut *sw)
 {
+    if (!sw) {
+        return 0;
+    }
+
     return sw->hw->samples * sw->hw->info.bytes_per_frame;
 }
 
-- 
2.43.0

[PATCH 3/7] audio: fix size calculation in AUD_get_buffer_size_out()

[Volker Rümelin]

The buffer size calculated by AUD_get_buffer_size_out() is often
incorrect. sw->hw->samples * sw->hw->info.bytes_per_frame is the
size of the mixing engine buffer in audio frames multiplied by
the size of one frame of the audio backend. Due to resampling or
format conversion, the size of the frontend buffer can differ
significantly.

Return the correct buffer size when the mixing engine is used.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index 70ef22b1a4..3f5baf0cc6 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -909,6 +909,10 @@ int AUD_get_buffer_size_out(SWVoiceOut *sw)
         return 0;
     }
 
+    if (audio_get_pdo_out(sw->s->dev)->mixing_engine) {
+        return sw->resample_buf.size * sw->info.bytes_per_frame;
+    }
+
     return sw->hw->samples * sw->hw->info.bytes_per_frame;
 }
 
-- 
2.43.0

[PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Volker Rümelin]

AUD_open_out() may fail and return NULL. This may then lead to
a segmentation fault in memset() below. The memset() behaviour
is undefined if the pointer to the destination object is a null
pointer.

Add the missing error handling code.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/audio/asc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/audio/asc.c b/hw/audio/asc.c
index 18382ccf6a..03dee0fcc7 100644
--- a/hw/audio/asc.c
+++ b/hw/audio/asc.c
@@ -12,6 +12,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/timer.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "audio/audio.h"
@@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error **errp)
 
     s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s, asc_out_cb,
                             &as);
+    if (!s->voice) {
+        asc_unrealize(dev);
+        error_setg(errp, "Initializing audio stream failed");
+        return;
+    }
+
     s->shift = 1;
     s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
     s->mixbuf = g_malloc0(s->samples << s->shift);
-- 
2.43.0

[PATCH 5/7] hw/audio/asc: replace g_malloc0() with g_malloc()

[Volker Rümelin]

There is no need to allocate initialized memory with g_malloc0()
if it's directly followed by a memset() function call. g_malloc()
is sufficient.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 hw/audio/asc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/audio/asc.c b/hw/audio/asc.c
index 03dee0fcc7..f12268e296 100644
--- a/hw/audio/asc.c
+++ b/hw/audio/asc.c
@@ -664,7 +664,7 @@ static void asc_realize(DeviceState *dev, Error **errp)
     s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
     s->mixbuf = g_malloc0(s->samples << s->shift);
 
-    s->silentbuf = g_malloc0(s->samples << s->shift);
+    s->silentbuf = g_malloc(s->samples << s->shift);
     memset(s->silentbuf, 0x80, s->samples << s->shift);
 
     /* Add easc registers if required */
-- 
2.43.0

[PATCH 6/7] audio/mixeng: remove unnecessary pointer type casts

[Volker Rümelin]

A simple assignment automatically converts a void pointer type
to any other pointer type.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/mixeng.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/audio/mixeng.c b/audio/mixeng.c
index 69f6549224..13e1ff9b08 100644
--- a/audio/mixeng.c
+++ b/audio/mixeng.c
@@ -286,7 +286,7 @@ static const float float_scale_reciprocal = 1.f / ((int64_t)INT32_MAX + 1);
 static void conv_natural_float_to_mono(struct st_sample *dst, const void *src,
                                        int samples)
 {
-    float *in = (float *)src;
+    const float *in = src;
 
     while (samples--) {
         dst->r = dst->l = CONV_NATURAL_FLOAT(*in++);
@@ -297,7 +297,7 @@ static void conv_natural_float_to_mono(struct st_sample *dst, const void *src,
 static void conv_natural_float_to_stereo(struct st_sample *dst, const void *src,
                                          int samples)
 {
-    float *in = (float *)src;
+    const float *in = src;
 
     while (samples--) {
         dst->l = CONV_NATURAL_FLOAT(*in++);
@@ -314,7 +314,7 @@ t_sample *mixeng_conv_float[2] = {
 static void clip_natural_float_from_mono(void *dst, const struct st_sample *src,
                                          int samples)
 {
-    float *out = (float *)dst;
+    float *out = dst;
 
     while (samples--) {
         *out++ = CLIP_NATURAL_FLOAT(src->l + src->r);
@@ -325,7 +325,7 @@ static void clip_natural_float_from_mono(void *dst, const struct st_sample *src,
 static void clip_natural_float_from_stereo(
     void *dst, const struct st_sample *src, int samples)
 {
-    float *out = (float *)dst;
+    float *out = dst;
 
     while (samples--) {
         *out++ = CLIP_NATURAL_FLOAT(src->l);
-- 
2.43.0

[PATCH 7/7] audio: add float sample endianness converters

[Volker Rümelin]

Commit ed2a4a7941 ("audio: proper support for float samples in
mixeng") added support for float audio samples. As there were no
audio frontend devices with float support at that time, the code
was limited to native endian float samples.

When nobody was paying attention, an audio device that supports
floating point samples crept in with commit eb9ad377bb
("virtio-sound: handle control messages and streams").

Add code for the audio subsystem to convert float samples to the
correct endianness.

The type punning code was taken from the PipeWire project.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 audio/audio.c          |  3 +-
 audio/audio_template.h | 12 ++++---
 audio/mixeng.c         | 75 ++++++++++++++++++++++++++++++++++++++----
 audio/mixeng.h         |  6 ++--
 4 files changed, 82 insertions(+), 14 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 3f5baf0cc6..b58ad74433 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1892,7 +1892,8 @@ CaptureVoiceOut *AUD_add_capture(
         cap->buf = g_malloc0_n(hw->mix_buf.size, hw->info.bytes_per_frame);
 
         if (hw->info.is_float) {
-            hw->clip = mixeng_clip_float[hw->info.nchannels == 2];
+            hw->clip = mixeng_clip_float[hw->info.nchannels == 2]
+                [hw->info.swap_endianness];
         } else {
             hw->clip = mixeng_clip
                 [hw->info.nchannels == 2]
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 7ccfec0116..c29d79c443 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -174,9 +174,11 @@ static int glue (audio_pcm_sw_init_, TYPE) (
 
     if (sw->info.is_float) {
 #ifdef DAC
-        sw->conv = mixeng_conv_float[sw->info.nchannels == 2];
+        sw->conv = mixeng_conv_float[sw->info.nchannels == 2]
+            [sw->info.swap_endianness];
 #else
-        sw->clip = mixeng_clip_float[sw->info.nchannels == 2];
+        sw->clip = mixeng_clip_float[sw->info.nchannels == 2]
+            [sw->info.swap_endianness];
 #endif
     } else {
 #ifdef DAC
@@ -303,9 +305,11 @@ static HW *glue(audio_pcm_hw_add_new_, TYPE)(AudioState *s,
 
     if (hw->info.is_float) {
 #ifdef DAC
-        hw->clip = mixeng_clip_float[hw->info.nchannels == 2];
+        hw->clip = mixeng_clip_float[hw->info.nchannels == 2]
+            [hw->info.swap_endianness];
 #else
-        hw->conv = mixeng_conv_float[hw->info.nchannels == 2];
+        hw->conv = mixeng_conv_float[hw->info.nchannels == 2]
+            [hw->info.swap_endianness];
 #endif
     } else {
 #ifdef DAC
diff --git a/audio/mixeng.c b/audio/mixeng.c
index 13e1ff9b08..703ee5448f 100644
--- a/audio/mixeng.c
+++ b/audio/mixeng.c
@@ -283,6 +283,11 @@ static const float float_scale_reciprocal = 1.f / ((int64_t)INT32_MAX + 1);
 #endif
 #endif
 
+#define F32_TO_F32S(v) \
+    bswap32((union { uint32_t i; float f; }){ .f = (v) }.i)
+#define F32S_TO_F32(v) \
+    ((union { uint32_t i; float f; }){ .i = bswap32(v) }.f)
+
 static void conv_natural_float_to_mono(struct st_sample *dst, const void *src,
                                        int samples)
 {
@@ -294,6 +299,17 @@ static void conv_natural_float_to_mono(struct st_sample *dst, const void *src,
     }
 }
 
+static void conv_swap_float_to_mono(struct st_sample *dst, const void *src,
+                                    int samples)
+{
+    const uint32_t *in_f32s = src;
+
+    while (samples--) {
+        dst->r = dst->l = CONV_NATURAL_FLOAT(F32S_TO_F32(*in_f32s++));
+        dst++;
+    }
+}
+
 static void conv_natural_float_to_stereo(struct st_sample *dst, const void *src,
                                          int samples)
 {
@@ -306,9 +322,27 @@ static void conv_natural_float_to_stereo(struct st_sample *dst, const void *src,
     }
 }
 
-t_sample *mixeng_conv_float[2] = {
-    conv_natural_float_to_mono,
-    conv_natural_float_to_stereo,
+static void conv_swap_float_to_stereo(struct st_sample *dst, const void *src,
+                                      int samples)
+{
+    const uint32_t *in_f32s = src;
+
+    while (samples--) {
+        dst->l = CONV_NATURAL_FLOAT(F32S_TO_F32(*in_f32s++));
+        dst->r = CONV_NATURAL_FLOAT(F32S_TO_F32(*in_f32s++));
+        dst++;
+    }
+}
+
+t_sample *mixeng_conv_float[2][2] = {
+    {
+        conv_natural_float_to_mono,
+        conv_swap_float_to_mono,
+    },
+    {
+        conv_natural_float_to_stereo,
+        conv_swap_float_to_stereo,
+    }
 };
 
 static void clip_natural_float_from_mono(void *dst, const struct st_sample *src,
@@ -322,6 +356,17 @@ static void clip_natural_float_from_mono(void *dst, const struct st_sample *src,
     }
 }
 
+static void clip_swap_float_from_mono(void *dst, const struct st_sample *src,
+                                      int samples)
+{
+    uint32_t *out_f32s = dst;
+
+    while (samples--) {
+        *out_f32s++ = F32_TO_F32S(CLIP_NATURAL_FLOAT(src->l + src->r));
+        src++;
+    }
+}
+
 static void clip_natural_float_from_stereo(
     void *dst, const struct st_sample *src, int samples)
 {
@@ -334,9 +379,27 @@ static void clip_natural_float_from_stereo(
     }
 }
 
-f_sample *mixeng_clip_float[2] = {
-    clip_natural_float_from_mono,
-    clip_natural_float_from_stereo,
+static void clip_swap_float_from_stereo(
+    void *dst, const struct st_sample *src, int samples)
+{
+    uint32_t *out_f32s = dst;
+
+    while (samples--) {
+        *out_f32s++ = F32_TO_F32S(CLIP_NATURAL_FLOAT(src->l));
+        *out_f32s++ = F32_TO_F32S(CLIP_NATURAL_FLOAT(src->r));
+        src++;
+    }
+}
+
+f_sample *mixeng_clip_float[2][2] = {
+    {
+        clip_natural_float_from_mono,
+        clip_swap_float_from_mono,
+    },
+    {
+        clip_natural_float_from_stereo,
+        clip_swap_float_from_stereo,
+    }
 };
 
 void audio_sample_to_uint64(const void *samples, int pos,
diff --git a/audio/mixeng.h b/audio/mixeng.h
index a5f56d2c26..ead93ac2f7 100644
--- a/audio/mixeng.h
+++ b/audio/mixeng.h
@@ -42,9 +42,9 @@ typedef void (f_sample) (void *dst, const struct st_sample *src, int samples);
 extern t_sample *mixeng_conv[2][2][2][3];
 extern f_sample *mixeng_clip[2][2][2][3];
 
-/* indices: [stereo] */
-extern t_sample *mixeng_conv_float[2];
-extern f_sample *mixeng_clip_float[2];
+/* indices: [stereo][swap endianness] */
+extern t_sample *mixeng_conv_float[2][2];
+extern f_sample *mixeng_clip_float[2][2];
 
 void *st_rate_start (int inrate, int outrate);
 void st_rate_flow(void *opaque, st_sample *ibuf, st_sample *obuf,
-- 
2.43.0

Re: [PATCH 0/7] audio related fixes for 10.1

[Marc-André Lureau]

Hi

On Sun, May 11, 2025 at 9:37 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>
> A few audio related fixes for 10.1.
>
> The virtio-sound device is the first QEMU audio front end that supports floating point samples. The audio subsystem is only partially prepared for this. The commit message of patch 7/7 "audio: add float sample endianness converters" has the details. The new code paths in patch 7/7 are only compile tested. I don't have a big endian host to test.
>
> checkpatch.pl complains about missing space characters in the type punning macros in patch 7/7. I don't agree.
>
> Volker Rümelin (7):
>   tests/functional: use 'none' audio driver for q800 tests
>   audio: fix SIGSEGV in AUD_get_buffer_size_out()
>   audio: fix size calculation in AUD_get_buffer_size_out()
>   hw/audio/asc: fix SIGSEGV in asc_realize()
>   hw/audio/asc: replace g_malloc0() with g_malloc()
>   audio/mixeng: remove unnecessary pointer type casts
>   audio: add float sample endianness converters
>
>  audio/audio.c                        | 11 +++-
>  audio/audio_template.h               | 12 ++--
>  audio/mixeng.c                       | 83 ++++++++++++++++++++++++----
>  audio/mixeng.h                       |  6 +-
>  hw/audio/asc.c                       |  9 ++-
>  tests/functional/test_m68k_q800.py   |  3 +-
>  tests/functional/test_m68k_replay.py |  3 +-
>  7 files changed, 106 insertions(+), 21 deletions(-)
>
> --
> 2.43.0
>
>

For the series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau

Re: [PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Mark Cave-Ayland]

On 11/05/2025 08:38, Volker Rümelin wrote:

> AUD_open_out() may fail and return NULL. This may then lead to
> a segmentation fault in memset() below. The memset() behaviour
> is undefined if the pointer to the destination object is a null
> pointer.
> 
> Add the missing error handling code.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   hw/audio/asc.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
> index 18382ccf6a..03dee0fcc7 100644
> --- a/hw/audio/asc.c
> +++ b/hw/audio/asc.c
> @@ -12,6 +12,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/timer.h"
> +#include "qapi/error.h"
>   #include "hw/sysbus.h"
>   #include "hw/irq.h"
>   #include "audio/audio.h"
> @@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error **errp)
>   
>       s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s, asc_out_cb,
>                               &as);
> +    if (!s->voice) {
> +        asc_unrealize(dev);

I don't think it is ever right for a device to unrealize itself. If the aim is to 
handle cleanup, then since s->mixbuf and s->silentbuf are yet to be allocated by this 
point then I think you can simply call error_setg() and return.

> +        error_setg(errp, "Initializing audio stream failed");
> +        return;
> +    }
> +
>       s->shift = 1;
>       s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
>       s->mixbuf = g_malloc0(s->samples << s->shift);


ATB,

Mark.

Re: [PATCH 5/7] hw/audio/asc: replace g_malloc0() with g_malloc()

[Mark Cave-Ayland]

On 11/05/2025 08:38, Volker Rümelin wrote:

> There is no need to allocate initialized memory with g_malloc0()
> if it's directly followed by a memset() function call. g_malloc()
> is sufficient.
> 
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>   hw/audio/asc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
> index 03dee0fcc7..f12268e296 100644
> --- a/hw/audio/asc.c
> +++ b/hw/audio/asc.c
> @@ -664,7 +664,7 @@ static void asc_realize(DeviceState *dev, Error **errp)
>       s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
>       s->mixbuf = g_malloc0(s->samples << s->shift);
>   
> -    s->silentbuf = g_malloc0(s->samples << s->shift);
> +    s->silentbuf = g_malloc(s->samples << s->shift);
>       memset(s->silentbuf, 0x80, s->samples << s->shift);
>   
>       /* Add easc registers if required */

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Volker Rümelin]

Am 11.05.25 um 13:52 schrieb Mark Cave-Ayland:
> On 11/05/2025 08:38, Volker Rümelin wrote:
>
>> AUD_open_out() may fail and return NULL. This may then lead to
>> a segmentation fault in memset() below. The memset() behaviour
>> is undefined if the pointer to the destination object is a null
>> pointer.
>>
>> Add the missing error handling code.
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>   hw/audio/asc.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
>> index 18382ccf6a..03dee0fcc7 100644
>> --- a/hw/audio/asc.c
>> +++ b/hw/audio/asc.c
>> @@ -12,6 +12,7 @@
>>     #include "qemu/osdep.h"
>>   #include "qemu/timer.h"
>> +#include "qapi/error.h"
>>   #include "hw/sysbus.h"
>>   #include "hw/irq.h"
>>   #include "audio/audio.h"
>> @@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error
>> **errp)
>>         s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s,
>> asc_out_cb,
>>                               &as);
>> +    if (!s->voice) {
>> +        asc_unrealize(dev);
>
> I don't think it is ever right for a device to unrealize itself. If
> the aim is to handle cleanup, then since s->mixbuf and s->silentbuf
> are yet to be allocated by this point then I think you can simply call
> error_setg() and return.

Hi Mark,

yes my aim was to handle cleanup. When calling asc_unrealize() I don't
have to think about which steps are necessary. In this case I would have
to call AUD_remove_card(&s->card). Therefore, I would like to keep my
patch version.

With best regards,
Volker

>
>> +        error_setg(errp, "Initializing audio stream failed");
>> +        return;
>> +    }
>> +
>>       s->shift = 1;
>>       s->samples = AUD_get_buffer_size_out(s->voice) >> s->shift;
>>       s->mixbuf = g_malloc0(s->samples << s->shift);
>
>
> ATB,
>
> Mark.
>

Re: [PATCH 0/7] audio related fixes for 10.1

[Michael Tokarev]

11.05.2025 10:36, Volker Rümelin wrote:
> A few audio related fixes for 10.1.
> 
> The virtio-sound device is the first QEMU audio front end that supports floating point samples. The audio subsystem is only partially prepared for this. The commit message of patch 7/7 "audio: add float sample endianness converters" has the details. The new code paths in patch 7/7 are only compile tested. I don't have a big endian host to test.
> 
> checkpatch.pl complains about missing space characters in the type punning macros in patch 7/7. I don't agree.
> 
> Volker Rümelin (7):
>    tests/functional: use 'none' audio driver for q800 tests
>    audio: fix SIGSEGV in AUD_get_buffer_size_out()
>    audio: fix size calculation in AUD_get_buffer_size_out()
>    hw/audio/asc: fix SIGSEGV in asc_realize()
>    hw/audio/asc: replace g_malloc0() with g_malloc()
>    audio/mixeng: remove unnecessary pointer type casts
>    audio: add float sample endianness converters
> 
>   audio/audio.c                        | 11 +++-
>   audio/audio_template.h               | 12 ++--
>   audio/mixeng.c                       | 83 ++++++++++++++++++++++++----
>   audio/mixeng.h                       |  6 +-
>   hw/audio/asc.c                       |  9 ++-
>   tests/functional/test_m68k_q800.py   |  3 +-
>   tests/functional/test_m68k_replay.py |  3 +-
>   7 files changed, 106 insertions(+), 21 deletions(-)

It looks like (some of) these patches should go to 10.0-stable too,
what do you think?

Thanks,

/mjt

Re: [PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Mark Cave-Ayland]

On 13/05/2025 07:14, Volker Rümelin wrote:

> Am 11.05.25 um 13:52 schrieb Mark Cave-Ayland:
>> On 11/05/2025 08:38, Volker Rümelin wrote:
>>
>>> AUD_open_out() may fail and return NULL. This may then lead to
>>> a segmentation fault in memset() below. The memset() behaviour
>>> is undefined if the pointer to the destination object is a null
>>> pointer.
>>>
>>> Add the missing error handling code.
>>>
>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>> ---
>>>    hw/audio/asc.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
>>> index 18382ccf6a..03dee0fcc7 100644
>>> --- a/hw/audio/asc.c
>>> +++ b/hw/audio/asc.c
>>> @@ -12,6 +12,7 @@
>>>      #include "qemu/osdep.h"
>>>    #include "qemu/timer.h"
>>> +#include "qapi/error.h"
>>>    #include "hw/sysbus.h"
>>>    #include "hw/irq.h"
>>>    #include "audio/audio.h"
>>> @@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error
>>> **errp)
>>>          s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s,
>>> asc_out_cb,
>>>                                &as);
>>> +    if (!s->voice) {
>>> +        asc_unrealize(dev);
>>
>> I don't think it is ever right for a device to unrealize itself. If
>> the aim is to handle cleanup, then since s->mixbuf and s->silentbuf
>> are yet to be allocated by this point then I think you can simply call
>> error_setg() and return.
> 
> Hi Mark,
> 
> yes my aim was to handle cleanup. When calling asc_unrealize() I don't
> have to think about which steps are necessary. In this case I would have
> to call AUD_remove_card(&s->card). Therefore, I would like to keep my
> patch version.

Hi Volker,

I can see why you want to call a single routine to handle tidy-up, however it is 
still expected that the various qdev device callbacks are only called internally by 
the qdev APIs. For example when debugging lifecycle issues you typically put 
breakpoints on init/realize/unrealize/finalize to figure out why something is not 
working as expected.

A common example of this is reset in that a chip can be reset by qdev, but also by 
writing to a reset register via MMIO: in this case both the Resettable interface and 
the MMIO access simply call the same internal reset function.

You could have a separate cleanup function and call it for the error case here, but 
since the qdev device callbacks are called in a strict order, you can guarantee that 
s->mixbuf and s->silentbuf will be unset at this point. So it's probably easiest just 
to call AUD_remove_card(&s->card) before the return and that guarantees that you 
won't interfere with any internal qdev management logic.


ATB,

Mark.

Re: [PATCH 4/7] hw/audio/asc: fix SIGSEGV in asc_realize()

[Volker Rümelin]

Am 13.05.25 um 10:17 schrieb Mark Cave-Ayland:
> On 13/05/2025 07:14, Volker Rümelin wrote:
>
>> Am 11.05.25 um 13:52 schrieb Mark Cave-Ayland:
>>> On 11/05/2025 08:38, Volker Rümelin wrote:
>>>
>>>> AUD_open_out() may fail and return NULL. This may then lead to
>>>> a segmentation fault in memset() below. The memset() behaviour
>>>> is undefined if the pointer to the destination object is a null
>>>> pointer.
>>>>
>>>> Add the missing error handling code.
>>>>
>>>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>>>> ---
>>>>    hw/audio/asc.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/hw/audio/asc.c b/hw/audio/asc.c
>>>> index 18382ccf6a..03dee0fcc7 100644
>>>> --- a/hw/audio/asc.c
>>>> +++ b/hw/audio/asc.c
>>>> @@ -12,6 +12,7 @@
>>>>      #include "qemu/osdep.h"
>>>>    #include "qemu/timer.h"
>>>> +#include "qapi/error.h"
>>>>    #include "hw/sysbus.h"
>>>>    #include "hw/irq.h"
>>>>    #include "audio/audio.h"
>>>> @@ -653,6 +654,12 @@ static void asc_realize(DeviceState *dev, Error
>>>> **errp)
>>>>          s->voice = AUD_open_out(&s->card, s->voice, "asc.out", s,
>>>> asc_out_cb,
>>>>                                &as);
>>>> +    if (!s->voice) {
>>>> +        asc_unrealize(dev);
>>>
>>> I don't think it is ever right for a device to unrealize itself. If
>>> the aim is to handle cleanup, then since s->mixbuf and s->silentbuf
>>> are yet to be allocated by this point then I think you can simply call
>>> error_setg() and return.
>>
>> Hi Mark,
>>
>> yes my aim was to handle cleanup. When calling asc_unrealize() I don't
>> have to think about which steps are necessary. In this case I would have
>> to call AUD_remove_card(&s->card). Therefore, I would like to keep my
>> patch version.
>
> Hi Volker,
>
> I can see why you want to call a single routine to handle tidy-up,
> however it is still expected that the various qdev device callbacks
> are only called internally by the qdev APIs. For example when
> debugging lifecycle issues you typically put breakpoints on
> init/realize/unrealize/finalize to figure out why something is not
> working as expected.
>
> A common example of this is reset in that a chip can be reset by qdev,
> but also by writing to a reset register via MMIO: in this case both
> the Resettable interface and the MMIO access simply call the same
> internal reset function.
>
> You could have a separate cleanup function and call it for the error
> case here, but since the qdev device callbacks are called in a strict
> order, you can guarantee that s->mixbuf and s->silentbuf will be unset
> at this point. So it's probably easiest just to call
> AUD_remove_card(&s->card) before the return and that guarantees that
> you won't interfere with any internal qdev management logic.

Ok, thanks for the explanation. I will send a version 2 patch series.

With best regards,
Volker

Re: [PATCH 0/7] audio related fixes for 10.1

[Volker Rümelin]

Am 13.05.25 um 08:40 schrieb Michael Tokarev:
> 11.05.2025 10:36, Volker Rümelin wrote:
>> A few audio related fixes for 10.1.
>>
>> The virtio-sound device is the first QEMU audio front end that
>> supports floating point samples. The audio subsystem is only
>> partially prepared for this. The commit message of patch 7/7 "audio:
>> add float sample endianness converters" has the details. The new code
>> paths in patch 7/7 are only compile tested. I don't have a big endian
>> host to test.
>>
>> checkpatch.pl complains about missing space characters in the type
>> punning macros in patch 7/7. I don't agree.
>>
>> Volker Rümelin (7):
>>    tests/functional: use 'none' audio driver for q800 tests
>>    audio: fix SIGSEGV in AUD_get_buffer_size_out()
>>    audio: fix size calculation in AUD_get_buffer_size_out()
>>    hw/audio/asc: fix SIGSEGV in asc_realize()
>>    hw/audio/asc: replace g_malloc0() with g_malloc()
>>    audio/mixeng: remove unnecessary pointer type casts
>>    audio: add float sample endianness converters
>>
>>   audio/audio.c                        | 11 +++-
>>   audio/audio_template.h               | 12 ++--
>>   audio/mixeng.c                       | 83 ++++++++++++++++++++++++----
>>   audio/mixeng.h                       |  6 +-
>>   hw/audio/asc.c                       |  9 ++-
>>   tests/functional/test_m68k_q800.py   |  3 +-
>>   tests/functional/test_m68k_replay.py |  3 +-
>>   7 files changed, 106 insertions(+), 21 deletions(-)
>
> It looks like (some of) these patches should go to 10.0-stable too,
> what do you think?

I would perhaps select patch 1/7 (‘tests/functional: use “none” audio
driver for q800 tests’). I don't see any serious bugs fixed by these
patches.

With best regards,
Volker

>
> Thanks,
>
> /mjt
>
>

Re: [PATCH 0/7] audio related fixes for 10.1

[Michael Tokarev]

On 14.05.2025 09:26, Volker Rümelin wrote:
..
> I would perhaps select patch 1/7 (‘tests/functional: use “none” audio
> driver for q800 tests’). I don't see any serious bugs fixed by these
> patches.

This makes sense. Thank you, and please excuse me for the extra
noise - sometimes maintainers are forgetting about stable, so I
might ask extra questions :)

/mjt