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

[Mark Cave-Ayland]

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.