| Message ID | e3e57dd125611eeb5e563eb7fab8eb89194ed50e.1693252037.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add VIRTIO sound card | expand |
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 <alex.bennee@linaro.org> > Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com> > Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com> > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > 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, > + &cmd->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, > + &cmd->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(&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); > + > + /* process command */ > + process_cmd(s, cmd); > + > + QTAILQ_REMOVE(&s->cmdq, cmd, next); > + > + g_free(cmd); > + } ... here? > + qatomic_set(&s->processing_cmdq, false); > + } > +}
Good morning Philippe, On Mon, 04 Sep 2023 13:08, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> + iov_from_buf(cmd->elem->in_sg, >> + cmd->elem->in_num, >> + 0, >> + &cmd->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.
On 4/9/23 12:18, Manos Pitsidianakis wrote: > Good morning Philippe, > > On Mon, 04 Sep 2023 13:08, Philippe Mathieu-Daudé <philmd@linaro.org> > wrote: >>> + iov_from_buf(cmd->elem->in_sg, >>> + cmd->elem->in_num, >>> + 0, >>> + &cmd->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;
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 <alex.bennee@linaro.org> > Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com> > Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com> > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > 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, > + &cmd->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, > + &cmd->resp, > + sizeof(cmd->resp)); > + virtqueue_push(cmd->vq, cmd->elem, sizeof(cmd->elem)); > + virtio_notify(VIRTIO_DEVICE(s), cmd->vq); > +}
On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> + size_t sz = iov_to_buf(cmd->elem->out_sg, >> + cmd->elem->out_num, >> + 0, >> + &cmd->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.
On 4/9/23 13:00, Manos Pitsidianakis wrote: > On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé <philmd@linaro.org> > wrote: >>> + size_t sz = iov_to_buf(cmd->elem->out_sg, >>> + cmd->elem->out_num, >>> + 0, >>> + &cmd->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.
On Mon, 04 Sep 2023 14:30, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >On 4/9/23 13:00, Manos Pitsidianakis wrote: >> On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé <philmd@linaro.org> >> wrote: >>>> + size_t sz = iov_to_buf(cmd->elem->out_sg, >>>> + cmd->elem->out_num, >>>> + 0, >>>> + &cmd->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?
On 4/9/23 13:46, Manos Pitsidianakis wrote: > On Mon, 04 Sep 2023 14:30, Philippe Mathieu-Daudé <philmd@linaro.org> > wrote: >> On 4/9/23 13:00, Manos Pitsidianakis wrote: >>> On Mon, 04 Sep 2023 13:46, Philippe Mathieu-Daudé <philmd@linaro.org> >>> wrote: >>>>> + size_t sz = iov_to_buf(cmd->elem->out_sg, >>>>> + cmd->elem->out_num, >>>>> + 0, >>>>> + &cmd->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.
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 <alex.bennee@linaro.org> > Signed-off-by: Igor Skalkin <Igor.Skalkin@opensynergy.com> > Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com> > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > 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, > + &cmd->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, > + &cmd->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(&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); > + > + /* process command */ > + process_cmd(s, cmd); > + > + QTAILQ_REMOVE(&s->cmdq, cmd, next); > + > + g_free(cmd); > + } > + qatomic_set(&s->processing_cmdq, false); > + } > +} > + > +/* > + * The control message handler. Pops an element from the control virtqueue, > + * and stores them to VirtIOSound's cmdq queue and finally calls > + * virtio_snd_process_cmdq() for processing. > + * > + * @vdev: VirtIOSound device > + * @vq: Control virtqueue > + */ > +static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) > +{ > + VirtIOSound *s = VIRTIO_SND(vdev); > + VirtQueueElement *elem; > + virtio_snd_ctrl_command *cmd; > + > + trace_virtio_snd_handle_ctrl(vdev, vq); > + > + if (!virtio_queue_ready(vq)) { > + return; > + } > + > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > + while (elem) { > + cmd = g_new0(virtio_snd_ctrl_command, 1); > + cmd->elem = elem; > + cmd->vq = vq; > + cmd->resp.code = VIRTIO_SND_S_OK; > + QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next); > + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); > + } > + > + virtio_snd_process_cmdq(s); > +} > + > +/* > + * The event virtqueue handler. > + * Not implemented yet. > + * > + * @vdev: VirtIOSound device > + * @vq: event vq > + */ > +static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq) > +{ > + qemu_log_mask(LOG_UNIMP, "virtio_snd: event queue is unimplemented."); > + trace_virtio_snd_handle_event(); > +} > + > +/* > + * Stub buffer virtqueue handler. > * > * @vdev: VirtIOSound device > * @vq: virtqueue > */ > -static void virtio_snd_handle_queue(VirtIODevice *vdev, VirtQueue *vq) {} > +static void virtio_snd_handle_xfer(VirtIODevice *vdev, VirtQueue *vq) {} > > static uint64_t get_features(VirtIODevice *vdev, uint64_t features, > Error **errp) > @@ -123,17 +282,32 @@ virtio_snd_vm_state_change(void *opaque, bool running, > } > } > > +static void virtio_snd_set_pcm(VirtIOSound *snd) > +{ > + VirtIOSoundPCM *pcm; > + > + pcm = g_new0(VirtIOSoundPCM, 1); > + pcm->snd = snd; > + pcm->streams = g_new0(VirtIOSoundPCMStream *, snd->snd_conf.streams); > + pcm->pcm_params = g_new0(VirtIOSoundPCMParams, snd->snd_conf.streams); > + > + snd->pcm = pcm; > +} This modelling seems a bit odd: I'll comment a bit further below. > static void virtio_snd_realize(DeviceState *dev, Error **errp) > { > ERRP_GUARD(); > VirtIOSound *vsnd = VIRTIO_SND(dev); > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > + vsnd->pcm = NULL; > vsnd->vmstate = > qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd); > > trace_virtio_snd_realize(vsnd); > > + virtio_snd_set_pcm(vsnd); > + > virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config)); > virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1); > > @@ -161,31 +335,70 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) > AUD_register_card("virtio-sound", &vsnd->card); > > vsnd->queues[VIRTIO_SND_VQ_CONTROL] = > - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); > + virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl); > vsnd->queues[VIRTIO_SND_VQ_EVENT] = > - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); > + virtio_add_queue(vdev, 64, virtio_snd_handle_event); > vsnd->queues[VIRTIO_SND_VQ_TX] = > - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); > + virtio_add_queue(vdev, 64, virtio_snd_handle_xfer); > vsnd->queues[VIRTIO_SND_VQ_RX] = > - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); > + virtio_add_queue(vdev, 64, virtio_snd_handle_xfer); > + qemu_mutex_init(&vsnd->cmdq_mutex); > + QTAILQ_INIT(&vsnd->cmdq); > +} > + > +/* > + * Close the stream and free its resources. > + * > + * @stream: VirtIOSoundPCMStream *stream > + */ > +static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream) > +{ > } > > static void virtio_snd_unrealize(DeviceState *dev) > { > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VirtIOSound *vsnd = VIRTIO_SND(dev); > + VirtIOSoundPCMStream *stream; > > qemu_del_vm_change_state_handler(vsnd->vmstate); > virtio_del_queue(vdev, 0); > > trace_virtio_snd_unrealize(vsnd); > > + if (vsnd->pcm) { > + 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); > + } > + g_free(vsnd->pcm); > + vsnd->pcm = NULL; > + } > AUD_remove_card(&vsnd->card); > virtio_cleanup(vdev); > } > > > -static void virtio_snd_reset(VirtIODevice *vdev) {} > +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); > + g_free(cmd); > + } > + } > +} > > static void virtio_snd_class_init(ObjectClass *klass, void *data) > { > diff --git a/include/hw/virtio/virtio-snd.h b/include/hw/virtio/virtio-snd.h > index b3c0e6f079..b7046418cf 100644 > --- a/include/hw/virtio/virtio-snd.h > +++ b/include/hw/virtio/virtio-snd.h > @@ -67,12 +67,78 @@ typedef struct virtio_snd_pcm_xfer virtio_snd_pcm_xfer; > /* I/O request status */ > typedef struct virtio_snd_pcm_status virtio_snd_pcm_status; > > -typedef struct VirtIOSound { > +/* device structs */ > + > +typedef struct VirtIOSound VirtIOSound; > + > +typedef struct VirtIOSoundPCMStream VirtIOSoundPCMStream; > + > +typedef struct virtio_snd_ctrl_command virtio_snd_ctrl_command; > + > +typedef struct VirtIOSoundPCMParams VirtIOSoundPCMParams; > + > +typedef struct VirtIOSoundPCM VirtIOSoundPCM; > + > +/* Stream params */ > +struct VirtIOSoundPCMParams { > + uint32_t features; > + uint32_t buffer_bytes; /* size of hardware buffer in bytes */ > + uint32_t period_bytes; /* size of hardware period in bytes */ > + uint8_t channels; > + uint8_t format; > + uint8_t rate; > +}; > + > +struct VirtIOSoundPCM { > + VirtIOSound *snd; > + VirtIOSoundPCMParams *pcm_params; > + VirtIOSoundPCMStream **streams; > +}; > + Both pcm_params and streams are set in virtio_snd_set_pcm() to match the number of available streams. > +struct VirtIOSoundPCMStream { > + VirtIOSoundPCM *pcm; > + virtio_snd_pcm_info info; > + uint32_t id; > + uint32_t buffer_bytes; > + uint32_t period_bytes; > + /* channel position values (VIRTIO_SND_CHMAP_XXX) */ > + uint8_t positions[VIRTIO_SND_CHMAP_MAX_SIZE]; > + VirtIOSound *s; > + uint32_t features; /* 1 << VIRTIO_SND_PCM_F_XXX */ > + uint64_t formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */ > + uint64_t rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */ > + uint8_t direction; > + uint8_t channels_min; > + uint8_t channels_max; > + bool flushing; > + audsettings as; > + audsettings desired_as; > + union { > + SWVoiceIn *in; > + SWVoiceOut *out; > + } voice; > + QemuMutex queue_mutex; > + QSIMPLEQ_HEAD(, VirtIOSoundPCMBlock) queue; > +}; > + > +struct VirtIOSound { > VirtIODevice parent_obj; Missing newline here. > VirtQueue *queues[VIRTIO_SND_VQ_MAX]; > uint64_t features; > + VirtIOSoundPCM *pcm; See comment below. > QEMUSoundCard card; > VMChangeStateEntry *vmstate; > virtio_snd_config snd_conf; > -} VirtIOSound; > + QemuMutex cmdq_mutex; > + QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq; > + bool processing_cmdq; > +}; > + > +struct virtio_snd_ctrl_command { > + VirtQueueElement *elem; > + VirtQueue *vq; > + virtio_snd_hdr ctrl; > + virtio_snd_hdr resp; > + QTAILQ_ENTRY(virtio_snd_ctrl_command) next; > +}; > #endif On that basis it seems a reasonable idea to embed the config directly in the stream and drop VirtIOSoundPCM i.e.: struct VirtIOSoundPCMStream { VirtIOSoundPCMParams pcm_param; virtio_snd_pcm_info info; uint32_t id; uint32_t buffer_bytes; uint32_t period_bytes; ... ... }; You can then alter VirtIOSound to point to the streams directly i.e. struct VirtIOSound { VirtIODevice parent_obj; VirtQueue *queues[VIRTIO_SND_VQ_MAX]; uint64_t features; VirtIOSoundPCMStream **streams; QEMUSoundCard card; VMChangeStateEntry *vmstate; virtio_snd_config snd_conf; QemuMutex cmdq_mutex; QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq; bool processing_cmdq; }; This should also save maintaining extra "back" pointers, and you can still obtain a reference to elements in the parent object via container_of(). ATB, Mark.
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, + &cmd->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, + &cmd->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(&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); + + /* process command */ + process_cmd(s, cmd); + + QTAILQ_REMOVE(&s->cmdq, cmd, next); + + g_free(cmd); + } + qatomic_set(&s->processing_cmdq, false); + } +} + +/* + * The control message handler. Pops an element from the control virtqueue, + * and stores them to VirtIOSound's cmdq queue and finally calls + * virtio_snd_process_cmdq() for processing. + * + * @vdev: VirtIOSound device + * @vq: Control virtqueue + */ +static void virtio_snd_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) +{ + VirtIOSound *s = VIRTIO_SND(vdev); + VirtQueueElement *elem; + virtio_snd_ctrl_command *cmd; + + trace_virtio_snd_handle_ctrl(vdev, vq); + + if (!virtio_queue_ready(vq)) { + return; + } + + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); + while (elem) { + cmd = g_new0(virtio_snd_ctrl_command, 1); + cmd->elem = elem; + cmd->vq = vq; + cmd->resp.code = VIRTIO_SND_S_OK; + QTAILQ_INSERT_TAIL(&s->cmdq, cmd, next); + elem = virtqueue_pop(vq, sizeof(VirtQueueElement)); + } + + virtio_snd_process_cmdq(s); +} + +/* + * The event virtqueue handler. + * Not implemented yet. + * + * @vdev: VirtIOSound device + * @vq: event vq + */ +static void virtio_snd_handle_event(VirtIODevice *vdev, VirtQueue *vq) +{ + qemu_log_mask(LOG_UNIMP, "virtio_snd: event queue is unimplemented."); + trace_virtio_snd_handle_event(); +} + +/* + * Stub buffer virtqueue handler. * * @vdev: VirtIOSound device * @vq: virtqueue */ -static void virtio_snd_handle_queue(VirtIODevice *vdev, VirtQueue *vq) {} +static void virtio_snd_handle_xfer(VirtIODevice *vdev, VirtQueue *vq) {} static uint64_t get_features(VirtIODevice *vdev, uint64_t features, Error **errp) @@ -123,17 +282,32 @@ virtio_snd_vm_state_change(void *opaque, bool running, } } +static void virtio_snd_set_pcm(VirtIOSound *snd) +{ + VirtIOSoundPCM *pcm; + + pcm = g_new0(VirtIOSoundPCM, 1); + pcm->snd = snd; + pcm->streams = g_new0(VirtIOSoundPCMStream *, snd->snd_conf.streams); + pcm->pcm_params = g_new0(VirtIOSoundPCMParams, snd->snd_conf.streams); + + snd->pcm = pcm; +} + static void virtio_snd_realize(DeviceState *dev, Error **errp) { ERRP_GUARD(); VirtIOSound *vsnd = VIRTIO_SND(dev); VirtIODevice *vdev = VIRTIO_DEVICE(dev); + vsnd->pcm = NULL; vsnd->vmstate = qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd); trace_virtio_snd_realize(vsnd); + virtio_snd_set_pcm(vsnd); + virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config)); virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1); @@ -161,31 +335,70 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp) AUD_register_card("virtio-sound", &vsnd->card); vsnd->queues[VIRTIO_SND_VQ_CONTROL] = - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); + virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl); vsnd->queues[VIRTIO_SND_VQ_EVENT] = - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); + virtio_add_queue(vdev, 64, virtio_snd_handle_event); vsnd->queues[VIRTIO_SND_VQ_TX] = - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); + virtio_add_queue(vdev, 64, virtio_snd_handle_xfer); vsnd->queues[VIRTIO_SND_VQ_RX] = - virtio_add_queue(vdev, 64, virtio_snd_handle_queue); + virtio_add_queue(vdev, 64, virtio_snd_handle_xfer); + qemu_mutex_init(&vsnd->cmdq_mutex); + QTAILQ_INIT(&vsnd->cmdq); +} + +/* + * Close the stream and free its resources. + * + * @stream: VirtIOSoundPCMStream *stream + */ +static void virtio_snd_pcm_close(VirtIOSoundPCMStream *stream) +{ } static void virtio_snd_unrealize(DeviceState *dev) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); VirtIOSound *vsnd = VIRTIO_SND(dev); + VirtIOSoundPCMStream *stream; qemu_del_vm_change_state_handler(vsnd->vmstate); virtio_del_queue(vdev, 0); trace_virtio_snd_unrealize(vsnd); + if (vsnd->pcm) { + 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); + } + g_free(vsnd->pcm); + vsnd->pcm = NULL; + } AUD_remove_card(&vsnd->card); virtio_cleanup(vdev); } -static void virtio_snd_reset(VirtIODevice *vdev) {} +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); + g_free(cmd); + } + } +} static void virtio_snd_class_init(ObjectClass *klass, void *data) { diff --git a/include/hw/virtio/virtio-snd.h b/include/hw/virtio/virtio-snd.h index b3c0e6f079..b7046418cf 100644 --- a/include/hw/virtio/virtio-snd.h +++ b/include/hw/virtio/virtio-snd.h @@ -67,12 +67,78 @@ typedef struct virtio_snd_pcm_xfer virtio_snd_pcm_xfer; /* I/O request status */ typedef struct virtio_snd_pcm_status virtio_snd_pcm_status; -typedef struct VirtIOSound { +/* device structs */ + +typedef struct VirtIOSound VirtIOSound; + +typedef struct VirtIOSoundPCMStream VirtIOSoundPCMStream; + +typedef struct virtio_snd_ctrl_command virtio_snd_ctrl_command; + +typedef struct VirtIOSoundPCMParams VirtIOSoundPCMParams; + +typedef struct VirtIOSoundPCM VirtIOSoundPCM; + +/* Stream params */ +struct VirtIOSoundPCMParams { + uint32_t features; + uint32_t buffer_bytes; /* size of hardware buffer in bytes */ + uint32_t period_bytes; /* size of hardware period in bytes */ + uint8_t channels; + uint8_t format; + uint8_t rate; +}; + +struct VirtIOSoundPCM { + VirtIOSound *snd; + VirtIOSoundPCMParams *pcm_params; + VirtIOSoundPCMStream **streams; +}; + +struct VirtIOSoundPCMStream { + VirtIOSoundPCM *pcm; + virtio_snd_pcm_info info; + uint32_t id; + uint32_t buffer_bytes; + uint32_t period_bytes; + /* channel position values (VIRTIO_SND_CHMAP_XXX) */ + uint8_t positions[VIRTIO_SND_CHMAP_MAX_SIZE]; + VirtIOSound *s; + uint32_t features; /* 1 << VIRTIO_SND_PCM_F_XXX */ + uint64_t formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */ + uint64_t rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */ + uint8_t direction; + uint8_t channels_min; + uint8_t channels_max; + bool flushing; + audsettings as; + audsettings desired_as; + union { + SWVoiceIn *in; + SWVoiceOut *out; + } voice; + QemuMutex queue_mutex; + QSIMPLEQ_HEAD(, VirtIOSoundPCMBlock) queue; +}; + +struct VirtIOSound { VirtIODevice parent_obj; VirtQueue *queues[VIRTIO_SND_VQ_MAX]; uint64_t features; + VirtIOSoundPCM *pcm; QEMUSoundCard card; VMChangeStateEntry *vmstate; virtio_snd_config snd_conf; -} VirtIOSound; + QemuMutex cmdq_mutex; + QTAILQ_HEAD(, virtio_snd_ctrl_command) cmdq; + bool processing_cmdq; +}; + +struct virtio_snd_ctrl_command { + VirtQueueElement *elem; + VirtQueue *vq; + virtio_snd_hdr ctrl; + virtio_snd_hdr resp; + QTAILQ_ENTRY(virtio_snd_ctrl_command) next; +}; #endif