system/qemu
system/qemu
1
0
Fork 0
mirror of https://gitlab.com/qemu-project/qemu synced 2024-10-03 01:34:41 +00:00
Code Issues Packages Projects Releases Wiki Activity

virtio-snd: rewrite invalid tx/rx message handling

Browse source 
The current handling of invalid virtqueue elements inside the TX/RX virt
queue handlers is wrong.

They are added in a per-stream invalid queue to be processed after the
handler is done examining each message, but the invalid message might
not be specifying any stream_id; which means it's invalid to add it to
any stream->invalid queue since stream could be NULL at this point.

This commit moves the invalid queue to the VirtIOSound struct which
guarantees there will always be a valid temporary place to store them
inside the tx/rx handlers. The queue will be emptied before the handler
returns, so the queue must be empty at any other point of the device's
lifetime.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Message-Id: <virtio-snd-rewrite-invalid-tx-rx-message-handling-v1.manos.pitsidianakis@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
This commit is contained in:
Manos Pitsidianakis 2024-03-24 12:04:59 +02:00 committed by Michael S. Tsirkin
parent a45f09935c
commit 731655f87f
2 changed files with 77 additions and 76 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

137
hw/audio/virtio-snd.c
View file

@ -456,7 +456,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
stream->s = s; stream->s = s;
qemu_mutex_init(&stream->queue_mutex); qemu_mutex_init(&stream->queue_mutex);
QSIMPLEQ_INIT(&stream->queue); QSIMPLEQ_INIT(&stream->queue);
QSIMPLEQ_INIT(&stream->invalid);
/* /*
* stream_id >= s->snd_conf.streams was checked before so this is * 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) { QSIMPLEQ_FOREACH_SAFE(buffer, &stream->queue, entry, next) {
count += 1; count += 1;
} }
QSIMPLEQ_FOREACH_SAFE(buffer, &stream->invalid, entry, next) {
count += 1;
}
} }
return count; return count;
} }
@ -831,47 +827,36 @@ static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq)
trace_virtio_snd_handle_event(); trace_virtio_snd_handle_event();
} }
/*
* Must only be called if vsnd->invalid is not empty.
*/
static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq) static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
{ {
VirtIOSoundPCMBuffer *buffer = NULL; VirtIOSoundPCMBuffer *buffer = NULL;
VirtIOSoundPCMStream *stream = NULL;
virtio_snd_pcm_status resp = { 0 }; virtio_snd_pcm_status resp = { 0 };
VirtIOSound *vsnd = VIRTIO_SND(vdev); VirtIOSound *vsnd = VIRTIO_SND(vdev);
bool any = false;
for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { g_assert(!QSIMPLEQ_EMPTY(&vsnd->invalid));
stream = vsnd->pcm->streams[i];
if (stream) { while (!QSIMPLEQ_EMPTY(&vsnd->invalid)) {
any = false; buffer = QSIMPLEQ_FIRST(&vsnd->invalid);
WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { /* If buffer->vq != vq, our logic is fundamentally wrong, so bail out */
while (!QSIMPLEQ_EMPTY(&stream->invalid)) { g_assert(buffer->vq == vq);
buffer = QSIMPLEQ_FIRST(&stream->invalid);
if (buffer->vq != vq) { resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG);
break; iov_from_buf(buffer->elem->in_sg,
} buffer->elem->in_num,
any = true; 0,
resp.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); &resp,
iov_from_buf(buffer->elem->in_sg, sizeof(virtio_snd_pcm_status));
buffer->elem->in_num, virtqueue_push(vq,
0, buffer->elem,
&resp, sizeof(virtio_snd_pcm_status));
sizeof(virtio_snd_pcm_status)); QSIMPLEQ_REMOVE_HEAD(&vsnd->invalid, entry);
virtqueue_push(vq, virtio_snd_pcm_buffer_free(buffer);
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);
}
}
}
} }
/* Notify vq about virtio_snd_pcm_status responses. */
virtio_notify(vdev, vq);
} }
/* /*
@ -883,15 +868,14 @@ static inline void empty_invalid_queue(VirtIODevice *vdev, VirtQueue *vq)
*/ */
static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq) static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
{ {
VirtIOSound *s = VIRTIO_SND(vdev); VirtIOSound *vsnd = VIRTIO_SND(vdev);
VirtIOSoundPCMStream *stream = NULL;
VirtIOSoundPCMBuffer *buffer; VirtIOSoundPCMBuffer *buffer;
VirtQueueElement *elem; VirtQueueElement *elem;
size_t msg_sz, size; size_t msg_sz, size;
virtio_snd_pcm_xfer hdr; virtio_snd_pcm_xfer hdr;
uint32_t stream_id; uint32_t stream_id;
/* /*
* If any of the I/O messages are invalid, put them in stream->invalid and * If any of the I/O messages are invalid, put them in vsnd->invalid and
* return them after the for loop. * return them after the for loop.
*/ */
bool must_empty_invalid_queue = false; bool must_empty_invalid_queue = false;
@ -901,7 +885,7 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
} }
trace_virtio_snd_handle_tx_xfer(); trace_virtio_snd_handle_tx_xfer();
for (;;) { for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) {
elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
if (!elem) { if (!elem) {
break; break;
@ -913,16 +897,16 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
&hdr, &hdr,
sizeof(virtio_snd_pcm_xfer)); sizeof(virtio_snd_pcm_xfer));
if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
continue; goto tx_err;
} }
stream_id = le32_to_cpu(hdr.stream_id); stream_id = le32_to_cpu(hdr.stream_id);
if (stream_id >= s->snd_conf.streams if (stream_id >= vsnd->snd_conf.streams
|| s->pcm->streams[stream_id] == NULL) { || vsnd->pcm->streams[stream_id] == NULL) {
continue; goto tx_err;
} }
stream = s->pcm->streams[stream_id]; stream = vsnd->pcm->streams[stream_id];
if (stream->info.direction != VIRTIO_SND_D_OUTPUT) { if (stream->info.direction != VIRTIO_SND_D_OUTPUT) {
goto tx_err; goto tx_err;
} }
@ -942,13 +926,11 @@ static void virtio_snd_handle_tx_xfer(VirtIODevice *vdev, VirtQueue *vq)
continue; continue;
tx_err: tx_err:
WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { must_empty_invalid_queue = true;
must_empty_invalid_queue = true; buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); buffer->elem = elem;
buffer->elem = elem; buffer->vq = vq;
buffer->vq = vq; QSIMPLEQ_INSERT_TAIL(&vsnd->invalid, buffer, entry);
QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
}
} }
if (must_empty_invalid_queue) { if (must_empty_invalid_queue) {
@ -965,15 +947,14 @@ tx_err:
*/ */
static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq) static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
{ {
VirtIOSound *s = VIRTIO_SND(vdev); VirtIOSound *vsnd = VIRTIO_SND(vdev);
VirtIOSoundPCMStream *stream = NULL;
VirtIOSoundPCMBuffer *buffer; VirtIOSoundPCMBuffer *buffer;
VirtQueueElement *elem; VirtQueueElement *elem;
size_t msg_sz, size; size_t msg_sz, size;
virtio_snd_pcm_xfer hdr; virtio_snd_pcm_xfer hdr;
uint32_t stream_id; uint32_t stream_id;
/* /*
* if any of the I/O messages are invalid, put them in stream->invalid and * if any of the I/O messages are invalid, put them in vsnd->invalid and
* return them after the for loop. * return them after the for loop.
*/ */
bool must_empty_invalid_queue = false; bool must_empty_invalid_queue = false;
@ -983,7 +964,7 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
} }
trace_virtio_snd_handle_rx_xfer(); trace_virtio_snd_handle_rx_xfer();
for (;;) { for (VirtIOSoundPCMStream *stream = NULL;; stream = NULL) {
elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
if (!elem) { if (!elem) {
break; break;
@ -995,16 +976,16 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
&hdr, &hdr,
sizeof(virtio_snd_pcm_xfer)); sizeof(virtio_snd_pcm_xfer));
if (msg_sz != sizeof(virtio_snd_pcm_xfer)) { if (msg_sz != sizeof(virtio_snd_pcm_xfer)) {
continue; goto rx_err;
} }
stream_id = le32_to_cpu(hdr.stream_id); stream_id = le32_to_cpu(hdr.stream_id);
if (stream_id >= s->snd_conf.streams if (stream_id >= vsnd->snd_conf.streams
|| !s->pcm->streams[stream_id]) { || !vsnd->pcm->streams[stream_id]) {
continue; goto rx_err;
} }
stream = s->pcm->streams[stream_id]; stream = vsnd->pcm->streams[stream_id];
if (stream == NULL || stream->info.direction != VIRTIO_SND_D_INPUT) { if (stream == NULL || stream->info.direction != VIRTIO_SND_D_INPUT) {
goto rx_err; goto rx_err;
} }
@ -1021,13 +1002,11 @@ static void virtio_snd_handle_rx_xfer(VirtIODevice *vdev, VirtQueue *vq)
continue; continue;
rx_err: rx_err:
WITH_QEMU_LOCK_GUARD(&stream->queue_mutex) { must_empty_invalid_queue = true;
must_empty_invalid_queue = true; buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer));
buffer = g_malloc0(sizeof(VirtIOSoundPCMBuffer)); buffer->elem = elem;
buffer->elem = elem; buffer->vq = vq;
buffer->vq = vq; QSIMPLEQ_INSERT_TAIL(&vsnd->invalid, buffer, entry);
QSIMPLEQ_INSERT_TAIL(&stream->invalid, buffer, entry);
}
} }
if (must_empty_invalid_queue) { if (must_empty_invalid_queue) {
@ -1127,6 +1106,7 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer); virtio_add_queue(vdev, 64, virtio_snd_handle_rx_xfer);
qemu_mutex_init(&vsnd->cmdq_mutex); qemu_mutex_init(&vsnd->cmdq_mutex);
QTAILQ_INIT(&vsnd->cmdq); QTAILQ_INIT(&vsnd->cmdq);
QSIMPLEQ_INIT(&vsnd->invalid);
for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) { for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
status = virtio_snd_set_pcm_params(vsnd, i, &default_params); status = virtio_snd_set_pcm_params(vsnd, i, &default_params);
@ -1376,13 +1356,20 @@ static void virtio_snd_unrealize(DeviceState *dev)
static void virtio_snd_reset(VirtIODevice *vdev) static void virtio_snd_reset(VirtIODevice *vdev)
{ {
VirtIOSound *s = VIRTIO_SND(vdev); VirtIOSound *vsnd = VIRTIO_SND(vdev);
virtio_snd_ctrl_command *cmd; virtio_snd_ctrl_command *cmd;
WITH_QEMU_LOCK_GUARD(&s->cmdq_mutex) { /*
while (!QTAILQ_EMPTY(&s->cmdq)) { * Sanity check that the invalid buffer message queue is emptied at the end
cmd = QTAILQ_FIRST(&s->cmdq); * of every virtio_snd_handle_tx_xfer/virtio_snd_handle_rx_xfer call, and
QTAILQ_REMOVE(&s->cmdq, cmd, next); * must be empty otherwise.
*/
g_assert(QSIMPLEQ_EMPTY(&vsnd->invalid));
WITH_QEMU_LOCK_GUARD(&vsnd->cmdq_mutex) {
while (!QTAILQ_EMPTY(&vsnd->cmdq)) {
cmd = QTAILQ_FIRST(&vsnd->cmdq);
QTAILQ_REMOVE(&vsnd->cmdq, cmd, next);
virtio_snd_ctrl_cmd_free(cmd); virtio_snd_ctrl_cmd_free(cmd);
} }
} }

16
include/hw/audio/virtio-snd.h
View file

@ -151,7 +151,6 @@ struct VirtIOSoundPCMStream {
QemuMutex queue_mutex; QemuMutex queue_mutex;
bool active; bool active;
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue; QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) queue;
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
}; };
/* /*
@ -223,6 +222,21 @@ struct VirtIOSound {
QemuMutex cmdq_mutex; QemuMutex cmdq_mutex;
QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq; QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq;
bool processing_cmdq; bool processing_cmdq;
/*
* Convenience queue to keep track of invalid tx/rx queue messages inside
* the tx/rx callbacks.
*
* In the callbacks as a first step we are emptying the virtqueue to handle
* each message and we cannot add an invalid message back to the queue: we
* would re-process it in subsequent loop iterations.
*
* Instead, we add them to this queue and after finishing examining every
* virtqueue element, we inform the guest for each invalid message.
*
* This queue must be empty at all times except for inside the tx/rx
* callbacks.
*/
QSIMPLEQ_HEAD(, VirtIOSoundPCMBuffer) invalid;
}; };
struct virtio_snd_ctrl_command { struct virtio_snd_ctrl_command {