Am Mi., 9. Sept. 2026 um 09:29 Uhr schrieb Manos Pitsidianakis <[email protected]>: > > On Mon, 03 Aug 2026 11:11, Alexander Mikhalitsyn <[email protected]> > wrote: > >From: Volker Rümelin <[email protected]> > > > >So far, only rudimentary checks have been made to ensure that > >the guest only performs state transitions described in > >virtio-v1.2-csd01 5.14.6.6.1 PCM Command Lifecycle. While this > >is not a Device Requirement, let's add a state variable > >per audio stream and check all state transitions. > > > >Because only permitted state transitions are now possible, only one > >copy of the audio stream parameters is required and these do not > >need to be initialised with default values. > > > >The state variable will also make it easier to restore the audio > >stream after migration. > > > >Signed-off-by: Volker Rümelin <[email protected]> > >[AM: there were too many conflicts, I did `git checkout --ours -- <.>` > > and then reimplemented the patch idea > >/AM] > >Signed-off-by: Alexander Mikhalitsyn <[email protected]> > >--- > >v5: > > - checkpatch fix > >v4: > > - switched to enum virtio_snd_pcm_state > > (suggested by Manos Pitsidianakis) > > - call virtio_snd_pcm_close() when switching to > > VSND_PCMSTREAM_STATE_PARAMS_SET and VSND_PCMSTREAM_STATE_RELEASED > > states > > - added virtio_error() in case when stream->state is unexpected > > - fixed an error message text in virtio_snd_set_pcm_params > > (suggestions from Marc-André Lureau) > >v3: > > - explicitly call virtio_snd_pcm_close() from unrealize > > - remove a call to virtio_snd_pcm_flush() from virtio_snd_pcm_close() > > [ ^ this was my rebase mistake ] > >--- > > hw/audio/virtio-snd.c | 213 ++++++++++++++++++++-------------- > > include/hw/audio/virtio-snd.h | 17 +-- > > 2 files changed, 124 insertions(+), 106 deletions(-) > > > >diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c > >index 46026b8cffc..1c218569cfd 100644 > >--- a/hw/audio/virtio-snd.c > >+++ b/hw/audio/virtio-snd.c > >@@ -30,11 +30,26 @@ > > #define VIRTIO_SOUND_CHMAP_DEFAULT 0 > > #define VIRTIO_SOUND_HDA_FN_NID 0 > > > >+typedef enum virtio_snd_pcm_state { > >+ VIRTIO_SND_PCM_STATE_UNINIT = 0, > >+ VIRTIO_SND_PCM_STATE_PARAMS_SET, > >+ VIRTIO_SND_PCM_STATE_PREPARED, > >+ VIRTIO_SND_PCM_STATE_STARTED, > >+ VIRTIO_SND_PCM_STATE_STOPPED, > >+ VIRTIO_SND_PCM_STATE_RELEASED, > >+} virtio_snd_pcm_state; > >+ > >+static inline bool virtio_snd_pcm_state_prepared(virtio_snd_pcm_state s) > >+{ > >+ return s > VIRTIO_SND_PCM_STATE_PARAMS_SET && > >+ s < VIRTIO_SND_PCM_STATE_RELEASED; > >+} > >+ > > 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_close(VirtIOSoundPCMStream *stream); > > static void virtio_snd_pcm_in_cb(void *data, int available); > >-static void virtio_snd_unrealize(DeviceState *dev); > > > > static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8) > > | BIT(VIRTIO_SND_PCM_FMT_U8) > >@@ -129,7 +144,7 @@ static VirtIOSoundPCMStream > >*virtio_snd_pcm_get_stream(VirtIOSound *s, > > uint32_t stream_id) > > { > > return stream_id >= s->snd_conf.streams ? NULL : > >- s->pcm.streams[stream_id]; > >+ &s->streams[stream_id]; > > } > > > > /* > >@@ -141,8 +156,8 @@ static VirtIOSoundPCMStream > >*virtio_snd_pcm_get_stream(VirtIOSound *s, > > static virtio_snd_pcm_set_params *virtio_snd_pcm_get_params(VirtIOSound *s, > > uint32_t > > stream_id) > > { > >- return stream_id >= s->snd_conf.streams ? NULL > >- : &s->pcm.pcm_params[stream_id]; > >+ return stream_id >= s->snd_conf.streams ? NULL : > >+ &s->streams[stream_id].params; > > } > > > > /* > >@@ -245,11 +260,10 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s, > > > > /* > > * Set the given stream params. > >- * Called by both virtio_snd_handle_pcm_set_params and during device > >- * initialization. > > * Returns the response status code. (VIRTIO_SND_S_*). > > * > > * @s: VirtIOSound device > >+ * @stream_id: stream id > > * @params: The PCM params as defined in the virtio specification > > */ > > static > >@@ -257,14 +271,27 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, > > uint32_t stream_id, > > virtio_snd_pcm_set_params *params) > > { > >+ VirtIOSoundPCMStream *stream; > > virtio_snd_pcm_set_params *st_params; > > > >- if (stream_id >= s->snd_conf.streams || s->pcm.pcm_params == NULL) { > >- virtio_error(VIRTIO_DEVICE(s), "Streams have not been > >initialized.\n"); > >+ stream = virtio_snd_pcm_get_stream(s, stream_id); > >+ if (!stream) { > >+ virtio_error(VIRTIO_DEVICE(s), "invalid stream id: %"PRIu32, > >+ stream_id); > > return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > > } > > > >- st_params = virtio_snd_pcm_get_params(s, stream_id); > >+ switch (stream->state) { > >+ case VIRTIO_SND_PCM_STATE_UNINIT: > >+ case VIRTIO_SND_PCM_STATE_PARAMS_SET: > >+ case VIRTIO_SND_PCM_STATE_PREPARED: > >+ case VIRTIO_SND_PCM_STATE_RELEASED: > >+ break; > >+ default: > >+ virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, > >+ stream->state); > >+ return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > >+ } > > > > if (params->channels < 1 || params->channels > AUDIO_MAX_CHANNELS) { > > error_report("Number of channels is not supported."); > >@@ -281,6 +308,8 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, > > return cpu_to_le32(VIRTIO_SND_S_NOT_SUPP); > > } > > > >+ st_params = virtio_snd_pcm_get_params(s, stream_id); > >+ > > st_params->buffer_bytes = le32_to_cpu(params->buffer_bytes); > > st_params->period_bytes = le32_to_cpu(params->period_bytes); > > st_params->features = le32_to_cpu(params->features); > >@@ -289,6 +318,15 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s, > > st_params->format = params->format; > > st_params->rate = params->rate; > > > >+ if (virtio_snd_pcm_state_prepared(stream->state)) { > >+ /* implicit VIRTIO_SND_R_PCM_RELEASE */ > >+ virtio_snd_pcm_flush(stream); > >+ } > >+ > >+ virtio_snd_pcm_close(stream); > >+ > >+ stream->state = VIRTIO_SND_PCM_STATE_PARAMS_SET; > >+ > > return cpu_to_le32(VIRTIO_SND_S_OK); > > } > > > >@@ -398,15 +436,12 @@ static void > >virtio_snd_get_qemu_audsettings(audsettings *as, > > */ > > static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream) > > { > >- if (stream) { > >- virtio_snd_pcm_flush(stream); > >- if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { > >- audio_be_close_out(stream->s->audio_be, stream->voice.out); > >- stream->voice.out = NULL; > >- } else if (stream->info.direction == VIRTIO_SND_D_INPUT) { > >- audio_be_close_in(stream->s->audio_be, stream->voice.in); > >- stream->voice.in = NULL; > >- } > >+ if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { > >+ audio_be_close_out(stream->s->audio_be, stream->voice.out); > >+ stream->voice.out = NULL; > >+ } else if (stream->info.direction == VIRTIO_SND_D_INPUT) { > >+ audio_be_close_in(stream->s->audio_be, stream->voice.in); > >+ stream->voice.in = NULL; > > } > > } > > > >@@ -423,33 +458,26 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, > >uint32_t stream_id) > > virtio_snd_pcm_set_params *params; > > VirtIOSoundPCMStream *stream; > > > >- if (s->pcm.streams == NULL || > >- s->pcm.pcm_params == NULL || > >- stream_id >= s->snd_conf.streams) { > >+ stream = virtio_snd_pcm_get_stream(s, stream_id); > >+ if (!stream) { > > return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > > } > > > >- params = virtio_snd_pcm_get_params(s, stream_id); > >- if (params == NULL) { > >+ switch (stream->state) { > >+ case VIRTIO_SND_PCM_STATE_PARAMS_SET: > >+ case VIRTIO_SND_PCM_STATE_PREPARED: > >+ case VIRTIO_SND_PCM_STATE_RELEASED: > >+ break; > >+ default: > >+ virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, > >+ stream->state); > > return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > > } > > > >- stream = virtio_snd_pcm_get_stream(s, stream_id); > >- if (stream == NULL) { > >- stream = &s->streams[stream_id]; > >- stream->active = false; > >- stream->latency_bytes = 0; > >- > >- /* > >- * stream_id >= s->snd_conf.streams was checked before so this is > >- * in-bounds > >- */ > >- s->pcm.streams[stream_id] = stream; > >- } > >+ params = virtio_snd_pcm_get_params(s, stream_id); > > > > virtio_snd_get_qemu_audsettings(&as, params); > > stream->info.channels_max = as.nchannels; > >- stream->params = *params; > > > > stream->as = as; > > > >@@ -471,6 +499,8 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, > >uint32_t stream_id) > > audio_be_set_volume_in_lr(s->audio_be, stream->voice.in, 0, 255, > > 255); > > } > > > >+ stream->state = VIRTIO_SND_PCM_STATE_PREPARED; > >+ > > return cpu_to_le32(VIRTIO_SND_S_OK); > > } > > > >@@ -545,7 +575,31 @@ static uint32_t virtio_snd_pcm_start_stop(VirtIOSound > >*s, > > return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > > } > > > >- stream->active = start; > >+ if (start) { > >+ switch (stream->state) { > >+ case VIRTIO_SND_PCM_STATE_PREPARED: > >+ case VIRTIO_SND_PCM_STATE_STOPPED: > >+ break; > >+ default: > >+ virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: > >%"PRIu32, > >+ stream->state); > >+ return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > >+ } > >+ > >+ stream->state = VIRTIO_SND_PCM_STATE_STARTED; > >+ } else { > >+ switch (stream->state) { > >+ case VIRTIO_SND_PCM_STATE_STARTED: > >+ break; > >+ default: > >+ virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: > >%"PRIu32, > >+ stream->state); > >+ return cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > >+ } > >+ > >+ stream->state = VIRTIO_SND_PCM_STATE_STOPPED; > >+ } > >+ > > if (stream->info.direction == VIRTIO_SND_D_OUTPUT) { > > audio_be_set_active_out(s->audio_be, stream->voice.out, start); > > } else { > >@@ -639,6 +693,17 @@ static void virtio_snd_handle_pcm_release(VirtIOSound > >*s, > > return; > > } > > > >+ switch (stream->state) { > >+ case VIRTIO_SND_PCM_STATE_PREPARED: > >+ case VIRTIO_SND_PCM_STATE_STOPPED: > >+ break; > >+ default: > >+ virtio_error(VIRTIO_DEVICE(s), "unexpected stream state: %"PRIu32, > >+ stream->state); > >+ cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > >+ return; > >+ } > >+ > > if (virtio_snd_pcm_get_io_msgs_count(stream)) { > > /* > > * virtio-v1.2-csd01, 5.14.6.6.5.1, > >@@ -653,6 +718,10 @@ static void virtio_snd_handle_pcm_release(VirtIOSound > >*s, > > virtio_snd_pcm_flush(stream); > > } > > > >+ virtio_snd_pcm_close(stream); > >+ > >+ stream->state = VIRTIO_SND_PCM_STATE_RELEASED; > >+ > > cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK); > > } > > > >@@ -876,12 +945,11 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice > >*vdev, VirtQueue *vq) > > size = iov_size(elem->out_sg, elem->out_num) - msg_sz; > > stream_id = le32_to_cpu(hdr.stream_id); > > > >- if (stream_id >= vsnd->snd_conf.streams > >- || vsnd->pcm.streams[stream_id] == NULL) { > >+ if (stream_id >= vsnd->snd_conf.streams) { > > goto tx_err; > > } > > > >- stream = vsnd->pcm.streams[stream_id]; > >+ stream = &vsnd->streams[stream_id]; > > if (stream->info.direction != VIRTIO_SND_D_OUTPUT) { > > goto tx_err; > > } > >@@ -959,15 +1027,13 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice > >*vdev, VirtQueue *vq) > > } > > stream_id = le32_to_cpu(hdr.stream_id); > > > >- if (stream_id >= vsnd->snd_conf.streams > >- || !vsnd->pcm.streams[stream_id]) { > >+ if (stream_id >= vsnd->snd_conf.streams) { > > goto rx_err; > > } > > > >- stream = vsnd->pcm.streams[stream_id]; > >+ stream = &vsnd->streams[stream_id]; > > size = iov_size(elem->in_sg, elem->in_num); > >- if (stream == NULL > >- || stream->info.direction != VIRTIO_SND_D_INPUT > >+ if (stream->info.direction != VIRTIO_SND_D_INPUT > > || size < sizeof(virtio_snd_pcm_status)) { > > goto rx_err; > > } > >@@ -1031,8 +1097,6 @@ static void virtio_snd_realize(DeviceState *dev, 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); > > > >@@ -1069,6 +1133,7 @@ static void virtio_snd_realize(DeviceState *dev, Error > >**errp) > > for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { > > VirtIOSoundPCMStream *stream = &vsnd->streams[i]; > > > >+ stream->state = VIRTIO_SND_PCM_STATE_UNINIT; > > stream->s = vsnd; > > QSIMPLEQ_INIT(&stream->queue); > > stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID; > >@@ -1082,21 +1147,9 @@ static void virtio_snd_realize(DeviceState *dev, > >Error **errp) > > stream->info.channels_max = 2; > > } > > > >- vsnd->pcm.streams = > >- g_new0(VirtIOSoundPCMStream *, vsnd->snd_conf.streams); > >- 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); > >- default_params.period_bytes = cpu_to_le32(2048); > >- default_params.channels = 2; > >- default_params.format = VIRTIO_SND_PCM_FMT_S16; > >- default_params.rate = VIRTIO_SND_PCM_RATE_48000; > > vsnd->queues[VIRTIO_SND_VQ_CONTROL] = > > virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl); > > vsnd->queues[VIRTIO_SND_VQ_EVENT] = > >@@ -1107,28 +1160,6 @@ static void virtio_snd_realize(DeviceState *dev, > >Error **errp) > > virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer); > > 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; > >- } > >- } > >- > >- return; > >- > >-error_cleanup: > >- virtio_snd_unrealize(dev); > > } > > > > static inline void update_latency(VirtIOSoundPCMStream *s, size_t used) > >@@ -1177,7 +1208,7 @@ static void virtio_snd_pcm_out_cb(void *data, int > >available) > > if (!virtio_queue_ready(buffer->vq)) { > > return; > > } > >- if (!stream->active) { > >+ if (stream->state != VIRTIO_SND_PCM_STATE_STARTED) { > > /* Stream has stopped, so do not perform audio_be_write. */ > > > The spec says in 5.14.6.6.1 PCM Command Lifecycle (between PREPARE and > START states): > > 3. Output only: the driver transfers data for pre-buffing. > > Does this mean the guest cannot prebuf?
Yes, it looks like so. My interpretation is that because of `return_tx_buffer` call we shouldn't lose anything but guest driver may be a bit surprised by this behavior.. > > This is not introduced by this patch but it made me notice it. > > Reviewed-by: Manos Pitsidianakis <[email protected]> Thank you! ;-) > > > return_tx_buffer(stream, buffer); > > continue; > >@@ -1271,7 +1302,7 @@ static void virtio_snd_pcm_in_cb(void *data, int > >available) > > if (!virtio_queue_ready(buffer->vq)) { > > return; > > } > >- if (!stream->active) { > >+ if (stream->state != VIRTIO_SND_PCM_STATE_STARTED) { > > /* Stream has stopped, so do not perform audio_be_read. */ > > return_rx_buffer(stream, buffer); > > continue; > >@@ -1344,17 +1375,16 @@ static void virtio_snd_unrealize(DeviceState *dev) > > qemu_del_vm_change_state_handler(vsnd->vmstate); > > trace_virtio_snd_unrealize(vsnd); > > > >- if (vsnd->pcm.streams) { > >+ if (vsnd->streams) { > >+ virtio_snd_process_cmdq(vsnd); > > for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { > >- stream = vsnd->pcm.streams[i]; > >- if (stream) { > >- virtio_snd_process_cmdq(stream->s); > >- virtio_snd_pcm_close(stream); > >+ stream = &vsnd->streams[i]; > >+ if (virtio_snd_pcm_state_prepared(stream->state)) { > >+ virtio_snd_pcm_flush(stream); > > } > >+ virtio_snd_pcm_close(stream); > > } > >- g_free(vsnd->pcm.streams); > > } > >- g_free(vsnd->pcm.pcm_params); > > g_free(vsnd->streams); > > vsnd->streams = NULL; > > virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]); > >@@ -1387,6 +1417,9 @@ static void virtio_snd_reset(VirtIODevice *vdev) > > VirtIOSoundPCMStream *stream = &vsnd->streams[i]; > > VirtIOSoundPCMBuffer *buffer; > > > >+ virtio_snd_pcm_close(stream); > >+ stream->state = VIRTIO_SND_PCM_STATE_UNINIT; > >+ > > while ((buffer = QSIMPLEQ_FIRST(&stream->queue))) { > > QSIMPLEQ_REMOVE_HEAD(&stream->queue, entry); > > virtio_snd_pcm_buffer_free(buffer); > >diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h > >index 97ffaae18be..85d5d7c8619 100644 > >--- a/include/hw/audio/virtio-snd.h > >+++ b/include/hw/audio/virtio-snd.h > >@@ -75,8 +75,6 @@ typedef struct VirtIOSoundPCMStream VirtIOSoundPCMStream; > > > > typedef struct virtio_snd_ctrl_command virtio_snd_ctrl_command; > > > >-typedef struct VirtIOSoundPCM VirtIOSoundPCM; > >- > > typedef struct VirtIOSoundPCMBuffer VirtIOSoundPCMBuffer; > > > > /* > >@@ -121,28 +119,16 @@ struct VirtIOSoundPCMBuffer { > > uint8_t data[]; > > }; > > > >-struct VirtIOSoundPCM { > >- /* > >- * PCM parameters are a separate field instead of a VirtIOSoundPCMStream > >- * field, because the operation of PCM control requests is first > >- * VIRTIO_SND_R_PCM_SET_PARAMS and then VIRTIO_SND_R_PCM_PREPARE; this > >- * means that some times we get parameters without having an allocated > >- * stream yet. > >- */ > >- virtio_snd_pcm_set_params *pcm_params; > >- VirtIOSoundPCMStream **streams; > >-}; > >- > > struct VirtIOSoundPCMStream { > > virtio_snd_pcm_info info; > > virtio_snd_pcm_set_params params; > >+ uint32_t state; > > VirtIOSound *s; > > audsettings as; > > union { > > SWVoiceIn *in; > > SWVoiceOut *out; > > } voice; > >- bool active; > > uint32_t latency_bytes; > > QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue; > > }; > >@@ -209,7 +195,6 @@ struct VirtIOSound { > > > > VirtQueue *queues[VIRTIO_SND_VQ_MAX]; > > uint64_t features; > >- VirtIOSoundPCM pcm; > > VirtIOSoundPCMStream *streams; > > AudioBackend *audio_be; > > VMChangeStateEntry *vmstate; > >-- > >2.47.3 > >
