Re: [PATCH v7 01/12] Add virtio-sound device stub

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

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

> Add a new VIRTIO device for the virtio sound device id. Functionality
> will be added in the following commits.
>
> Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  MAINTAINERS                    |   6 +
>  hw/virtio/Kconfig              |   5 +
>  hw/virtio/meson.build          |   1 +
>  hw/virtio/trace-events         |   9 ++
>  hw/virtio/virtio-snd.c         | 230 +++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-snd.h |  78 +++++++++++
>  6 files changed, 329 insertions(+)
>  create mode 100644 hw/virtio/virtio-snd.c
>  create mode 100644 include/hw/virtio/virtio-snd.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6111b6b4d9..ba365d621c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2245,6 +2245,12 @@ F: hw/virtio/virtio-mem-pci.h
>  F: hw/virtio/virtio-mem-pci.c
>  F: include/hw/virtio/virtio-mem.h
>  
> +virtio-snd
> +M: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> +S: Supported
> +F: hw/virtio/virtio-snd*.c
> +F: include/hw/virtio/virtio-snd.h
> +
>  nvme
>  M: Keith Busch <kbusch@kernel.org>
>  M: Klaus Jensen <its@irrelevant.dk>
> diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
> index 92c9cf6c96..d6f20657b3 100644
> --- a/hw/virtio/Kconfig
> +++ b/hw/virtio/Kconfig
> @@ -17,6 +17,11 @@ config VIRTIO_PCI
>      depends on PCI
>      select VIRTIO
>  
> +config VIRTIO_SND
> +    bool
> +    default y
> +    depends on VIRTIO
> +
>  config VIRTIO_MMIO
>      bool
>      select VIRTIO
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index 13e7c6c272..120d4bfa0a 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -31,6 +31,7 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock.c
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vsock.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
> +specific_virtio_ss.add(when: 'CONFIG_VIRTIO_SND', if_true: files('virtio-snd.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
>  specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index 7109cf1a3b..3ed7da35f2 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -154,3 +154,12 @@ virtio_pmem_flush_done(int type) "fsync return=%d"
>  virtio_gpio_start(void) "start"
>  virtio_gpio_stop(void) "stop"
>  virtio_gpio_set_status(uint8_t status) "0x%x"
> +
> +#virtio-snd.c
> +virtio_snd_get_config(void *vdev, uint32_t jacks, uint32_t streams, uint32_t chmaps) "snd %p: get_config jacks=%"PRIu32" streams=%"PRIu32" chmaps=%"PRIu32""
> +virtio_snd_set_config(void *vdev, uint32_t jacks, uint32_t new_jacks, uint32_t streams, uint32_t new_streams, uint32_t chmaps, uint32_t new_chmaps) "snd %p: set_config jacks from %"PRIu32"->%"PRIu32", streams from %"PRIu32"->%"PRIu32", chmaps from %"PRIu32"->%"PRIu32
> +virtio_snd_get_features(void *vdev, uint64_t features) "snd %p: get_features 0x%"PRIx64
> +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"
> diff --git a/hw/virtio/virtio-snd.c b/hw/virtio/virtio-snd.c
> new file mode 100644
> index 0000000000..0498e660a5
> --- /dev/null
> +++ b/hw/virtio/virtio-snd.c
> @@ -0,0 +1,230 @@
> +/*
> + * VIRTIO Sound Device conforming to
> + *
> + * "Virtual I/O Device (VIRTIO) Version 1.2
> + * Committee Specification Draft 01
> + * 09 May 2022"
> + *
> + * <https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-52900014>
> + *
> + * Copyright (c) 2023 Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> + * Copyright (C) 2019 OpenSynergy GmbH

Given its based on OpenSynergies patches we probably should bring their
s-o-b's across or add Based-on: references in the commit messages.

> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "qemu/log.h"
> +#include "include/qemu/lockable.h"
> +#include "sysemu/runstate.h"
> +#include "trace.h"
> +#include "qapi/error.h"
> +#include "hw/virtio/virtio-snd.h"
> +
> +#define VIRTIO_SOUND_VM_VERSION 1
> +#define VIRTIO_SOUND_JACK_DEFAULT 0
> +#define VIRTIO_SOUND_STREAM_DEFAULT 1
> +#define VIRTIO_SOUND_CHMAP_DEFAULT 0
> +#define VIRTIO_SOUND_HDA_FN_NID 0
> +
> +static const VMStateDescription vmstate_virtio_snd_device = {
> +    .name = TYPE_VIRTIO_SND,
> +    .version_id = VIRTIO_SOUND_VM_VERSION,
> +    .minimum_version_id = VIRTIO_SOUND_VM_VERSION,
> +};
> +
> +static const VMStateDescription vmstate_virtio_snd = {
> +    .name = "virtio-sound",
> +    .minimum_version_id = VIRTIO_SOUND_VM_VERSION,
> +    .version_id = VIRTIO_SOUND_VM_VERSION,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_VIRTIO_DEVICE,
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static Property virtio_snd_properties[] = {
> +    DEFINE_PROP_UINT32("jacks", VirtIOSound, snd_conf.jacks,
> +                       VIRTIO_SOUND_JACK_DEFAULT),
> +    DEFINE_PROP_UINT32("streams", VirtIOSound, snd_conf.streams,
> +                       VIRTIO_SOUND_STREAM_DEFAULT),
> +    DEFINE_PROP_UINT32("chmaps", VirtIOSound, snd_conf.chmaps,
> +                       VIRTIO_SOUND_CHMAP_DEFAULT),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void
> +virtio_snd_get_config(VirtIODevice *vdev, uint8_t *config)
> +{
> +    VirtIOSound *s = VIRTIO_SND(vdev);
> +    trace_virtio_snd_get_config(vdev,
> +                                s->snd_conf.jacks,
> +                                s->snd_conf.streams,
> +                                s->snd_conf.chmaps);
> +
> +    memcpy(config, &s->snd_conf, sizeof(s->snd_conf));
> +}
> +
> +static void
> +virtio_snd_set_config(VirtIODevice *vdev, const uint8_t *config)
> +{
> +    VirtIOSound *s = VIRTIO_SND(vdev);
> +    const virtio_snd_config *sndconfig =
> +        (const virtio_snd_config *)config;
> +
> +
> +   trace_virtio_snd_set_config(vdev,
> +                               s->snd_conf.jacks,
> +                               sndconfig->jacks,
> +                               s->snd_conf.streams,
> +                               sndconfig->streams,
> +                               s->snd_conf.chmaps,
> +                               sndconfig->chmaps);
> +
> +    memcpy(&s->snd_conf, sndconfig, sizeof(s->snd_conf));
> +}
> +
> +/*
> + * Queue handler stub.

"Temporary queue handler stub"? seeing as it disappears once everything
is complete.

> + *
> + * @vdev: VirtIOSound device
> + * @vq: virtqueue
> + */
> +static void virtio_snd_handle_queue(VirtIODevice *vdev, VirtQueue
> *vq) {}
> +
> +static uint64_t get_features(VirtIODevice *vdev, uint64_t features,
> +                             Error **errp)
> +{
> +    /*
> +     * virtio-v1.2-csd01, 5.14.3,
> +     * Feature Bits
> +     * None currently defined.
> +     */
> +    VirtIOSound *s = VIRTIO_SND(vdev);
> +    features |= s->features;
> +
> +    trace_virtio_snd_get_features(vdev, features);
> +
> +    return features;
> +}
> +
> +static void virtio_snd_common_realize(DeviceState *dev,
> +                                      VirtIOHandleOutput ctrl,
> +                                      VirtIOHandleOutput evt,
> +                                      VirtIOHandleOutput txq,
> +                                      VirtIOHandleOutput rxq,
> +                                      Error **errp)
> +{

Why split this off from _realize? It only ever has one caller.

> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VirtIOSound *vsnd = VIRTIO_SND(dev);
> +
> +    virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
> +    virtio_add_feature(&vsnd->features, VIRTIO_F_VERSION_1);
> +
> +    /* set number of jacks and streams */
> +    if (vsnd->snd_conf.jacks > 8) {
> +        error_setg(errp,
> +                   "Invalid number of jacks: %"PRIu32,
> +                   vsnd->snd_conf.jacks);
> +        return;
> +    }
> +    if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
> +        error_setg(errp,
> +                   "Invalid number of streams: %"PRIu32,
> +                    vsnd->snd_conf.streams);
> +        return;
> +    }
> +
> +    if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
> +        error_setg(errp,
> +                   "Invalid number of channel maps: %"PRIu32,
> +                   vsnd->snd_conf.chmaps);
> +        return;
> +    }
> +
> +    AUD_register_card("virtio-sound", &vsnd->card);
> +
> +    vsnd->queues[VIRTIO_SND_VQ_CONTROL] = virtio_add_queue(vdev, 64, ctrl);
> +    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);
> +}
> +
> +static void
> +virtio_snd_vm_state_change(void *, bool running, RunState)
> +{
> +    if (running) {
> +        trace_virtio_snd_vm_state_running();
> +    } else {
> +        trace_virtio_snd_vm_state_stopped();
> +    }
> +}
> +
> +static void virtio_snd_realize(DeviceState *dev, Error **errp)
> +{
> +    ERRP_GUARD();
> +    VirtIOSound *vsnd = VIRTIO_SND(dev);
> +
> +    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,
> +                              errp);
> +}
> +
> +static void virtio_snd_unrealize(DeviceState *dev)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VirtIOSound *vsnd = VIRTIO_SND(dev);
> +
> +    qemu_del_vm_change_state_handler(vsnd->vmstate);
> +    virtio_del_queue(vdev, 0);
> +
> +    trace_virtio_snd_unrealize(vsnd);
> +
> +    AUD_remove_card(&vsnd->card);
> +    virtio_cleanup(vdev);
> +}
> +
> +
> +static void virtio_snd_reset(VirtIODevice *vdev) {}
> +
> +static void virtio_snd_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> +
> +    set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> +    device_class_set_props(dc, virtio_snd_properties);
> +
> +    dc->vmsd = &vmstate_virtio_snd;
> +    vdc->vmsd = &vmstate_virtio_snd_device;
> +    vdc->realize = virtio_snd_realize;
> +    vdc->unrealize = virtio_snd_unrealize;
> +    vdc->get_config = virtio_snd_get_config;
> +    vdc->set_config = virtio_snd_set_config;
> +    vdc->get_features = get_features;
> +    vdc->reset = virtio_snd_reset;
> +    vdc->legacy_features = 0;
> +}
> +
> +static const TypeInfo virtio_snd_types[] = {
> +    {
> +      .name          = TYPE_VIRTIO_SND,
> +      .parent        = TYPE_VIRTIO_DEVICE,
> +      .instance_size = sizeof(VirtIOSound),
> +      .class_init    = virtio_snd_class_init,
> +    }
> +};
> +
> +DEFINE_TYPES(virtio_snd_types)
> diff --git a/include/hw/virtio/virtio-snd.h b/include/hw/virtio/virtio-snd.h
> new file mode 100644
> index 0000000000..b3c0e6f079
> --- /dev/null
> +++ b/include/hw/virtio/virtio-snd.h
> @@ -0,0 +1,78 @@
> +/*
> + * VIRTIO Sound Device conforming to
> + *
> + * "Virtual I/O Device (VIRTIO) Version 1.2
> + * Committee Specification Draft 01
> + * 09 May 2022"
> + *
> + * Copyright (c) 2023 Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
> + * Copyright (C) 2019 OpenSynergy GmbH
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#ifndef QEMU_VIRTIO_SOUND_H
> +#define QEMU_VIRTIO_SOUND_H
> +
> +#include "hw/virtio/virtio.h"
> +#include "audio/audio.h"
> +#include "standard-headers/linux/virtio_ids.h"
> +#include "standard-headers/linux/virtio_snd.h"
> +
> +#define TYPE_VIRTIO_SND "virtio-sound"
> +#define VIRTIO_SND(obj) \
> +        OBJECT_CHECK(VirtIOSound, (obj), TYPE_VIRTIO_SND)
> +
> +/* CONFIGURATION SPACE */
> +
> +typedef struct virtio_snd_config virtio_snd_config;
> +
> +/* COMMON DEFINITIONS */
> +
> +/* common header for request/response*/
> +typedef struct virtio_snd_hdr virtio_snd_hdr;
> +
> +/* event notification */
> +typedef struct virtio_snd_event virtio_snd_event;
> +
> +/* common control request to query an item information */
> +typedef struct virtio_snd_query_info virtio_snd_query_info;
> +
> +/* JACK CONTROL MESSAGES */
> +
> +typedef struct virtio_snd_jack_hdr virtio_snd_jack_hdr;
> +
> +/* jack information structure */
> +typedef struct virtio_snd_jack_info virtio_snd_jack_info;
> +
> +/* jack remapping control request */
> +typedef struct virtio_snd_jack_remap virtio_snd_jack_remap;
> +
> +/*
> + * PCM CONTROL MESSAGES
> + */
> +typedef struct virtio_snd_pcm_hdr virtio_snd_pcm_hdr;
> +
> +/* PCM stream info structure */
> +typedef struct virtio_snd_pcm_info virtio_snd_pcm_info;
> +
> +/* set PCM stream params */
> +typedef struct virtio_snd_pcm_set_params virtio_snd_pcm_set_params;
> +
> +/* I/O request header */
> +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 {
> +    VirtIODevice parent_obj;
> +    VirtQueue *queues[VIRTIO_SND_VQ_MAX];
> +    uint64_t features;
> +    QEMUSoundCard card;
> +    VMChangeStateEntry *vmstate;
> +    virtio_snd_config snd_conf;
> +} VirtIOSound;
> +#endif

Otherwise:

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro