On Mon, Feb 19, 2024 at 02:59:45PM +0200, Manos Pitsidianakis wrote:
> Hello Volker,
>
> On Sun, 18 Feb 2024 at 10:34, Volker Rümelin <vr_q...@t-online.de> wrote:
> >
> > 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.
>
> 5.14.6.6.1 is non-normative in virtio v1.2. You can even see it's not
> in device requirements. The state transitions were thus not checked on
> purpose.
>
> There is nothing in the standard that describes error conditions
> pertaining to the "PCM lifecycle" state machine. It really should, but
> it doesn't, unfortunately.
Not too late to add it. It will have to be non-mandatory
(e.g. SHOULD) or depend on a feature bit, though.
> > 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.
>
> I suggest you instead add the state tracking but only use it for the
> post_load code for migration.
>
>
>
> > Signed-off-by: Volker Rümelin <vr_q...@t-online.de>
> > ---
> > hw/audio/virtio-snd.c | 213 ++++++++++++++++++----------------
> > include/hw/audio/virtio-snd.h | 20 +---
> > 2 files changed, 111 insertions(+), 122 deletions(-)
> >
> > diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> > index 435ce26430..bbbdd01aa9 100644
> > --- a/hw/audio/virtio-snd.c
> > +++ b/hw/audio/virtio-snd.c
> > @@ -31,11 +31,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)
> > +
> > 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)
> > @@ -153,8 +172,8 @@ virtio_snd_ctrl_cmd_free(virtio_snd_ctrl_command *cmd)
> > 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];
> > + return stream_id >= s->snd_conf.streams ? NULL
> > + : &s->streams[stream_id];
> > }
> >
> > /*
> > @@ -167,7 +186,7 @@ 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];
> > + : &s->streams[stream_id].params;
> > }
> >
> > /*
> > @@ -253,11 +272,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
> > @@ -265,9 +283,10 @@ 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) {
> > /*
> > * TODO: do we need to set DEVICE_NEEDS_RESET?
> > */
> > @@ -275,7 +294,17 @@ uint32_t virtio_snd_set_pcm_params(VirtIOSound *s,
> > 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:
> > + 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.");
> > @@ -290,6 +319,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);
> > @@ -298,6 +329,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);
> > + }
> > +
> > + stream->state = VSND_PCMSTREAM_STATE_PARAMS_SET;
> > +
> > return cpu_to_le32(VIRTIO_SND_S_OK);
> > }
> >
> > @@ -410,15 +448,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) {
> > - AUD_close_out(&stream->pcm->snd->card, stream->voice.out);
> > - stream->voice.out = NULL;
> > - } else if (stream->info.direction == VIRTIO_SND_D_INPUT) {
> > - AUD_close_in(&stream->pcm->snd->card, stream->voice.in);
> > - stream->voice.in = NULL;
> > - }
> > + if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
> > + AUD_close_out(&stream->s->card, stream->voice.out);
> > + stream->voice.out = NULL;
> > + } else {
> > + AUD_close_in(&stream->s->card, stream->voice.in);
> > + stream->voice.in = NULL;
> > }
> > }
> >
> > @@ -435,33 +470,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->pcm = s->pcm;
> > -
> > - /*
> > - * 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;
> > stream->as = as;
> > @@ -484,6 +509,8 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s,
> > uint32_t stream_id)
> > AUD_set_volume_in(stream->voice.in, 0, 255, 255);
> > }
> >
> > + stream->state = VSND_PCMSTREAM_STATE_PREPARED;
> > +
> > return cpu_to_le32(VIRTIO_SND_S_OK);
> > }
> >
> > @@ -552,12 +579,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) {
> > AUD_set_active_out(stream->voice.out, start);
> > } else {
> > @@ -644,15 +687,18 @@ static void virtio_snd_handle_pcm_release(VirtIOSound
> > *s,
> >
> > stream_id = le32_to_cpu(stream_id);
> > trace_virtio_snd_handle_pcm_release(stream_id);
> > +
> > stream = virtio_snd_pcm_get_stream(s, stream_id);
> > - if (stream == NULL) {
> > - /*
> > - * TODO: do we need to set DEVICE_NEEDS_RESET?
> > - */
> > - error_report("already released stream %"PRIu32, stream_id);
> > - virtio_error(VIRTIO_DEVICE(s),
> > - "already released stream %"PRIu32,
> > - stream_id);
> > + if (!stream) {
> > + cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> > + 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;
> > }
> > @@ -671,6 +717,8 @@ static void virtio_snd_handle_pcm_release(VirtIOSound
> > *s,
> > virtio_snd_pcm_flush(stream);
> > }
> >
> > + stream->state = VSND_PCMSTREAM_STATE_RELEASED;
> > +
> > cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
> > }
> >
> > @@ -873,12 +921,11 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice
> > *vdev, VirtQueue *vq)
> > }
> > stream_id = le32_to_cpu(hdr.stream_id);
> >
> > - if (stream_id >= s->snd_conf.streams
> > - || s->pcm->streams[stream_id] == NULL) {
> > + if (stream_id >= s->snd_conf.streams) {
> > goto tx_err;
> > }
> >
> > - stream = s->pcm->streams[stream_id];
> > + stream = &s->streams[stream_id];
> > if (stream->info.direction != VIRTIO_SND_D_OUTPUT) {
> > goto tx_err;
> > }
> > @@ -948,13 +995,12 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice
> > *vdev, VirtQueue *vq)
> > }
> > stream_id = le32_to_cpu(hdr.stream_id);
> >
> > - if (stream_id >= s->snd_conf.streams
> > - || !s->pcm->streams[stream_id]) {
> > + if (stream_id >= s->snd_conf.streams) {
> > goto rx_err;
> > }
> >
> > - stream = s->pcm->streams[stream_id];
> > - if (stream == NULL || stream->info.direction !=
> > VIRTIO_SND_D_INPUT) {
> > + stream = &s->streams[stream_id];
> > + if (stream->info.direction != VIRTIO_SND_D_INPUT) {
> > goto rx_err;
> > }
> > size = iov_size(elem->in_sg, elem->in_num) -
> > @@ -1013,8 +1059,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);
> >
> > @@ -1052,6 +1096,7 @@ static void virtio_snd_realize(DeviceState *dev,
> > Error **errp)
> > VirtIOSoundPCMStream *stream = &vsnd->streams[i];
> >
> > stream->id = i;
> > + stream->state = VSND_PCMSTREAM_STATE_UNINITIALIZED;
> > stream->s = vsnd;
> > QSIMPLEQ_INIT(&stream->queue);
> > stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
> > @@ -1065,23 +1110,9 @@ static void virtio_snd_realize(DeviceState *dev,
> > Error **errp)
> > stream->info.channels_max = 2;
> > }
> >
> > - vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
> > - vsnd->pcm->snd = vsnd;
> > - 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] =
> > @@ -1091,28 +1122,6 @@ static void virtio_snd_realize(DeviceState *dev,
> > Error **errp)
> > vsnd->queues[VIRTIO_SND_VQ_RX] =
> > virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer);
> > QTAILQ_INIT(&vsnd->cmdq);
> > -
> > - 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 return_tx_buffer(VirtIOSoundPCMStream *stream,
> > @@ -1154,7 +1163,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 & VSND_PCMSTREAM_STATE_F_ACTIVE)) {
> > /* Stream has stopped, so do not perform AUD_write. */
> > return_tx_buffer(stream, buffer);
> > continue;
> > @@ -1246,7 +1255,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 & VSND_PCMSTREAM_STATE_F_ACTIVE)) {
> > /* Stream has stopped, so do not perform AUD_read. */
> > return_rx_buffer(stream, buffer);
> > continue;
> > @@ -1306,19 +1315,14 @@ static void virtio_snd_unrealize(DeviceState *dev)
> > trace_virtio_snd_unrealize(vsnd);
> >
> > if (vsnd->streams) {
> > - if (vsnd->pcm->streams) {
> > - 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);
> > - }
> > + virtio_snd_process_cmdq(vsnd);
> > + for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
> > + stream = &vsnd->streams[i];
> > + if (stream->state & VSND_PCMSTREAM_STATE_F_PREPARED) {
> > + virtio_snd_pcm_flush(stream);
> > }
> > - g_free(vsnd->pcm->streams);
> > + virtio_snd_pcm_close(stream);
> > }
> > - g_free(vsnd->pcm->pcm_params);
> > - g_free(vsnd->pcm);
> > - vsnd->pcm = NULL;
> > g_free(vsnd->streams);
> > vsnd->streams = NULL;
> > }
> > @@ -1347,6 +1351,9 @@ static void virtio_snd_reset(VirtIODevice *vdev)
> > VirtIOSoundPCMStream *stream = &s->streams[i];
> > VirtIOSoundPCMBuffer *buffer;
> >
> > + virtio_snd_pcm_close(stream);
> > + stream->state = VSND_PCMSTREAM_STATE_UNINITIALIZED;
> > +
> > 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 95aef8192a..65afa6c184 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,34 +119,19 @@ struct VirtIOSoundPCMBuffer {
> > uint8_t data[];
> > };
> >
> > -struct VirtIOSoundPCM {
> > - VirtIOSound *snd;
> > - /*
> > - * 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 {
> > - VirtIOSoundPCM *pcm;
> > virtio_snd_pcm_info info;
> > virtio_snd_pcm_set_params params;
> > uint32_t id;
> > + 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;
> > QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
> > };
> >
> > @@ -214,7 +197,6 @@ struct VirtIOSound {
> >
> > VirtQueue *queues[VIRTIO_SND_VQ_MAX];
> > uint64_t features;
> > - VirtIOSoundPCM *pcm;
> > VirtIOSoundPCMStream *streams;
> > QEMUSoundCard card;
> > VMChangeStateEntry *vmstate;
> > --
> > 2.35.3
> >
