Am Fr., 26. Juni 2026 um 16:12 Uhr schrieb <[email protected]>:
>
> > So far, only rudimentary checks have been made to ensure that
> > the guest only performs state transitions permitted in
> > virtio-v1.2-csd01 5.14.6.6.1 PCM Command Lifecycle. Add a state
> > variable per audio stream and check all state transitions.
> >
> > Because only permitted state transitions are 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]>
> > Message-ID: <[email protected]>
> >
> > diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
> > index 41c63b3f23b..72ef34e0976 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,31 +119,18 @@ 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;
> > /* channel position values (VIRTIO_SND_CHMAP_XXX) */
> > uint8_t positions[VIRTIO_SND_CHMAP_MAX_SIZE];
> > VirtIOSound *s;
> > - bool flushing;
> > audsettings as;
> > union {
> > SWVoiceIn *in;
> > SWVoiceOut *out;
> > } voice;
> > - bool active;
> > uint32_t latency_bytes;
> > QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
> > };
> > @@ -212,7 +197,6 @@ struct VirtIOSound {
> >
> > VirtQueue *queues[VIRTIO_SND_VQ_MAX];
> > uint64_t features;
> > - VirtIOSoundPCM pcm;
> > VirtIOSoundPCMStream *streams;
> > AudioBackend *audio_be;
> > VMChangeStateEntry *vmstate;
> > diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> > index 300ba13ffeb..68d737478f5 100644
> > --- a/hw/audio/virtio-snd.c
> > +++ b/hw/audio/virtio-snd.c
> > @@ -30,11 +30,30 @@
> > #define VIRTIO_SOUND_CHMAP_DEFAULT 0
> > #define VIRTIO_SOUND_HDA_FN_NID 0
> >
> > +#define VSND_PCMSTREAM_STATE_F_PARAMS_SET 0x10000
> > +#define VSND_PCMSTREAM_STATE_F_PREPARED 0x20000
> > +#define VSND_PCMSTREAM_STATE_F_ACTIVE 0x40000
> > +
> > +#define VSND_PCMSTREAM_STATE_UNINITIALIZED 0
> > +#define VSND_PCMSTREAM_STATE_PARAMS_SET (1 \
> > + |
> > VSND_PCMSTREAM_STATE_F_PARAMS_SET)
> > +#define VSND_PCMSTREAM_STATE_PREPARED (2 \
> > + |
> > VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> > + |
> > VSND_PCMSTREAM_STATE_F_PREPARED)
> > +#define VSND_PCMSTREAM_STATE_STARTED (4 \
> > + |
> > VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> > + |
> > VSND_PCMSTREAM_STATE_F_PREPARED \
> > + | VSND_PCMSTREAM_STATE_F_ACTIVE)
> > +#define VSND_PCMSTREAM_STATE_STOPPED (6 \
> > + |
> > VSND_PCMSTREAM_STATE_F_PARAMS_SET \
> > + |
> > VSND_PCMSTREAM_STATE_F_PREPARED)
> > +#define VSND_PCMSTREAM_STATE_RELEASED (7 \
> > + |
> > VSND_PCMSTREAM_STATE_F_PARAMS_SET)
>
Dear Marc-André,
> Hmm, those 1,2,4,6,7 values matches the spec 5.14.6.6.1 states. Ok, but worth
> a
> comment imho.
Manos made a point that we can simplify these definitions and instead
of using bitmask,
just have enum like:
+ 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,
+ }
(see https://lore.kernel.org/qemu-devel/[email protected]/#t )
WDYT?
>
> > +
> > 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 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 +148,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 +160,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 +264,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 +275,25 @@ 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) {
> > + if (stream_id >= s->snd_conf.streams) {
> > virtio_error(VIRTIO_DEVICE(s), "Streams have not been
> > initialized.\n");
>
> The error message should be updated.
Thanks, I'll fix this in v4.
>
> > return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> > }
> >
> > - st_params = virtio_snd_pcm_get_params(s, stream_id);
> > + stream = virtio_snd_pcm_get_stream(s, stream_id);
> > +
> > + switch (stream->state) {
> > + case VSND_PCMSTREAM_STATE_UNINITIALIZED:
> > + case VSND_PCMSTREAM_STATE_PARAMS_SET:
> > + case VSND_PCMSTREAM_STATE_PREPARED:
> > + case VSND_PCMSTREAM_STATE_RELEASED:
> > + break;
> > + default:
>
> Keep a virtio_error()?
Sure, will do in v4.
>
> > + 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 +310,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 +320,13 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
> > st_params->format = params->format;
> > st_params->rate = params->rate;
> >
> > + if (stream->state & VSND_PCMSTREAM_STATE_F_PREPARED) {
> > + /* implicit VIRTIO_SND_R_PCM_RELEASE */
> > + virtio_snd_pcm_flush(stream);
> > + }
>
> I'd leave a comment to explain we keep the SWVoice opened.
I decided to add virtio_snd_pcm_close(stream) call to
virtio_snd_set_pcm_params().
>
> > +
> > + stream->state = VSND_PCMSTREAM_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,32 +458,23 @@ 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 VSND_PCMSTREAM_STATE_PARAMS_SET:
> > + case VSND_PCMSTREAM_STATE_PREPARED:
> > + case VSND_PCMSTREAM_STATE_RELEASED:
> > + break;
> > + default:
> > 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->params = *params;
> >
> > stream->positions[0] = VIRTIO_SND_CHMAP_FL;
> > stream->positions[1] = VIRTIO_SND_CHMAP_FR;
> > @@ -472,6 +498,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 = VSND_PCMSTREAM_STATE_PREPARED;
> > +
> > return cpu_to_le32(VIRTIO_SND_S_OK);
> > }
> >
> > @@ -542,12 +570,28 @@ static uint32_t virtio_snd_pcm_start_stop(VirtIOSound
> > *s,
> > }
> >
> > if (start) {
> > + switch (stream->state) {
> > + case VSND_PCMSTREAM_STATE_PREPARED:
> > + case VSND_PCMSTREAM_STATE_STOPPED:
> > + break;
> > + default:
> > + return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> > + }
> > +
> > trace_virtio_snd_handle_pcm_start(stream_id);
> > + stream->state = VSND_PCMSTREAM_STATE_STARTED;
> > } else {
> > + switch (stream->state) {
> > + case VSND_PCMSTREAM_STATE_STARTED:
> > + break;
> > + default:
> > + return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> > + }
> > +
> > trace_virtio_snd_handle_pcm_stop(stream_id);
> > + stream->state = VSND_PCMSTREAM_STATE_STOPPED;
> > }
> >
> > - stream->active = start;
> > if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
> > audio_be_set_active_out(s->audio_be, stream->voice.out, start);
> > } else {
> > @@ -641,6 +685,15 @@ static void virtio_snd_handle_pcm_release(VirtIOSound
> > *s,
> > return;
> > }
> >
> > + switch (stream->state) {
> > + case VSND_PCMSTREAM_STATE_PREPARED:
> > + case VSND_PCMSTREAM_STATE_STOPPED:
> > + break;
> > + default:
> > + 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,
> > @@ -655,6 +708,8 @@ static void virtio_snd_handle_pcm_release(VirtIOSound
> > *s,
> > virtio_snd_pcm_flush(stream);
> > }
>
> Same here.
I decided to add virtio_snd_pcm_close(stream) call to
virtio_snd_handle_pcm_release().
Kind regards,
Alex
>
> --
> Marc-André Lureau <[email protected]>
>
