Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On 28/08/2023 20:55, Emmanouil Pitsidianakis wrote:
Hi Emmanouil,
Receive guest requests in the control (CTRL) queue of the virtio sound
device and reply with a NOT SUPPORTED error to all control commands.
The receiving handler is virtio_snd_handle_ctrl(). It stores all control
messages in the queue in the device's command queue. Then it calls
virtio_snd_process_cmdq() to handle each message.
The handler is process_cmd() which replies with VIRTIO_SND_S_NOT_SUPP.
Based-on:
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée
Signed-off-by: Igor Skalkin
Signed-off-by: Anton Yakovlev
Signed-off-by: Emmanouil Pitsidianakis
---
hw/virtio/trace-events | 4 +
hw/virtio/virtio-snd.c | 227 -
include/hw/virtio/virtio-snd.h | 70 +-
3 files changed, 292 insertions(+), 9 deletions(-)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 3ed7da35f2..8a223e36e9 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -163,3 +163,7 @@ virtio_snd_vm_state_running(void) "vm state running"
virtio_snd_vm_state_stopped(void) "vm state stopped"
virtio_snd_realize(void *snd) "snd %p: realize"
virtio_snd_unrealize(void *snd) "snd %p: unrealize"
+virtio_snd_handle_ctrl(void *vdev, void *vq) "snd %p: handle ctrl event for queue
%p"
+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"
+virtio_snd_handle_event(void) "event queue callback called"
diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
index a056a7bcc6..b921903905 100644
--- a/hw/virtio/virtio-snd.c
+++ b/hw/virtio/virtio-snd.c
@@ -31,6 +31,29 @@
#define VIRTIO_SOUND_CHMAP_DEFAULT 0
#define VIRTIO_SOUND_HDA_FN_NID 0
+static const char *print_code(uint32_t code)
+{
+#define CASE(CODE)\
+case VIRTIO_SND_R_##CODE: \
+return "VIRTIO_SND_R_"#CODE
+
+switch (code) {
+CASE(JACK_INFO);
+CASE(JACK_REMAP);
+CASE(PCM_INFO);
+CASE(PCM_SET_PARAMS);
+CASE(PCM_PREPARE);
+CASE(PCM_RELEASE);
+CASE(PCM_START);
+CASE(PCM_STOP);
+CASE(CHMAP_INFO);
+default:
+return "invalid code";
+}
+
+#undef CASE
+};
+
static const VMStateDescription vmstate_virtio_snd_device = {
.name = TYPE_VIRTIO_SND,
.version_id = VIRTIO_SOUND_VM_VERSION,
@@ -89,12 +112,148 @@ virtio_snd_set_config(VirtIODevice *vdev, const uint8_t
*config)
}
/*
- * Queue handler stub.
+ * The actual processing done in virtio_snd_process_cmdq().
+ *
+ * @s: VirtIOSound device
+ * @cmd: control command request
+ */
+static inline void
+process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
+{
+size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+if (sz != sizeof(cmd->ctrl)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, sz, sizeof(cmd->ctrl));
+return;
+}
+
+trace_virtio_snd_handle_code(cmd->ctrl.code,
+ print_code(cmd->ctrl.code));
+
+switch (cmd->ctrl.code) {
+case VIRTIO_SND_R_JACK_INFO:
+case VIRTIO_SND_R_JACK_REMAP:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: jack functionality is unimplemented.");
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_PCM_INFO:
+case VIRTIO_SND_R_PCM_SET_PARAMS:
+case VIRTIO_SND_R_PCM_PREPARE:
+case VIRTIO_SND_R_PCM_START:
+case VIRTIO_SND_R_PCM_STOP:
+case VIRTIO_SND_R_PCM_RELEASE:
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_CHMAP_INFO:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: chmap info functionality is unimplemented.");
+trace_virtio_snd_handle_chmap_info();
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+default:
+/* error */
+error_report("virtio snd header not recognized: %"PRIu32,
+ cmd->ctrl.code);
+cmd->resp.code = VIRTIO_SND_S_BAD_MSG;
+}
+
+iov_from_buf(cmd->elem->in_sg,
+ cmd->elem->in_num,
+ 0,
+ >resp,
+ sizeof(cmd->resp));
+virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem));
+virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
+}
+
+/*
+ * Consume all elements in command queue.
+ *
+ * @s: VirtIOSound device
+ */
+static void virtio_snd_process_cmdq(VirtIOSound *s)
+{
+virtio_snd_ctrl_command *cmd;
+
+if (unlikely(qatomic_read(>processing_cmdq))) {
+return;
+}
+
+WITH_QEMU_LOCK_GUARD(>cmdq_mutex) {
+qatomic_set(>processing_cmdq, true);
+
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On 4/9/23 13:46, Manos Pitsidianakis wrote:
On Mon, 04 Sep 2023 14:30, Philippe Mathieu-Daudé
wrote:
On 4/9/23 13:00, Manos Pitsidianakis wrote:
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé
wrote:
+ size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+ if (sz != sizeof(cmd->ctrl)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: virtio-snd command size incorrect %zu vs \
+ %zu\n", __func__, sz, sizeof(cmd->ctrl));
+ return;
+ }
+
+ trace_virtio_snd_handle_code(cmd->ctrl.code,
IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?
Not sure about the answer to this. Need input from someone more
knowledgeable in virtio.
You can test running a big-endian guest (m68k, s390x, sparc64) on your
little-endian host.
The linux driver uses le{32,64}_to_cpu for reading fields, if there's
any problem then it would be with the host?
Ah right. Since using a BE guest is often simpler for most developers
than accessing a BE host -- in particular to test recent kernel -- I was
hoping this would be an easy enough suggestion.
Anyhow Linux swapping the fields confirms the virtio-sound model has to
do the same.
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On Mon, 04 Sep 2023 14:30, Philippe Mathieu-Daudé wrote:
On 4/9/23 13:00, Manos Pitsidianakis wrote:
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé
wrote:
+ size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+ if (sz != sizeof(cmd->ctrl)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: virtio-snd command size incorrect %zu vs \
+ %zu\n", __func__, sz, sizeof(cmd->ctrl));
+ return;
+ }
+
+ trace_virtio_snd_handle_code(cmd->ctrl.code,
IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?
Not sure about the answer to this. Need input from someone more
knowledgeable in virtio.
You can test running a big-endian guest (m68k, s390x, sparc64) on your
little-endian host.
The linux driver uses le{32,64}_to_cpu for reading fields, if there's
any problem then it would be with the host?
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On 4/9/23 13:00, Manos Pitsidianakis wrote:
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé
wrote:
+ size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+ if (sz != sizeof(cmd->ctrl)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: virtio-snd command size incorrect %zu vs \
+ %zu\n", __func__, sz, sizeof(cmd->ctrl));
+ return;
+ }
+
+ trace_virtio_snd_handle_code(cmd->ctrl.code,
IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?
Not sure about the answer to this. Need input from someone more
knowledgeable in virtio.
You can test running a big-endian guest (m68k, s390x, sparc64) on your
little-endian host.
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé wrote:
+size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+if (sz != sizeof(cmd->ctrl)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, sz, sizeof(cmd->ctrl));
+return;
+}
+
+trace_virtio_snd_handle_code(cmd->ctrl.code,
IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?
Not sure about the answer to this. Need input from someone more
knowledgeable in virtio.
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On 28/8/23 21:55, Emmanouil Pitsidianakis wrote:
Receive guest requests in the control (CTRL) queue of the virtio sound
device and reply with a NOT SUPPORTED error to all control commands.
The receiving handler is virtio_snd_handle_ctrl(). It stores all control
messages in the queue in the device's command queue. Then it calls
virtio_snd_process_cmdq() to handle each message.
The handler is process_cmd() which replies with VIRTIO_SND_S_NOT_SUPP.
Based-on:
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée
Signed-off-by: Igor Skalkin
Signed-off-by: Anton Yakovlev
Signed-off-by: Emmanouil Pitsidianakis
---
hw/virtio/trace-events | 4 +
hw/virtio/virtio-snd.c | 227 -
include/hw/virtio/virtio-snd.h | 70 +-
3 files changed, 292 insertions(+), 9 deletions(-)
/*
- * Queue handler stub.
+ * The actual processing done in virtio_snd_process_cmdq().
+ *
+ * @s: VirtIOSound device
+ * @cmd: control command request
+ */
+static inline void
+process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
+{
+size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+if (sz != sizeof(cmd->ctrl)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, sz, sizeof(cmd->ctrl));
+return;
+}
+
+trace_virtio_snd_handle_code(cmd->ctrl.code,
IIUC the spec, this structure is in little endian, is that right?
So shouldn't swap various fields in this series?
+ print_code(cmd->ctrl.code));
+
+switch (cmd->ctrl.code) {
+case VIRTIO_SND_R_JACK_INFO:
+case VIRTIO_SND_R_JACK_REMAP:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: jack functionality is unimplemented.");
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_PCM_INFO:
+case VIRTIO_SND_R_PCM_SET_PARAMS:
+case VIRTIO_SND_R_PCM_PREPARE:
+case VIRTIO_SND_R_PCM_START:
+case VIRTIO_SND_R_PCM_STOP:
+case VIRTIO_SND_R_PCM_RELEASE:
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_CHMAP_INFO:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: chmap info functionality is unimplemented.");
+trace_virtio_snd_handle_chmap_info();
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+default:
+/* error */
+error_report("virtio snd header not recognized: %"PRIu32,
+ cmd->ctrl.code);
+cmd->resp.code = VIRTIO_SND_S_BAD_MSG;
+}
+
+iov_from_buf(cmd->elem->in_sg,
+ cmd->elem->in_num,
+ 0,
+ >resp,
+ sizeof(cmd->resp));
+virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem));
+virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
+}
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
On 4/9/23 12:18, Manos Pitsidianakis wrote: Good morning Philippe, On Mon, 04 Sep 2023 13:08, Philippe Mathieu-Daudé wrote: + iov_from_buf(cmd->elem->in_sg, + cmd->elem->in_num, + 0, + >resp, + sizeof(cmd->resp)); + virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem)); + virtio_notify(VIRTIO_DEVICE(s), cmd->vq); I have very few understanding of virtio, but I'm wondering here, since this function is called under cmdq_mutex(), could it be useful to batch the queue by calling virtio_notify() only once in the caller once the whole cmdq is processed ... In the linux driver (sound/virtio/virtio_ctl_msg.c), the guest has a timeout for receiving the message. I found that if I did not notify as fast as possible, I got timeout errors on the guest. Ah, I see, 1ms per default: sound/virtio/virtio_card.c:14:u32 virtsnd_msg_timeout_ms = MSEC_PER_SEC;
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
Good morning Philippe, On Mon, 04 Sep 2023 13:08, Philippe Mathieu-Daudé wrote: +iov_from_buf(cmd->elem->in_sg, + cmd->elem->in_num, + 0, + >resp, + sizeof(cmd->resp)); +virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem)); +virtio_notify(VIRTIO_DEVICE(s), cmd->vq); I have very few understanding of virtio, but I'm wondering here, since this function is called under cmdq_mutex(), could it be useful to batch the queue by calling virtio_notify() only once in the caller once the whole cmdq is processed ... In the linux driver (sound/virtio/virtio_ctl_msg.c), the guest has a timeout for receiving the message. I found that if I did not notify as fast as possible, I got timeout errors on the guest.
Re: [PATCH v8 03/12] virtio-sound: handle control messages and streams
Hi Emmanouil,
On 28/8/23 21:55, Emmanouil Pitsidianakis wrote:
Receive guest requests in the control (CTRL) queue of the virtio sound
device and reply with a NOT SUPPORTED error to all control commands.
The receiving handler is virtio_snd_handle_ctrl(). It stores all control
messages in the queue in the device's command queue. Then it calls
virtio_snd_process_cmdq() to handle each message.
The handler is process_cmd() which replies with VIRTIO_SND_S_NOT_SUPP.
Based-on:
https://github.com/OpenSynergy/qemu/commit/5a2f350eec5d157b90d9c7b40a8e603f4da92471
Reviewed-by: Alex Bennée
Signed-off-by: Igor Skalkin
Signed-off-by: Anton Yakovlev
Signed-off-by: Emmanouil Pitsidianakis
---
hw/virtio/trace-events | 4 +
hw/virtio/virtio-snd.c | 227 -
include/hw/virtio/virtio-snd.h | 70 +-
3 files changed, 292 insertions(+), 9 deletions(-)
/*
- * Queue handler stub.
+ * The actual processing done in virtio_snd_process_cmdq().
+ *
+ * @s: VirtIOSound device
+ * @cmd: control command request
+ */
+static inline void
+process_cmd(VirtIOSound *s, virtio_snd_ctrl_command *cmd)
+{
+size_t sz = iov_to_buf(cmd->elem->out_sg,
+ cmd->elem->out_num,
+ 0,
+ >ctrl,
+ sizeof(cmd->ctrl));
+if (sz != sizeof(cmd->ctrl)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+"%s: virtio-snd command size incorrect %zu vs \
+%zu\n", __func__, sz, sizeof(cmd->ctrl));
+return;
+}
+
+trace_virtio_snd_handle_code(cmd->ctrl.code,
+ print_code(cmd->ctrl.code));
+
+switch (cmd->ctrl.code) {
+case VIRTIO_SND_R_JACK_INFO:
+case VIRTIO_SND_R_JACK_REMAP:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: jack functionality is unimplemented.");
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_PCM_INFO:
+case VIRTIO_SND_R_PCM_SET_PARAMS:
+case VIRTIO_SND_R_PCM_PREPARE:
+case VIRTIO_SND_R_PCM_START:
+case VIRTIO_SND_R_PCM_STOP:
+case VIRTIO_SND_R_PCM_RELEASE:
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+case VIRTIO_SND_R_CHMAP_INFO:
+qemu_log_mask(LOG_UNIMP,
+ "virtio_snd: chmap info functionality is unimplemented.");
+trace_virtio_snd_handle_chmap_info();
+cmd->resp.code = VIRTIO_SND_S_NOT_SUPP;
+break;
+default:
+/* error */
+error_report("virtio snd header not recognized: %"PRIu32,
+ cmd->ctrl.code);
+cmd->resp.code = VIRTIO_SND_S_BAD_MSG;
+}
+
+iov_from_buf(cmd->elem->in_sg,
+ cmd->elem->in_num,
+ 0,
+ >resp,
+ sizeof(cmd->resp));
+virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem));
+virtio_notify(VIRTIO_DEVICE(s), cmd->vq);
I have very few understanding of virtio, but I'm wondering here,
since this function is called under cmdq_mutex(), could it be
useful to batch the queue by calling virtio_notify() only once
in the caller once the whole cmdq is processed ...
+}
+
+/*
+ * Consume all elements in command queue.
+ *
+ * @s: VirtIOSound device
+ */
+static void virtio_snd_process_cmdq(VirtIOSound *s)
+{
+virtio_snd_ctrl_command *cmd;
+
+if (unlikely(qatomic_read(>processing_cmdq))) {
+return;
+}
+
+WITH_QEMU_LOCK_GUARD(>cmdq_mutex) {
+qatomic_set(>processing_cmdq, true);
+while (!QTAILQ_EMPTY(>cmdq)) {
+cmd = QTAILQ_FIRST(>cmdq);
+
+/* process command */
+process_cmd(s, cmd);
+
+QTAILQ_REMOVE(>cmdq, cmd, next);
+
+g_free(cmd);
+}
... here?
+qatomic_set(>processing_cmdq, false);
+}
+}
