Fix guest sound playback XRUN issue

README.md

@@ -12,12 +12,7 @@ A minimalist RISC-V system emulator capable of running Linux the kernel and corr
 - I/O support using VirtIO standard:
     - virtio-blk acquires disk image from the host.
     - virtio-net is mapped as TAP interface.
-    - virtio-snd uses [PortAudio](https://github.com/PortAudio/portaudio) for sound playback on the host with one limitations:
-        - As some unknown issues in guest Linux OS (confirmed in v6.7 and v6.12), you need
-          to adjust the buffer size to more than four times of period size, or
-          the program cannot write the PCM frames into guest OS ALSA stack.
-            - For instance, the following buffer/period size settings on `aplay` has been tested
-              with broken and stutter effects yet complete with no any errors: `aplay --buffer-size=32768 --period-size=4096 /usr/share/sounds/alsa/Front_Center.wav`.
+    - virtio-snd uses [PortAudio](https://github.com/PortAudio/portaudio) for sound playback on the host.   
     - virtio-input exposes SDL-backed keyboard and mouse devices to the guest.
     - virtio-gpu exposes a minimal 2D DRM/KMS device to the guest. Linux can
       bind the `virtio_gpu` driver and create `/dev/dri/card0`.

device.h

@@ -581,7 +581,7 @@ typedef struct {
 #if SEMU_HAS(VIRTIOGPU)
     virtio_gpu_state_t vgpu;
 #endif
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     /* Use self-pipe trick to unblock the emulator loop when the window backend
      * has queued work, such as input events or window shutdown. When all harts
      * are idle, 'semu_run()' can call 'poll(-1)' and block indefinitely

main.c

@@ -32,7 +32,7 @@
 #if SEMU_HAS(VIRTIOGPU)
 #include "vgpu-display.h"
 #endif
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
 #include "window.h"
 #endif
 #include "riscv.h"
@@ -267,7 +267,7 @@ static inline void emu_tick_peripherals(emu_state_t *emu)
         if (virtio_input_irq_pending(&emu->vmouse))
             emu_update_vinput_mouse_interrupts(vm);
 #endif
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
         /* A closed window is treated like a frontend shutdown request. */
         if (g_window.window_is_closed())
             emu->stopped = true;
@@ -1069,7 +1069,7 @@ static int semu_init(emu_state_t *emu, int argc, char **argv)
     vgpu_display_set_scanout_count(scanout_id + 1U);
 #endif
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     g_window.window_init(headless, SCREEN_WIDTH, SCREEN_HEIGHT);
 
     emu->wake_fd[0] = emu->wake_fd[1] = -1;
@@ -1112,7 +1112,7 @@ static int semu_init(emu_state_t *emu, int argc, char **argv)
         if (!coro_init(total_slots, vm->n_hart)) {
             fprintf(stderr, "Failed to initialize coroutine subsystem\n");
             fflush(stderr);
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
             if (emu->wake_fd[0] >= 0)
                 close(emu->wake_fd[0]);
             if (emu->wake_fd[1] >= 0)
@@ -1128,7 +1128,7 @@ static int semu_init(emu_state_t *emu, int argc, char **argv)
             if (!coro_create_hart(i, hart_exec_loop, vm->hart[i])) {
                 fprintf(stderr, "Failed to create coroutine for hart %u\n", i);
                 coro_cleanup();
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
                 if (emu->wake_fd[0] >= 0)
                     close(emu->wake_fd[0]);
                 if (emu->wake_fd[1] >= 0)
@@ -1333,7 +1333,7 @@ static void signal_handler(int sig UNUSED)
     }
 }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
 static void semu_close_wake_pipe(emu_state_t *emu)
 {
     signal_wake_fd = -1;
@@ -1525,7 +1525,7 @@ static void semu_run(emu_state_t *emu)
              * plus an optional wake pipe when a window backend is enabled.
              */
             size_t needed = 2;
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
             if (emu->wake_fd[0] >= 0)
                 needed++;
 #endif
@@ -1626,7 +1626,7 @@ static void semu_run(emu_state_t *emu)
                 pfd_count++;
             }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
             /* Always watch the wake pipe so that backend work such as input
              * events or SDL window close unblocks 'poll(-1)' immediately.
              */
@@ -1692,7 +1692,7 @@ static void semu_run(emu_state_t *emu)
                 perror("failed to poll emulator events");
             }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
             /* Drain one wake byte if the pipe fired. The virtio-input path
              * coalesces backend wakeups behind a bool gate, so it contributes
              * at most one queued notification byte before the emulator thread
@@ -1743,7 +1743,7 @@ static void semu_run(emu_state_t *emu)
         coro_cleanup();
 
         /* A closed window is a normal user action, not an error. */
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
         if (emu->stopped && !g_window.window_is_closed())
 #else
         if (emu->stopped)
@@ -1851,7 +1851,7 @@ static gdb_action_t semu_cont(void *args)
      * commands can run guest code again.
      */
     signal_received = 0;
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     while (!semu_is_interrupt(emu) && !g_window.window_is_closed()) {
 #else
     while (!semu_is_interrupt(emu)) {
@@ -1867,7 +1867,7 @@ static gdb_action_t semu_cont(void *args)
     /* Clear the interrupt if it's pending */
     __atomic_store_n(&emu->is_interrupted, false, __ATOMIC_RELAXED);
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     /* Tell gdbstub_run() to exit cleanly when the window is closed. */
     if (g_window.window_is_closed())
         return ACT_SHUTDOWN;
@@ -1942,7 +1942,7 @@ static void semu_run_debug(emu_state_t *emu)
     emu->exit_code = ok ? 0 : 1;
 }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
 /* Thread wrapper for backends that reserve the main thread for
  * 'window_main_loop()'.
  */
@@ -1990,7 +1990,7 @@ int main(int argc, char **argv)
         sigaction(SIGTERM, &sa, NULL);
     }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     /* Publish the wake pipe to the signal handler so SIGINT/SIGTERM can
      * unblock the emulator thread's poll() in the threaded window path.
      */
@@ -2032,7 +2032,7 @@ int main(int argc, char **argv)
             semu_run(&emu);
     }
 
-#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU)
+#if SEMU_HAS(VIRTIOINPUT) || SEMU_HAS(VIRTIOGPU) || SEMU_HAS(VIRTIOSND)
     semu_close_wake_pipe(&emu);
     g_window.window_cleanup();
 #endif

virtio-snd.c

@@ -304,6 +304,7 @@ typedef struct {
 } virtio_snd_prop_t;
 
 static virtio_snd_config_t vsnd_configs[VSND_DEV_CNT_MAX];
+
 static virtio_snd_prop_t vsnd_props[VSND_DEV_CNT_MAX] = {
     [0 ... VSND_DEV_CNT_MAX - 1].pp.hdr.hdr.code = VIRTIO_SND_R_PCM_SET_PARAMS,
     [0 ... VSND_DEV_CNT_MAX - 1].lock =
@@ -646,23 +647,10 @@ static void virtio_snd_read_pcm_prepare(const virtio_snd_pcm_hdr_t *query,
     props->pp.hdr.hdr.code = VIRTIO_SND_R_PCM_PREPARE;
     props->v.stream_id = stream_id;
 
-    uint32_t channels = props->pp.channels;
     uint32_t rate = pcm_rate_tbl[props->pp.rate];
 
-    /* Rather use the period_bytes/buffer_bytes set by driver,
-     * we calculate bps rate to achieve the strema's desired parameters
-     * suggested by [1].
-     */
-    /* Reference:
-     * [1]
-     * https://github.com/rust-vmm/vhost-device/blob/main/vhost-device-sound/src/audio_backends/alsa.rs#L153
-     */
-    /* Calculate bps rate. */
-    uint32_t bps_rate = channels * VSND_CNFA_FRAME_SZ * rate;
-    /* Calculate period bytes for ~100ms interrupt period. */
-    uint32_t cnfa_period_bytes = bps_rate / 10;
-    /* Calculate the period size (in frames) for CNFA . */
-    uint32_t cnfa_period_frames = cnfa_period_bytes / VSND_CNFA_FRAME_SZ;
+    /* Calculate the period size (in frames) from the guest's period_bytes */
+    uint32_t cnfa_period_frames = props->pp.period_bytes / VSND_CNFA_FRAME_SZ;
 
     INIT_LIST_HEAD(&props->buf_queue_head);
     props->lock.releasing = 0;
@@ -701,6 +689,11 @@ static void virtio_snd_read_pcm_start(const virtio_snd_pcm_hdr_t *query,
 
     virtio_snd_prop_t *props = &vsnd_props[stream_id];
 
+    /* Clear the releasing flag in case we are restarting from a STOP state */
+    pthread_mutex_lock(&props->lock.lock);
+    props->lock.releasing = 0;
+    pthread_mutex_unlock(&props->lock.lock);
+
     /* Control the callback to start playing */
     props->pp.hdr.hdr.code = VIRTIO_SND_R_PCM_START;
     PaError err = Pa_StartStream(props->pa_stream);
@@ -728,6 +721,14 @@ static void virtio_snd_read_pcm_stop(const virtio_snd_pcm_hdr_t *query,
 
     virtio_snd_prop_t *props = &vsnd_props[stream_id];
 
+    /* Wait for PortAudio to consume all pending buffers before stopping. */
+    pthread_mutex_lock(&props->lock.lock);
+    while (props->lock.buf_ev_notify > 0)
+        pthread_cond_wait(&props->lock.writable, &props->lock.lock);
+    props->lock.releasing = 1;
+    pthread_cond_broadcast(&props->lock.readable);
+    pthread_mutex_unlock(&props->lock.lock);
+
     /* Control the callback to stop playing */
     props->pp.hdr.hdr.code = VIRTIO_SND_R_PCM_STOP;
     PaError err = Pa_StopStream(props->pa_stream);
@@ -856,6 +857,7 @@ static int virtio_snd_stream_cb(const void *input,
 }
 
 #define VSND_DESC_CNT 3
+
 static int virtio_snd_ctrl_desc_handler(virtio_snd_state_t *vsnd,
                                         const virtio_snd_queue_t *queue,
                                         uint32_t desc_idx,
@@ -1049,16 +1051,24 @@ static void virtio_queue_notify_handler(virtio_snd_state_t *vsnd, int index)
         ram[vq_used_addr + 1] = len;    /* virtq_used_elem.len (le32) */
         queue->last_avail++;
         new_used++;
-    }
-
-    /* Check le32 len field of struct virtq_used_elem on the spec  */
-    vsnd->ram[queue->QueueUsed] &= MASK(16); /* Reset low 16 bits to zero */
-    vsnd->ram[queue->QueueUsed] |= ((uint32_t) new_used) << 16; /* len */
+        /* Check le32 len field of struct virtq_used_elem on the spec  */
+        vsnd->ram[queue->QueueUsed] &= MASK(16); /* Reset low 16 bits to zero */
+        vsnd->ram[queue->QueueUsed] |= ((uint32_t) new_used) << 16; /* len */
 
-    /* Publish used-ring writes before making the IRQ visible to the guest. */
-    if (!(ram[queue->QueueAvail] & 1))
-        __atomic_fetch_or(&vsnd->InterruptStatus, VIRTIO_INT__USED_RING,
-                          __ATOMIC_RELEASE);
+        /* Publish used-ring writes before making the IRQ visible to the guest.
+         */
+        if (!(ram[queue->QueueAvail] & 1)) {
+            __atomic_fetch_or(&vsnd->InterruptStatus, VIRTIO_INT__USED_RING,
+                              __ATOMIC_RELEASE);
+            emu_state_t *emu = container_of(vsnd, emu_state_t, vsnd);
+            if (emu->wake_fd[1] >= 0) {
+                char wake_byte = 1;
+                if (write(emu->wake_fd[1], &wake_byte, 1) < 0) {
+                    /* ignore error */
+                }
+            }
+        }
+    }
 }
 
 /* TX thread context */