Re: [PATCH v7 03/12] virtio-sound: handle control messages and streams

["Alex Bennée" <alex.bennee@linaro.org>]

Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes:

> 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.
>
> 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 0498e660a5..b23f8040e1 100644
> --- a/hw/virtio/virtio-snd.c
> +++ b/hw/virtio/virtio-snd.c
> @@ -30,6 +30,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,
> @@ -88,12 +111,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)
> @@ -111,6 +270,18 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
>      return features;
>  }
>  
> +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_common_realize(DeviceState *dev,
>                                        VirtIOHandleOutput ctrl,
>                                        VirtIOHandleOutput evt,
> @@ -121,6 +292,8 @@ static void virtio_snd_common_realize(DeviceState *dev,
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSound *vsnd = VIRTIO_SND(dev);
>  
> +    virtio_snd_set_pcm(vsnd);
> +
>      virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
>      virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
>  
> @@ -151,6 +324,8 @@ static void virtio_snd_common_realize(DeviceState *dev,
>      vsnd->queues[VIRTIO_SND_VQ_EVENT] = virtio_add_queue(vdev, 64, evt);
>      vsnd->queues[VIRTIO_SND_VQ_TX] = virtio_add_queue(vdev, 64, txq);
>      vsnd->queues[VIRTIO_SND_VQ_RX] = virtio_add_queue(vdev, 64, rxq);
> +    qemu_mutex_init(&vsnd->cmdq_mutex);
> +    QTAILQ_INIT(&vsnd->cmdq);
>  }
>  
>  static void
> @@ -168,35 +343,73 @@ static void virtio_snd_realize(DeviceState *dev, Error **errp)
>      ERRP_GUARD();
>      VirtIOSound *vsnd = VIRTIO_SND(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_common_realize(dev,
> -                              virtio_snd_handle_queue,
> -                              virtio_snd_handle_queue,
> -                              virtio_snd_handle_queue,
> -                              virtio_snd_handle_queue,

Given handle queue only lasts for 2 patches you might as well start with
3 stubs for the queues in the first patch.

> +                              virtio_snd_handle_ctrl,
> +                              virtio_snd_handle_event,
> +                              virtio_snd_handle_xfer,
> +                              virtio_snd_handle_xfer,
>                                errp);
>  }

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro