Re: [PATCH v2] sndif: add ABI for Para-virtual sound

["Roger Pau Monné" <roger.pau@citrix.com>]

El 20/01/15 a les 11.22, Oleksandr Dmytryshyn ha escrit:
> Frontend driver registers an virtual sound card
> and sends an PCM streams to the backend driver.
> Backend driver is an user-space application.
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> ---
> Changes since v1:
>  * removed __attribute__((__packed__)) from all structures definitions
> 
>  xen/include/public/io/sndif.h | 205 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 205 insertions(+)
>  create mode 100644 xen/include/public/io/sndif.h
> 
> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
> new file mode 100644
> index 0000000..1b93520
> --- /dev/null
> +++ b/xen/include/public/io/sndif.h
> @@ -0,0 +1,205 @@
> +/******************************************************************************
> + * sndif.h
> + *
> + * Unified sound-device I/O interface for Xen guest OSes.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (C) 2013-2015 GlobalLogic Inc.
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_SNDIF_H__
> +#define __XEN_PUBLIC_IO_SNDIF_H__
> +
> +/*
> + * Feature and Parameter Negotiation
> + * =================================
> + * The two halves of a Xen vsnd driver utilize nodes within the XenStore to
> + * communicate capabilities and to negotiate operating parameters.  This
> + * section enumerates these nodes which reside in the respective front and
> + * backend portions of the XenStore, following the XenBus convention.
> + *
> + * All data in the XenStore is stored as strings.  Nodes specifying numeric
> + * values are encoded in decimal.  Integer value ranges listed below are
> + * expressed as fixed sized integer types capable of storing the conversion
> + * of a properly formated node string, without loss of information.
> + *
> + * Any specified default value is in effect if the corresponding XenBus node
> + * is not present in the XenStore.
> + *
> + * XenStore nodes in sections marked "PRIVATE" are solely for use by the
> + * driver side whose XenBus tree contains them.
> + *
> + *****************************************************************************
> + *                            Backend XenBus Nodes
> + *****************************************************************************
> + *
> + *------------------ Backend Device Identification (PRIVATE) ------------------
> + *
> + * stream_id
> + *      Values:         <uint32_t>
> + *
> + *      Virtuelized stream number
> + *
> + *****************************************************************************
> + *                            Frontend XenBus Nodes
> + *****************************************************************************
> + *
> + *----------------------- Request Transport Parameters -----------------------
> + *
> + * event-channel
> + *      Values:         <uint32_t>
> + *
> + *      The identifier of the Xen event channel used to signal activity
> + *      in the ring buffer.
> + *
> + * ring-ref
> + *      Values:         <uint32_t>
> + *      Notes:          6
> + *
> + *      The Xen grant reference granting permission for the backend to map
> + *      the sole page in a single page sized ring buffer.
> + */
> +
> +/*
> + * PCM FORMATS.
> + */
> +#define SNDIF_PCM_FORMAT_S8             (0)
> +#define SNDIF_PCM_FORMAT_U8             (1)
> +#define SNDIF_PCM_FORMAT_S16_LE         (2)
> +#define SNDIF_PCM_FORMAT_S16_BE         (3)
> +#define SNDIF_PCM_FORMAT_U16_LE         (4)
> +#define SNDIF_PCM_FORMAT_U16_BE         (5)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_S24_LE         (6)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_S24_BE         (7)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_U24_LE         (8)
> +
> +/* low three bytes */
> +#define SNDIF_PCM_FORMAT_U24_BE         (9)
> +
> +#define SNDIF_PCM_FORMAT_S32_LE         (10)
> +#define SNDIF_PCM_FORMAT_S32_BE         (11)
> +#define SNDIF_PCM_FORMAT_U32_LE         (12)
> +#define SNDIF_PCM_FORMAT_U32_BE         (13)
> +
> +/* 4-byte float, IEEE-754 32-bit, range -1.0 to 1.0 */
> +#define SNDIF_PCM_FORMAT_FLOAT_LE       (14)
> +
> +/* 4-byte float, IEEE-754 32-bit, range -1.0 to 1.0 */
> +#define SNDIF_PCM_FORMAT_FLOAT_BE       (15)
> +
> +/* 8-byte float, IEEE-754 64-bit, range -1.0 to 1.0 */
> +#define SNDIF_PCM_FORMAT_FLOAT64_LE     (16)
> +
> +/* 8-byte float, IEEE-754 64-bit, range -1.0 to 1.0 */
> +#define SNDIF_PCM_FORMAT_FLOAT64_BE     (17)
> +
> +/* IEC-958 subframe, Little Endian */
> +#define SNDIF_PCM_FORMAT_IEC958_SUBFRAME_LE (18)
> +
> +/* IEC-958 subframe, Big Endian */
> +#define SNDIF_PCM_FORMAT_IEC958_SUBFRAME_BE (19)
> +
> +#define SNDIF_PCM_FORMAT_MU_LAW         (20)
> +#define SNDIF_PCM_FORMAT_A_LAW          (21)
> +#define SNDIF_PCM_FORMAT_IMA_ADPCM      (22)
> +#define SNDIF_PCM_FORMAT_MPEG           (23)
> +#define SNDIF_PCM_FORMAT_GSM            (24)
> +#define SNDIF_PCM_FORMAT_SPECIAL        (31)
> +
> +/*
> + * REQUEST CODES.
> + */
> +#define SNDIF_OP_OPEN                   0
> +#define SNDIF_OP_CLOSE                  1
> +#define SNDIF_OP_READ                   2
> +#define SNDIF_OP_WRITE                  3
> +#define SNDIF_SET_VOLUME                4
> +#define SNDIF_GET_VOLUME                5
> +
> +#define SNDIF_MAX_PAGES_PER_REQUEST     10
> +
> +/*
> + * STATUS RETURN CODES.
> + */
> + /* Operation failed for some unspecified reason (-EIO). */
> +#define SNDIF_RSP_ERROR       -1
> + /* Operation completed successfully. */
> +#define SNDIF_RSP_OKAY         0
> +
> +struct snd_params {
> +    uint32_t format;            /* SNDIF_PCM_FORMAT_??? */
> +    uint32_t channels;          /* Channels count       */
> +    uint32_t rate;              /* Data rate            */
> +};
> +
> +struct sndif_request_common {
> +    uint64_t id;                /* private guest value, echoed in resp  */
> +    struct snd_params _pad1;
> +    uint32_t _pad2;
> +    uint32_t _pad3;
> +};
> +
> +struct sndif_request_open {
> +    uint64_t id;                /* private guest value, echoed in resp  */
> +    struct snd_params snd_params;
> +    uint32_t stream;
> +    uint32_t _pad2;
> +};
> +
> +struct sndif_request_rw {
> +    uint64_t id;                /* private guest value, echoed in resp  */
> +    struct snd_params _pad1;
> +    uint32_t len;
> +    uint32_t _pad2;
> +    grant_ref_t gref[SNDIF_MAX_PAGES_PER_REQUEST];
> +};
> +
> +struct sndif_request_volume {
> +    uint64_t id;                /* private guest value, echoed in resp  */
> +    struct snd_params _pad1;
> +    uint32_t left;
> +    uint32_t right;
> +};
> +
> +struct sndif_request {
> +    uint8_t operation;          /* SNDIF_OP_??? */
> +    union {
> +        struct sndif_request_common common;
> +        struct sndif_request_open open;
> +        struct sndif_request_rw rw;
> +        struct sndif_request_volume vol;
> +    } u;
> +};

Describing protocols in terms of structs has given us problems in the
past (like the blk protocol, that has two different layouts depending on
the bitness).

In general I would prefer any new protocol that's added to Xen to be
described with plain binary layouts, just like protocols are usually
written. For example like it's done in:
http://tools.ietf.org/html/rfc793#page-15.

Providing suitable structs would be a bonus, but not a way to describe it.

Roger.