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

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

[Andrushchenko, Oleksandr]

Hi, there!

We would like to bring back to life this thread.
We do hope that the re-worked version of the original patch
addresses all the issues discussed before.
What is more we already have a working version of the
frontend and backend which prove that the protocol works
(those are in the process of preparing to be pushed to
the community as well).

Hope you can review the changes and provide some feedback,
so finally sound devices find their way in the world of Xen

Best regards,
Andrushchenko, Oleksandr
Grytsov, Oleksandr

Andrushchenko, Oleksandr (1):
  This is ABI for the two halves of a Para-virtual     sound driver to
    communicate with each to other.

 xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
 xen/include/public/io/sndif_linux.h | 114 +++++++
 2 files changed, 697 insertions(+)
 create mode 100644 xen/include/public/io/sndif.h
 create mode 100644 xen/include/public/io/sndif_linux.h

-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Andrushchenko, Oleksandr]

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
Signed-off-by: Andrushchenko, Oleksandr <andr2000@gmail.com>
Signed-off-by: Oleksandr Grytsov <al1img@gmail.com>
---
Changes since v1:
 * removed __attribute__((__packed__)) from all structures definitions

Changes since v2:
 * removed all C structures
 * added protocol description between frontend and backend drivers

Changes since v3:
 * fixed some typos
 * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_**
 * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_**
 * added 'id' field to the request and response packets
 * renamed 'stream_id' to 'stream' in the packets description
 * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description
 * renamed 'pcm_stream_type' to 'pcm_type' in the packets description
 * removed 'stream_id' field from the response packets

Changes since v4:
 * renamed 'stream_id' back to the to 'stream' in the packets description
 * moved 'id' field to the upper position in the response packets

Changes since v5:
 * Slightly reworked request/response packets
 * Size of the request/response packet is changed to the 64 bytes
 * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are
   passed via shared page
 * Added parameters for the XenBus nodes (now each stream can be mapped
   to the defined sound device in the backend using those parameters)
 * Added XenBus state diagrams description

Changes since v6:
 * Reworked streams description  in the Backend XenBus Nodes

Changes since v7:
 * re-worked backend device parameters to be more generic and flexible
 * extended frontend device parameters
 * slightly updated state machine description added mute/unmute commands
 * added constants for XenStore configuration strings
   (fields, PCM formats etc.)
 * changed request/response structure size from 64 octets to 16
 * introduced dynamic buffer allocation instead of
   static XENSND_MAX_PAGES_PER_REQUEST
 * re-worked open request to allow dynamic buffer allocation
 * re-worked read/write/volume requests, so they don't pass grefs:
   buffer from the open request is used for these operations to pass data
 * specified type of the volume value to be a signed value in steps
   of 0.001 dBm, while 0 being 0dBm.
 * added Linux include file with structure definitions
---
---
 xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
 xen/include/public/io/sndif_linux.h | 114 +++++++
 2 files changed, 697 insertions(+)
 create mode 100644 xen/include/public/io/sndif.h
 create mode 100644 xen/include/public/io/sndif_linux.h

diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
new file mode 100644
index 0000000..df705a6
--- /dev/null
+++ b/xen/include/public/io/sndif.h
@@ -0,0 +1,583 @@
+/******************************************************************************
+ * 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.
+ * Copyright (C) 2016 EPAM Systems Inc.
+ */
+
+#ifndef __XEN_PUBLIC_IO_XENSND_H__
+#define __XEN_PUBLIC_IO_XENSND_H__
+
+/*
+ * Front->back notifications: When enqueuing a new request, sending a
+ * notification can be made conditional on req_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Backends must set
+ * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
+ *
+ * Back->front notifications: When enqueuing a new response, sending a
+ * notification can be made conditional on rsp_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Frontends must set
+ * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
+ */
+
+/*
+ * Feature and Parameter Negotiation
+ * =================================
+ * The two halves of a Para-virtual sound card 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.
+ *
+ *****************************************************************************
+ *                            Backend XenBus Nodes
+ *****************************************************************************
+ *
+ *-------------------------------- Addressing ---------------------------------
+ *
+ * Indices used to address frontends, driver instances, cards,
+ * devices and streams.
+ *
+ * frontend-id
+ *      Values:         <uint>
+ *
+ *      Domain ID of the sound frontend.
+ *
+ * drv_idx
+ *      Values:         <uint>
+ *
+ *      Zero based index of the virtualized sound driver instance in this domain.
+ *      Multiple PV drivers are allowed in the domain at the same time.
+ *
+ * card_idx
+ *      Values:         <uint>
+ *
+ *      Zero based index of the card within the driver.
+ *
+ * dev_id
+ *      Values:         <uint>
+ *
+ *      Unique (within given card instance) device ID.
+ *      Doesn't have to be zero based and/or be contiguous.
+ *
+ * stream_idx
+ *      Values:         <uint>
+ *
+ *      Zero based index of the stream of the device.
+ *
+ * Example for the frontend running in domain 5, instance of the driver
+ * in the front is 0 (single or first PV driver), sound card 0, device id 2,
+ * first stream (0):
+ * /local/domain/<frontend-id>/device/vsnd/<drv_idx>/card/<card_idx>/
+ * 	device/<dev_id>/stream/<stream_idx>/type = "p"
+ * /local/domain/5/device/vsnd/0/card/0/device/2/stream/0/type = "p"
+ *
+ *------------------------------- PCM settings --------------------------------
+ *
+ * Every virtualized sound frontend has set of cards, devices and streams, each
+ * is individually configured. Part of the PCM configuration can be defined at
+ * higher level and be fully or partially re-used by the underlying layers.
+ * These configuration values are:
+ *  o number of channels (min/max)
+ *  o supported sample rates
+ *  o supported sample formats.
+ * E.g. one can define these values for the whole card, whole device or stream.
+ * Every underlying layer in turn can re-define some or all of them to better
+ * fit its needs. For example, card may define number of channels to be
+ * in [1; 8] range, and some particular stream may be limited to [1; 2] only.
+ * The rule is that the underlying layer must be a subset of the upper layer
+ * range.
+ *
+ * channels-min
+ *      Values:         <string representing a positive integer>
+ *
+ *      The minimum amount of channels that is supported.
+ *      Must be at least 1. If not defined then use frontend's default.
+ *
+ * channels-max
+ *      Values:         <string representing a positive integer>
+ *
+ *      The maximum amount of channels that is supported.
+ *      Must be at least <channels-min>. If not defined then use frontend's
+ *      default.
+ *
+ * sample-rates
+ *      Values:         <list of uints>
+ *
+ *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
+ *      If not defined then use frontend's default.
+ *
+ * sample-formats
+ *      Values:         <list of XENSND_PCM_FORMAT_XXX_STR>
+ *
+ *      List of supported sample formats separated by XENSND_LIST_SEPARATOR.
+ *      If not defined then use frontend's default.
+ *
+ * buffer-size
+ *      Values:         <string representing a positive integer>
+ *
+ *      The maximum size in octets of the buffer to allocate per stream.
+ *
+ * Example configuration:
+ *
+ * Card configuration used by all streams:
+ * /local/domain/5/device/vsnd/0/card/0/sample-formats = "s8;u8;s16_le;s16_be"
+ * Stream overrides sample rates supported:
+ * /local/domain/5/device/vsnd/0/card/1/device/2/stream/0/sample-rates = "8000;22050;44100;48000"
+ *
+ *------------------------------ Card settings --------------------------------
+ * short-name
+ *      Values:         <char[32]>
+ *
+ *      Short name of the card.
+ *
+ * long-name
+ *      Values:         <char[80]>
+ *
+ *      Long name of the card.
+ *
+ * For example,
+ * /local/domain/5/device/vsnd/0/card/0/short-name = "Virtual audio"
+ * /local/domain/5/device/vsnd/0/card/0/long-name = "Virtual audio at center stack"
+ *
+ *----------------------------- Device settings --------------------------------
+ * name
+ *      Values:         <char[80]>
+ *
+ *      Name of the sound device of the card given.
+ *
+ * For example,
+ * /local/domain/5/device/vsnd/0/card/0/device/0/name = "General analog"
+ *
+ *----------------------------- Stream settings -------------------------------
+ *
+ * type
+ *      Values:         "p", "c"
+ *
+ *      Stream type: "p" - playback stream, "c" - capture stream
+ *
+ *      If both capture and playback are needed then two streams need to be
+ *      defined under the same device. For example,
+ *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/0/type = "p"
+ *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/1/type = "c"
+ *
+ *****************************************************************************
+ *                            Frontend XenBus Nodes
+ *****************************************************************************
+ *
+ *----------------------- Request Transport Parameters -----------------------
+ *
+ * These are per stream.
+ *
+ * event-channel
+ *      Values:         <string representing a positive integer>
+ *
+ *      The identifier of the Xen event channel used to signal activity
+ *      in the ring buffer.
+ *
+ * ring-ref
+ *      Values:         <string representing a positive integer>
+ *
+ *      The Xen grant reference granting permission for the backend to map
+ *      a sole page in a single page sized ring buffer.
+ *
+ * index
+ *      Values:         <string representing a positive integer>
+ *
+ *      After stream initialization it is assigned a unique ID (within the front
+ *      driver), so every stream of the frontend can be identified by the
+ *      backend by this ID.
+ */
+
+/*
+ * STATE DIAGRAMS
+ *
+ *****************************************************************************
+ *                                   Startup                                 *
+ *****************************************************************************
+ *
+ * Tool stack creates front and back state nodes with initial state
+ * XenbusStateInitialising.
+ * Tool stack creates and sets up frontend sound configuration nodes per domain.
+ *
+ * Front                                Back
+ * =================================    =====================================
+ * XenbusStateInitialising              XenbusStateInitialising
+ *                                       o Query backend device identification
+ *                                         data.
+ *                                       o Open and validate backend device.
+ *                                                      |
+ *                                                      |
+ *                                                      V
+ *                                      XenbusStateInitWait
+ *
+ * o Query frontend configuration
+ * o Allocate and initialize
+ *   event channels per configured
+ *   playback/capture stream.
+ * o Publish transport parameters
+ *   that will be in effect during
+ *   this connection.
+ *              |
+ *              |
+ *              V
+ * XenbusStateInitialised
+ *
+ *                                       o Query frontend transport parameters.
+ *                                       o Connect to the event channels.
+ *                                                      |
+ *                                                      |
+ *                                                      V
+ *                                      XenbusStateConnected
+ *
+ *  o Create and initialize OS
+ *  virtual sound device instances
+ *  as per configuration.
+ *              |
+ *              |
+ *              V
+ * XenbusStateConnected
+ *
+ */
+
+/*
+ * PCM FORMATS
+ *
+ * XENSND_PCM_FORMAT_<format>[_<endian>]
+ *
+ * format: <S/U/F><bits> or <name>
+ *     S - signed, U - unsigned, F - float
+ *     bits - 8, 16, 24, 32
+ *     name - MU_LAW, GSM, etc.
+ *
+ * endian: <LE/BE>, may be absent
+ *     LE - Little endian, BE - Big endian
+ */
+#define XENSND_PCM_FORMAT_S8            0
+#define XENSND_PCM_FORMAT_U8            1
+#define XENSND_PCM_FORMAT_S16_LE        2
+#define XENSND_PCM_FORMAT_S16_BE        3
+#define XENSND_PCM_FORMAT_U16_LE        4
+#define XENSND_PCM_FORMAT_U16_BE        5
+#define XENSND_PCM_FORMAT_S24_LE        6
+#define XENSND_PCM_FORMAT_S24_BE        7
+#define XENSND_PCM_FORMAT_U24_LE        8
+#define XENSND_PCM_FORMAT_U24_BE        9
+#define XENSND_PCM_FORMAT_S32_LE        10
+#define XENSND_PCM_FORMAT_S32_BE        11
+#define XENSND_PCM_FORMAT_U32_LE        12
+#define XENSND_PCM_FORMAT_U32_BE        13
+#define XENSND_PCM_FORMAT_F32_LE        14 /* 4-byte float, IEEE-754 32-bit, */
+#define XENSND_PCM_FORMAT_F32_BE        15 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_F64_LE        16 /* 8-byte float, IEEE-754 64-bit, */
+#define XENSND_PCM_FORMAT_F64_BE        17 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19
+#define XENSND_PCM_FORMAT_MU_LAW        20
+#define XENSND_PCM_FORMAT_A_LAW         21
+#define XENSND_PCM_FORMAT_IMA_ADPCM     22
+#define XENSND_PCM_FORMAT_MPEG          23
+#define XENSND_PCM_FORMAT_GSM           24
+#define XENSND_PCM_FORMAT_SPECIAL       31 /* Any other unspecified format */
+
+/*
+ * REQUEST CODES.
+ */
+#define XENSND_OP_OPEN                  0
+#define XENSND_OP_CLOSE                 1
+#define XENSND_OP_READ                  2
+#define XENSND_OP_WRITE                 3
+#define XENSND_OP_SET_VOLUME            4
+#define XENSND_OP_GET_VOLUME            5
+#define XENSND_OP_MUTE                  6
+#define XENSND_OP_UNMUTE                7
+
+/*
+ * XENSTORE FIELD AND PATH NAME STRINGS, HELPERS.
+ */
+#define XENSND_DRIVER_NAME                   "vsnd"
+
+#define XENSND_LIST_SEPARATOR                ";"
+/* Path entries */
+#define XENSND_PATH_CARD                     "card"
+#define XENSND_PATH_DEVICE                   "device"
+#define XENSND_PATH_STREAM                   "stream"
+/* Field names */
+#define XENSND_FIELD_CARD_SHORT_NAME         "short-name"
+#define XENSND_FIELD_CARD_LONG_NAME          "long-name"
+#define XENSND_FIELD_RING_REF                "ring-ref"
+#define XENSND_FIELD_EVT_CHNL                "event-channel"
+#define XENSND_FIELD_DEVICE_NAME             "name"
+#define XENSND_FIELD_TYPE                    "type"
+#define XENSND_FIELD_STREAM_INDEX            "index"
+#define XENSND_FIELD_CHANNELS_MIN            "channels-min"
+#define XENSND_FIELD_CHANNELS_MAX            "channels-max"
+#define XENSND_FIELD_SAMPLE_RATES            "sample-rates"
+#define XENSND_FIELD_SAMPLE_FORMATS          "sample-formats"
+#define XENSND_FIELD_BUFFER_SIZE             "buffer-size"
+
+/* Stream type field values. */
+#define XENSND_STREAM_TYPE_PLAYBACK          "p"
+#define XENSND_STREAM_TYPE_CAPTURE           "c"
+/* Sample rate max string length */
+#define XENSND_SAMPLE_RATE_MAX_LEN            6
+/* Sample format field values */
+#define XENSND_SAMPLE_FORMAT_MAX_LEN         24
+
+#define XENSND_PCM_FORMAT_S8_STR                 "s8"
+#define XENSND_PCM_FORMAT_U8_STR                 "u8"
+#define XENSND_PCM_FORMAT_S16_LE_STR             "s16_le"
+#define XENSND_PCM_FORMAT_S16_BE_STR             "s16_be"
+#define XENSND_PCM_FORMAT_U16_LE_STR             "u16_le"
+#define XENSND_PCM_FORMAT_U16_BE_STR             "u16_be"
+#define XENSND_PCM_FORMAT_S24_LE_STR             "s24_le"
+#define XENSND_PCM_FORMAT_S24_BE_STR             "s24_be"
+#define XENSND_PCM_FORMAT_U24_LE_STR             "u24_le"
+#define XENSND_PCM_FORMAT_U24_BE_STR             "u24_be"
+#define XENSND_PCM_FORMAT_S32_LE_STR             "s32_le"
+#define XENSND_PCM_FORMAT_S32_BE_STR             "s32_be"
+#define XENSND_PCM_FORMAT_U32_LE_STR             "u32_le"
+#define XENSND_PCM_FORMAT_U32_BE_STR             "u32_be"
+#define XENSND_PCM_FORMAT_F32_LE_STR             "float_le"
+#define XENSND_PCM_FORMAT_F32_BE_STR             "float_be"
+#define XENSND_PCM_FORMAT_F64_LE_STR             "float64_le"
+#define XENSND_PCM_FORMAT_F64_BE_STR             "float64_be"
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE_STR "iec958_subframe_le"
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE_STR "iec958_subframe_be"
+#define XENSND_PCM_FORMAT_MU_LAW_STR             "mu_law"
+#define XENSND_PCM_FORMAT_A_LAW_STR              "a_law"
+#define XENSND_PCM_FORMAT_IMA_ADPCM_STR          "ima_adpcm"
+#define XENSND_PCM_FORMAT_MPEG_STR               "mpeg"
+#define XENSND_PCM_FORMAT_GSM_STR                "gsm"
+#define XENSND_PCM_FORMAT_SPECIAL_STR            "special"
+
+/*
+ * STATUS RETURN CODES.
+ */
+ /* Operation failed for some unspecified reason (e. g. -EIO). */
+#define XENSND_RSP_ERROR                 (-1)
+ /* Operation completed successfully. */
+#define XENSND_RSP_OKAY                  0
+
+/*
+ * Description of the protocol between frontend and backend driver.
+ *
+ * The two halves of a Para-virtual sound driver communicates with
+ * each other using a shared page and an event channel.
+ * Shared page contains a ring with request/response packets.
+ *
+ *
+ * All request packets have the same length (16 octets)
+ *
+ *
+ * Request open - open a PCM stream for playback or capture:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                pcm_rate                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |  pcm_format     |  pcm_channels   |             reserved              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                         gref_directory_start                          |
+ * +-----------------+-----------------+-----------------+-----------------+
+
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_OPEN
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * pcm_rate - uint32_t, stream data rate, Hz
+ * pcm_format - uint8_t, XENSND_PCM_FORMAT_XXX value
+ * pcm_channels - uint8_t, number of channels of this stream
+ * gref_directory_start - grant_ref_t, a reference to the first shared page
+ *   describing shared buffer references. At least one page exists. If shared
+ *   buffer size exceeds what can be addressed by this single page, then
+ *   reference to the next page must be supplied (gref_dir_next_page below
+ *   is not NULL)
+ *
+ * Shared page for XENSND_OP_OPEN buffer descriptor (gref_directory in the
+ *   request) employs a list of pages, describing all pages of the shared data
+ *   buffer:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                          gref_dir_next_page                           |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                num_grefs                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[0]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[1]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[N -1]                             |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * gref_dir_next_page - grant_ref_t, reference to the next page describing
+ *   page directory
+ * num_grefs - number of grefs in this page
+ * gref[i] - grant_ref_t, reference to a shared page of the buffer
+ *   allocated at XENSND_OP_OPEN
+ *
+ * Request close - close an opened pcm stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_CLOSE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ *
+ *
+ * Request read/write - used for read (for capture) or write (for playback):
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                offset                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                length                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_READ/XENSND_OP_WRITE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * offset - uint32_t, read or write data offset within the shared buffer
+ *   passed with XENSND_OP_OPEN request
+ * length - uint32_t, read or write data length
+ *
+ *
+ * Request set/get volume - set/get channels' volume of the stream given:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_SET_VOLUME or XENSND_OP_GET_VOLUME
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * Buffer passed with XENSND_OP_OPEN is used to exchange volume
+ * values:
+ *
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[0]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[1]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ *                  channel[XENSND_OP_OPEN.pcm_channels - 1]               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * channel[i] - sint32_t, volume of i-th channel
+ * Volume is expressed as a signed value in steps of 0.001 dBm,
+ * while 0 being 0dBm.
+ *
+ *
+ * Request mute/unmute - mute/unmute stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_MUTE/XENSND_OP_UNMUTE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
+ * values:
+ *
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
+ * Number of channels passed is equal to XENSND_OP_OPEN request pcm_channels
+ * field
+ *
+ *
+ * All response packets have the same length (64 bytes)
+ *
+ * Response for all requests:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |      status     |                      reserved                       |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                              reserved                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                              reserved                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, copied from the request
+ * stream_idx - uint8_t, copied from request
+ * operation - uint8_t, XENSND_OP_XXX - copied from request
+ * status - int8_t, response status (XENSND_RSP_???)
+ */
+
+struct xensnd_request {
+    uint8_t raw[16];
+};
+
+struct xensnd_response {
+    uint8_t raw[16];
+};
+
+#endif /* __XEN_PUBLIC_IO_XENSND_H__ */
diff --git a/xen/include/public/io/sndif_linux.h b/xen/include/public/io/sndif_linux.h
new file mode 100644
index 0000000..58ec96c
--- /dev/null
+++ b/xen/include/public/io/sndif_linux.h
@@ -0,0 +1,114 @@
+/*
+ *  Unified sound-device I/O interface for Xen guest OSes
+ *
+ *   This program is free software; you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation; either version 2 of the License, or
+ *   (at your option) any later version.
+ *
+ *   This program is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with this program; if not, write to the Free Software
+ *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
+ *
+ * Copyright (C) 2016 EPAM Systems Inc.
+ */
+
+#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__
+#define __XEN_PUBLIC_IO_XENSND_LINUX_H__
+
+#ifdef __KERNEL__
+#include <xen/interface/io/ring.h>
+#include <xen/interface/grant_table.h>
+#else
+#include <xen/io/ring.h>
+#include <xen/grant_table.h>
+#endif
+#if 0
+#include <xen/include/public/io/sndif.h>
+#else
+#warning "TODO: properly include base protocol header"
+#include "sndif.h"
+#endif
+
+struct xensnd_open_req {
+	uint32_t pcm_rate;
+	uint8_t pcm_format;
+	uint8_t pcm_channels;
+	/* in Hz */
+	uint16_t __reserved0;
+	grant_ref_t gref_directory_start;
+} __attribute__((packed));
+
+struct xensnd_page_directory {
+	grant_ref_t gref_dir_next_page;
+	uint32_t num_grefs;
+	grant_ref_t gref[0];
+} __attribute__((packed));
+
+struct xensnd_close_req {
+} __attribute__((packed));
+
+struct xensnd_write_req {
+	uint32_t offset;
+	uint32_t len;
+} __attribute__((packed));
+
+struct xensnd_read_req {
+	uint32_t offset;
+	uint32_t len;
+} __attribute__((packed));
+
+struct xensnd_get_vol_req {
+} __attribute__((packed));
+
+struct xensnd_set_vol_req {
+} __attribute__((packed));
+
+struct xensnd_mute_req {
+} __attribute__((packed));
+
+struct xensnd_unmute_req {
+} __attribute__((packed));
+
+struct xensnd_req {
+	union {
+		struct xensnd_request raw;
+		struct {
+			uint16_t id;
+			uint8_t operation;
+			uint8_t stream_idx;
+			union {
+				struct xensnd_open_req open;
+				struct xensnd_close_req close;
+				struct xensnd_write_req write;
+				struct xensnd_read_req read;
+				struct xensnd_get_vol_req get_vol;
+				struct xensnd_set_vol_req set_vol;
+				struct xensnd_mute_req mute;
+				struct xensnd_unmute_req unmute;
+			} op;
+		} data;
+	} u;
+};
+
+struct xensnd_resp {
+	union {
+		struct xensnd_response raw;
+		struct {
+			uint16_t id;
+			uint8_t operation;
+			uint8_t stream_idx;
+			int8_t status;
+		} data;
+	} u;
+};
+
+DEFINE_RING_TYPES(xen_sndif, struct xensnd_req,
+		struct xensnd_resp);
+
+#endif /* __XEN_PUBLIC_IO_XENSND_LINUX_H__ */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Embedded-pv-devel] [PATCH v8] sndif: add ABI for Para-virtual sound

[Lars Kurth]

Oleksandr,

you may want to update the CC list, as there have been changes to maintainership. Remove Ian Campell and Keir Fraser, add Stefano Stabellini <sstabellini@kernel.org> and Julien Grall <julien.grall@arm.com>. Also, as this is a change in the Hypervisor, you probably want to use xen-devel@ as the main mailing list. embedded-pv-devel <embedded-pv-devel@lists.xenproject.org> is not properly moderated, which means that it may take time until a mail which is posted gets to everyone.

The same applies to the other mail.

Regards
Lars

> On 4 Nov 2016, at 20:51, Andrushchenko, Oleksandr <andr2000@gmail.com> wrote:
> 
> Hi, there!
> 
> We would like to bring back to life this thread.
> We do hope that the re-worked version of the original patch
> addresses all the issues discussed before.
> What is more we already have a working version of the
> frontend and backend which prove that the protocol works
> (those are in the process of preparing to be pushed to
> the community as well).
> 
> Hope you can review the changes and provide some feedback,
> so finally sound devices find their way in the world of Xen
> 
> Best regards,
> Andrushchenko, Oleksandr
> Grytsov, Oleksandr
> 
> Andrushchenko, Oleksandr (1):
>  This is ABI for the two halves of a Para-virtual     sound driver to
>    communicate with each to other.
> 
> xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
> xen/include/public/io/sndif_linux.h | 114 +++++++
> 2 files changed, 697 insertions(+)
> create mode 100644 xen/include/public/io/sndif.h
> create mode 100644 xen/include/public/io/sndif_linux.h
> 
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Embedded-pv-devel mailing list
> Embedded-pv-devel@lists.xenproject.org
> https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Embedded-pv-devel] [PATCH v8] sndif: add ABI for Para-virtual sound

[Oleksandr Andrushchenko]

Hi, Lars!

Thank you for your inputs,

I've just added the persons you mentioned to the CC list and hope that 
someone

will have time to review the protocol.

Thank you,
Oleksandr

On 11/08/2016 03:11 PM, Lars Kurth wrote:
> Oleksandr,
>
> you may want to update the CC list, as there have been changes to maintainership. Remove Ian Campell and Keir Fraser, add Stefano Stabellini <sstabellini@kernel.org> and Julien Grall <julien.grall@arm.com>. Also, as this is a change in the Hypervisor, you probably want to use xen-devel@ as the main mailing list. embedded-pv-devel <embedded-pv-devel@lists.xenproject.org> is not properly moderated, which means that it may take time until a mail which is posted gets to everyone.
>
> The same applies to the other mail.
>
> Regards
> Lars
>
>> On 4 Nov 2016, at 20:51, Andrushchenko, Oleksandr <andr2000@gmail.com> wrote:
>>
>> Hi, there!
>>
>> We would like to bring back to life this thread.
>> We do hope that the re-worked version of the original patch
>> addresses all the issues discussed before.
>> What is more we already have a working version of the
>> frontend and backend which prove that the protocol works
>> (those are in the process of preparing to be pushed to
>> the community as well).
>>
>> Hope you can review the changes and provide some feedback,
>> so finally sound devices find their way in the world of Xen
>>
>> Best regards,
>> Andrushchenko, Oleksandr
>> Grytsov, Oleksandr
>>
>> Andrushchenko, Oleksandr (1):
>>   This is ABI for the two halves of a Para-virtual     sound driver to
>>     communicate with each to other.
>>
>> xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
>> xen/include/public/io/sndif_linux.h | 114 +++++++
>> 2 files changed, 697 insertions(+)
>> create mode 100644 xen/include/public/io/sndif.h
>> create mode 100644 xen/include/public/io/sndif_linux.h
>>
>> -- 
>> 2.7.4
>>
>>
>> _______________________________________________
>> Embedded-pv-devel mailing list
>> Embedded-pv-devel@lists.xenproject.org
>> https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Embedded-pv-devel] [PATCH v8] sndif: add ABI for Para-virtual sound

[Lars Kurth]

> On 8 Nov 2016, at 13:27, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
> 
> Hi, Lars!
> 
> Thank you for your inputs,
> 
> I've just added the persons you mentioned to the CC list and hope that someone
> 
> will have time to review the protocol.

...
> 
> Yes, PV audio ABI update is also from us. Oleksandr just used the same =
> thread to update the patchset, lots of stuff changed since it was =
> initially published, indeed. Oleksndr will update the thread as you have =
> suggested.

I added Kyle, Chris and Christopher to this thread who are involved in OpenXT & Xenbedded, and who I believe may have some experience in the field also. As far as I recall, one of the sticking points in the past, was that Xen maintainers could not verify whether the PV protocol proposal maps well onto sound protocols.

I don't know whether Xenbedded or OpenXT contain any PV sound drivers yet. In any case, it seems that maybe we are starting to get a little bit of fragmentation and overlap in some areas around drivers for embedded/client/tablets/automotive between Xen Project, OpenXT and Xenbedded. 

It may make sense at some point to try and resolve this overlap and establish one place, where this functionality should live. I don't have any opinions, just throwing the question out and trying to figure out who has done what already.

Best Regards
Lars
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote:
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> Signed-off-by: Andrushchenko, Oleksandr <andr2000@gmail.com>
> Signed-off-by: Oleksandr Grytsov <al1img@gmail.com>
> ---
> Changes since v1:
>  * removed __attribute__((__packed__)) from all structures definitions
> 
> Changes since v2:
>  * removed all C structures
>  * added protocol description between frontend and backend drivers
> 
> Changes since v3:
>  * fixed some typos
>  * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_**
>  * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_**
>  * added 'id' field to the request and response packets
>  * renamed 'stream_id' to 'stream' in the packets description
>  * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description
>  * renamed 'pcm_stream_type' to 'pcm_type' in the packets description
>  * removed 'stream_id' field from the response packets
> 
> Changes since v4:
>  * renamed 'stream_id' back to the to 'stream' in the packets description
>  * moved 'id' field to the upper position in the response packets
> 
> Changes since v5:
>  * Slightly reworked request/response packets
>  * Size of the request/response packet is changed to the 64 bytes
>  * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are
>    passed via shared page
>  * Added parameters for the XenBus nodes (now each stream can be mapped
>    to the defined sound device in the backend using those parameters)
>  * Added XenBus state diagrams description
> 
> Changes since v6:
>  * Reworked streams description  in the Backend XenBus Nodes
> 
> Changes since v7:
>  * re-worked backend device parameters to be more generic and flexible
>  * extended frontend device parameters
>  * slightly updated state machine description added mute/unmute commands
>  * added constants for XenStore configuration strings
>    (fields, PCM formats etc.)
>  * changed request/response structure size from 64 octets to 16
>  * introduced dynamic buffer allocation instead of
>    static XENSND_MAX_PAGES_PER_REQUEST
>  * re-worked open request to allow dynamic buffer allocation
>  * re-worked read/write/volume requests, so they don't pass grefs:
>    buffer from the open request is used for these operations to pass data
>  * specified type of the volume value to be a signed value in steps
>    of 0.001 dBm, while 0 being 0dBm.
>  * added Linux include file with structure definitions
> ---
> ---
>  xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
>  xen/include/public/io/sndif_linux.h | 114 +++++++
>  2 files changed, 697 insertions(+)
>  create mode 100644 xen/include/public/io/sndif.h
>  create mode 100644 xen/include/public/io/sndif_linux.h
> 
> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
> new file mode 100644
> index 0000000..df705a6
> --- /dev/null
> +++ b/xen/include/public/io/sndif.h
> @@ -0,0 +1,583 @@
> +/******************************************************************************
> + * 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.
> + * Copyright (C) 2016 EPAM Systems Inc.
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_XENSND_H__
> +#define __XEN_PUBLIC_IO_XENSND_H__
> +
> +/*
> + * Front->back notifications: When enqueuing a new request, sending a
> + * notification can be made conditional on req_event (i.e., the generic
> + * hold-off mechanism provided by the ring macros). Backends must set
> + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
> + *
> + * Back->front notifications: When enqueuing a new response, sending a
> + * notification can be made conditional on rsp_event (i.e., the generic
> + * hold-off mechanism provided by the ring macros). Frontends must set
> + * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
> + */
> +
> +/*
> + * Feature and Parameter Negotiation
> + * =================================
> + * The two halves of a Para-virtual sound card 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.
> + *
> + *****************************************************************************
> + *                            Backend XenBus Nodes
> + *****************************************************************************
> + *
> + *-------------------------------- Addressing ---------------------------------
> + *
> + * Indices used to address frontends, driver instances, cards,
> + * devices and streams.
> + *
> + * frontend-id
> + *      Values:         <uint>
> + *
> + *      Domain ID of the sound frontend.
> + *
> + * drv_idx

How come you use _ here but frontend-id has an '-'? Do you want to keep
them all the same?

> + *      Values:         <uint>
> + *
> + *      Zero based index of the virtualized sound driver instance in this domain.
> + *      Multiple PV drivers are allowed in the domain at the same time.

I think that is given. You can have multiple NICs for example. I woudl
remove that sentence.
> + *
> + * card_idx
> + *      Values:         <uint>
> + *
> + *      Zero based index of the card within the driver.


Can it be discontingous? Aka: 0, 1, 2, 5 ?

> + *
> + * dev_id
> + *      Values:         <uint>
> + *
> + *      Unique (within given card instance) device ID.
> + *      Doesn't have to be zero based and/or be contiguous.

Oh, so they card_idx and drv_idx MUST be contingous? You may want
to say that in detail.

> + *
> + * stream_idx
> + *      Values:         <uint>
> + *
> + *      Zero based index of the stream of the device.

Can it be contingous or discountingous?

> + *
> + * Example for the frontend running in domain 5, instance of the driver
> + * in the front is 0 (single or first PV driver), sound card 0, device id 2,
> + * first stream (0):
> + * /local/domain/<frontend-id>/device/vsnd/<drv_idx>/card/<card_idx>/
> + * 	device/<dev_id>/stream/<stream_idx>/type = "p"
> + * /local/domain/5/device/vsnd/0/card/0/device/2/stream/0/type = "p"

Ah perfect!

> + *
> + *------------------------------- PCM settings --------------------------------
> + *
> + * Every virtualized sound frontend has set of cards, devices and streams, each
> + * is individually configured. Part of the PCM configuration can be defined at
> + * higher level and be fully or partially re-used by the underlying layers.
> + * These configuration values are:
> + *  o number of channels (min/max)
> + *  o supported sample rates
> + *  o supported sample formats.
> + * E.g. one can define these values for the whole card, whole device or stream.
> + * Every underlying layer in turn can re-define some or all of them to better
> + * fit its needs. For example, card may define number of channels to be
> + * in [1; 8] range, and some particular stream may be limited to [1; 2] only.
> + * The rule is that the underlying layer must be a subset of the upper layer
> + * range.
> + *
> + * channels-min
> + *      Values:         <string representing a positive integer>
> + *
> + *      The minimum amount of channels that is supported.
> + *      Must be at least 1. If not defined then use frontend's default.

I don't see the frontend's default later in the doc?
> + *
> + * channels-max
> + *      Values:         <string representing a positive integer>
> + *
> + *      The maximum amount of channels that is supported.
> + *      Must be at least <channels-min>. If not defined then use frontend's
> + *      default.

Again, not seeing the frontend's default? Perhaps have:

"See XYZ"
> + *
> + * sample-rates
> + *      Values:         <list of uints>
> + *
> + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.

Any ordering rules? Or can it be random: 44100;9600;0xFFFFF

And does it have to be decimal or can you use hex values?

> + *      If not defined then use frontend's default.
> + *
> + * sample-formats
> + *      Values:         <list of XENSND_PCM_FORMAT_XXX_STR>
> + *
> + *      List of supported sample formats separated by XENSND_LIST_SEPARATOR.
> + *      If not defined then use frontend's default.

Any ordering values? Can it be: "s16_be;s8" ?


And where can I find the frontend's default?
> + *
> + * buffer-size
> + *      Values:         <string representing a positive integer>
> + *
> + *      The maximum size in octets of the buffer to allocate per stream.

Where is the maximum stream value defined?

Or would that be channels-max?

> + *
> + * Example configuration:
> + *
> + * Card configuration used by all streams:
> + * /local/domain/5/device/vsnd/0/card/0/sample-formats = "s8;u8;s16_le;s16_be"
> + * Stream overrides sample rates supported:

Not sure I follow that sentence.
> + * /local/domain/5/device/vsnd/0/card/1/device/2/stream/0/sample-rates = "8000;22050;44100;48000"
> + *
> + *------------------------------ Card settings --------------------------------
> + * short-name
> + *      Values:         <char[32]>
> + *
> + *      Short name of the card.

How come it is limited to 32 bytes?
> + *
> + * long-name
> + *      Values:         <char[80]>
> + *
> + *      Long name of the card.

Why two names?
CAn one of them be skipped (say you only want to use 'short-name' ?

> + *
> + * For example,
> + * /local/domain/5/device/vsnd/0/card/0/short-name = "Virtual audio"
> + * /local/domain/5/device/vsnd/0/card/0/long-name = "Virtual audio at center stack"
> + *
> + *----------------------------- Device settings --------------------------------
> + * name
> + *      Values:         <char[80]>
> + *
> + *      Name of the sound device of the card given.

I think the 'given' can go away. Why is there a limit of 80 characters?

> + *
> + * For example,
> + * /local/domain/5/device/vsnd/0/card/0/device/0/name = "General analog"
> + *
> + *----------------------------- Stream settings -------------------------------
> + *
> + * type
> + *      Values:         "p", "c"
> + *
> + *      Stream type: "p" - playback stream, "c" - capture stream
> + *
> + *      If both capture and playback are needed then two streams need to be
> + *      defined under the same device. For example,
> + *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/0/type = "p"
> + *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/1/type = "c"
> + *
> + *****************************************************************************
> + *                            Frontend XenBus Nodes
> + *****************************************************************************
> + *
> + *----------------------- Request Transport Parameters -----------------------
> + *
> + * These are per stream.
> + *
> + * event-channel
> + *      Values:         <string representing a positive integer>
> + *
> + *      The identifier of the Xen event channel used to signal activity
> + *      in the ring buffer.
> + *
> + * ring-ref
> + *      Values:         <string representing a positive integer>
> + *
> + *      The Xen grant reference granting permission for the backend to map
> + *      a sole page in a single page sized ring buffer.
> + *
> + * index
> + *      Values:         <string representing a positive integer>
> + *
> + *      After stream initialization it is assigned a unique ID (within the front
> + *      driver), so every stream of the frontend can be identified by the
> + *      backend by this ID.

But doesn't it have that already by the virtue of stream_idx ?

> + */
> +
> +/*
> + * STATE DIAGRAMS
> + *
> + *****************************************************************************
> + *                                   Startup                                 *
> + *****************************************************************************
> + *
> + * Tool stack creates front and back state nodes with initial state
> + * XenbusStateInitialising.
> + * Tool stack creates and sets up frontend sound configuration nodes per domain.
> + *
> + * Front                                Back
> + * =================================    =====================================
> + * XenbusStateInitialising              XenbusStateInitialising
> + *                                       o Query backend device identification
> + *                                         data.
> + *                                       o Open and validate backend device.
> + *                                                      |
> + *                                                      |
> + *                                                      V
> + *                                      XenbusStateInitWait
> + *
> + * o Query frontend configuration
> + * o Allocate and initialize
> + *   event channels per configured
> + *   playback/capture stream.
> + * o Publish transport parameters
> + *   that will be in effect during
> + *   this connection.
> + *              |
> + *              |
> + *              V
> + * XenbusStateInitialised
> + *
> + *                                       o Query frontend transport parameters.
> + *                                       o Connect to the event channels.
> + *                                                      |
> + *                                                      |
> + *                                                      V
> + *                                      XenbusStateConnected
> + *
> + *  o Create and initialize OS
> + *  virtual sound device instances
> + *  as per configuration.
> + *              |
> + *              |
> + *              V
> + * XenbusStateConnected
> + *


No migration/disconnection state?

> + */
> +
> +/*
> + * PCM FORMATS
> + *
> + * XENSND_PCM_FORMAT_<format>[_<endian>]
> + *
> + * format: <S/U/F><bits> or <name>
> + *     S - signed, U - unsigned, F - float
> + *     bits - 8, 16, 24, 32
> + *     name - MU_LAW, GSM, etc.
> + *
> + * endian: <LE/BE>, may be absent
> + *     LE - Little endian, BE - Big endian
> + */
> +#define XENSND_PCM_FORMAT_S8            0
> +#define XENSND_PCM_FORMAT_U8            1
> +#define XENSND_PCM_FORMAT_S16_LE        2
> +#define XENSND_PCM_FORMAT_S16_BE        3
> +#define XENSND_PCM_FORMAT_U16_LE        4
> +#define XENSND_PCM_FORMAT_U16_BE        5
> +#define XENSND_PCM_FORMAT_S24_LE        6
> +#define XENSND_PCM_FORMAT_S24_BE        7
> +#define XENSND_PCM_FORMAT_U24_LE        8
> +#define XENSND_PCM_FORMAT_U24_BE        9
> +#define XENSND_PCM_FORMAT_S32_LE        10
> +#define XENSND_PCM_FORMAT_S32_BE        11
> +#define XENSND_PCM_FORMAT_U32_LE        12
> +#define XENSND_PCM_FORMAT_U32_BE        13
> +#define XENSND_PCM_FORMAT_F32_LE        14 /* 4-byte float, IEEE-754 32-bit, */
> +#define XENSND_PCM_FORMAT_F32_BE        15 /* range -1.0 to 1.0              */
> +#define XENSND_PCM_FORMAT_F64_LE        16 /* 8-byte float, IEEE-754 64-bit, */
> +#define XENSND_PCM_FORMAT_F64_BE        17 /* range -1.0 to 1.0              */
> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18
> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19
> +#define XENSND_PCM_FORMAT_MU_LAW        20
> +#define XENSND_PCM_FORMAT_A_LAW         21
> +#define XENSND_PCM_FORMAT_IMA_ADPCM     22
> +#define XENSND_PCM_FORMAT_MPEG          23
> +#define XENSND_PCM_FORMAT_GSM           24
> +#define XENSND_PCM_FORMAT_SPECIAL       31 /* Any other unspecified format */
> +
> +/*
> + * REQUEST CODES.
> + */
> +#define XENSND_OP_OPEN                  0
> +#define XENSND_OP_CLOSE                 1
> +#define XENSND_OP_READ                  2
> +#define XENSND_OP_WRITE                 3
> +#define XENSND_OP_SET_VOLUME            4
> +#define XENSND_OP_GET_VOLUME            5
> +#define XENSND_OP_MUTE                  6
> +#define XENSND_OP_UNMUTE                7
> +
> +/*
> + * XENSTORE FIELD AND PATH NAME STRINGS, HELPERS.
> + */
> +#define XENSND_DRIVER_NAME                   "vsnd"
> +
> +#define XENSND_LIST_SEPARATOR                ";"
> +/* Path entries */
> +#define XENSND_PATH_CARD                     "card"
> +#define XENSND_PATH_DEVICE                   "device"
> +#define XENSND_PATH_STREAM                   "stream"
> +/* Field names */
> +#define XENSND_FIELD_CARD_SHORT_NAME         "short-name"
> +#define XENSND_FIELD_CARD_LONG_NAME          "long-name"
> +#define XENSND_FIELD_RING_REF                "ring-ref"
> +#define XENSND_FIELD_EVT_CHNL                "event-channel"
> +#define XENSND_FIELD_DEVICE_NAME             "name"
> +#define XENSND_FIELD_TYPE                    "type"
> +#define XENSND_FIELD_STREAM_INDEX            "index"
> +#define XENSND_FIELD_CHANNELS_MIN            "channels-min"
> +#define XENSND_FIELD_CHANNELS_MAX            "channels-max"
> +#define XENSND_FIELD_SAMPLE_RATES            "sample-rates"
> +#define XENSND_FIELD_SAMPLE_FORMATS          "sample-formats"
> +#define XENSND_FIELD_BUFFER_SIZE             "buffer-size"
> +
> +/* Stream type field values. */
> +#define XENSND_STREAM_TYPE_PLAYBACK          "p"
> +#define XENSND_STREAM_TYPE_CAPTURE           "c"
> +/* Sample rate max string length */
> +#define XENSND_SAMPLE_RATE_MAX_LEN            6
> +/* Sample format field values */
> +#define XENSND_SAMPLE_FORMAT_MAX_LEN         24
> +
> +#define XENSND_PCM_FORMAT_S8_STR                 "s8"
> +#define XENSND_PCM_FORMAT_U8_STR                 "u8"
> +#define XENSND_PCM_FORMAT_S16_LE_STR             "s16_le"
> +#define XENSND_PCM_FORMAT_S16_BE_STR             "s16_be"
> +#define XENSND_PCM_FORMAT_U16_LE_STR             "u16_le"
> +#define XENSND_PCM_FORMAT_U16_BE_STR             "u16_be"
> +#define XENSND_PCM_FORMAT_S24_LE_STR             "s24_le"
> +#define XENSND_PCM_FORMAT_S24_BE_STR             "s24_be"
> +#define XENSND_PCM_FORMAT_U24_LE_STR             "u24_le"
> +#define XENSND_PCM_FORMAT_U24_BE_STR             "u24_be"
> +#define XENSND_PCM_FORMAT_S32_LE_STR             "s32_le"
> +#define XENSND_PCM_FORMAT_S32_BE_STR             "s32_be"
> +#define XENSND_PCM_FORMAT_U32_LE_STR             "u32_le"
> +#define XENSND_PCM_FORMAT_U32_BE_STR             "u32_be"
> +#define XENSND_PCM_FORMAT_F32_LE_STR             "float_le"
> +#define XENSND_PCM_FORMAT_F32_BE_STR             "float_be"
> +#define XENSND_PCM_FORMAT_F64_LE_STR             "float64_le"
> +#define XENSND_PCM_FORMAT_F64_BE_STR             "float64_be"
> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE_STR "iec958_subframe_le"
> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE_STR "iec958_subframe_be"
> +#define XENSND_PCM_FORMAT_MU_LAW_STR             "mu_law"
> +#define XENSND_PCM_FORMAT_A_LAW_STR              "a_law"
> +#define XENSND_PCM_FORMAT_IMA_ADPCM_STR          "ima_adpcm"
> +#define XENSND_PCM_FORMAT_MPEG_STR               "mpeg"
> +#define XENSND_PCM_FORMAT_GSM_STR                "gsm"
> +#define XENSND_PCM_FORMAT_SPECIAL_STR            "special"
> +
> +/*
> + * STATUS RETURN CODES.
> + */
> + /* Operation failed for some unspecified reason (e. g. -EIO). */

Would it make sense to say where the errno value is stored in the ring
structure?

> +#define XENSND_RSP_ERROR                 (-1)
> + /* Operation completed successfully. */
> +#define XENSND_RSP_OKAY                  0
> +
> +/*
> + * Description of the protocol between frontend and backend driver.
> + *
> + * The two halves of a Para-virtual sound driver communicates with
> + * each other using a shared page and an event channel.
> + * Shared page contains a ring with request/response packets.
> + *
> + *
> + * All request packets have the same length (16 octets)
> + *
> + *
> + * Request open - open a PCM stream for playback or capture:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                pcm_rate                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |  pcm_format     |  pcm_channels   |             reserved              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                         gref_directory_start                          |
> + * +-----------------+-----------------+-----------------+-----------------+
> +
> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_OPEN
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)

Is this necessary?
> + * pcm_rate - uint32_t, stream data rate, Hz
> + * pcm_format - uint8_t, XENSND_PCM_FORMAT_XXX value
> + * pcm_channels - uint8_t, number of channels of this stream
> + * gref_directory_start - grant_ref_t, a reference to the first shared page
> + *   describing shared buffer references. At least one page exists. If shared
> + *   buffer size exceeds what can be addressed by this single page, then
> + *   reference to the next page must be supplied (gref_dir_next_page below
> + *   is not NULL)
> + *
> + * Shared page for XENSND_OP_OPEN buffer descriptor (gref_directory in the
> + *   request) employs a list of pages, describing all pages of the shared data
> + *   buffer:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                          gref_dir_next_page                           |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                num_grefs                              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[0]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[1]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[N -1]                             |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
> + *   page directory
> + * num_grefs - number of grefs in this page
> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
> + *   allocated at XENSND_OP_OPEN
> + *
> + * Request close - close an opened pcm stream:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_CLOSE
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)
> + *
> + *
> + * Request read/write - used for read (for capture) or write (for playback):
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                offset                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                length                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_READ/XENSND_OP_WRITE
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)
> + * offset - uint32_t, read or write data offset within the shared buffer

offset or index ?
> + *   passed with XENSND_OP_OPEN request
> + * length - uint32_t, read or write data length

What if it is more than one page? Would it spill over to the next 'offset' ?
> + *
> + *
> + * Request set/get volume - set/get channels' volume of the stream given:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_SET_VOLUME or XENSND_OP_GET_VOLUME
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)
> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
> + * values:

I presume this is the first one? Or would it make sense to include the 'offset'
to mention which of the pages it has?

You could potentially have XENSND_OP_WRITE.offset=1 and right after
that have XENSND_OP_SET_VOLUME (and hopefully it would be using the next
grant page? But what if it wants to use a specific one - say always 255 one?)

> + *
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               channel[0]                              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               channel[1]                              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + *                  channel[XENSND_OP_OPEN.pcm_channels - 1]               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * channel[i] - sint32_t, volume of i-th channel
> + * Volume is expressed as a signed value in steps of 0.001 dBm,
> + * while 0 being 0dBm.
> + *
> + *
> + * Request mute/unmute - mute/unmute stream:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * id - uint16_t, private guest value, echoed in response
> + * operation - uint8_t, XENSND_OP_MUTE/XENSND_OP_UNMUTE
> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *   of the stream)

Again, I think you need an index in the grant pages ..
> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> + * values:
> + *
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
> + * Number of channels passed is equal to XENSND_OP_OPEN request pcm_channels
> + * field
> + *
> + *
> + * All response packets have the same length (64 bytes)
> + *
> + * Response for all requests:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |      status     |                      reserved                       |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                              reserved                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                              reserved                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * id - uint16_t, copied from the request
> + * stream_idx - uint8_t, copied from request
> + * operation - uint8_t, XENSND_OP_XXX - copied from request
> + * status - int8_t, response status (XENSND_RSP_???)

Hmm, what about also including an 'int errno' which is more of an OS specific
value.

> + */
> +
> +struct xensnd_request {
> +    uint8_t raw[16];
> +};

How come you pick 16, not say 32 or something more cache-line friendly?

It is OK to add padding.
> +
> +struct xensnd_response {
> +    uint8_t raw[16];
> +};
> +
> +#endif /* __XEN_PUBLIC_IO_XENSND_H__ */
> diff --git a/xen/include/public/io/sndif_linux.h b/xen/include/public/io/sndif_linux.h
> new file mode 100644
> index 0000000..58ec96c
> --- /dev/null
> +++ b/xen/include/public/io/sndif_linux.h
> @@ -0,0 +1,114 @@
> +/*
> + *  Unified sound-device I/O interface for Xen guest OSes
> + *
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of the GNU General Public License as published by
> + *   the Free Software Foundation; either version 2 of the License, or
> + *   (at your option) any later version.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA


You are putting an GPL header in the 'public' directory.

The public headers should be BSD, see COPYING in the root of Xen
directory.

Your previous file had the BSD, how come you are doing GPL here?



> + *
> + * Copyright (C) 2016 EPAM Systems Inc.
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__
> +#define __XEN_PUBLIC_IO_XENSND_LINUX_H__
> +
> +#ifdef __KERNEL__
> +#include <xen/interface/io/ring.h>
> +#include <xen/interface/grant_table.h>
> +#else
> +#include <xen/io/ring.h>
> +#include <xen/grant_table.h>
> +#endif
> +#if 0
> +#include <xen/include/public/io/sndif.h>
> +#else
> +#warning "TODO: properly include base protocol header"

Aha. Presumarily that should go away?

> +#include "sndif.h"
> +#endif
> +
> +struct xensnd_open_req {
> +	uint32_t pcm_rate;
> +	uint8_t pcm_format;
> +	uint8_t pcm_channels;
> +	/* in Hz */
> +	uint16_t __reserved0;
> +	grant_ref_t gref_directory_start;
> +} __attribute__((packed));

Hm, I recall folks frowning on using GCC-ism like this. These are
public headers and can be ingested by compilers like Microsoft
or other that don't grok __packet__.

Could you remove it and still make sure the structure has the right
size/offset? pahole helps a lot.

> +
> +struct xensnd_page_directory {
> +	grant_ref_t gref_dir_next_page;
> +	uint32_t num_grefs;
> +	grant_ref_t gref[0];
> +} __attribute__((packed));
> +
> +struct xensnd_close_req {
> +} __attribute__((packed));
> +
> +struct xensnd_write_req {
> +	uint32_t offset;
> +	uint32_t len;
> +} __attribute__((packed));
> +
> +struct xensnd_read_req {
> +	uint32_t offset;
> +	uint32_t len;
> +} __attribute__((packed));
> +
> +struct xensnd_get_vol_req {
> +} __attribute__((packed));
> +
> +struct xensnd_set_vol_req {
> +} __attribute__((packed));
> +
> +struct xensnd_mute_req {
> +} __attribute__((packed));
> +
> +struct xensnd_unmute_req {
> +} __attribute__((packed));
> +
> +struct xensnd_req {
> +	union {
> +		struct xensnd_request raw;
> +		struct {
> +			uint16_t id;
> +			uint8_t operation;
> +			uint8_t stream_idx;
> +			union {
> +				struct xensnd_open_req open;
> +				struct xensnd_close_req close;
> +				struct xensnd_write_req write;
> +				struct xensnd_read_req read;
> +				struct xensnd_get_vol_req get_vol;
> +				struct xensnd_set_vol_req set_vol;
> +				struct xensnd_mute_req mute;
> +				struct xensnd_unmute_req unmute;
> +			} op;
> +		} data;
> +	} u;
> +};
> +
> +struct xensnd_resp {
> +	union {
> +		struct xensnd_response raw;
> +		struct {
> +			uint16_t id;
> +			uint8_t operation;
> +			uint8_t stream_idx;
> +			int8_t status;
> +		} data;
> +	} u;
> +};
> +
> +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req,
> +		struct xensnd_resp);
> +
> +#endif /* __XEN_PUBLIC_IO_XENSND_LINUX_H__ */
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

Hi, Konrad!
First of all thank you for reviewing and providing valuable comments!
I will put action items/TODOs at the very beginning, so we can see the
summary. Also, please see answers inline.

1. Change frontend-id to frontend_id
2. Think about having a single sound card configured with a bunch of
different devices/streams
3. State that sample rates and formats expressed as decimals w/o any
particular ordering
4. Put description of migration/disconnection state
5. Change __attribute__((packed)) to __packed (scripts/checkpatch.pl)
6. Padding to fit cache line? What is its size (ARM/x86...) - need to discuss
7. Change GPL header to BSD
8. Remove #ifdef __KERNEL
9. Remove "Multiple PV drivers are allowed in the domain at the same time."
10. Support a single card as device/stream configuration allows fine
tuning already
11. Explicitly say which indices in XenStore configuration are contiguous
12. For strings "If not defined then use frontend's default." add more
description:
"This default is depends on concrete PV frontend implementation"
13. Make names of cards/devices optional

Thank you very much,
Oleksandr

On Fri, Nov 11, 2016 at 11:24 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote:
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
>> Signed-off-by: Andrushchenko, Oleksandr <andr2000@gmail.com>
>> Signed-off-by: Oleksandr Grytsov <al1img@gmail.com>
>> ---
>> Changes since v1:
>>  * removed __attribute__((__packed__)) from all structures definitions
>>
>> Changes since v2:
>>  * removed all C structures
>>  * added protocol description between frontend and backend drivers
>>
>> Changes since v3:
>>  * fixed some typos
>>  * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_**
>>  * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_**
>>  * added 'id' field to the request and response packets
>>  * renamed 'stream_id' to 'stream' in the packets description
>>  * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description
>>  * renamed 'pcm_stream_type' to 'pcm_type' in the packets description
>>  * removed 'stream_id' field from the response packets
>>
>> Changes since v4:
>>  * renamed 'stream_id' back to the to 'stream' in the packets description
>>  * moved 'id' field to the upper position in the response packets
>>
>> Changes since v5:
>>  * Slightly reworked request/response packets
>>  * Size of the request/response packet is changed to the 64 bytes
>>  * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are
>>    passed via shared page
>>  * Added parameters for the XenBus nodes (now each stream can be mapped
>>    to the defined sound device in the backend using those parameters)
>>  * Added XenBus state diagrams description
>>
>> Changes since v6:
>>  * Reworked streams description  in the Backend XenBus Nodes
>>
>> Changes since v7:
>>  * re-worked backend device parameters to be more generic and flexible
>>  * extended frontend device parameters
>>  * slightly updated state machine description added mute/unmute commands
>>  * added constants for XenStore configuration strings
>>    (fields, PCM formats etc.)
>>  * changed request/response structure size from 64 octets to 16
>>  * introduced dynamic buffer allocation instead of
>>    static XENSND_MAX_PAGES_PER_REQUEST
>>  * re-worked open request to allow dynamic buffer allocation
>>  * re-worked read/write/volume requests, so they don't pass grefs:
>>    buffer from the open request is used for these operations to pass data
>>  * specified type of the volume value to be a signed value in steps
>>    of 0.001 dBm, while 0 being 0dBm.
>>  * added Linux include file with structure definitions
>> ---
>> ---
>>  xen/include/public/io/sndif.h       | 583 ++++++++++++++++++++++++++++++++++++
>>  xen/include/public/io/sndif_linux.h | 114 +++++++
>>  2 files changed, 697 insertions(+)
>>  create mode 100644 xen/include/public/io/sndif.h
>>  create mode 100644 xen/include/public/io/sndif_linux.h
>>
>> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
>> new file mode 100644
>> index 0000000..df705a6
>> --- /dev/null
>> +++ b/xen/include/public/io/sndif.h
>> @@ -0,0 +1,583 @@
>> +/******************************************************************************
>> + * 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.
>> + * Copyright (C) 2016 EPAM Systems Inc.
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_IO_XENSND_H__
>> +#define __XEN_PUBLIC_IO_XENSND_H__
>> +
>> +/*
>> + * Front->back notifications: When enqueuing a new request, sending a
>> + * notification can be made conditional on req_event (i.e., the generic
>> + * hold-off mechanism provided by the ring macros). Backends must set
>> + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
>> + *
>> + * Back->front notifications: When enqueuing a new response, sending a
>> + * notification can be made conditional on rsp_event (i.e., the generic
>> + * hold-off mechanism provided by the ring macros). Frontends must set
>> + * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
>> + */
>> +
>> +/*
>> + * Feature and Parameter Negotiation
>> + * =================================
>> + * The two halves of a Para-virtual sound card 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.
>> + *
>> + *****************************************************************************
>> + *                            Backend XenBus Nodes
>> + *****************************************************************************
>> + *
>> + *-------------------------------- Addressing ---------------------------------
>> + *
>> + * Indices used to address frontends, driver instances, cards,
>> + * devices and streams.
>> + *
>> + * frontend-id
>> + *      Values:         <uint>
>> + *
>> + *      Domain ID of the sound frontend.
>> + *
>> + * drv_idx
>
> How come you use _ here but frontend-id has an '-'? Do you want to keep
> them all the same?
>

Will change to "_"

>> + *      Values:         <uint>
>> + *
>> + *      Zero based index of the virtualized sound driver instance in this domain.
>> + *      Multiple PV drivers are allowed in the domain at the same time.
>
> I think that is given. You can have multiple NICs for example. I woudl
> remove that sentence.

Will remove

>> + *
>> + * card_idx
>> + *      Values:         <uint>
>> + *
>> + *      Zero based index of the card within the driver.
>
>
> Can it be discontingous? Aka: 0, 1, 2, 5 ?
>

Card index cannot be discontingous.
What is more, after implementing the front driver I am thinking if we
can live with
a single card at all. Stream configuration allows you to group
playback/capture by
whatever means you prefer and at the end of the day user doesn't care from
where a stream comes in. This is just like in network you may have eth0, eth1 or
eth0 and eth0.1 - do you care of it (given that anyways the same PV
driver is in use)?
As each stream allows you to set its settings individually I would
drop card_idx from the
protocol

>> + *
>> + * dev_id
>> + *      Values:         <uint>
>> + *
>> + *      Unique (within given card instance) device ID.
>> + *      Doesn't have to be zero based and/or be contiguous.
>
> Oh, so they card_idx and drv_idx MUST be contingous? You may want
> to say that in detail.

Will do that

>
>> + *
>> + * stream_idx
>> + *      Values:         <uint>
>> + *
>> + *      Zero based index of the stream of the device.
>
> Can it be contingous or discountingous?

This is contiguous and card based, e.g. the very first stream of every
device starts from 0.
In order to uniquely identify the stream to the backend, there is
"stream/index" (see below)
which is contiguous for the whole driver.

>
>> + *
>> + * Example for the frontend running in domain 5, instance of the driver
>> + * in the front is 0 (single or first PV driver), sound card 0, device id 2,
>> + * first stream (0):
>> + * /local/domain/<frontend-id>/device/vsnd/<drv_idx>/card/<card_idx>/
>> + *   device/<dev_id>/stream/<stream_idx>/type = "p"
>> + * /local/domain/5/device/vsnd/0/card/0/device/2/stream/0/type = "p"
>
> Ah perfect!
>
>> + *
>> + *------------------------------- PCM settings --------------------------------
>> + *
>> + * Every virtualized sound frontend has set of cards, devices and streams, each
>> + * is individually configured. Part of the PCM configuration can be defined at
>> + * higher level and be fully or partially re-used by the underlying layers.
>> + * These configuration values are:
>> + *  o number of channels (min/max)
>> + *  o supported sample rates
>> + *  o supported sample formats.
>> + * E.g. one can define these values for the whole card, whole device or stream.
>> + * Every underlying layer in turn can re-define some or all of them to better
>> + * fit its needs. For example, card may define number of channels to be
>> + * in [1; 8] range, and some particular stream may be limited to [1; 2] only.
>> + * The rule is that the underlying layer must be a subset of the upper layer
>> + * range.
>> + *
>> + * channels-min
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      The minimum amount of channels that is supported.
>> + *      Must be at least 1. If not defined then use frontend's default.
>
> I don't see the frontend's default later in the doc?
Will add a statement that this value is frontend's driver specific in this case,
e.g. in my PV frontend I will use 1.
>> + *
>> + * channels-max
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      The maximum amount of channels that is supported.
>> + *      Must be at least <channels-min>. If not defined then use frontend's
>> + *      default.
>
> Again, not seeing the frontend's default? Perhaps have:
>
> "See XYZ"

As above

>> + *
>> + * sample-rates
>> + *      Values:         <list of uints>
>> + *
>> + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
>
> Any ordering rules? Or can it be random: 44100;9600;0xFFFFF
>
> And does it have to be decimal or can you use hex values?
>
>> + *      If not defined then use frontend's default.
>> + *
>> + * sample-formats
>> + *      Values:         <list of XENSND_PCM_FORMAT_XXX_STR>
>> + *
>> + *      List of supported sample formats separated by XENSND_LIST_SEPARATOR.
>> + *      If not defined then use frontend's default.
>
> Any ordering values? Can it be: "s16_be;s8" ?
>
Will add
>
> And where can I find the frontend's default?
>> + *
>> + * buffer-size
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      The maximum size in octets of the buffer to allocate per stream.
>
> Where is the maximum stream value defined?

You are not limited in number of streams, for that reason I am
thinking to remove
card concept, so still you can define as many streams as you need.
>
> Or would that be channels-max?
>
See above
>> + *
>> + * Example configuration:
>> + *
>> + * Card configuration used by all streams:
>> + * /local/domain/5/device/vsnd/0/card/0/sample-formats = "s8;u8;s16_le;s16_be"
>> + * Stream overrides sample rates supported:
>
> Not sure I follow that sentence.
See above: you can define PCM settings at any level you want, for example:
1. If all the streams will use same settings for number of channels,
rates etc,, then
define these values at card level
2. If device (which groups streams) want's to override some/all of
these settings it can
define those at its level
3. If a stream wants to redefine something at its level then it can
also be done.
Think of PCM config as reusing parent's configuration structure with a
possibility
to override whatever you need at whatever level.

>> + * /local/domain/5/device/vsnd/0/card/1/device/2/stream/0/sample-rates = "8000;22050;44100;48000"
>> + *
>> + *------------------------------ Card settings --------------------------------
>> + * short-name
>> + *      Values:         <char[32]>
>> + *
>> + *      Short name of the card.
>
> How come it is limited to 32 bytes?
This and below string limits actually come from ALSA and seem to be a good start
Why not?
>> + *
>> + * long-name
>> + *      Values:         <char[80]>
>> + *
>> + *      Long name of the card.
>
> Why two names?
Short name can be used as a simple reference, long name can be used to
fully describe what is it
> CAn one of them be skipped (say you only want to use 'short-name' ?
Yes, I believe these are optional. I will put a note on this
>
>> + *
>> + * For example,
>> + * /local/domain/5/device/vsnd/0/card/0/short-name = "Virtual audio"
>> + * /local/domain/5/device/vsnd/0/card/0/long-name = "Virtual audio at center stack"
>> + *
>> + *----------------------------- Device settings --------------------------------
>> + * name
>> + *      Values:         <char[80]>
>> + *
>> + *      Name of the sound device of the card given.
>
> I think the 'given' can go away. Why is there a limit of 80 characters?
>
Ok, for the length please see above

>> + *
>> + * For example,
>> + * /local/domain/5/device/vsnd/0/card/0/device/0/name = "General analog"
>> + *
>> + *----------------------------- Stream settings -------------------------------
>> + *
>> + * type
>> + *      Values:         "p", "c"
>> + *
>> + *      Stream type: "p" - playback stream, "c" - capture stream
>> + *
>> + *      If both capture and playback are needed then two streams need to be
>> + *      defined under the same device. For example,
>> + *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/0/type = "p"
>> + *      /local/domain/5/device/vsnd/0/card/0/device/0/stream/1/type = "c"
>> + *
>> + *****************************************************************************
>> + *                            Frontend XenBus Nodes
>> + *****************************************************************************
>> + *
>> + *----------------------- Request Transport Parameters -----------------------
>> + *
>> + * These are per stream.
>> + *
>> + * event-channel
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      The identifier of the Xen event channel used to signal activity
>> + *      in the ring buffer.
>> + *
>> + * ring-ref
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      The Xen grant reference granting permission for the backend to map
>> + *      a sole page in a single page sized ring buffer.
>> + *
>> + * index
>> + *      Values:         <string representing a positive integer>
>> + *
>> + *      After stream initialization it is assigned a unique ID (within the front
>> + *      driver), so every stream of the frontend can be identified by the
>> + *      backend by this ID.
>
> But doesn't it have that already by the virtue of stream_idx ?
>
No, stream_idx above is unique and contiguous within a device. Thus,
the same stream
index can exist inside multiple devices. So, the index in question is
a unique value, so
backend can identify the stream.
>> + */
>> +
>> +/*
>> + * STATE DIAGRAMS
>> + *
>> + *****************************************************************************
>> + *                                   Startup                                 *
>> + *****************************************************************************
>> + *
>> + * Tool stack creates front and back state nodes with initial state
>> + * XenbusStateInitialising.
>> + * Tool stack creates and sets up frontend sound configuration nodes per domain.
>> + *
>> + * Front                                Back
>> + * =================================    =====================================
>> + * XenbusStateInitialising              XenbusStateInitialising
>> + *                                       o Query backend device identification
>> + *                                         data.
>> + *                                       o Open and validate backend device.
>> + *                                                      |
>> + *                                                      |
>> + *                                                      V
>> + *                                      XenbusStateInitWait
>> + *
>> + * o Query frontend configuration
>> + * o Allocate and initialize
>> + *   event channels per configured
>> + *   playback/capture stream.
>> + * o Publish transport parameters
>> + *   that will be in effect during
>> + *   this connection.
>> + *              |
>> + *              |
>> + *              V
>> + * XenbusStateInitialised
>> + *
>> + *                                       o Query frontend transport parameters.
>> + *                                       o Connect to the event channels.
>> + *                                                      |
>> + *                                                      |
>> + *                                                      V
>> + *                                      XenbusStateConnected
>> + *
>> + *  o Create and initialize OS
>> + *  virtual sound device instances
>> + *  as per configuration.
>> + *              |
>> + *              |
>> + *              V
>> + * XenbusStateConnected
>> + *
>
>
> No migration/disconnection state?
>
Will add

>> + */
>> +
>> +/*
>> + * PCM FORMATS
>> + *
>> + * XENSND_PCM_FORMAT_<format>[_<endian>]
>> + *
>> + * format: <S/U/F><bits> or <name>
>> + *     S - signed, U - unsigned, F - float
>> + *     bits - 8, 16, 24, 32
>> + *     name - MU_LAW, GSM, etc.
>> + *
>> + * endian: <LE/BE>, may be absent
>> + *     LE - Little endian, BE - Big endian
>> + */
>> +#define XENSND_PCM_FORMAT_S8            0
>> +#define XENSND_PCM_FORMAT_U8            1
>> +#define XENSND_PCM_FORMAT_S16_LE        2
>> +#define XENSND_PCM_FORMAT_S16_BE        3
>> +#define XENSND_PCM_FORMAT_U16_LE        4
>> +#define XENSND_PCM_FORMAT_U16_BE        5
>> +#define XENSND_PCM_FORMAT_S24_LE        6
>> +#define XENSND_PCM_FORMAT_S24_BE        7
>> +#define XENSND_PCM_FORMAT_U24_LE        8
>> +#define XENSND_PCM_FORMAT_U24_BE        9
>> +#define XENSND_PCM_FORMAT_S32_LE        10
>> +#define XENSND_PCM_FORMAT_S32_BE        11
>> +#define XENSND_PCM_FORMAT_U32_LE        12
>> +#define XENSND_PCM_FORMAT_U32_BE        13
>> +#define XENSND_PCM_FORMAT_F32_LE        14 /* 4-byte float, IEEE-754 32-bit, */
>> +#define XENSND_PCM_FORMAT_F32_BE        15 /* range -1.0 to 1.0              */
>> +#define XENSND_PCM_FORMAT_F64_LE        16 /* 8-byte float, IEEE-754 64-bit, */
>> +#define XENSND_PCM_FORMAT_F64_BE        17 /* range -1.0 to 1.0              */
>> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18
>> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19
>> +#define XENSND_PCM_FORMAT_MU_LAW        20
>> +#define XENSND_PCM_FORMAT_A_LAW         21
>> +#define XENSND_PCM_FORMAT_IMA_ADPCM     22
>> +#define XENSND_PCM_FORMAT_MPEG          23
>> +#define XENSND_PCM_FORMAT_GSM           24
>> +#define XENSND_PCM_FORMAT_SPECIAL       31 /* Any other unspecified format */
>> +
>> +/*
>> + * REQUEST CODES.
>> + */
>> +#define XENSND_OP_OPEN                  0
>> +#define XENSND_OP_CLOSE                 1
>> +#define XENSND_OP_READ                  2
>> +#define XENSND_OP_WRITE                 3
>> +#define XENSND_OP_SET_VOLUME            4
>> +#define XENSND_OP_GET_VOLUME            5
>> +#define XENSND_OP_MUTE                  6
>> +#define XENSND_OP_UNMUTE                7
>> +
>> +/*
>> + * XENSTORE FIELD AND PATH NAME STRINGS, HELPERS.
>> + */
>> +#define XENSND_DRIVER_NAME                   "vsnd"
>> +
>> +#define XENSND_LIST_SEPARATOR                ";"
>> +/* Path entries */
>> +#define XENSND_PATH_CARD                     "card"
>> +#define XENSND_PATH_DEVICE                   "device"
>> +#define XENSND_PATH_STREAM                   "stream"
>> +/* Field names */
>> +#define XENSND_FIELD_CARD_SHORT_NAME         "short-name"
>> +#define XENSND_FIELD_CARD_LONG_NAME          "long-name"
>> +#define XENSND_FIELD_RING_REF                "ring-ref"
>> +#define XENSND_FIELD_EVT_CHNL                "event-channel"
>> +#define XENSND_FIELD_DEVICE_NAME             "name"
>> +#define XENSND_FIELD_TYPE                    "type"
>> +#define XENSND_FIELD_STREAM_INDEX            "index"
>> +#define XENSND_FIELD_CHANNELS_MIN            "channels-min"
>> +#define XENSND_FIELD_CHANNELS_MAX            "channels-max"
>> +#define XENSND_FIELD_SAMPLE_RATES            "sample-rates"
>> +#define XENSND_FIELD_SAMPLE_FORMATS          "sample-formats"
>> +#define XENSND_FIELD_BUFFER_SIZE             "buffer-size"
>> +
>> +/* Stream type field values. */
>> +#define XENSND_STREAM_TYPE_PLAYBACK          "p"
>> +#define XENSND_STREAM_TYPE_CAPTURE           "c"
>> +/* Sample rate max string length */
>> +#define XENSND_SAMPLE_RATE_MAX_LEN            6
>> +/* Sample format field values */
>> +#define XENSND_SAMPLE_FORMAT_MAX_LEN         24
>> +
>> +#define XENSND_PCM_FORMAT_S8_STR                 "s8"
>> +#define XENSND_PCM_FORMAT_U8_STR                 "u8"
>> +#define XENSND_PCM_FORMAT_S16_LE_STR             "s16_le"
>> +#define XENSND_PCM_FORMAT_S16_BE_STR             "s16_be"
>> +#define XENSND_PCM_FORMAT_U16_LE_STR             "u16_le"
>> +#define XENSND_PCM_FORMAT_U16_BE_STR             "u16_be"
>> +#define XENSND_PCM_FORMAT_S24_LE_STR             "s24_le"
>> +#define XENSND_PCM_FORMAT_S24_BE_STR             "s24_be"
>> +#define XENSND_PCM_FORMAT_U24_LE_STR             "u24_le"
>> +#define XENSND_PCM_FORMAT_U24_BE_STR             "u24_be"
>> +#define XENSND_PCM_FORMAT_S32_LE_STR             "s32_le"
>> +#define XENSND_PCM_FORMAT_S32_BE_STR             "s32_be"
>> +#define XENSND_PCM_FORMAT_U32_LE_STR             "u32_le"
>> +#define XENSND_PCM_FORMAT_U32_BE_STR             "u32_be"
>> +#define XENSND_PCM_FORMAT_F32_LE_STR             "float_le"
>> +#define XENSND_PCM_FORMAT_F32_BE_STR             "float_be"
>> +#define XENSND_PCM_FORMAT_F64_LE_STR             "float64_le"
>> +#define XENSND_PCM_FORMAT_F64_BE_STR             "float64_be"
>> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE_STR "iec958_subframe_le"
>> +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE_STR "iec958_subframe_be"
>> +#define XENSND_PCM_FORMAT_MU_LAW_STR             "mu_law"
>> +#define XENSND_PCM_FORMAT_A_LAW_STR              "a_law"
>> +#define XENSND_PCM_FORMAT_IMA_ADPCM_STR          "ima_adpcm"
>> +#define XENSND_PCM_FORMAT_MPEG_STR               "mpeg"
>> +#define XENSND_PCM_FORMAT_GSM_STR                "gsm"
>> +#define XENSND_PCM_FORMAT_SPECIAL_STR            "special"
>> +
>> +/*
>> + * STATUS RETURN CODES.
>> + */
>> + /* Operation failed for some unspecified reason (e. g. -EIO). */
>
> Would it make sense to say where the errno value is stored in the ring
> structure?

It is described in the response structure definition
>
>> +#define XENSND_RSP_ERROR                 (-1)
>> + /* Operation completed successfully. */
>> +#define XENSND_RSP_OKAY                  0
>> +
>> +/*
>> + * Description of the protocol between frontend and backend driver.
>> + *
>> + * The two halves of a Para-virtual sound driver communicates with
>> + * each other using a shared page and an event channel.
>> + * Shared page contains a ring with request/response packets.
>> + *
>> + *
>> + * All request packets have the same length (16 octets)
>> + *
>> + *
>> + * Request open - open a PCM stream for playback or capture:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                pcm_rate                               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |  pcm_format     |  pcm_channels   |             reserved              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                         gref_directory_start                          |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> +
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_OPEN
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
>
> Is this necessary?
Yes, at the very start a shared buffer is allocated. Then it is used by all the
commands which need it. At this stage backend prepares (maps pages etc.)
Taking into account the fact that the protocol is synchronous this buffer is
used to pass audio samples, volume and mute information

>> + * pcm_rate - uint32_t, stream data rate, Hz
>> + * pcm_format - uint8_t, XENSND_PCM_FORMAT_XXX value
>> + * pcm_channels - uint8_t, number of channels of this stream
>> + * gref_directory_start - grant_ref_t, a reference to the first shared page
>> + *   describing shared buffer references. At least one page exists. If shared
>> + *   buffer size exceeds what can be addressed by this single page, then
>> + *   reference to the next page must be supplied (gref_dir_next_page below
>> + *   is not NULL)
>> + *
>> + * Shared page for XENSND_OP_OPEN buffer descriptor (gref_directory in the
>> + *   request) employs a list of pages, describing all pages of the shared data
>> + *   buffer:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                          gref_dir_next_page                           |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                num_grefs                              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[0]                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[1]                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[N -1]                             |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
>> + *   page directory
>> + * num_grefs - number of grefs in this page
>> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
>> + *   allocated at XENSND_OP_OPEN
>> + *
>> + * Request close - close an opened pcm stream:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_CLOSE
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
>> + *
>> + *
>> + * Request read/write - used for read (for capture) or write (for playback):
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                offset                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                length                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_READ/XENSND_OP_WRITE
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
>> + * offset - uint32_t, read or write data offset within the shared buffer
>
> offset or index ?

Offset. After OPEN command both front and back are talking in terms of
a virtually contiguous buffer, thus no more pages. So, offset is used to address
data inside of this virtual buffer.

>> + *   passed with XENSND_OP_OPEN request
>> + * length - uint32_t, read or write data length
>
> What if it is more than one page? Would it spill over to the next 'offset' ?
See above, after OPEN we work with virtually contiguos buffer.
>> + *
>> + *
>> + * Request set/get volume - set/get channels' volume of the stream given:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_SET_VOLUME or XENSND_OP_GET_VOLUME
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
>> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
>> + * values:
>
> I presume this is the first one? Or would it make sense to include the 'offset'
> to mention which of the pages it has?

No pages as above

>
> You could potentially have XENSND_OP_WRITE.offset=1 and right after
> that have XENSND_OP_SET_VOLUME (and hopefully it would be using the next
> grant page? But what if it wants to use a specific one - say always 255 one?)
>
>> + *
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               channel[0]                              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               channel[1]                              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *                  channel[XENSND_OP_OPEN.pcm_channels - 1]               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * channel[i] - sint32_t, volume of i-th channel
>> + * Volume is expressed as a signed value in steps of 0.001 dBm,
>> + * while 0 being 0dBm.
>> + *
>> + *
>> + * Request mute/unmute - mute/unmute stream:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_MUTE/XENSND_OP_UNMUTE
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
>
> Again, I think you need an index in the grant pages ..

As above, no pages

>> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
>> + * values:
>> + *
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
>> + * Number of channels passed is equal to XENSND_OP_OPEN request pcm_channels
>> + * field
>> + *
>> + *
>> + * All response packets have the same length (64 bytes)
>> + *
>> + * Response for all requests:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |      status     |                      reserved                       |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                              reserved                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                              reserved                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * id - uint16_t, copied from the request
>> + * stream_idx - uint8_t, copied from request
>> + * operation - uint8_t, XENSND_OP_XXX - copied from request
>> + * status - int8_t, response status (XENSND_RSP_???)
>
> Hmm, what about also including an 'int errno' which is more of an OS specific
> value.
>

As per previous discussions we must stay away from some OS specifics and
be agnostic to the OSes used by back and front. That said, what if
front is Windows
and back QNX? What if errno values differ?

>> + */
>> +
>> +struct xensnd_request {
>> +    uint8_t raw[16];
>> +};
>
> How come you pick 16, not say 32 or something more cache-line friendly?
>
I was thinking about cache lines while picking 16. If I pick 32 or 64
will it always
be of the cache line size? On ARM? On x86? On some other platform (remember,
the requirement is to be agnostic...)

> It is OK to add padding.
>> +
>> +struct xensnd_response {
>> +    uint8_t raw[16];
>> +};
>> +
>> +#endif /* __XEN_PUBLIC_IO_XENSND_H__ */
>> diff --git a/xen/include/public/io/sndif_linux.h b/xen/include/public/io/sndif_linux.h
>> new file mode 100644
>> index 0000000..58ec96c
>> --- /dev/null
>> +++ b/xen/include/public/io/sndif_linux.h
>> @@ -0,0 +1,114 @@
>> +/*
>> + *  Unified sound-device I/O interface for Xen guest OSes
>> + *
>> + *   This program is free software; you can redistribute it and/or modify
>> + *   it under the terms of the GNU General Public License as published by
>> + *   the Free Software Foundation; either version 2 of the License, or
>> + *   (at your option) any later version.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + *
>> + *   You should have received a copy of the GNU General Public License
>> + *   along with this program; if not, write to the Free Software
>> + *   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307 USA
>
>
> You are putting an GPL header in the 'public' directory.
>
> The public headers should be BSD, see COPYING in the root of Xen
> directory.
>
> Your previous file had the BSD, how come you are doing GPL here?
>
>
Will fix
>
>> + *
>> + * Copyright (C) 2016 EPAM Systems Inc.
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_IO_XENSND_LINUX_H__
>> +#define __XEN_PUBLIC_IO_XENSND_LINUX_H__
>> +
>> +#ifdef __KERNEL__
>> +#include <xen/interface/io/ring.h>
>> +#include <xen/interface/grant_table.h>
>> +#else
>> +#include <xen/io/ring.h>
>> +#include <xen/grant_table.h>
>> +#endif
>> +#if 0
>> +#include <xen/include/public/io/sndif.h>
>> +#else
>> +#warning "TODO: properly include base protocol header"
>
> Aha. Presumarily that should go away?
>
It has already in the PATCH v9

>> +#include "sndif.h"
>> +#endif
>> +
>> +struct xensnd_open_req {
>> +     uint32_t pcm_rate;
>> +     uint8_t pcm_format;
>> +     uint8_t pcm_channels;
>> +     /* in Hz */
>> +     uint16_t __reserved0;
>> +     grant_ref_t gref_directory_start;
>> +} __attribute__((packed));
>
> Hm, I recall folks frowning on using GCC-ism like this. These are
> public headers and can be ingested by compilers like Microsoft
> or other that don't grok __packet__.
>
> Could you remove it and still make sure the structure has the right
> size/offset? pahole helps a lot.
>

As was requested in the previous discussions of this protocol we
should have structs
and packaging options in the protocol header file. For that same
reason I have created
a Linux specific file which can be used on Unix systems with gcc.
So, once someone needs to run on Windows they will add their own
platform specific
header...
>> +
>> +struct xensnd_page_directory {
>> +     grant_ref_t gref_dir_next_page;
>> +     uint32_t num_grefs;
>> +     grant_ref_t gref[0];
>> +} __attribute__((packed));
>> +
>> +struct xensnd_close_req {
>> +} __attribute__((packed));
>> +
>> +struct xensnd_write_req {
>> +     uint32_t offset;
>> +     uint32_t len;
>> +} __attribute__((packed));
>> +
>> +struct xensnd_read_req {
>> +     uint32_t offset;
>> +     uint32_t len;
>> +} __attribute__((packed));
>> +
>> +struct xensnd_get_vol_req {
>> +} __attribute__((packed));
>> +
>> +struct xensnd_set_vol_req {
>> +} __attribute__((packed));
>> +
>> +struct xensnd_mute_req {
>> +} __attribute__((packed));
>> +
>> +struct xensnd_unmute_req {
>> +} __attribute__((packed));
>> +
>> +struct xensnd_req {
>> +     union {
>> +             struct xensnd_request raw;
>> +             struct {
>> +                     uint16_t id;
>> +                     uint8_t operation;
>> +                     uint8_t stream_idx;
>> +                     union {
>> +                             struct xensnd_open_req open;
>> +                             struct xensnd_close_req close;
>> +                             struct xensnd_write_req write;
>> +                             struct xensnd_read_req read;
>> +                             struct xensnd_get_vol_req get_vol;
>> +                             struct xensnd_set_vol_req set_vol;
>> +                             struct xensnd_mute_req mute;
>> +                             struct xensnd_unmute_req unmute;
>> +                     } op;
>> +             } data;
>> +     } u;
>> +};
>> +
>> +struct xensnd_resp {
>> +     union {
>> +             struct xensnd_response raw;
>> +             struct {
>> +                     uint16_t id;
>> +                     uint8_t operation;
>> +                     uint8_t stream_idx;
>> +                     int8_t status;
>> +             } data;
>> +     } u;
>> +};
>> +
>> +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req,
>> +             struct xensnd_resp);
>> +
>> +#endif /* __XEN_PUBLIC_IO_XENSND_LINUX_H__ */
>> --
>> 2.7.4
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel



-- 
Best regards,
Oleksandr Andrushchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Jan Beulich]

>>> On 11.11.16 at 22:24, <konrad.wilk@oracle.com> wrote:
> On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote:
>> + *-------------------------------- Addressing ---------------------------------
>> + *
>> + * Indices used to address frontends, driver instances, cards,
>> + * devices and streams.
>> + *
>> + * frontend-id
>> + *      Values:         <uint>
>> + *
>> + *      Domain ID of the sound frontend.
>> + *
>> + * drv_idx
> 
> How come you use _ here but frontend-id has an '-'? Do you want to keep
> them all the same?
> 
>> + *      Values:         <uint>
>> + *
>> + *      Zero based index of the virtualized sound driver instance in this domain.
>> + *      Multiple PV drivers are allowed in the domain at the same time.
> 
> I think that is given. You can have multiple NICs for example. I woudl
> remove that sentence.

Is that a given? Multiple devices yes, but multiple drivers?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Mon, Nov 14, 2016 at 03:39:34AM -0700, Jan Beulich wrote:
> >>> On 11.11.16 at 22:24, <konrad.wilk@oracle.com> wrote:
> > On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote:
> >> + *-------------------------------- Addressing ---------------------------------
> >> + *
> >> + * Indices used to address frontends, driver instances, cards,
> >> + * devices and streams.
> >> + *
> >> + * frontend-id
> >> + *      Values:         <uint>
> >> + *
> >> + *      Domain ID of the sound frontend.
> >> + *
> >> + * drv_idx
> > 
> > How come you use _ here but frontend-id has an '-'? Do you want to keep
> > them all the same?
> > 
> >> + *      Values:         <uint>
> >> + *
> >> + *      Zero based index of the virtualized sound driver instance in this domain.
> >> + *      Multiple PV drivers are allowed in the domain at the same time.
> > 
> > I think that is given. You can have multiple NICs for example. I woudl
> > remove that sentence.
> 
> Is that a given? Multiple devices yes, but multiple drivers?

Oh, good eye! I misread it as multiple PV devices, not multiple PV drivers
of the same kind.

That is interesting.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

If these are all the comments then I'll start preparing patch v9
Thank you all for reviewing and providing feedback

Oleksandr Andrushchenko

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[David Vrabel]

Sound is not my area of expertise but I have a few points that you may
like to consider.

1. You can only say how many channels a stream has, not what the
channels correspond to (e.g., "left", "right" for a 2 channel stereo
stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround
sound stream).

2. Volume control uses absolute power levels (dBm). How is this supposed
to map to the standard 0-100% volume controls that are most commonly
presented to a user?

3. For the PCM formats you need to precisely specify the format or
reference external specifications that do so.  For example, "mpeg" is a
bit vague as I'm sure this standard body has produced many different
audio formats.  The specifications don't need to be freely available,
just correctly referenced.

4. 8 bits for stream index, operation and (particularly) pcm format feel
a little limiting.

5. For the read/write operations is the stream buffer considered to be
circular?  I think it probably should be.

6. PCM_FORMAT_SPECIAL doesn't seem useful to me.  New formats should be
added via an update to this spec.

David


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Volodymyr Babchuk]

Hello David,

I helped Oleksandr with parts of this protocol, so I want to address
some questions you asked.

On 16 November 2016 at 12:38, David Vrabel <david.vrabel@citrix.com> wrote:
>
> Sound is not my area of expertise but I have a few points that you may
> like to consider.
>
> 1. You can only say how many channels a stream has, not what the
> channels correspond to (e.g., "left", "right" for a 2 channel stereo
> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround
> sound stream).
>
Yes, we can specify mapping. But problem that is no one expects this
information.
There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on
chosen platform and all audio software just know and use them.

But now I'm thinking that it is a good idea to add predefined orders
to the protocol.
If target platform have a different order (e.g. backend running on
Windows), than backend should reorder data.

> 2. Volume control uses absolute power levels (dBm). How is this supposed
> to map to the standard 0-100% volume controls that are most commonly
> presented to a user?
Aaaah, volumes. They are problematic. Different hardware use different
scales. They can be linear, logarithmic, they can have different
ranges, there can be integrated amplifiers, etc, etc. But good news
that audio subsystem always know how to remap those scales into dBm
scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound
hardware have internal gain, it will support volume above 0dBm (or
above 100%) and also will add distortion to sound, btw. There are
special formula that maps dBms to percens. This formula is non-linear.
Also, different sound systems can use different formulas. So it is
better not to stick to percents, because percents on Linux and on
Windows running on the same hardware will be different percents. Even
percents reported by ALSA and reported by PulseAudio are different.
We can't represent -inf as integer, but this is not needed, thanks to
logarithmic nature of dBm. Any sufficiently small value like -120dBm
will do the job and smallest possible value of -2147483,648dBm will be
indistinguishable from silence on any present or future hardware.

> 3. For the PCM formats you need to precisely specify the format or
> reference external specifications that do so.  For example, "mpeg" is a
> bit vague as I'm sure this standard body has produced many different
> audio formats.  The specifications don't need to be freely available,
> just correctly referenced.
>
> 4. 8 bits for stream index, operation and (particularly) pcm format feel
> a little limiting.
>
> 5. For the read/write operations is the stream buffer considered to be
> circular?  I think it probably should be.
>
> 6. PCM_FORMAT_SPECIAL doesn't seem useful to me.  New formats should be
> added via an update to this spec.
You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to
prepare data for my device and my device expects audio in that
format". You don't need this on generic x86 system,
but it can be absolutely crucial on embedded system with hardware
accelerated audio/video playback for example.

-- 
WBR Volodymyr Babchuk aka lorc [+380976646013]
mailto: vlad.babchuk@gmail.com

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[David Vrabel]

On 16/11/16 13:12, Volodymyr Babchuk wrote:
> Hello David,
> 
> I helped Oleksandr with parts of this protocol, so I want to address
> some questions you asked.
> 
> On 16 November 2016 at 12:38, David Vrabel <david.vrabel@citrix.com> wrote:
>>
>> Sound is not my area of expertise but I have a few points that you may
>> like to consider.
>>
>> 1. You can only say how many channels a stream has, not what the
>> channels correspond to (e.g., "left", "right" for a 2 channel stereo
>> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround
>> sound stream).
>>
> Yes, we can specify mapping. But problem that is no one expects this
> information.
> There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on
> chosen platform and all audio software just know and use them.

Specifying a fixed ordering of channels is fine.

> But now I'm thinking that it is a good idea to add predefined orders
> to the protocol.
> If target platform have a different order (e.g. backend running on
> Windows), than backend should reorder data.
> 
>> 2. Volume control uses absolute power levels (dBm). How is this supposed
>> to map to the standard 0-100% volume controls that are most commonly
>> presented to a user?
> Aaaah, volumes. They are problematic. Different hardware use different
> scales. They can be linear, logarithmic, they can have different
> ranges, there can be integrated amplifiers, etc, etc. But good news
> that audio subsystem always know how to remap those scales into dBm
> scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound
> hardware have internal gain, it will support volume above 0dBm (or
> above 100%) and also will add distortion to sound, btw. There are
> special formula that maps dBms to percens. This formula is non-linear.
> Also, different sound systems can use different formulas. So it is
> better not to stick to percents, because percents on Linux and on
> Windows running on the same hardware will be different percents. Even
> percents reported by ALSA and reported by PulseAudio are different.
> We can't represent -inf as integer, but this is not needed, thanks to
> logarithmic nature of dBm. Any sufficiently small value like -120dBm
> will do the job and smallest possible value of -2147483,648dBm will be
> indistinguishable from silence on any present or future hardware.

This all sounds fine to me except you've got the units wrong and you
really mean dB (not dBm where 0 dBm == 1 mW of output power which
doesn't make a whole lot of sense in this context).

>> 6. PCM_FORMAT_SPECIAL doesn't seem useful to me.  New formats should be
>> added via an update to this spec.
> You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to
> prepare data for my device and my device expects audio in that
> format". You don't need this on generic x86 system,
> but it can be absolutely crucial on embedded system with hardware
> accelerated audio/video playback for example.

The frontend doesn't know what the underlying hardware is so how can it
know what format SPECIAL means?

Perhaps you could give an example of what one of these "special" formats
looks like?

I think it would be better to define a format number/name for each
special format.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Volodymyr Babchuk]

On 16 November 2016 at 15:31, David Vrabel <david.vrabel@citrix.com> wrote:
> On 16/11/16 13:12, Volodymyr Babchuk wrote:
>> Hello David,
>>
>> I helped Oleksandr with parts of this protocol, so I want to address
>> some questions you asked.
>>
>> On 16 November 2016 at 12:38, David Vrabel <david.vrabel@citrix.com> wrote:
>>>
>>> Sound is not my area of expertise but I have a few points that you may
>>> like to consider.
>>>
>>> 1. You can only say how many channels a stream has, not what the
>>> channels correspond to (e.g., "left", "right" for a 2 channel stereo
>>> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround
>>> sound stream).
>>>
>> Yes, we can specify mapping. But problem that is no one expects this
>> information.
>> There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on
>> chosen platform and all audio software just know and use them.
>
> Specifying a fixed ordering of channels is fine.
>
>> But now I'm thinking that it is a good idea to add predefined orders
>> to the protocol.
>> If target platform have a different order (e.g. backend running on
>> Windows), than backend should reorder data.
>>
>>> 2. Volume control uses absolute power levels (dBm). How is this supposed
>>> to map to the standard 0-100% volume controls that are most commonly
>>> presented to a user?
>> Aaaah, volumes. They are problematic. Different hardware use different
>> scales. They can be linear, logarithmic, they can have different
>> ranges, there can be integrated amplifiers, etc, etc. But good news
>> that audio subsystem always know how to remap those scales into dBm
>> scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound
>> hardware have internal gain, it will support volume above 0dBm (or
>> above 100%) and also will add distortion to sound, btw. There are
>> special formula that maps dBms to percens. This formula is non-linear.
>> Also, different sound systems can use different formulas. So it is
>> better not to stick to percents, because percents on Linux and on
>> Windows running on the same hardware will be different percents. Even
>> percents reported by ALSA and reported by PulseAudio are different.
>> We can't represent -inf as integer, but this is not needed, thanks to
>> logarithmic nature of dBm. Any sufficiently small value like -120dBm
>> will do the job and smallest possible value of -2147483,648dBm will be
>> indistinguishable from silence on any present or future hardware.
>
> This all sounds fine to me except you've got the units wrong and you
> really mean dB (not dBm where 0 dBm == 1 mW of output power which
> doesn't make a whole lot of sense in this context).
Oops. Yes, you are right. Have no idea where dBm came from.


>>> 6. PCM_FORMAT_SPECIAL doesn't seem useful to me.  New formats should be
>>> added via an update to this spec.
>> You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to
>> prepare data for my device and my device expects audio in that
>> format". You don't need this on generic x86 system,
>> but it can be absolutely crucial on embedded system with hardware
>> accelerated audio/video playback for example.
>
> The frontend doesn't know what the underlying hardware is so how can it
> know what format SPECIAL means?
In general case yes, there are no way to know something about real
hardware. But we tried to make protocol more generic. Imagine embedded
system, where guests can know about hardware and can exploit this
knowledge to improve performance. But I think you are right. Better to
remove SPECIAL format and then extend protocol locally if there will
arise need for this.
> Perhaps you could give an example of what one of these "special" formats
> looks like?
It can be anything. If we have an audio decoding accelerator, we can
dump mp3 or ac3 data right into sound card for example.

> I think it would be better to define a format number/name for each
> special format.
Yes.

Oleksadr, what is your opinion on this?


-- 
WBR Volodymyr Babchuk aka lorc [+380976646013]
mailto: vlad.babchuk@gmail.com

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

Thank you all for the review and comments!
I guess only these are not addressed (or still need comments):

> 1. You can only say how many channels a stream has, not what the
> channels correspond to (e.g., "left", "right" for a 2 channel stereo
> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround
> sound stream).

PV driver may be configured to have number of channels which are
not supported by the back's HW. In that case this is up to back how to treat
those. If we introduce some specific ordering or mapping that will only be
a recommendation to the back, which it may follow.
Thus, I would stick to what we have now

> 3. For the PCM formats you need to precisely specify the format or
> reference external specifications that do so.  For example, "mpeg" is a
> bit vague as I'm sure this standard body has produced many different
> audio formats.  The specifications don't need to be freely available,
> just correctly referenced.

I do understand the intent here, but I blieve we cannot make the protocol
being simple and fit all at once. So, I would suggest leaving it as is
at least for now
or removing it.

> 4. 8 bits for stream index, operation and (particularly) pcm format feel
> a little limiting.
8 bits mean you can have 256 streams, operations and formats (which are
indices). I would prefer to stay with 8 bits

> 5. For the read/write operations is the stream buffer considered to be
> circular?  I think it probably should be.
I guess it shouldn't. The reason of having offset is the fact that software on
front side may mmap part of the buffer and tell the driver the offset of
the "dirty" region of the buffer.

Please find updated list of TODOs for the next version:

1. Change frontend-id to frontend_id
2. Have a single sound card configured with bunch of
different devices/streams
3. State that sample rates and formats expressed as decimals w/o any
particular ordering
4. Put description of migration/disconnection state
5. Change __attribute__((packed)) to __packed (scripts/checkpatch.pl)
6. Padding to fit cache line? What is its size (ARM/x86...) - need to discuss
7. Change GPL header to BSD
8. Remove #ifdef __KERNEL
9. Remove "Multiple PV drivers are allowed in the domain at the same time."
10. Support a single card as device/stream configuration allows fine
tuning already
11. Explicitly say which indices in XenStore configuration are contiguous
12. For strings "If not defined then use frontend's default." add more
description:
"This default is depends on concrete PV frontend implementation"
13. Make names of cards/devices optional
14. Remove PCM_FORMAT_SPECIAL
15. Change volume units from dBm to dB

Thank you,
Oleksandr

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel