* [PATCH v2 01/11] hw/audio/virtio-sound: return correct command response size
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler Volker Rümelin
` (10 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel, qemu-stable
The payload size returned by command VIRTIO_SND_R_PCM_INFO is
wrong. The code in process_cmd() assumes that all commands
return only a virtio_snd_hdr payload, but some commands like
VIRTIO_SND_R_PCM_INFO may return an additional payload.
Add a zero initialized payload_size variable to struct
virtio_snd_ctrl_command to allow for additional payloads.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 7 +++++--
include/hw/audio/virtio-snd.h | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index ea2aeaef14..e604d8f30c 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -243,12 +243,13 @@ static void virtio_snd_handle_pcm_info(VirtIOSound *s,
memset(&pcm_info[i].padding, 0, 5);
}
+ cmd->payload_size = sizeof(virtio_snd_pcm_info) * count;
cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
iov_from_buf(cmd->elem->in_sg,
cmd->elem->in_num,
sizeof(virtio_snd_hdr),
pcm_info,
- sizeof(virtio_snd_pcm_info) * count);
+ cmd->payload_size);
}
/*
@@ -749,7 +750,8 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
0,
&cmd->resp,
sizeof(virtio_snd_hdr));
- virtqueue_push(cmd->vq, cmd->elem, sizeof(virtio_snd_hdr));
+ virtqueue_push(cmd->vq, cmd->elem,
+ sizeof(virtio_snd_hdr) + cmd->payload_size);
virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
}
@@ -808,6 +810,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
cmd->elem = elem;
cmd->vq = vq;
cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
+ /* implicit cmd->payload_size = 0; */
QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next);
elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
}
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index c3767f442b..3d79181364 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -230,6 +230,7 @@ struct virtio_snd_ctrl_command {
VirtQueue *vq;
virtio_snd_hdr ctrl;
virtio_snd_hdr resp;
+ size_t payload_size;
QTAILQ_ENTRY(virtio_snd_ctrl_command) next;
};
#endif
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 01/11] hw/audio/virtio-sound: return correct command response size Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-19 12:42 ` Manos Pitsidianakis
2024-02-18 8:33 ` [PATCH v2 03/11] hw/audio/virtio-sound: remove command and stream mutexes Volker Rümelin
` (9 subsequent siblings)
11 siblings, 1 reply; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel, qemu-stable
A malicious guest may trigger a segmentation fault in the tx/rx xfer
handlers. On handler entry the stream variable is initialized with
NULL. If the first element of the virtio queue has an invalid size
or an invalid stream id, the error handling code dereferences the
stream variable NULL pointer.
Don't try to handle the invalid virtio queue element with a stream
queue. Instead, push the invalid queue element back to the guest
immediately.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 100 ++++++++++------------------------
include/hw/audio/virtio-snd.h | 1 -
2 files changed, 29 insertions(+), 72 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e604d8f30c..b87653daf4 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
stream->s = s;
qemu_mutex_init(&stream->queue_mutex);
QSIMPLEQ_INIT(&stream->queue);
- QSIMPLEQ_INIT(&stream->invalid);
/*
* stream_id >= s->snd_conf.streams was checked before so this is
@@ -611,9 +610,6 @@ static size_t virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) {
count += 1;
}
- QSIMPLEQ_FOREACH_SAFE(buffer, &stream->invalid, entry, next) {
- count += 1;
- }
}
return count;
}
@@ -831,47 +827,19 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq)
trace_virtio_snd_handle_event();
}
-static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
+static void push_bad_msg_resp(VirtQueue *vq, VirtQueueElement *elem)
{
- VirtIOSoundPCMBuffer *buffer = NULL;
- VirtIOSoundPCMStream *stream = NULL;
virtio_snd_pcm_status resp = { 0 };
- VirtIOSound *vsnd = VIRTIO_SND(vdev);
- bool any = false;
-
- for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
- stream = vsnd->pcm->streams[i];
- if (stream) {
- any = false;
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- while (!QSIMPLEQ_EMPTY(&stream->invalid)) {
- buffer = QSIMPLEQ_FIRST(&stream->invalid);
- if (buffer->vq != vq) {
- break;
- }
- any = true;
- resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
- iov_from_buf(buffer->elem->in_sg,
- buffer->elem->in_num,
- 0,
- &resp,
- sizeof(virtio_snd_pcm_status));
- virtqueue_push(vq,
- buffer->elem,
- sizeof(virtio_snd_pcm_status));
- QSIMPLEQ_REMOVE_HEAD(&stream->invalid, entry);
- virtio_snd_pcm_buffer_free(buffer);
- }
- if (any) {
- /*
- * Notify vq about virtio_snd_pcm_status responses.
- * Buffer responses must be notified separately later.
- */
- virtio_notify(vdev, vq);
- }
- }
- }
- }
+ size_t msg_sz;
+
+ resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+ msg_sz = iov_from_buf(elem->in_sg,
+ elem->in_num,
+ 0,
+ &resp,
+ sizeof(virtio_snd_pcm_status));
+ virtqueue_push(vq, elem, msg_sz);
+ g_free(elem);
}
/*
@@ -890,11 +858,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
size_t msg_sz, size;
virtio_snd_pcm_xfer hdr;
uint32_t stream_id;
- /*
- * If any of the I/O messages are invalid, put them in stream->invalid and
- * return them after the for loop.
- */
- bool must_empty_invalid_queue = false;
+ bool notify = false;
if (!virtio_queue_ready(vq)) {
return;
@@ -942,17 +906,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
continue;
tx_err:
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- must_empty_invalid_queue = true;
- buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
- buffer->elem = elem;
- buffer->vq = vq;
- QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
- }
+ push_bad_msg_resp(vq, elem);
+ notify = true;
}
- if (must_empty_invalid_queue) {
- empty_invalid_queue(vdev, vq);
+ if (notify) {
+ /*
+ * Notify vq about virtio_snd_pcm_status responses.
+ * Buffer responses must be notified separately later.
+ */
+ virtio_notify(vdev, vq);
}
}
@@ -972,11 +935,7 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
size_t msg_sz, size;
virtio_snd_pcm_xfer hdr;
uint32_t stream_id;
- /*
- * if any of the I/O messages are invalid, put them in stream->invalid and
- * return them after the for loop.
- */
- bool must_empty_invalid_queue = false;
+ bool notify = false;
if (!virtio_queue_ready(vq)) {
return;
@@ -1021,17 +980,16 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
continue;
rx_err:
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- must_empty_invalid_queue = true;
- buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
- buffer->elem = elem;
- buffer->vq = vq;
- QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
- }
+ push_bad_msg_resp(vq, elem);
+ notify = true;
}
- if (must_empty_invalid_queue) {
- empty_invalid_queue(vdev, vq);
+ if (notify) {
+ /*
+ * Notify vq about virtio_snd_pcm_status responses.
+ * Buffer responses must be notified separately later.
+ */
+ virtio_notify(vdev, vq);
}
}
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index 3d79181364..1209b122b9 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -151,7 +151,6 @@ struct VirtIOSoundPCMStream {
QemuMutex queue_mutex;
bool active;
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
- QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
};
/*
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler
2024-02-18 8:33 ` [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler Volker Rümelin
@ 2024-02-19 12:42 ` Manos Pitsidianakis
0 siblings, 0 replies; 17+ messages in thread
From: Manos Pitsidianakis @ 2024-02-19 12:42 UTC (permalink / raw)
To: Volker Rümelin
Cc: Marc-André Lureau, Gerd Hoffmann, Michael S. Tsirkin,
qemu-devel, qemu-stable
Hello Volker, thanks for working on this,
On Sun, 18 Feb 2024 at 10:33, Volker Rümelin <vr_qemu@t-online.de> wrote:
>
> A malicious guest may trigger a segmentation fault in the tx/rx xfer
> handlers. On handler entry the stream variable is initialized with
> NULL. If the first element of the virtio queue has an invalid size
> or an invalid stream id, the error handling code dereferences the
> stream variable NULL pointer.
Why not just add a bounds check and a null check instead?
>
> Don't try to handle the invalid virtio queue element with a stream
> queue. Instead, push the invalid queue element back to the guest
> immediately.
IIRC this will result in an infinite loop, because the code is
emptying the vq until virtqueue_pop returns NULL.
So if you add the invalid message back, the vq will never be empty.
Eventually you will loop over all invalid messages forever.
(Please correct me if I'm wrong of course!)
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
> hw/audio/virtio-snd.c | 100 ++++++++++------------------------
> include/hw/audio/virtio-snd.h | 1 -
> 2 files changed, 29 insertions(+), 72 deletions(-)
>
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index e604d8f30c..b87653daf4 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
> stream->s = s;
> qemu_mutex_init(&stream->queue_mutex);
> QSIMPLEQ_INIT(&stream->queue);
> - QSIMPLEQ_INIT(&stream->invalid);
>
> /*
> * stream_id >= s->snd_conf.streams was checked before so this is
> @@ -611,9 +610,6 @@ static size_t virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
> QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) {
> count += 1;
> }
> - QSIMPLEQ_FOREACH_SAFE(buffer, &stream->invalid, entry, next) {
> - count += 1;
> - }
> }
> return count;
> }
> @@ -831,47 +827,19 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq)
> trace_virtio_snd_handle_event();
> }
>
> -static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
> +static void push_bad_msg_resp(VirtQueue *vq, VirtQueueElement *elem)
> {
> - VirtIOSoundPCMBuffer *buffer = NULL;
> - VirtIOSoundPCMStream *stream = NULL;
> virtio_snd_pcm_status resp = { 0 };
> - VirtIOSound *vsnd = VIRTIO_SND(vdev);
> - bool any = false;
> -
> - for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
> - stream = vsnd->pcm->streams[i];
> - if (stream) {
> - any = false;
> - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> - while (!QSIMPLEQ_EMPTY(&stream->invalid)) {
> - buffer = QSIMPLEQ_FIRST(&stream->invalid);
> - if (buffer->vq != vq) {
> - break;
> - }
> - any = true;
> - resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> - iov_from_buf(buffer->elem->in_sg,
> - buffer->elem->in_num,
> - 0,
> - &resp,
> - sizeof(virtio_snd_pcm_status));
> - virtqueue_push(vq,
> - buffer->elem,
> - sizeof(virtio_snd_pcm_status));
> - QSIMPLEQ_REMOVE_HEAD(&stream->invalid, entry);
> - virtio_snd_pcm_buffer_free(buffer);
> - }
> - if (any) {
> - /*
> - * Notify vq about virtio_snd_pcm_status responses.
> - * Buffer responses must be notified separately later.
> - */
> - virtio_notify(vdev, vq);
> - }
> - }
> - }
> - }
> + size_t msg_sz;
> +
> + resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
> + msg_sz = iov_from_buf(elem->in_sg,
> + elem->in_num,
> + 0,
> + &resp,
> + sizeof(virtio_snd_pcm_status));
> + virtqueue_push(vq, elem, msg_sz);
> + g_free(elem);
> }
>
> /*
> @@ -890,11 +858,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
> size_t msg_sz, size;
> virtio_snd_pcm_xfer hdr;
> uint32_t stream_id;
> - /*
> - * If any of the I/O messages are invalid, put them in stream->invalid and
> - * return them after the for loop.
> - */
> - bool must_empty_invalid_queue = false;
> + bool notify = false;
>
> if (!virtio_queue_ready(vq)) {
> return;
> @@ -942,17 +906,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
> continue;
>
> tx_err:
> - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> - must_empty_invalid_queue = true;
> - buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
> - buffer->elem = elem;
> - buffer->vq = vq;
> - QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
> - }
> + push_bad_msg_resp(vq, elem);
> + notify = true;
> }
>
> - if (must_empty_invalid_queue) {
> - empty_invalid_queue(vdev, vq);
> + if (notify) {
> + /*
> + * Notify vq about virtio_snd_pcm_status responses.
> + * Buffer responses must be notified separately later.
> + */
> + virtio_notify(vdev, vq);
> }
> }
>
> @@ -972,11 +935,7 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
> size_t msg_sz, size;
> virtio_snd_pcm_xfer hdr;
> uint32_t stream_id;
> - /*
> - * if any of the I/O messages are invalid, put them in stream->invalid and
> - * return them after the for loop.
> - */
> - bool must_empty_invalid_queue = false;
> + bool notify = false;
>
> if (!virtio_queue_ready(vq)) {
> return;
> @@ -1021,17 +980,16 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
> continue;
>
> rx_err:
> - WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
> - must_empty_invalid_queue = true;
> - buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
> - buffer->elem = elem;
> - buffer->vq = vq;
> - QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
> - }
> + push_bad_msg_resp(vq, elem);
> + notify = true;
> }
>
> - if (must_empty_invalid_queue) {
> - empty_invalid_queue(vdev, vq);
> + if (notify) {
> + /*
> + * Notify vq about virtio_snd_pcm_status responses.
> + * Buffer responses must be notified separately later.
> + */
> + virtio_notify(vdev, vq);
> }
> }
>
> diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
> index 3d79181364..1209b122b9 100644
> --- a/include/hw/audio/virtio-snd.h
> +++ b/include/hw/audio/virtio-snd.h
> @@ -151,7 +151,6 @@ struct VirtIOSoundPCMStream {
> QemuMutex queue_mutex;
> bool active;
> QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
> - QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
> };
>
> /*
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 03/11] hw/audio/virtio-sound: remove command and stream mutexes
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 01/11] hw/audio/virtio-sound: return correct command response size Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 02/11] hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 04/11] hw/audio/virtio-sound: allocate an array of streams Volker Rümelin
` (8 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
All code in virtio-snd.c runs with the BQL held. Remove the
command queue mutex and the stream queue mutexes. The qatomic
functions are also not needed.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 215 +++++++++++++++-------------------
include/hw/audio/virtio-snd.h | 3 -
2 files changed, 93 insertions(+), 125 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index b87653daf4..7ed5f3de3e 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -19,7 +19,6 @@
#include "qemu/iov.h"
#include "qemu/log.h"
#include "qemu/error-report.h"
-#include "include/qemu/lockable.h"
#include "sysemu/runstate.h"
#include "trace.h"
#include "qapi/error.h"
@@ -454,7 +453,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
stream->id = stream_id;
stream->pcm = s->pcm;
stream->s = s;
- qemu_mutex_init(&stream->queue_mutex);
QSIMPLEQ_INIT(&stream->queue);
/*
@@ -580,9 +578,7 @@ static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
stream = virtio_snd_pcm_get_stream(s, stream_id);
if (stream) {
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- stream->active = start;
- }
+ stream->active = start;
if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
AUD_set_active_out(stream->voice.out, start);
} else {
@@ -606,10 +602,8 @@ static size_t virtio_snd_pcm_get_io_msgs_count(VirtIOSoundPCMStream *stream)
VirtIOSoundPCMBuffer *buffer, *next;
size_t count = 0;
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) {
- count += 1;
- }
+ QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) {
+ count += 1;
}
return count;
}
@@ -760,23 +754,15 @@ static void virtio_snd_process_cmdq(VirtIOSound *s)
{
virtio_snd_ctrl_command *cmd;
- if (unlikely(qatomic_read(&s->processing_cmdq))) {
- return;
- }
-
- WITH_QEMU_LOCK_GUARD(&s->cmdq_mutex) {
- qatomic_set(&s->processing_cmdq, true);
- while (!QTAILQ_EMPTY(&s->cmdq)) {
- cmd = QTAILQ_FIRST(&s->cmdq);
+ while (!QTAILQ_EMPTY(&s->cmdq)) {
+ cmd = QTAILQ_FIRST(&s->cmdq);
- /* process command */
- process_cmd(s, cmd);
+ /* process command */
+ process_cmd(s, cmd);
- QTAILQ_REMOVE(&s->cmdq, cmd, next);
+ QTAILQ_REMOVE(&s->cmdq, cmd, next);
- virtio_snd_ctrl_cmd_free(cmd);
- }
- qatomic_set(&s->processing_cmdq, false);
+ virtio_snd_ctrl_cmd_free(cmd);
}
}
@@ -891,18 +877,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
goto tx_err;
}
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- size = iov_size(elem->out_sg, elem->out_num) - msg_sz;
+ size = iov_size(elem->out_sg, elem->out_num) - msg_sz;
- buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
- buffer->elem = elem;
- buffer->populated = false;
- buffer->vq = vq;
- buffer->size = size;
- buffer->offset = 0;
+ buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
+ buffer->elem = elem;
+ buffer->populated = false;
+ buffer->vq = vq;
+ buffer->size = size;
+ buffer->offset = 0;
- QSIMPLEQ_INSERT_TAIL(&stream->queue, buffer, entry);
- }
+ QSIMPLEQ_INSERT_TAIL(&stream->queue, buffer, entry);
continue;
tx_err:
@@ -967,16 +951,14 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
if (stream == NULL || stream->info.direction != VIRTIO_SND_D_INPUT) {
goto rx_err;
}
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- size = iov_size(elem->in_sg, elem->in_num) -
- sizeof(virtio_snd_pcm_status);
- buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
- buffer->elem = elem;
- buffer->vq = vq;
- buffer->size = 0;
- buffer->offset = 0;
- QSIMPLEQ_INSERT_TAIL(&stream->queue, buffer, entry);
- }
+ size = iov_size(elem->in_sg, elem->in_num) -
+ sizeof(virtio_snd_pcm_status);
+ buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
+ buffer->elem = elem;
+ buffer->vq = vq;
+ buffer->size = 0;
+ buffer->offset = 0;
+ QSIMPLEQ_INSERT_TAIL(&stream->queue, buffer, entry);
continue;
rx_err:
@@ -1083,7 +1065,6 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
virtio_add_queue(vdev, 64, virtio_snd_handle_tx_xfer);
vsnd->queues[VIRTIO_SND_VQ_RX] =
virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer);
- qemu_mutex_init(&vsnd->cmdq_mutex);
QTAILQ_INIT(&vsnd->cmdq);
for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
@@ -1143,50 +1124,48 @@ static void virtio_snd_pcm_out_cb(void *data, int available)
VirtIOSoundPCMBuffer *buffer;
size_t size;
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- while (!QSIMPLEQ_EMPTY(&stream->queue)) {
- buffer = QSIMPLEQ_FIRST(&stream->queue);
- if (!virtio_queue_ready(buffer->vq)) {
- return;
+ while (!QSIMPLEQ_EMPTY(&stream->queue)) {
+ buffer = QSIMPLEQ_FIRST(&stream->queue);
+ if (!virtio_queue_ready(buffer->vq)) {
+ return;
+ }
+ if (!stream->active) {
+ /* Stream has stopped, so do not perform AUD_write. */
+ return_tx_buffer(stream, buffer);
+ continue;
+ }
+ if (!buffer->populated) {
+ iov_to_buf(buffer->elem->out_sg,
+ buffer->elem->out_num,
+ sizeof(virtio_snd_pcm_xfer),
+ buffer->data,
+ buffer->size);
+ buffer->populated = true;
+ }
+ for (;;) {
+ size = AUD_write(stream->voice.out,
+ buffer->data + buffer->offset,
+ MIN(buffer->size, available));
+ assert(size <= MIN(buffer->size, available));
+ if (size == 0) {
+ /* break out of both loops */
+ available = 0;
+ break;
}
- if (!stream->active) {
- /* Stream has stopped, so do not perform AUD_write. */
+ buffer->size -= size;
+ buffer->offset += size;
+ available -= size;
+ if (buffer->size < 1) {
return_tx_buffer(stream, buffer);
- continue;
- }
- if (!buffer->populated) {
- iov_to_buf(buffer->elem->out_sg,
- buffer->elem->out_num,
- sizeof(virtio_snd_pcm_xfer),
- buffer->data,
- buffer->size);
- buffer->populated = true;
- }
- for (;;) {
- size = AUD_write(stream->voice.out,
- buffer->data + buffer->offset,
- MIN(buffer->size, available));
- assert(size <= MIN(buffer->size, available));
- if (size == 0) {
- /* break out of both loops */
- available = 0;
- break;
- }
- buffer->size -= size;
- buffer->offset += size;
- available -= size;
- if (buffer->size < 1) {
- return_tx_buffer(stream, buffer);
- break;
- }
- if (!available) {
- break;
- }
+ break;
}
if (!available) {
break;
}
}
+ if (!available) {
+ break;
+ }
}
}
@@ -1237,41 +1216,39 @@ static void virtio_snd_pcm_in_cb(void *data, int available)
VirtIOSoundPCMBuffer *buffer;
size_t size;
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- while (!QSIMPLEQ_EMPTY(&stream->queue)) {
- buffer = QSIMPLEQ_FIRST(&stream->queue);
- if (!virtio_queue_ready(buffer->vq)) {
- return;
+ while (!QSIMPLEQ_EMPTY(&stream->queue)) {
+ buffer = QSIMPLEQ_FIRST(&stream->queue);
+ if (!virtio_queue_ready(buffer->vq)) {
+ return;
+ }
+ if (!stream->active) {
+ /* Stream has stopped, so do not perform AUD_read. */
+ return_rx_buffer(stream, buffer);
+ continue;
+ }
+
+ for (;;) {
+ size = AUD_read(stream->voice.in,
+ buffer->data + buffer->size,
+ MIN(available, (stream->params.period_bytes -
+ buffer->size)));
+ if (!size) {
+ available = 0;
+ break;
}
- if (!stream->active) {
- /* Stream has stopped, so do not perform AUD_read. */
+ buffer->size += size;
+ available -= size;
+ if (buffer->size >= stream->params.period_bytes) {
return_rx_buffer(stream, buffer);
- continue;
- }
-
- for (;;) {
- size = AUD_read(stream->voice.in,
- buffer->data + buffer->size,
- MIN(available, (stream->params.period_bytes -
- buffer->size)));
- if (!size) {
- available = 0;
- break;
- }
- buffer->size += size;
- available -= size;
- if (buffer->size >= stream->params.period_bytes) {
- return_rx_buffer(stream, buffer);
- break;
- }
- if (!available) {
- break;
- }
+ break;
}
if (!available) {
break;
}
}
+ if (!available) {
+ break;
+ }
}
}
@@ -1288,11 +1265,9 @@ static inline void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
(stream->info.direction == VIRTIO_SND_D_OUTPUT) ? return_tx_buffer :
return_rx_buffer;
- WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) {
- while (!QSIMPLEQ_EMPTY(&stream->queue)) {
- buffer = QSIMPLEQ_FIRST(&stream->queue);
- cb(stream, buffer);
- }
+ while (!QSIMPLEQ_EMPTY(&stream->queue)) {
+ buffer = QSIMPLEQ_FIRST(&stream->queue);
+ cb(stream, buffer);
}
}
@@ -1312,7 +1287,6 @@ static void virtio_snd_unrealize(DeviceState *dev)
if (stream) {
virtio_snd_process_cmdq(stream->s);
virtio_snd_pcm_close(stream);
- qemu_mutex_destroy(&stream->queue_mutex);
g_free(stream);
}
}
@@ -1323,7 +1297,6 @@ static void virtio_snd_unrealize(DeviceState *dev)
vsnd->pcm = NULL;
}
AUD_remove_card(&vsnd->card);
- qemu_mutex_destroy(&vsnd->cmdq_mutex);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_EVENT]);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_TX]);
@@ -1337,12 +1310,10 @@ static void virtio_snd_reset(VirtIODevice *vdev)
VirtIOSound *s = VIRTIO_SND(vdev);
virtio_snd_ctrl_command *cmd;
- WITH_QEMU_LOCK_GUARD(&s->cmdq_mutex) {
- while (!QTAILQ_EMPTY(&s->cmdq)) {
- cmd = QTAILQ_FIRST(&s->cmdq);
- QTAILQ_REMOVE(&s->cmdq, cmd, next);
- virtio_snd_ctrl_cmd_free(cmd);
- }
+ while (!QTAILQ_EMPTY(&s->cmdq)) {
+ cmd = QTAILQ_FIRST(&s->cmdq);
+ QTAILQ_REMOVE(&s->cmdq, cmd, next);
+ virtio_snd_ctrl_cmd_free(cmd);
}
}
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index 1209b122b9..a2868067fb 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -148,7 +148,6 @@ struct VirtIOSoundPCMStream {
SWVoiceIn *in;
SWVoiceOut *out;
} voice;
- QemuMutex queue_mutex;
bool active;
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
};
@@ -219,9 +218,7 @@ struct VirtIOSound {
QEMUSoundCard card;
VMChangeStateEntry *vmstate;
virtio_snd_config snd_conf;
- QemuMutex cmdq_mutex;
QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq;
- bool processing_cmdq;
};
struct virtio_snd_ctrl_command {
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 04/11] hw/audio/virtio-sound: allocate an array of streams
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (2 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 03/11] hw/audio/virtio-sound: remove command and stream mutexes Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 05/11] hw/audio/virtio-sound: free all stream buffers on reset Volker Rümelin
` (7 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
It is much easier to migrate an array of structs than individual
structs that are accessed via a pointer to a pointer to an array
of pointers to struct.
For this reason, allocate an array of streams in
virtio_snd_realize() and initialise all stream variables that
are constant at runtime immediately after allocation.
This makes it easier to remove the virtio_snd_set_pcm_params()
and virtio_snd_pcm_prepare() calls in the realisation phase and
to migrate the audio streams of the virtio sound device after
the next few patches.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 37 ++++++++++++++++++++++-------------
include/hw/audio/virtio-snd.h | 1 +
2 files changed, 24 insertions(+), 14 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 7ed5f3de3e..e5497b6bf6 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -448,12 +448,9 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
stream = virtio_snd_pcm_get_stream(s, stream_id);
if (stream == NULL) {
- stream = g_new0(VirtIOSoundPCMStream, 1);
+ stream = &s->streams[stream_id];
stream->active = false;
- stream->id = stream_id;
stream->pcm = s->pcm;
- stream->s = s;
- QSIMPLEQ_INIT(&stream->queue);
/*
* stream_id >= s->snd_conf.streams was checked before so this is
@@ -463,14 +460,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
}
virtio_snd_get_qemu_audsettings(&as, params);
- stream->info.direction = stream_id < s->snd_conf.streams / 2 +
- (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
- stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
- stream->info.features = 0;
- stream->info.channels_min = 1;
- stream->info.channels_max = as.nchannels;
- stream->info.formats = supported_formats;
- stream->info.rates = supported_rates;
stream->params = *params;
stream->positions[0] = VIRTIO_SND_CHMAP_FL;
@@ -1040,6 +1029,25 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
vsnd->vmstate =
qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+ vsnd->streams = g_new0(VirtIOSoundPCMStream, vsnd->snd_conf.streams);
+
+ for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
+ VirtIOSoundPCMStream *stream = &vsnd->streams[i];
+
+ stream->id = i;
+ stream->s = vsnd;
+ QSIMPLEQ_INIT(&stream->queue);
+ stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
+ stream->info.features = 0;
+ stream->info.formats = supported_formats;
+ stream->info.rates = supported_rates;
+ stream->info.direction =
+ i < vsnd->snd_conf.streams / 2 + (vsnd->snd_conf.streams & 1)
+ ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
+ stream->info.channels_min = 1;
+ stream->info.channels_max = 2;
+ }
+
vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
vsnd->pcm->snd = vsnd;
vsnd->pcm->streams =
@@ -1280,14 +1288,13 @@ static void virtio_snd_unrealize(DeviceState *dev)
qemu_del_vm_change_state_handler(vsnd->vmstate);
trace_virtio_snd_unrealize(vsnd);
- if (vsnd->pcm) {
+ 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);
- g_free(stream);
}
}
g_free(vsnd->pcm->streams);
@@ -1295,6 +1302,8 @@ static void virtio_snd_unrealize(DeviceState *dev)
g_free(vsnd->pcm->pcm_params);
g_free(vsnd->pcm);
vsnd->pcm = NULL;
+ g_free(vsnd->streams);
+ vsnd->streams = NULL;
}
AUD_remove_card(&vsnd->card);
virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index a2868067fb..95aef8192a 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -215,6 +215,7 @@ struct VirtIOSound {
VirtQueue *queues[VIRTIO_SND_VQ_MAX];
uint64_t features;
VirtIOSoundPCM *pcm;
+ VirtIOSoundPCMStream *streams;
QEMUSoundCard card;
VMChangeStateEntry *vmstate;
virtio_snd_config snd_conf;
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 05/11] hw/audio/virtio-sound: free all stream buffers on reset
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (3 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 04/11] hw/audio/virtio-sound: allocate an array of streams Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 06/11] hw/audio/virtio-sound: split out virtio_snd_pcm_start_stop() Volker Rümelin
` (6 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel, qemu-stable
All remaining stream buffers in the stream queues must
be freed after a reset. This is the initial state of the
virtio-sound device.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index e5497b6bf6..2b630ada82 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1318,12 +1318,23 @@ static void virtio_snd_reset(VirtIODevice *vdev)
{
VirtIOSound *s = VIRTIO_SND(vdev);
virtio_snd_ctrl_command *cmd;
+ uint32_t i;
while (!QTAILQ_EMPTY(&s->cmdq)) {
cmd = QTAILQ_FIRST(&s->cmdq);
QTAILQ_REMOVE(&s->cmdq, cmd, next);
virtio_snd_ctrl_cmd_free(cmd);
}
+
+ for (i = 0; i < s->snd_conf.streams; i++) {
+ VirtIOSoundPCMStream *stream = &s->streams[i];
+ VirtIOSoundPCMBuffer *buffer;
+
+ while ((buffer = QSIMPLEQ_FIRST(&stream->queue))) {
+ QSIMPLEQ_REMOVE_HEAD(&stream->queue, entry);
+ virtio_snd_pcm_buffer_free(buffer);
+ }
+ }
}
static void virtio_snd_class_init(ObjectClass *klass, void *data)
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 06/11] hw/audio/virtio-sound: split out virtio_snd_pcm_start_stop()
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (4 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 05/11] hw/audio/virtio-sound: free all stream buffers on reset Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable Volker Rümelin
` (5 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
Split out virtio_snd_pcm_start_stop(). This is a preparation
for the next patch so that it doesn't become too big.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/trace-events | 3 ++-
hw/audio/virtio-snd.c | 57 ++++++++++++++++++++++++++++---------------
2 files changed, 39 insertions(+), 21 deletions(-)
diff --git a/hw/audio/trace-events b/hw/audio/trace-events
index b1870ff224..7554bfcc60 100644
--- a/hw/audio/trace-events
+++ b/hw/audio/trace-events
@@ -50,7 +50,8 @@ virtio_snd_unrealize(void *snd) "snd %p: unrealize"
virtio_snd_handle_pcm_set_params(uint32_t stream) "VIRTIO_SND_PCM_SET_PARAMS called for stream %"PRIu32
virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for queue %p"
virtio_snd_handle_pcm_info(uint32_t stream) "VIRTIO_SND_R_PCM_INFO called for stream %"PRIu32
-virtio_snd_handle_pcm_start_stop(const char *code, uint32_t stream) "%s called for stream %"PRIu32
+virtio_snd_handle_pcm_start(uint32_t stream) "VIRTIO_SND_R_PCM_START called for stream %"PRIu32
+virtio_snd_handle_pcm_stop(uint32_t stream) "VIRTIO_SND_R_PCM_STOP called for stream %"PRIu32
virtio_snd_handle_pcm_release(uint32_t stream) "VIRTIO_SND_PCM_RELEASE called for stream %"PRIu32
virtio_snd_handle_code(uint32_t val, const char *code) "ctrl code msg val = %"PRIu32" == %s"
virtio_snd_handle_chmap_info(void) "VIRTIO_SND_CHMAP_INFO called"
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 2b630ada82..435ce26430 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -533,7 +533,42 @@ static void virtio_snd_handle_pcm_prepare(VirtIOSound *s,
}
/*
- * Handles VIRTIO_SND_R_PCM_START.
+ * Starts/Stops a VirtIOSound card stream.
+ * Returns the response status code. (VIRTIO_SND_S_*).
+ *
+ * @s: VirtIOSound device
+ * @stream_id: stream id
+ * @start: whether to start or stop the stream
+ */
+static uint32_t virtio_snd_pcm_start_stop(VirtIOSound *s,
+ uint32_t stream_id,
+ bool start)
+{
+ VirtIOSoundPCMStream *stream;
+
+ stream = virtio_snd_pcm_get_stream(s, stream_id);
+ if (!stream) {
+ return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
+ }
+
+ if (start) {
+ trace_virtio_snd_handle_pcm_start(stream_id);
+ } else {
+ trace_virtio_snd_handle_pcm_stop(stream_id);
+ }
+
+ stream->active = start;
+ if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
+ AUD_set_active_out(stream->voice.out, start);
+ } else {
+ AUD_set_active_in(stream->voice.in, start);
+ }
+
+ return cpu_to_le32(VIRTIO_SND_S_OK);
+}
+
+/*
+ * Handles VIRTIO_SND_R_PCM_START and VIRTIO_SND_R_PCM_STOP.
*
* @s: VirtIOSound device
* @cmd: The request command queue element from VirtIOSound cmdq field
@@ -543,7 +578,6 @@ static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
virtio_snd_ctrl_command *cmd,
bool start)
{
- VirtIOSoundPCMStream *stream;
virtio_snd_pcm_hdr req;
uint32_t stream_id;
size_t msg_sz = iov_to_buf(cmd->elem->out_sg,
@@ -561,24 +595,7 @@ static void virtio_snd_handle_pcm_start_stop(VirtIOSound *s,
}
stream_id = le32_to_cpu(req.stream_id);
- cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_OK);
- trace_virtio_snd_handle_pcm_start_stop(start ? "VIRTIO_SND_R_PCM_START" :
- "VIRTIO_SND_R_PCM_STOP", stream_id);
-
- stream = virtio_snd_pcm_get_stream(s, stream_id);
- if (stream) {
- stream->active = start;
- if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
- AUD_set_active_out(stream->voice.out, start);
- } else {
- AUD_set_active_in(stream->voice.in, start);
- }
- } else {
- error_report("Invalid stream id: %"PRIu32, stream_id);
- cmd->resp.code = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
- return;
- }
- stream->active = start;
+ cmd->resp.code = virtio_snd_pcm_start_stop(s, stream_id, start);
}
/*
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (5 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 06/11] hw/audio/virtio-sound: split out virtio_snd_pcm_start_stop() Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-19 12:59 ` Manos Pitsidianakis
2024-02-18 8:33 ` [PATCH v2 08/11] hw/audio/virtio-sound: introduce virtio_snd_pcm_open() Volker Rümelin
` (4 subsequent siblings)
11 siblings, 1 reply; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
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 <vr_qemu@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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable
2024-02-18 8:33 ` [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable Volker Rümelin
@ 2024-02-19 12:59 ` Manos Pitsidianakis
2024-02-19 13:01 ` Michael S. Tsirkin
2024-03-12 15:35 ` Michael S. Tsirkin
0 siblings, 2 replies; 17+ messages in thread
From: Manos Pitsidianakis @ 2024-02-19 12:59 UTC (permalink / raw)
To: Volker Rümelin
Cc: Marc-André Lureau, Gerd Hoffmann, Michael S. Tsirkin,
qemu-devel
Hello Volker,
On Sun, 18 Feb 2024 at 10:34, Volker Rümelin <vr_qemu@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.
> 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_qemu@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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable
2024-02-19 12:59 ` Manos Pitsidianakis
@ 2024-02-19 13:01 ` Michael S. Tsirkin
2024-03-12 15:35 ` Michael S. Tsirkin
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2024-02-19 13:01 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Volker Rümelin, Marc-André Lureau, Gerd Hoffmann,
qemu-devel
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_qemu@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_qemu@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
> >
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable
2024-02-19 12:59 ` Manos Pitsidianakis
2024-02-19 13:01 ` Michael S. Tsirkin
@ 2024-03-12 15:35 ` Michael S. Tsirkin
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2024-03-12 15:35 UTC (permalink / raw)
To: Manos Pitsidianakis
Cc: Volker Rümelin, Marc-André Lureau, Gerd Hoffmann,
qemu-devel
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_qemu@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.
Well it's not a big deal, if something does not make sense
but we did not require it, as long as we can be reasonably
sure no one does it we can always add them later
and if drivers practically behave correctly then qemu can
just assume that, any drivers doing stupid things will only
hurt themselves.
> > 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_qemu@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
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 08/11] hw/audio/virtio-sound: introduce virtio_snd_pcm_open()
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (6 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 07/11] hw/audio/virtio-sound: add stream state variable Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 09/11] hw/audio/virtio-sound: introduce virtio_snd_set_active() Volker Rümelin
` (3 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
Split out the function virtio_snd_pcm_open() from
virtio_snd_pcm_prepare(). A later patch also needs
the new function. There is no functional change.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 57 +++++++++++++++++++++++--------------------
1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index bbbdd01aa9..a1d14caba0 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -441,6 +441,36 @@ static void virtio_snd_get_qemu_audsettings(audsettings *as,
as->endianness = target_words_bigendian() ? 1 : 0;
}
+/*
+ * Open a stream.
+ *
+ * @stream: VirtIOSoundPCMStream *stream
+ */
+static void virtio_snd_pcm_open(VirtIOSoundPCMStream *stream)
+{
+ virtio_snd_get_qemu_audsettings(&stream->as, &stream->params);
+ stream->positions[0] = VIRTIO_SND_CHMAP_FL;
+ stream->positions[1] = VIRTIO_SND_CHMAP_FR;
+
+ if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
+ stream->voice.out = AUD_open_out(&stream->s->card,
+ stream->voice.out,
+ "virtio-sound.out",
+ stream,
+ virtio_snd_pcm_out_cb,
+ &stream->as);
+ AUD_set_volume_out(stream->voice.out, 0, 255, 255);
+ } else {
+ stream->voice.in = AUD_open_in(&stream->s->card,
+ stream->voice.in,
+ "virtio-sound.in",
+ stream,
+ virtio_snd_pcm_in_cb,
+ &stream->as);
+ AUD_set_volume_in(stream->voice.in, 0, 255, 255);
+ }
+}
+
/*
* Close a stream and free all its resources.
*
@@ -466,8 +496,6 @@ static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream)
*/
static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
{
- audsettings as;
- virtio_snd_pcm_set_params *params;
VirtIOSoundPCMStream *stream;
stream = virtio_snd_pcm_get_stream(s, stream_id);
@@ -484,30 +512,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
return cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
}
- params = virtio_snd_pcm_get_params(s, stream_id);
-
- virtio_snd_get_qemu_audsettings(&as, params);
- stream->positions[0] = VIRTIO_SND_CHMAP_FL;
- stream->positions[1] = VIRTIO_SND_CHMAP_FR;
- stream->as = as;
-
- if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
- stream->voice.out = AUD_open_out(&s->card,
- stream->voice.out,
- "virtio-sound.out",
- stream,
- virtio_snd_pcm_out_cb,
- &as);
- AUD_set_volume_out(stream->voice.out, 0, 255, 255);
- } else {
- stream->voice.in = AUD_open_in(&s->card,
- stream->voice.in,
- "virtio-sound.in",
- stream,
- virtio_snd_pcm_in_cb,
- &as);
- AUD_set_volume_in(stream->voice.in, 0, 255, 255);
- }
+ virtio_snd_pcm_open(stream);
stream->state = VSND_PCMSTREAM_STATE_PREPARED;
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 09/11] hw/audio/virtio-sound: introduce virtio_snd_set_active()
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (7 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 08/11] hw/audio/virtio-sound: introduce virtio_snd_pcm_open() Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 10/11] hw/audio/virtio-sound: add missing vmstate fields Volker Rümelin
` (2 subsequent siblings)
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
Split out the function virtio_snd_pcm_set_active() from
virtio_snd_pcm_start_stop(). A later patch also needs this
new funcion. There is no functional change.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a1d14caba0..06a27ef8d9 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -471,6 +471,21 @@ static void virtio_snd_pcm_open(VirtIOSoundPCMStream *stream)
}
}
+/*
+ * Activate/deactivate a stream.
+ *
+ * @stream: VirtIOSoundPCMStream *stream
+ * @active: whether to activate or deactivate the stream
+ */
+static void virtio_snd_pcm_set_active(VirtIOSoundPCMStream *stream, bool active)
+{
+ if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
+ AUD_set_active_out(stream->voice.out, active);
+ } else {
+ AUD_set_active_in(stream->voice.in, active);
+ }
+}
+
/*
* Close a stream and free all its resources.
*
@@ -606,11 +621,7 @@ static uint32_t virtio_snd_pcm_start_stop(VirtIOSound *s,
stream->state = VSND_PCMSTREAM_STATE_STOPPED;
}
- if (stream->info.direction == VIRTIO_SND_D_OUTPUT) {
- AUD_set_active_out(stream->voice.out, start);
- } else {
- AUD_set_active_in(stream->voice.in, start);
- }
+ virtio_snd_pcm_set_active(stream, start);
return cpu_to_le32(VIRTIO_SND_S_OK);
}
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 10/11] hw/audio/virtio-sound: add missing vmstate fields
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (8 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 09/11] hw/audio/virtio-sound: introduce virtio_snd_set_active() Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-02-18 8:33 ` [PATCH v2 11/11] hw/audio/virtio-sound: add placeholder for buffer write position Volker Rümelin
2024-03-12 15:38 ` [PATCH v2 00/11] virtio-sound migration part 1 Michael S. Tsirkin
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
The virtio-sound device is currently not migratable. Add the
missing VMSTATE fields, enable migration and reconnect the audio
streams after migration.
The queue_inuse[] array variables mimic the inuse variable in
struct VirtQueue which is private. They are needed to restart
the virtio queues after migration.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 84 +++++++++++++++++++++++++++++++----
include/hw/audio/virtio-snd.h | 1 +
2 files changed, 77 insertions(+), 8 deletions(-)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 06a27ef8d9..b0a0ff2456 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -25,7 +25,6 @@
#include "hw/audio/virtio-snd.h"
#include "hw/core/cpu.h"
-#define VIRTIO_SOUND_VM_VERSION 1
#define VIRTIO_SOUND_JACK_DEFAULT 0
#define VIRTIO_SOUND_STREAM_DEFAULT 2
#define VIRTIO_SOUND_CHMAP_DEFAULT 0
@@ -79,17 +78,40 @@ static uint32_t supported_rates = BIT(VIRTIO_SND_PCM_RATE_5512)
| BIT(VIRTIO_SND_PCM_RATE_192000)
| BIT(VIRTIO_SND_PCM_RATE_384000);
+static const VMStateDescription vmstate_virtio_snd_stream = {
+ .name = "virtio-sound-stream",
+ .fields = (const VMStateField[]) {
+ VMSTATE_UINT32(state, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(info.hdr.hda_fn_nid, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(info.features, VirtIOSoundPCMStream),
+ VMSTATE_UINT64(info.formats, VirtIOSoundPCMStream),
+ VMSTATE_UINT64(info.rates, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(info.direction, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(info.channels_min, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(info.channels_max, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(params.buffer_bytes, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(params.period_bytes, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(params.features, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(params.channels, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(params.format, VirtIOSoundPCMStream),
+ VMSTATE_UINT8(params.rate, VirtIOSoundPCMStream),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
static const VMStateDescription vmstate_virtio_snd_device = {
- .name = TYPE_VIRTIO_SND,
- .version_id = VIRTIO_SOUND_VM_VERSION,
- .minimum_version_id = VIRTIO_SOUND_VM_VERSION,
+ .name = "virtio-sound-device",
+ .fields = (const VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(queue_inuse, VirtIOSound, VIRTIO_SND_VQ_MAX),
+ VMSTATE_STRUCT_VARRAY_POINTER_UINT32(streams, VirtIOSound,
+ snd_conf.streams,
+ vmstate_virtio_snd_stream, VirtIOSoundPCMStream),
+ VMSTATE_END_OF_LIST()
+ },
};
static const VMStateDescription vmstate_virtio_snd = {
- .name = TYPE_VIRTIO_SND,
- .unmigratable = 1,
- .minimum_version_id = VIRTIO_SOUND_VM_VERSION,
- .version_id = VIRTIO_SOUND_VM_VERSION,
+ .name = "virtio-sound",
.fields = (const VMStateField[]) {
VMSTATE_VIRTIO_DEVICE,
VMSTATE_END_OF_LIST()
@@ -812,6 +834,7 @@ process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
sizeof(virtio_snd_hdr));
virtqueue_push(cmd->vq, cmd->elem,
sizeof(virtio_snd_hdr) + cmd->payload_size);
+ s->queue_inuse[VIRTIO_SND_VQ_CONTROL] -= 1;
virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
}
@@ -858,6 +881,7 @@ static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
while (elem) {
+ s->queue_inuse[VIRTIO_SND_VQ_CONTROL] += 1;
cmd = g_new0(virtio_snd_ctrl_command, 1);
cmd->elem = elem;
cmd->vq = vq;
@@ -946,6 +970,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
goto tx_err;
}
+ s->queue_inuse[VIRTIO_SND_VQ_TX] += 1;
size = iov_size(elem->out_sg, elem->out_num) - msg_sz;
buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
@@ -1019,6 +1044,8 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
if (stream->info.direction != VIRTIO_SND_D_INPUT) {
goto rx_err;
}
+
+ s->queue_inuse[VIRTIO_SND_VQ_RX] += 1;
size = iov_size(elem->in_sg, elem->in_num) -
sizeof(virtio_snd_pcm_status);
buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer) + size);
@@ -1154,6 +1181,7 @@ static inline void return_tx_buffer(VirtIOSoundPCMStream *stream,
virtqueue_push(buffer->vq,
buffer->elem,
sizeof(virtio_snd_pcm_status));
+ stream->s->queue_inuse[VIRTIO_SND_VQ_TX] -= 1;
virtio_notify(VIRTIO_DEVICE(stream->s), buffer->vq);
QSIMPLEQ_REMOVE(&stream->queue,
buffer,
@@ -1245,6 +1273,7 @@ static inline void return_rx_buffer(VirtIOSoundPCMStream *stream,
virtqueue_push(buffer->vq,
buffer->elem,
sizeof(virtio_snd_pcm_status) + buffer->size);
+ stream->s->queue_inuse[VIRTIO_SND_VQ_RX] -= 1;
virtio_notify(VIRTIO_DEVICE(stream->s), buffer->vq);
QSIMPLEQ_REMOVE(&stream->queue,
buffer,
@@ -1350,6 +1379,40 @@ static void virtio_snd_unrealize(DeviceState *dev)
virtio_cleanup(vdev);
}
+static int virtio_snd_post_load(VirtIODevice *vdev)
+{
+ VirtIOSound *s = VIRTIO_SND(vdev);
+ uint32_t i;
+
+ for (i = 0; i < s->snd_conf.streams; i++) {
+ struct VirtIOSoundPCMStream *stream;
+
+ stream = virtio_snd_pcm_get_stream(s, i);
+ if (stream->state & VSND_PCMSTREAM_STATE_F_PREPARED) {
+ virtio_snd_pcm_open(stream);
+
+ if (stream->state & VSND_PCMSTREAM_STATE_F_ACTIVE) {
+ virtio_snd_pcm_set_active(stream, true);
+ }
+ }
+ }
+
+ for (i = 0; i < VIRTIO_SND_VQ_MAX; i++) {
+ if (s->queue_inuse[i]) {
+ bool rc;
+
+ rc = virtqueue_rewind(s->queues[i], s->queue_inuse[i]);
+ if (!rc) {
+ error_report(
+ "virtio-sound: could not rewind %u elements in queue %u",
+ s->queue_inuse[i], i);
+ }
+ s->queue_inuse[i] = 0;
+ }
+ }
+
+ return 0;
+}
static void virtio_snd_reset(VirtIODevice *vdev)
{
@@ -1375,6 +1438,10 @@ static void virtio_snd_reset(VirtIODevice *vdev)
virtio_snd_pcm_buffer_free(buffer);
}
}
+
+ for (i = 0; i < VIRTIO_SND_VQ_MAX; i++) {
+ s->queue_inuse[i] = 0;
+ }
}
static void virtio_snd_class_init(ObjectClass *klass, void *data)
@@ -1388,6 +1455,7 @@ static void virtio_snd_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_virtio_snd;
vdc->vmsd = &vmstate_virtio_snd_device;
+ vdc->post_load = virtio_snd_post_load;
vdc->realize = virtio_snd_realize;
vdc->unrealize = virtio_snd_unrealize;
vdc->get_config = virtio_snd_get_config;
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index 65afa6c184..457d18d196 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -196,6 +196,7 @@ struct VirtIOSound {
VirtIODevice parent_obj;
VirtQueue *queues[VIRTIO_SND_VQ_MAX];
+ uint32_t queue_inuse[VIRTIO_SND_VQ_MAX];
uint64_t features;
VirtIOSoundPCMStream *streams;
QEMUSoundCard card;
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v2 11/11] hw/audio/virtio-sound: add placeholder for buffer write position
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (9 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 10/11] hw/audio/virtio-sound: add missing vmstate fields Volker Rümelin
@ 2024-02-18 8:33 ` Volker Rümelin
2024-03-12 15:38 ` [PATCH v2 00/11] virtio-sound migration part 1 Michael S. Tsirkin
11 siblings, 0 replies; 17+ messages in thread
From: Volker Rümelin @ 2024-02-18 8:33 UTC (permalink / raw)
To: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
Michael S. Tsirkin
Cc: qemu-devel
When a running audio stream is migrated, on average half of a
recording stream buffer is lost or half of a playback stream
buffer is played twice. Add a placeholder for the write position
of the current stream buffer to the migrated data. Additional
program code is required to resolve the above issues. However,
the placeholder makes it possible to add code in a backwards and
forwards compatible way.
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
hw/audio/virtio-snd.c | 2 ++
include/hw/audio/virtio-snd.h | 2 ++
2 files changed, 4 insertions(+)
diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index b0a0ff2456..453c3a37ba 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -82,6 +82,7 @@ static const VMStateDescription vmstate_virtio_snd_stream = {
.name = "virtio-sound-stream",
.fields = (const VMStateField[]) {
VMSTATE_UINT32(state, VirtIOSoundPCMStream),
+ VMSTATE_UINT32(buf_wpos, VirtIOSoundPCMStream),
VMSTATE_UINT32(info.hdr.hda_fn_nid, VirtIOSoundPCMStream),
VMSTATE_UINT32(info.features, VirtIOSoundPCMStream),
VMSTATE_UINT64(info.formats, VirtIOSoundPCMStream),
@@ -1395,6 +1396,7 @@ static int virtio_snd_post_load(VirtIODevice *vdev)
virtio_snd_pcm_set_active(stream, true);
}
}
+ stream->buf_wpos = 0;
}
for (i = 0; i < VIRTIO_SND_VQ_MAX; i++) {
diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
index 457d18d196..9be0276996 100644
--- a/include/hw/audio/virtio-snd.h
+++ b/include/hw/audio/virtio-snd.h
@@ -124,6 +124,8 @@ struct VirtIOSoundPCMStream {
virtio_snd_pcm_set_params params;
uint32_t id;
uint32_t state;
+ /* placeholder: write position in current VirtIOSoundPCMBuffer */
+ uint32_t buf_wpos;
/* channel position values (VIRTIO_SND_CHMAP_XXX) */
uint8_t positions[VIRTIO_SND_CHMAP_MAX_SIZE];
VirtIOSound *s;
--
2.35.3
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2 00/11] virtio-sound migration part 1
2024-02-18 8:31 [PATCH v2 00/11] virtio-sound migration part 1 Volker Rümelin
` (10 preceding siblings ...)
2024-02-18 8:33 ` [PATCH v2 11/11] hw/audio/virtio-sound: add placeholder for buffer write position Volker Rümelin
@ 2024-03-12 15:38 ` Michael S. Tsirkin
11 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2024-03-12 15:38 UTC (permalink / raw)
To: Volker Rümelin
Cc: Marc-André Lureau, Gerd Hoffmann, Manos Pitsidianakis,
qemu-devel, qemu-stable
On Sun, Feb 18, 2024 at 09:31:02AM +0100, Volker Rümelin wrote:
> Here is the first part of my virtio-sound patches. Most of them are a
> preparation to make migration work. Patch 10/11 enables migration.
>
> The second part isn't finished yet and will have to do with virtio-sound
> jack and channel maps configuration and migration.
>
> Patch 01/11 "hw/audio/virtio-sound: return correct command response
> size", patch 02/11 "hw/audio/virtio-sound: fix segmentation fault in
> tx/rx xfer handler" and patch 05/11 "hw/audio/virtio-sound: free all
> stream buffers on reset" are candidates for stable-8.2. Patch 05/11
> either needs patches 03/11 and 04/11 or has to be rewritten for stable-8.2.
So I queued patch 1 since that's a bugfix.
Patch 2 can go in after you address Manos's comment.
Rest - after release.
> v2:
> The patches were reordered to facilitate the backport of 3 patches to
> QEMU stable-8.2.
>
> Patch 02/11 "fix segmentation fault in tx/rx xfer handler" has been
> completely rewritten.
>
> Patch 04/11 "hw/audio/virtio-sound: allocate an array of streams" has
> been renamed. The subject and the commit message describe the patch better.
>
> Patch 05/11 "hw/audio/virtio-sound: free all stream buffers on reset" is
> an additional patch.
>
> Patch 07/11 "hw/audio/virtio-sound: add stream state variable" resets
> the state variable on reset. Once a stream has been opened, it will only
> be closed after a reset or when QEMU shuts down.
>
> Patch 10/11 "add missing vmstate fields" resets the inuse variables on
> reset.
>
> Volker Rümelin (11):
> hw/audio/virtio-sound: return correct command response size
> hw/audio/virtio-sound: fix segmentation fault in tx/rx xfer handler
> hw/audio/virtio-sound: remove command and stream mutexes
> hw/audio/virtio-sound: allocate an array of streams
> hw/audio/virtio-sound: free all stream buffers on reset
> hw/audio/virtio-sound: split out virtio_snd_pcm_start_stop()
> hw/audio/virtio-sound: add stream state variable
> hw/audio/virtio-sound: introduce virtio_snd_pcm_open()
> hw/audio/virtio-sound: introduce virtio_snd_set_active()
> hw/audio/virtio-sound: add missing vmstate fields
> hw/audio/virtio-sound: add placeholder for buffer write position
>
> hw/audio/trace-events | 3 +-
> hw/audio/virtio-snd.c | 776 ++++++++++++++++++----------------
> include/hw/audio/virtio-snd.h | 29 +-
> 3 files changed, 427 insertions(+), 381 deletions(-)
>
> --
> 2.35.3
^ permalink raw reply [flat|nested] 17+ messages in thread