[PATCH v15] sndif: add ABI for para-virtual sound

[PATCH v15] sndif: add ABI for para-virtual sound

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>

Hi, all!

Please find the next version of the ABI for the PV sound
after addressing review comments.

Thank you,
Oleksandr Andrushchenko
Oleksandr Grytsov

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

 xen/include/public/io/sndif.h | 671 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 671 insertions(+)
 create mode 100644 xen/include/public/io/sndif.h

-- 
2.7.4


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

[PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>

Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
Signed-off-by: Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>

---
Changes since v1:
 * removed __attribute__((__packed__)) from all structures definitions

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

Changes since v8:
 * changed frontend-id to frontend_id
 * single sound card support, configured with bunch of
   devices/streams
 * clarifucation made on sample rates and formats expressed as
   decimals w/o any particular ordering
 * put description of migration/disconnection state
 * replaced __attribute__((packed)) to __packed
 * changed padding of ring structures to 64 to fit cache line
 * removeed #ifdef __KERNEL
 * explicitly stated which indices in XenStore configuration
   are contiguous
 * added description to what frontend's defaults are
 * made names of virtual card/devices optional
 * removed PCM_FORMAT_SPECIAL
 * changed volume units from dBm to dB

Changes since v9:
 * removed sndif_linux.h
 * moved all structures from sndif_linux.h to sndif.h
 * structures padded where needed
 * fixed Hz comment

Changes since v10:
 * fixed tabs to 4 spaces to comply with Xen coding style
 * added placeholders to empty structures (C89 concern)
 * added missing header includes

Changes since v11:
 * added XENSND_RSP_NOTSUPP error code
 * changed gref[0] to gref[1] with comment
 * modified comments on empty structures
 * removed "__" from member names
 * fixed indentation
 * added padding in union xensnd_resp
 * changed __XEN_PUBLIC_IO_XENSND_H__ to __XEN_PUBLIC_IO_SNDIF_H__

Changes since v12:
 * changed indentation for defines
 * missed ";" after gref[1]
 * documentation changes
 * changed req/resp structures
 * changed xensnd_page_directory structure
 * pass buffer size in open request

Changes since v13:
 * note on usage of grant ref 0
 * all page sizes are XEN_PAGE_SIZE
 * padding/reserved cleanup/fixes
 * removed empty structures

Changes since v14:
 * turn padding into reserved
---
---
 xen/include/public/io/sndif.h | 671 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 671 insertions(+)
 create mode 100644 xen/include/public/io/sndif.h

diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
new file mode 100644
index 0000000..60a6d88
--- /dev/null
+++ b/xen/include/public/io/sndif.h
@@ -0,0 +1,671 @@
+/******************************************************************************
+ * 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.
+ *
+ * Authors: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
+ *          Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
+ *          Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
+ *          Iurii Konovalenko <iurii.konovalenko@globallogic.com>
+ */
+
+#ifndef __XEN_PUBLIC_IO_SNDIF_H__
+#define __XEN_PUBLIC_IO_SNDIF_H__
+
+#include "ring.h"
+#include "../grant_table.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 contiguous index of the virtualized sound driver instance in
+ *      this domain. Multiple PV drivers are allowed in the domain
+ *      at the same time.
+ *
+ * dev_id
+ *      Values:         <uint>
+ *
+ *      Unique device ID.
+ *      Doesn't have to be zero based and/or to be contiguous.
+ *
+ * stream_idx
+ *      Values:         <uint>
+ *
+ *      Zero based contiguous 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), device id 2,
+ * first stream (0):
+ * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
+ *         device/<dev_id>/stream/<stream_idx>/type = "p"
+ * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
+ *
+ *------------------------------- PCM settings --------------------------------
+ *
+ * Every virtualized sound frontend has set of 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 driver, device or stream.
+ * Every underlying layer in turn can re-define some or all of them to better
+ * fit its needs. For example, driver 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.
+ *
+ * Note: if any of the values are not defined then PV driver should use
+ * its default values instead.
+ *
+ * channels-min
+ *      Values:         <uint>
+ *
+ *      The minimum amount of channels that is supported.
+ *      Must be at least 1. If not defined then use frontend's default.
+ *
+ * channels-max
+ *      Values:         <uint>
+ *
+ *      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 rates are expressed
+ *      as a list of decimal values w/o any ordering requirement.
+ *
+ * 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:         <uint>
+ *
+ *      The maximum size in octets of the buffer to allocate per stream.
+ *
+ * Example configuration:
+ *
+ * Driver configuration used by all streams:
+ * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"
+ * Stream overrides sample rates supported:
+ * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates =
+ *        "8000;22050;44100;48000"
+ *
+ *----------------------- Virtual sound card settings --------------------------
+ * short-name
+ *      Values:         <char[32]>
+ *
+ *      Short name of the virtual sound card. Optional.
+ *
+ * long-name
+ *      Values:         <char[80]>
+ *
+ *      Long name of the virtual sound card. Optional.
+ *
+ * For example,
+ * /local/domain/5/device/vsnd/0/short-name = "Virtual audio"
+ * /local/domain/5/device/vsnd/0/long-name =
+ *         "Virtual audio at center stack"
+ *
+ *----------------------------- Device settings --------------------------------
+ * name
+ *      Values:         <char[80]>
+ *
+ *      Name of the sound device within the virtual sound card. Optional.
+ *
+ * For example,
+ * /local/domain/5/device/vsnd/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/device/0/stream/0/type = "p"
+ *      /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"
+ *
+ *****************************************************************************
+ *                            Frontend XenBus Nodes
+ *****************************************************************************
+ *
+ *----------------------- Request Transport Parameters -----------------------
+ *
+ * These are per stream.
+ *
+ * event-channel
+ *      Values:         <uint>
+ *
+ *      The identifier of the Xen event channel used to signal activity
+ *      in the ring buffer.
+ *
+ * ring-ref
+ *      Values:         <uint>
+ *
+ *      The Xen grant reference granting permission for the backend to map
+ *      a sole page in a single page sized ring buffer.
+ *
+ * index
+ *      Values:         <uint>
+ *
+ *      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. This is not equal to stream_idx as the later is
+ *      zero based within a device, but this index is contiguous within the
+ *      driver.
+ */
+
+/*
+ * 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
+ *
+ *                                      XenbusStateUnknown
+ *                                      XenbusStateClosed
+ *                                      XenbusStateClosing
+ * o Remove virtual sound device
+ * o Remove event channels
+ *              |
+ *              |
+ *              V
+ * XenbusStateClosed
+ *
+ */
+
+/*
+ * 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
+
+/*
+ * 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_DEVICE              "device"
+#define XENSND_PATH_STREAM              "stream"
+/* Field names */
+#define XENSND_FIELD_VCARD_SHORT_NAME   "short-name"
+#define XENSND_FIELD_VCARD_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"
+
+/*
+ * STATUS RETURN CODES.
+ */
+/* Operation not supported. */
+#define XENSND_RSP_NOTSUPP              (-2)
+/* Operation failed for some unspecified reason (e. g. -EIO). */
+#define XENSND_RSP_ERROR                (-1)
+/* Operation completed successfully. */
+#define XENSND_RSP_OKAY                 0
+
+/*
+ * Assumptions:
+ *   o usage of grant reference 0 as invalid grant reference:
+ *     grant reference 0 is valid, but never exposed to a PV driver,
+ *     because of the fact it is already in use/reserved by the PV console.
+ *   o all references in this document to page sizes must be treated
+ *     as pages of size XEN_PAGE_SIZE unless  otherwise noted.
+ *
+ * Description of the protocol between frontend and backend driver.
+ *
+ * The two halves of a Para-virtual sound driver communicate with
+ * each other using a shared page and an event channel.
+ * Shared page contains a ring with request/response packets.
+ *
+ * All reserved fields in the structures below must be 0.
+ *
+ * All request packets have the same length (32 octets)
+ * All request packets have common header:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                reserved                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *   id - uint16_t, private guest value, echoed in response
+ *   operation - uint8_t, operation code
+ *   stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *     of the stream)
+ *
+ *
+ * Request open - open a PCM stream for playback or capture:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                reserved                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                pcm_rate                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |  pcm_format     |  pcm_channels   |             reserved              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               buffer_sz                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                         gref_directory_start                          |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * 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
+ * buffer_sz - uint32_t, buffer size to be allocated in octets
+ * 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 (see gref_dir_next_page below)
+ */
+
+struct xensnd_open_req {
+    uint32_t pcm_rate; /* in Hz */
+    uint8_t pcm_format;
+    uint8_t pcm_channels;
+    uint16_t reserved;
+    uint32_t buffer_sz;
+    grant_ref_t gref_directory_start;
+};
+
+/*
+ * 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                           |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[0]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[i]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[N -1]                             |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * gref_dir_next_page - grant_ref_t, reference to the next page describing
+ *   page directory. Must be 0 if no more pages in the list.
+ * gref[i] - grant_ref_t, reference to a shared page of the buffer
+ *   allocated at XENSND_OP_OPEN
+ *
+ * Number of grant_ref_t entries in the whole page directory is not
+ * passed, but instead can be calculated as:
+ *   num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) /
+ *       XEN_PAGE_SIZE
+ */
+
+struct xensnd_page_directory {
+    grant_ref_t gref_dir_next_page;
+    grant_ref_t gref[1]; /* Variable length */
+};
+
+/*
+ *  Request close - close an opened pcm stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                | XENSND_OP_CLOSE |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ */
+
+/*
+ * Request read/write - used for read (for capture) or write (for playback):
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                offset                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                length                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
+ * 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
+ */
+
+struct xensnd_rw_req {
+    uint32_t offset;
+    uint32_t len;
+};
+
+/*
+ * Request set/get volume - set/get channels' volume of the stream given:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * operation - XENSND_OP_SET_VOLUME for volume set
+ *   or XENSND_OP_GET_VOLUME for volume get
+ * Buffer passed with XENSND_OP_OPEN is used to exchange volume
+ * values:
+ *
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[0]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[i]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                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 dB,
+ * while 0 being 0 dB.
+ */
+
+/*
+ * Request mute/unmute - mute/unmute stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
+ * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
+ * values:
+ *
+ *                                     0                                   octet
+ * +-----------------------------------------------------------------------+
+ * |                               channel[0]                              |
+ * +-----------------------------------------------------------------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------------------------------------------------------------+
+ * |                               channel[i]                              |
+ * +-----------------------------------------------------------------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------------------------------------------------------------+
+ * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
+ */
+
+/*
+ * All response packets have the same length (32 octets)
+ *
+ * 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_* - copied from request
+ * status - int8_t, response status (XENSND_RSP_*)
+ */
+
+struct xensnd_req {
+    uint16_t id;
+    uint8_t operation;
+    uint8_t stream_idx;
+    uint32_t reserved;
+    union {
+        struct xensnd_open_req open;
+        struct xensnd_rw_req rw;
+        uint8_t reserved[24];
+    } op;
+};
+
+struct xensnd_resp {
+    uint16_t id;
+    uint8_t operation;
+    uint8_t stream_idx;
+    int8_t status;
+    uint8_t reserved[27];
+};
+
+DEFINE_RING_TYPES(xen_sndif, struct xensnd_req, struct xensnd_resp);
+
+#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */
-- 
2.7.4


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

I'm just wondering if anybody had a chance to look at the patch...

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

Hi, Konrad!

I see no comments for almost 3 weeks now, so
probably there are no objections against this protocol.
Can we please move on on this?

Thank you in advance,
Oleksandr

On 12/08/2016 05:13 PM, Oleksandr Andrushchenko wrote:
> I'm just wondering if anybody had a chance to look at the patch...


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On December 22, 2016 2:21:26 AM EST, Oleksandr Andrushchenko <andr2000@gmail.com> wrote:
>Hi, Konrad!
>
>I see no comments for almost 3 weeks now, so
>probably there are no objections against this protocol.
>Can we please move on on this?

Sorry, I had been busy with internal high priority items. Let me take a look at it this Friday.


>
>Thank you in advance,
>Oleksandr
>
>On 12/08/2016 05:13 PM, Oleksandr Andrushchenko wrote:
>> I'm just wondering if anybody had a chance to look at the patch...


Thanks!


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>

Usually one also puts somethign in the commit description.

When I applied this to me tree I got:

[konrad@char xen]$ git log --oneline HEAD^..
ecc7711 This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.
[konrad@char xen]$ 

Also, thank you for the including the pahole output!

> 
> Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
> Signed-off-by: Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> ---
> Changes since v1:
>  * removed __attribute__((__packed__)) from all structures definitions
> 
> 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
> 
> Changes since v8:
>  * changed frontend-id to frontend_id
>  * single sound card support, configured with bunch of
>    devices/streams
>  * clarifucation made on sample rates and formats expressed as
>    decimals w/o any particular ordering
>  * put description of migration/disconnection state
>  * replaced __attribute__((packed)) to __packed
>  * changed padding of ring structures to 64 to fit cache line
>  * removeed #ifdef __KERNEL
>  * explicitly stated which indices in XenStore configuration
>    are contiguous
>  * added description to what frontend's defaults are
>  * made names of virtual card/devices optional
>  * removed PCM_FORMAT_SPECIAL
>  * changed volume units from dBm to dB
> 
> Changes since v9:
>  * removed sndif_linux.h
>  * moved all structures from sndif_linux.h to sndif.h
>  * structures padded where needed
>  * fixed Hz comment
> 
> Changes since v10:
>  * fixed tabs to 4 spaces to comply with Xen coding style
>  * added placeholders to empty structures (C89 concern)
>  * added missing header includes
> 
> Changes since v11:
>  * added XENSND_RSP_NOTSUPP error code
>  * changed gref[0] to gref[1] with comment
>  * modified comments on empty structures
>  * removed "__" from member names
>  * fixed indentation
>  * added padding in union xensnd_resp
>  * changed __XEN_PUBLIC_IO_XENSND_H__ to __XEN_PUBLIC_IO_SNDIF_H__
> 
> Changes since v12:
>  * changed indentation for defines
>  * missed ";" after gref[1]
>  * documentation changes
>  * changed req/resp structures
>  * changed xensnd_page_directory structure
>  * pass buffer size in open request
> 
> Changes since v13:
>  * note on usage of grant ref 0
>  * all page sizes are XEN_PAGE_SIZE
>  * padding/reserved cleanup/fixes
>  * removed empty structures
> 
> Changes since v14:
>  * turn padding into reserved
> ---
> ---
>  xen/include/public/io/sndif.h | 671 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 671 insertions(+)
>  create mode 100644 xen/include/public/io/sndif.h
> 
> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
> new file mode 100644
> index 0000000..60a6d88
> --- /dev/null
> +++ b/xen/include/public/io/sndif.h
> @@ -0,0 +1,671 @@
> +/******************************************************************************
> + * 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.
> + *
> + * Authors: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
> + *          Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
> + *          Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> + *          Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> + */
> +
> +#ifndef __XEN_PUBLIC_IO_SNDIF_H__
> +#define __XEN_PUBLIC_IO_SNDIF_H__
> +
> +#include "ring.h"
> +#include "../grant_table.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

The rest of the drivers use - instead of _.

Is there a particular reason you need to have _ ?

See vscsiif.h and console and vkbd. Actually the last ones
don't mention it in the header but the libxl constructs these.

> + *      Values:         <uint>

uint32_t
> + *
> + *      Domain ID of the sound frontend.
> + *
> + * drv_idx

Again, can this be - ?
> + *      Values:         <uint>

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

This is not needed.

We already have this information in the XenStore directory:

/local/domain/0/backend/vsnd/2/0/frontend-id = 2
/local/domain/0/backend/vsnd/2/1/frontend-id = 2
..
/local/domain/0/backend/vsnd/2/N/frontend-id = 2

(which would be similar to what 'vbd', 'console' and 'vif' do).

> + *
> + * dev_id

s/_/-/
> + *      Values:         <uint>

uint_32t
> + *
> + *      Unique device ID.
> + *      Doesn't have to be zero based and/or to be contiguous.

Is uint enough? Or would it be better to have an <string> if this is
some form of SHA-1 value?

> + *
> + * stream_idx

s/_/-/
> + *      Values:         <uint>

uint_32t

[edit, based on the struct xensnd_req  this has to be uint_8t] ?

> + *
> + *      Zero based contiguous 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), device id 2,
> + * first stream (0):
> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"

Why do you need 'device' ?

Could not this be:

/local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?


> + *
> + *------------------------------- PCM settings --------------------------------
> + *
> + * Every virtualized sound frontend has set of devices and streams, each

frontend or backend?

I would think backend since this is still the backend section?

> + * 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 driver, device or stream.
> + * Every underlying layer in turn can re-define some or all of them to better
> + * fit its needs. For example, driver 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.

OK, so these are more of 'global ones'?

> + *
> + * Note: if any of the values are not defined then PV driver should use
> + * its default values instead.


How is that suppose to work? What if the PV driver by default wants
to use say 314156 channels and the backend does not expose any? Can
it use that? I presume it could try.. and it would fail but that
hardly seems nice.

> + *
> + * channels-min
> + *      Values:         <uint>

uint32_t
> + *
> + *      The minimum amount of channels that is supported.
> + *      Must be at least 1. If not defined then use frontend's default.

This is odd. This whole section is for the backend, so how can the backend
not have this defined?

Do you mean to say that this parameter is optional? Perhaps
then say right before 'The minimum' include '(optional)'

Anyhow where do you envision this optional global value to be?

/local/domain/0/backend/vsnd/5/channels-[min|max] ?

Perhaps mention that in the start of this section:

These global values are under the backend 'vsnd' directory, as
so:

/local/domain/0/backends/vsnd/<front-id>/channels-[min|max]

Thought I think there is probably a better way to 
say this..

> + *
> + * channels-max
> + *      Values:         <uint>

uint32_t
> + *
> + *      The maximum amount of channels that is supported.
> + *      Must be at least <channels-min>. If not defined then use frontend's
> + *      default.

I am still having trouble understanding how the backend is suppose
to use the frontend's default.

Perhaps you want to say:

If this value is not exposed by the backend the frontend
is permitted to use its default values.

?
> + *
> + * sample-rates
> + *      Values:         <list of uints>

.. of uint32_t
> + *
> + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
> + *      If not defined then use frontend's default. Sample rates are expressed

Ahain this 'If not defined then use fronten'ds default' could be written
a bit differently.
> + *      as a list of decimal values w/o any ordering requirement.
> + *
> + * 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.

Again, this does not sound right. The backend has no clue what
the frontend defaults are.
> + *
> + * buffer-size
> + *      Values:         <uint>

uint32_t
> + *
> + *      The maximum size in octets of the buffer to allocate per stream.
> + *
> + * Example configuration:
> + *
> + * Driver configuration used by all streams:
> + * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"

Huh? But this is a backend value? Don't you mean:

/local/domain/0/backend/vsnd/5/sample-formats = "s8;u8;s16_le;s16_be"
?

> + * Stream overrides sample rates supported:

-EPARSE

> + * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates =
> + *        "8000;22050;44100;48000"

/local/domain/0/backend/vsnd/5/2/stream/0/sample-rates ="8000;22050;44100;48000"

But that is wrong as this section is about global values so they
would be more like:

/local/domain/0/backend/vsnd/5/sample-rates ="8000;22050;44100;48000"

?

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

This should be inverted. That is this is on the backend
side:

/local/domain/0/backend/vsnd/5/0/2/short-name = "Virtual audio"

And so on.

Also how come your example is missing the device part?

Wait, why do you have an an device below but ... not here?
Why not group them together?

That is why not have 'short-name' ,'long-name' and 'name' in
the same directory?

> + *
> + *----------------------------- Device settings --------------------------------
> + * name
> + *      Values:         <char[80]>
> + *
> + *      Name of the sound device within the virtual sound card. Optional.
> + *
> + * For example,
> + * /local/domain/5/device/vsnd/0/device/0/name = "General analog"

The example you used at start had a device value of 2. It may
be better to use that here, so :

/local/domain/0/backend/vsnd/5/2/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/device/0/stream/0/type = "p"
> + *      /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"

OK, but again:
/local/domain/0/backend/vsnd/5/2/0/stream/0/type = "p"
/local/domain/0/backend/vsnd/5/2/0/stream/1/type = "c"

Do you want also an 'feature-version' to identify what version
the backend exposes?


> + *
> + *****************************************************************************
> + *                            Frontend XenBus Nodes
> + *****************************************************************************
> + *
> + *----------------------- Request Transport Parameters -----------------------
> + *
> + * These are per stream.

.. which means they are also per device right?

So one can have something like this:

/local/domain/5/device/vsnd/2/stream/0/event-channel
/local/domain/5/device/vsnd/0/stream/0/event-channel
/local/domain/5/device/vsnd/2/stream/1/event-channel
/local/domain/5/device/vsnd/0/stream/0/event-channel

> + *
> + * event-channel
> + *      Values:         <uint>

uint32_t
> + *
> + *      The identifier of the Xen event channel used to signal activity
> + *      in the ring buffer.
> + *
> + * ring-ref
> + *      Values:         <uint>

uint32_t
> + *
> + *      The Xen grant reference granting permission for the backend to map
> + *      a sole page in a single page sized ring buffer.
> + *
> + * index

Why not 'unique-id' ?
> + *      Values:         <uint>

uint32_t

> + *
> + *      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. This is not equal to stream_idx as the later is

s/stream_idx/steam-idx/

> + *      zero based within a device, but this index is contiguous within the

contingous
> + *      driver.


> + */
> +
> +/*
> + * 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
> + *
> + *                                      XenbusStateUnknown
> + *                                      XenbusStateClosed
> + *                                      XenbusStateClosing
> + * o Remove virtual sound device
> + * o Remove event channels
> + *              |
> + *              |
> + *              V
> + * XenbusStateClosed
> + *
> + */
> +
> +/*
> + * 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
> +
> +/*
> + * 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_DEVICE              "device"
> +#define XENSND_PATH_STREAM              "stream"
> +/* Field names */
> +#define XENSND_FIELD_VCARD_SHORT_NAME   "short-name"
> +#define XENSND_FIELD_VCARD_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


Hm, but it is uint32_t which can be much much higher than 6.

4294967295

11 characters?

> +/* Sample format field values */
> +#define XENSND_SAMPLE_FORMAT_MAX_LEN    24

You sure? You may want to make that clear in 'sample-format'
section that the maximum of a string can be 24 characters.


And also explain why 24 characters.
> +
> +#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"
> +
> +/*
> + * STATUS RETURN CODES.
> + */
> +/* Operation not supported. */
> +#define XENSND_RSP_NOTSUPP              (-2)
> +/* Operation failed for some unspecified reason (e. g. -EIO). */

Is that the -EIO going to be exposed somewhere? Inside of the 
'struct xensnd_resp' ?

Would it make sense to have it exposed there? Is that what 'status'
is for?

Why not have an 'err' and mandate that it use POSIX return values?


> +#define XENSND_RSP_ERROR                (-1)
> +/* Operation completed successfully. */
> +#define XENSND_RSP_OKAY                 0
> +
> +/*
> + * Assumptions:
> + *   o usage of grant reference 0 as invalid grant reference:
> + *     grant reference 0 is valid, but never exposed to a PV driver,
> + *     because of the fact it is already in use/reserved by the PV console.
> + *   o all references in this document to page sizes must be treated
> + *     as pages of size XEN_PAGE_SIZE unless  otherwise noted.
                                                ^- extra space

> + *
> + * Description of the protocol between frontend and backend driver.

Is this suppose to have an:
-----------------------------------------
underneath it?

> + *
> + * The two halves of a Para-virtual sound driver communicate with
> + * each other using a shared page and an event channel.

shared pages and event channels? 

Since it looks like  you can have

/local/domain/<frontend-id>/device/vsnd/<device-id>/stream/<stream-id>/event-channel

And you can have N device-id and M stream-id ?

> + * Shared page contains a ring with request/response packets.
> + *
> + * All reserved fields in the structures below must be 0.
> + *
> + * All request packets have the same length (32 octets)

Which implies you can have at maximum 64 requests?

[64 bytes for the four RING_IDX along with the 48 of padding, that
means 4032 left, but since we need this to be modulo 2 the best
we can do is 2^6. 


> + * All request packets have common header:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |

Why the stream_idx ? Your 'ring-ref' is rooted from the '<stream-idx>' so
the frontend and backend already know this.

> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                reserved                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *   id - uint16_t, private guest value, echoed in response
> + *   operation - uint8_t, operation code
> + *   stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> + *     of the stream)

Why do you need this duplicate information?

> + *
> + *
> + * Request open - open a PCM stream for playback or capture:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                reserved                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                pcm_rate                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |  pcm_format     |  pcm_channels   |             reserved              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               buffer_sz                               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                         gref_directory_start                          |

Perhaps 'gref_list' ?

> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+


> + *
> + * 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

.. And I presume this should be between 'channels-min' and 'channels-max'?
Is it OK to have to say 255 ? What are the boundary values?


> + * buffer_sz - uint32_t, buffer size to be allocated in octets

Is there an maximum? Or a minimum? 

Why have it in bytes?  Why not in the amount of grants you need?
Perhaps call it 'gref_nr' ? That way your computation on how
many grants you need is well simplified.


> + * gref_directory_start - grant_ref_t, a reference to the first shared page

s/gref_directory_start/gref_list/ ?
> + *   describing shared buffer references. At least one page exists. If shared
> + *   buffer size exceeds what can be addressed by this single page, then

s/shared buffer size/buffer_sz/ ?

> + *   reference to the next page must be supplied (see gref_dir_next_page below)

Now what if grefs_nr (or buffer_sz) is say 1 page (4096).
Does that mean that gref_directory_start still needs to point to page
which only has two entries: 0, <grant ref> ?

Or can it be simplified and this gref_directory_start would be used for 
data instead?

I think that is what you saying ("If shared .. " which would imply
that if "if !shared" then something else can be done?) but the start
says: "a reference to the first shared page describind shared buffer
references" which implies you do
need this extra indirect page even if the buffer_sz is say 4096.
?



> + */
> +
> +struct xensnd_open_req {

s/_open_req/_req_open/

> +    uint32_t pcm_rate; /* in Hz */
> +    uint8_t pcm_format;
> +    uint8_t pcm_channels;
> +    uint16_t reserved;
> +    uint32_t buffer_sz;
> +    grant_ref_t gref_directory_start;
> +};
> +
> +/*
> + * 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                           |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[0]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[i]                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                gref[N -1]                             |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
> + *   page directory. Must be 0 if no more pages in the list.

s/no more page/there are no more/

> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
> + *   allocated at XENSND_OP_OPEN
> + *
> + * Number of grant_ref_t entries in the whole page directory is not
> + * passed, but instead can be calculated as:
> + *   num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) /
> + *       XEN_PAGE_SIZE


And what are the expected errors? Woudl it make sense to define those?
Say:

 Returns:
 
 -ENOBUFS: Cannot map that many buffers.
 -EINVAL: Incorrect values in the requst?

 -
> + */
> +
> +struct xensnd_page_directory {
> +    grant_ref_t gref_dir_next_page;
> +    grant_ref_t gref[1]; /* Variable length */
> +};
> +
> +/*
> + *  Request close - close an opened pcm stream:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                | XENSND_OP_CLOSE |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + */
> +
> +/*
> + * Request read/write - used for read (for capture) or write (for playback):

Maybe say also that XENSND_OP_OPEN MUST be called before these operations
are permitted.

> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                offset                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                                length                                 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
> + * offset - uint32_t, read or write data offset within the shared buffer
> + *   passed with XENSND_OP_OPEN request

May also want to say that this offset must be less than buffer_sz.

In octets? May want to say that explicitly.


> + * length - uint32_t, read or write data length

In octets?

> + */
> +
> +struct xensnd_rw_req {
> +    uint32_t offset;
> +    uint32_t len;
> +};
> +
> +/*
> + * Request set/get volume - set/get channels' volume of the stream given:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * operation - XENSND_OP_SET_VOLUME for volume set
> + *   or XENSND_OP_GET_VOLUME for volume get
> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
> + * values:

Oh. That means you these operation are in effect 'barrier' ones.

As the buffer must be flushed before hand otherwise you would be
overwriting data stream information.

You should probably mention this semantic need?

Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
a similar structure to 'struct xensnd_rw_req' so that you have
the offset and len?

> + *
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               channel[0]                              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               channel[i]                              |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * channel[i] - sint32_t, volume of i-th channel

You may want to say that i is uint8_t.


> + * Volume is expressed as a signed value in steps of 0.001 dB,
> + * while 0 being 0 dB.
> + */
> +
> +/*
> + * Request mute/unmute - mute/unmute stream:

> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                               reserved                                |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> + * values:

Again, same comments as above.
> + *
> + *                                     0                                   octet
> + * +-----------------------------------------------------------------------+
> + * |                               channel[0]                              |
> + * +-----------------------------------------------------------------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------------------------------------------------------------+
> + * |                               channel[i]                              |
> + * +-----------------------------------------------------------------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------------------------------------------------------------+
> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
> + * +-----------------+-----------------+-----------------+-----------------+
> + *
> + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
> + */
> +
> +/*
> + * All response packets have the same length (32 octets)
> + *
> + * 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_* - copied from request
> + * status - int8_t, response status (XENSND_RSP_*)

Could this become an 'err' and the POSIX values used for errors?

> + */
> +
> +struct xensnd_req {
> +    uint16_t id;
> +    uint8_t operation;
> +    uint8_t stream_idx;
> +    uint32_t reserved;
> +    union {
> +        struct xensnd_open_req open;
> +        struct xensnd_rw_req rw;
> +        uint8_t reserved[24];
> +    } op;
> +};
> +
> +struct xensnd_resp {
> +    uint16_t id;
> +    uint8_t operation;
> +    uint8_t stream_idx;
> +    int8_t status;
> +    uint8_t reserved[27];
> +};
> +
> +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req, struct xensnd_resp);
> +
> +#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */
> -- 
> 2.7.4
> 

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

Hi, Konrad!

First of all thank you very much for the valuable comments

and your time!

The number of changes (mostly in description) is going to

be huge, so do you think I can publish something like

"RFC v16" so we can discuss the updated patch?

Thank you,

Oleksandr


On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
> Usually one also puts somethign in the commit description.
>
> When I applied this to me tree I got:
>
> [konrad@char xen]$ git log --oneline HEAD^..
> ecc7711 This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.
> [konrad@char xen]$
fixed
> Also, thank you for the including the pahole output!
np
>> Signed-off-by: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
>> Signed-off-by: Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
>>
>> ---
>> Changes since v1:
>>   * removed __attribute__((__packed__)) from all structures definitions
>>
>> 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
>>
>> Changes since v8:
>>   * changed frontend-id to frontend_id
>>   * single sound card support, configured with bunch of
>>     devices/streams
>>   * clarifucation made on sample rates and formats expressed as
>>     decimals w/o any particular ordering
>>   * put description of migration/disconnection state
>>   * replaced __attribute__((packed)) to __packed
>>   * changed padding of ring structures to 64 to fit cache line
>>   * removeed #ifdef __KERNEL
>>   * explicitly stated which indices in XenStore configuration
>>     are contiguous
>>   * added description to what frontend's defaults are
>>   * made names of virtual card/devices optional
>>   * removed PCM_FORMAT_SPECIAL
>>   * changed volume units from dBm to dB
>>
>> Changes since v9:
>>   * removed sndif_linux.h
>>   * moved all structures from sndif_linux.h to sndif.h
>>   * structures padded where needed
>>   * fixed Hz comment
>>
>> Changes since v10:
>>   * fixed tabs to 4 spaces to comply with Xen coding style
>>   * added placeholders to empty structures (C89 concern)
>>   * added missing header includes
>>
>> Changes since v11:
>>   * added XENSND_RSP_NOTSUPP error code
>>   * changed gref[0] to gref[1] with comment
>>   * modified comments on empty structures
>>   * removed "__" from member names
>>   * fixed indentation
>>   * added padding in union xensnd_resp
>>   * changed __XEN_PUBLIC_IO_XENSND_H__ to __XEN_PUBLIC_IO_SNDIF_H__
>>
>> Changes since v12:
>>   * changed indentation for defines
>>   * missed ";" after gref[1]
>>   * documentation changes
>>   * changed req/resp structures
>>   * changed xensnd_page_directory structure
>>   * pass buffer size in open request
>>
>> Changes since v13:
>>   * note on usage of grant ref 0
>>   * all page sizes are XEN_PAGE_SIZE
>>   * padding/reserved cleanup/fixes
>>   * removed empty structures
>>
>> Changes since v14:
>>   * turn padding into reserved
>> ---
>> ---
>>   xen/include/public/io/sndif.h | 671 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 671 insertions(+)
>>   create mode 100644 xen/include/public/io/sndif.h
>>
>> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
>> new file mode 100644
>> index 0000000..60a6d88
>> --- /dev/null
>> +++ b/xen/include/public/io/sndif.h
>> @@ -0,0 +1,671 @@
>> +/******************************************************************************
>> + * 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.
>> + *
>> + * Authors: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
>> + *          Oleksandr Grytsov <Oleksandr_Grytsov@epam.com>
>> + *          Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> + *          Iurii Konovalenko <iurii.konovalenko@globallogic.com>
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_IO_SNDIF_H__
>> +#define __XEN_PUBLIC_IO_SNDIF_H__
>> +
>> +#include "ring.h"
>> +#include "../grant_table.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
> The rest of the drivers use - instead of _.
>
> Is there a particular reason you need to have _ ?
no reason
>
> See vscsiif.h and console and vkbd. Actually the last ones
> don't mention it in the header but the libxl constructs these.
Indeed, I will remove this section
>> + *      Values:         <uint>
> uint32_t
>> + *
>> + *      Domain ID of the sound frontend.
>> + *
>> + * drv_idx
> Again, can this be - ?
>> + *      Values:         <uint>
> uint32_t
>> + *
>> + *      Zero based contiguous index of the virtualized sound driver instance in
>> + *      this domain. Multiple PV drivers are allowed in the domain
>> + *      at the same time.
> This is not needed.
>
> We already have this information in the XenStore directory:
>
> /local/domain/0/backend/vsnd/2/0/frontend-id = 2
> /local/domain/0/backend/vsnd/2/1/frontend-id = 2
> ..
> /local/domain/0/backend/vsnd/2/N/frontend-id = 2
>
> (which would be similar to what 'vbd', 'console' and 'vif' do).
Removed
>
>> + *
>> + * dev_id
> s/_/-/
done
>> + *      Values:         <uint>
> uint_32t
>> + *
>> + *      Unique device ID.
>> + *      Doesn't have to be zero based and/or to be contiguous.
> Is uint enough? Or would it be better to have an <string> if this is
> some form of SHA-1 value?
changed to uint8_t
>> + *
>> + * stream_idx
> s/_/-/
done
>> + *      Values:         <uint>
> uint_32t
>
> [edit, based on the struct xensnd_req  this has to be uint_8t] ?
changed to uint8_t
>> + *
>> + *      Zero based contiguous 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), device id 2,
>> + * first stream (0):
>> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
>> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
> Why do you need 'device' ?
just for clarity: the hierarchy of the sound driver would
be that a device may have multiple different streams.
So, from readability POV I would still have "device" in place
 From xenstore documentation: "Data should generally be
human-readable for ease of management and debugging "
I assume this also applies to the structure as well
>
> Could not this be:
>
> /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
then one has to know that "2" stands for device.
see above, I would keep "device" here
>
>
>> + *
>> + *------------------------------- PCM settings --------------------------------
>> + *
>> + * Every virtualized sound frontend has set of devices and streams, each
> frontend or backend?
>
> I would think backend since this is still the backend section?
you are right, moved to frontend's section
>
>> + * 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 driver, device or stream.
>> + * Every underlying layer in turn can re-define some or all of them to better
>> + * fit its needs. For example, driver 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.
> OK, so these are more of 'global ones'?
kind of, what is more they can be "global" to all the
underlying levels, e.g. if device changes some of the
parameters they become adopted by streams if not
explicitly configured. The same is true if device changes
parameters set at the card level.
This way one can easily tailor particular pieces of the
system w/o writing all settings again
>> + *
>> + * Note: if any of the values are not defined then PV driver should use
>> + * its default values instead.
>
> How is that suppose to work? What if the PV driver by default wants
> to use say 314156 channels and the backend does not expose any? Can
> it use that? I presume it could try.. and it would fail but that
> hardly seems nice.
>
I will probably remove this sentence and the below at all.
If not defined this could mean an error state
>> + *
>> + * channels-min
>> + *      Values:         <uint>
> uint32_t
>> + *
>> + *      The minimum amount of channels that is supported.
>> + *      Must be at least 1. If not defined then use frontend's default.
> This is odd. This whole section is for the backend, so how can the backend
> not have this defined?
>
> Do you mean to say that this parameter is optional? Perhaps
> then say right before 'The minimum' include '(optional)'
>
> Anyhow where do you envision this optional global value to be?
>
> /local/domain/0/backend/vsnd/5/channels-[min|max] ?
>
> Perhaps mention that in the start of this section:
>
> These global values are under the backend 'vsnd' directory, as
> so:
>
> /local/domain/0/backends/vsnd/<front-id>/channels-[min|max]
>
> Thought I think there is probably a better way to
> say this..
well, the confusion comes from the fact that I have
described all this under backend's section
now when I move it to the frontend's section it should
make more sense
>> + *
>> + * channels-max
>> + *      Values:         <uint>
> uint32_t
uint8_t
>> + *
>> + *      The maximum amount of channels that is supported.
>> + *      Must be at least <channels-min>. If not defined then use frontend's
>> + *      default.
> I am still having trouble understanding how the backend is suppose
> to use the frontend's default.
>
> Perhaps you want to say:
>
> If this value is not exposed by the backend the frontend
> is permitted to use its default values.
>
> ?
moved to frontend's section
>> + *
>> + * sample-rates
>> + *      Values:         <list of uints>
> .. of uint32_t
done
>> + *
>> + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
>> + *      If not defined then use frontend's default. Sample rates are expressed
> Ahain this 'If not defined then use fronten'ds default' could be written
> a bit differently.
moved to frontend's section
>> + *      as a list of decimal values w/o any ordering requirement.
>> + *
>> + * 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.
> Again, this does not sound right. The backend has no clue what
> the frontend defaults are.
moved to frontend's section
>> + *
>> + * buffer-size
>> + *      Values:         <uint>
> uint32_t
done
>> + *
>> + *      The maximum size in octets of the buffer to allocate per stream.
>> + *
>> + * Example configuration:
>> + *
>> + * Driver configuration used by all streams:
>> + * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"
> Huh? But this is a backend value? Don't you mean:
>
> /local/domain/0/backend/vsnd/5/sample-formats = "s8;u8;s16_le;s16_be"
> ?
moved to frontend's section
>> + * Stream overrides sample rates supported:
> -EPARSE
done
>
>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates =
>> + *        "8000;22050;44100;48000"
> /local/domain/0/backend/vsnd/5/2/stream/0/sample-rates ="8000;22050;44100;48000"
>
> But that is wrong as this section is about global values so they
> would be more like:
>
> /local/domain/0/backend/vsnd/5/sample-rates ="8000;22050;44100;48000"
>
> ?
not anymore, as I move these to frontend's configuration
>> + *
>> + *----------------------- Virtual sound card settings --------------------------
>> + * short-name
>> + *      Values:         <char[32]>
>> + *
>> + *      Short name of the virtual sound card. Optional.
>> + *
>> + * long-name
>> + *      Values:         <char[80]>
>> + *
>> + *      Long name of the virtual sound card. Optional.
>> + *
>> + * For example,
>> + * /local/domain/5/device/vsnd/0/short-name = "Virtual audio"
>> + * /local/domain/5/device/vsnd/0/long-name =
>> + *         "Virtual audio at center stack"
> This should be inverted. That is this is on the backend
> side:
>
> /local/domain/0/backend/vsnd/5/0/2/short-name = "Virtual audio"
>
> And so on.
>
> Also how come your example is missing the device part?
I will put a complete configuration example
> Wait, why do you have an an device below but ... not here?
> Why not group them together?
>
> That is why not have 'short-name' ,'long-name' and 'name' in
> the same directory?
name is for a stream; short/long for the card
>
>> + *
>> + *----------------------------- Device settings --------------------------------
>> + * name
>> + *      Values:         <char[80]>
>> + *
>> + *      Name of the sound device within the virtual sound card. Optional.
>> + *
>> + * For example,
>> + * /local/domain/5/device/vsnd/0/device/0/name = "General analog"
> The example you used at start had a device value of 2. It may
> be better to use that here, so :
>
> /local/domain/0/backend/vsnd/5/2/name = "General analog"
done
>> + *
>> + *----------------------------- 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/device/0/stream/0/type = "p"
>> + *      /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"
> OK, but again:
> /local/domain/0/backend/vsnd/5/2/0/stream/0/type = "p"
> /local/domain/0/backend/vsnd/5/2/0/stream/1/type = "c"
>
> Do you want also an 'feature-version' to identify what version
> the backend exposes?
>
will add that
>> + *
>> + *****************************************************************************
>> + *                            Frontend XenBus Nodes
>> + *****************************************************************************
>> + *
>> + *----------------------- Request Transport Parameters -----------------------
>> + *
>> + * These are per stream.
> .. which means they are also per device right?
>
> So one can have something like this:
>
> /local/domain/5/device/vsnd/2/stream/0/event-channel
> /local/domain/5/device/vsnd/0/stream/0/event-channel
> /local/domain/5/device/vsnd/2/stream/1/event-channel
> /local/domain/5/device/vsnd/0/stream/0/event-channel
yes, every stream has its own communication channel
>> + *
>> + * event-channel
>> + *      Values:         <uint>
> uint32_t
done
>> + *
>> + *      The identifier of the Xen event channel used to signal activity
>> + *      in the ring buffer.
>> + *
>> + * ring-ref
>> + *      Values:         <uint>
> uint32_t
done
>> + *
>> + *      The Xen grant reference granting permission for the backend to map
>> + *      a sole page in a single page sized ring buffer.
>> + *
>> + * index
> Why not 'unique-id' ?
makes sense, will change
>> + *      Values:         <uint>
> uint32_t
done
>> + *
>> + *      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. This is not equal to stream_idx as the later is
> s/stream_idx/steam-idx/
done
>> + *      zero based within a device, but this index is contiguous within the
> contingous
done
>> + *      driver.
>
>> + */
>> +
>> +/*
>> + * 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
>> + *
>> + *                                      XenbusStateUnknown
>> + *                                      XenbusStateClosed
>> + *                                      XenbusStateClosing
>> + * o Remove virtual sound device
>> + * o Remove event channels
>> + *              |
>> + *              |
>> + *              V
>> + * XenbusStateClosed
>> + *
>> + */
>> +
>> +/*
>> + * 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
>> +
>> +/*
>> + * 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_DEVICE              "device"
>> +#define XENSND_PATH_STREAM              "stream"
>> +/* Field names */
>> +#define XENSND_FIELD_VCARD_SHORT_NAME   "short-name"
>> +#define XENSND_FIELD_VCARD_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
>
> Hm, but it is uint32_t which can be much much higher than 6.
>
> 4294967295
>
> 11 characters?
fixed
>> +/* Sample format field values */
>> +#define XENSND_SAMPLE_FORMAT_MAX_LEN    24
> You sure? You may want to make that clear in 'sample-format'
> section that the maximum of a string can be 24 characters.
done
>
> And also explain why 24 characters.
no particular reason, just to fit XENSND_PCM_FORMAT_???_STR
do you want me to change it to something else?
>> +
>> +#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"
>> +
>> +/*
>> + * STATUS RETURN CODES.
>> + */
>> +/* Operation not supported. */
>> +#define XENSND_RSP_NOTSUPP              (-2)
>> +/* Operation failed for some unspecified reason (e. g. -EIO). */
> Is that the -EIO going to be exposed somewhere?
no
>   Inside of the
> 'struct xensnd_resp' ?
no, XENSND_RSP_??? is used there
>
> Would it make sense to have it exposed there? Is that what 'status'
> is for?
see above, XENSND_RSP_???
>
> Why not have an 'err' and mandate that it use POSIX return values?
well, if we agree on POSIX values then I am more than ok
>
>> +#define XENSND_RSP_ERROR                (-1)
>> +/* Operation completed successfully. */
>> +#define XENSND_RSP_OKAY                 0
>> +
>> +/*
>> + * Assumptions:
>> + *   o usage of grant reference 0 as invalid grant reference:
>> + *     grant reference 0 is valid, but never exposed to a PV driver,
>> + *     because of the fact it is already in use/reserved by the PV console.
>> + *   o all references in this document to page sizes must be treated
>> + *     as pages of size XEN_PAGE_SIZE unless  otherwise noted.
>                                                  ^- extra space
fixed
>
>> + *
>> + * Description of the protocol between frontend and backend driver.
> Is this suppose to have an:
> -----------------------------------------
> underneath it?
done
>> + *
>> + * The two halves of a Para-virtual sound driver communicate with
>> + * each other using a shared page and an event channel.
> shared pages and event channels?
>
> Since it looks like  you can have
>
> /local/domain/<frontend-id>/device/vsnd/<device-id>/stream/<stream-id>/event-channel
>
> And you can have N device-id and M stream-id ?
right, done
>> + * Shared page contains a ring with request/response packets.
>> + *
>> + * All reserved fields in the structures below must be 0.
>> + *
>> + * All request packets have the same length (32 octets)
> Which implies you can have at maximum 64 requests?
>
> [64 bytes for the four RING_IDX along with the 48 of padding, that
> means 4032 left, but since we need this to be modulo 2 the best
> we can do is 2^6.
you mean we have to pad the structures so they are all
64 bytes long?
>
>
>> + * All request packets have common header:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
> Why the stream_idx ? Your 'ring-ref' is rooted from the '<stream-idx>' so
> the frontend and backend already know this.
indeed, we can probably remove this from all the structures
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                reserved                               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *   id - uint16_t, private guest value, echoed in response
>> + *   operation - uint8_t, operation code
>> + *   stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *     of the stream)
> Why do you need this duplicate information?
it describes common header and its values
>
>> + *
>> + *
>> + * Request open - open a PCM stream for playback or capture:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                reserved                               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                pcm_rate                               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |  pcm_format     |  pcm_channels   |             reserved              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               buffer_sz                               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                         gref_directory_start                          |
> Perhaps 'gref_list' ?
not sure, it is the start of the page directory, its gref
>
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>
>> + *
>> + * 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
> .. And I presume this should be between 'channels-min' and 'channels-max'?
> Is it OK to have to say 255 ? What are the boundary values?
>
yes, it is ok to be uint8_t, IMO. I will change the
xenstore values description as well
>> + * buffer_sz - uint32_t, buffer size to be allocated in octets
> Is there an maximum? Or a minimum?
>
> Why have it in bytes?  Why not in the amount of grants you need?
> Perhaps call it 'gref_nr' ? That way your computation on how
> many grants you need is well simplified.
there was a discussion on that: 
https://marc.info/?l=xen-devel&m=148008589320533&w=2
>
>
>> + * gref_directory_start - grant_ref_t, a reference to the first shared page
> s/gref_directory_start/gref_list/ ?
not sure
>> + *   describing shared buffer references. At least one page exists. If shared
>> + *   buffer size exceeds what can be addressed by this single page, then
> s/shared buffer size/buffer_sz/ ?
why, I am not using variable/field name here, but explaining
I can put the name in brackets)
>
>> + *   reference to the next page must be supplied (see gref_dir_next_page below)
> Now what if grefs_nr (or buffer_sz) is say 1 page (4096).
> Does that mean that gref_directory_start still needs to point to page
> which only has two entries: 0, <grant ref> ?
>
> Or can it be simplified and this gref_directory_start would be used for
> data instead?
>
> I think that is what you saying ("If shared .. " which would imply
> that if "if !shared" then something else can be done?) but the start
> says: "a reference to the first shared page describind shared buffer
> references" which implies you do
> need this extra indirect page even if the buffer_sz is say 4096.
> ?
yes, you got it right. the use-cases we have do use buffers
bigger then 4K, so this is why we never thought of such an overhead
>
>
>> + */
>> +
>> +struct xensnd_open_req {
> s/_open_req/_req_open/
it will be yet another flame here...
will keep as is
>> +    uint32_t pcm_rate; /* in Hz */
>> +    uint8_t pcm_format;
>> +    uint8_t pcm_channels;
>> +    uint16_t reserved;
>> +    uint32_t buffer_sz;
>> +    grant_ref_t gref_directory_start;
>> +};
>> +
>> +/*
>> + * 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                           |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[0]                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[i]                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                gref[N -1]                             |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
>> + *   page directory. Must be 0 if no more pages in the list.
> s/no more page/there are no more/
done
>
>> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
>> + *   allocated at XENSND_OP_OPEN
>> + *
>> + * Number of grant_ref_t entries in the whole page directory is not
>> + * passed, but instead can be calculated as:
>> + *   num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) /
>> + *       XEN_PAGE_SIZE
>
> And what are the expected errors? Woudl it make sense to define those?
> Say:
>
>   Returns:
>   
>   -ENOBUFS: Cannot map that many buffers.
>   -EINVAL: Incorrect values in the requst?
ATM, the XENSND_RSP_??? are returned
>   -
>> + */
>> +
>> +struct xensnd_page_directory {
>> +    grant_ref_t gref_dir_next_page;
>> +    grant_ref_t gref[1]; /* Variable length */
>> +};
>> +
>> +/*
>> + *  Request close - close an opened pcm stream:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                | XENSND_OP_CLOSE |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + */
>> +
>> +/*
>> + * Request read/write - used for read (for capture) or write (for playback):
> Maybe say also that XENSND_OP_OPEN MUST be called before these operations
> are permitted.
not sure we need this: normally you have to open something
before you can use it, e.g. a file
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                offset                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                                length                                 |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
>> + * offset - uint32_t, read or write data offset within the shared buffer
>> + *   passed with XENSND_OP_OPEN request
> May also want to say that this offset must be less than buffer_sz.
>
> In octets? May want to say that explicitly.
done
>
>> + * length - uint32_t, read or write data length
> In octets?
done
>> + */
>> +
>> +struct xensnd_rw_req {
>> +    uint32_t offset;
>> +    uint32_t len;
>> +};
>> +
>> +/*
>> + * Request set/get volume - set/get channels' volume of the stream given:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * operation - XENSND_OP_SET_VOLUME for volume set
>> + *   or XENSND_OP_GET_VOLUME for volume get
>> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
>> + * values:
> Oh. That means you these operation are in effect 'barrier' ones.
>
> As the buffer must be flushed before hand otherwise you would be
> overwriting data stream information.
>
> You should probably mention this semantic need?
I think this is implementation specific and shouldn't
be in a generic protocol
>
> Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
> a similar structure to 'struct xensnd_rw_req' so that you have
> the offset and len?
a page can hold enough values, IMO
>> + *
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               channel[0]                              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               channel[i]                              |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * channel[i] - sint32_t, volume of i-th channel
> You may want to say that i is uint8_t.
done
>
>> + * Volume is expressed as a signed value in steps of 0.001 dB,
>> + * while 0 being 0 dB.
>> + */
>> +
>> +/*
>> + * Request mute/unmute - mute/unmute stream:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                               reserved                                |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
>> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
>> + * values:
> Again, same comments as above.
>> + *
>> + *                                     0                                   octet
>> + * +-----------------------------------------------------------------------+
>> + * |                               channel[0]                              |
>> + * +-----------------------------------------------------------------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------------------------------------------------------------+
>> + * |                               channel[i]                              |
>> + * +-----------------------------------------------------------------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------------------------------------------------------------+
>> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + *
>> + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
>> + */
>> +
>> +/*
>> + * All response packets have the same length (32 octets)
>> + *
>> + * 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_* - copied from request
>> + * status - int8_t, response status (XENSND_RSP_*)
> Could this become an 'err' and the POSIX values used for errors?
>
we can discuss this, I am fine with this approach if the
Community accepts this
>> + */
>> +
>> +struct xensnd_req {
>> +    uint16_t id;
>> +    uint8_t operation;
>> +    uint8_t stream_idx;
>> +    uint32_t reserved;
>> +    union {
>> +        struct xensnd_open_req open;
>> +        struct xensnd_rw_req rw;
>> +        uint8_t reserved[24];
>> +    } op;
>> +};
>> +
>> +struct xensnd_resp {
>> +    uint16_t id;
>> +    uint8_t operation;
>> +    uint8_t stream_idx;
>> +    int8_t status;
>> +    uint8_t reserved[27];
>> +};
>> +
>> +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req, struct xensnd_resp);
>> +
>> +#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */
>> -- 
>> 2.7.4
>>


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2101 bytes --]

On Thu, 2017-01-26 at 12:02 +0200, Oleksandr Andrushchenko wrote:
> Hi, Konrad!
> 
> First of all thank you very much for the valuable comments
> 
> and your time!
> 
> The number of changes (mostly in description) is going to
> 
> be huge, so do you think I can publish something like
> 
> "RFC v16" so we can discuss the updated patch?
> 
Konrad's call, but why you want to introduce the 'RFC' tag now? I'd
just go with v16...

> On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko
> > wrote:
> > > 
> > > + * Example for the frontend running in domain 5, instance of the
> > > driver
> > > + * in the front is 0 (single or first PV driver), device id 2,
> > > + * first stream (0):
> > > + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> > > + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
> > Why do you need 'device' ?
> just for clarity: the hierarchy of the sound driver would
> be that a device may have multiple different streams.
> So, from readability POV I would still have "device" in place
>  From xenstore documentation: "Data should generally be
> human-readable for ease of management and debugging "
> I assume this also applies to the structure as well
>
Perhaps:

 /local/domain/<frontend_id>/device/vsnd/<drv_idx>/dev-<dev_id>/stream-<stream_idx>/...

> > Could not this be:
> > 
> > /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
> then one has to know that "2" stands for device.
> see above, I would keep "device" here
> > > 

 /local/domain/5/device/vsnd/0/dev-2/stream-0/type = "p"

Or, with no '-':

 /local/domain/5/device/vsnd/0/dev2/stream0/type = "p"

Just my 2 cents...

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 01/26/2017 01:09 PM, Dario Faggioli wrote:
> On Thu, 2017-01-26 at 12:02 +0200, Oleksandr Andrushchenko wrote:
>> Hi, Konrad!
>>
>> First of all thank you very much for the valuable comments
>>
>> and your time!
>>
>> The number of changes (mostly in description) is going to
>>
>> be huge, so do you think I can publish something like
>>
>> "RFC v16" so we can discuss the updated patch?
>>
> Konrad's call, but why you want to introduce the 'RFC' tag now? I'd
> just go with v16...
no particular reason, will use v16 )
>
>> On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
>>> On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko
>>> wrote:
>>>>   
>>>> + * Example for the frontend running in domain 5, instance of the
>>>> driver
>>>> + * in the front is 0 (single or first PV driver), device id 2,
>>>> + * first stream (0):
>>>> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
>>>> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
>>>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
>>> Why do you need 'device' ?
>> just for clarity: the hierarchy of the sound driver would
>> be that a device may have multiple different streams.
>> So, from readability POV I would still have "device" in place
>>   From xenstore documentation: "Data should generally be
>> human-readable for ease of management and debugging "
>> I assume this also applies to the structure as well
>>
> Perhaps:
>
>   /local/domain/<frontend_id>/device/vsnd/<drv_idx>/dev-<dev_id>/stream-<stream_idx>/...
>
>>> Could not this be:
>>>
>>> /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
>> then one has to know that "2" stands for device.
>> see above, I would keep "device" here
>   /local/domain/5/device/vsnd/0/dev-2/stream-0/type = "p"
>
> Or, with no '-':
>
>   /local/domain/5/device/vsnd/0/dev2/stream0/type = "p"
>
> Just my 2 cents...
1. Well, the only reason I have "device" here is for clarity
and consistency: sound card owns PCM devices, PCM device owns
streams
We could probably have "pcm-dev" instead of "device" here,
so we do not collide with xen device.
2. "dev-%d" or "dev%d", "stream-%d" or "stream%d"
IMO, we already have indices employed in xenstore,
e.g. "domain/5", not "domain-5" or "domain5"
So, is the PCM device in question any different from domain
from this POV? To me - not, so this is why I use "device/%d"
> Dario
Thank you,
Oleksandr

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2816 bytes --]

On Thu, 2017-01-26 at 13:23 +0200, Oleksandr Andrushchenko wrote:
> On 01/26/2017 01:09 PM, Dario Faggioli wrote:
> > > On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
> > > > On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr
> > > > Andrushchenko wrote:
> > > > > + * Example for the frontend running in domain 5, instance of
> > > > > the
> > > > > driver
> > > > > + * in the front is 0 (single or first PV driver), device id
> > > > > 2,
> > > > > + * first stream (0):
> > > > > + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> > > > > + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> > > > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/type =
> > > > > "p"
> > > > Why do you need 'device' ?
> > > > Could not this be:
> > > > 
> > > > /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
> > > then one has to know that "2" stands for device.
> > > see above, I would keep "device" here
> >   /local/domain/5/device/vsnd/0/dev-2/stream-0/type = "p"
> > 
> > Or, with no '-':
> > 
> >   /local/domain/5/device/vsnd/0/dev2/stream0/type = "p"
> > 
> > Just my 2 cents...
> 1. Well, the only reason I have "device" here is for clarity
> and consistency: sound card owns PCM devices, PCM device owns
> streams
> We could probably have "pcm-dev" instead of "device" here,
> so we do not collide with xen device.
>
Sure. Or maybe even just 'pcm' (matter of taste, to large extent).

> 2. "dev-%d" or "dev%d", "stream-%d" or "stream%d"
> IMO, we already have indices employed in xenstore,
> e.g. "domain/5", not "domain-5" or "domain5"
> So, is the PCM device in question any different from domain
> from this POV? To me - not, so this is why I use "device/%d"
> 
True. Well, actually, have both. For instance, blkif, when multiqueue
is available are enabled, looks like this:

 /local/domain/1/device/vbd/0/multi-queue-num-queues = "2"
 /local/domain/1/device/vbd/0/queue-0 = ""
 /local/domain/1/device/vbd/0/queue-0/ring-ref = "<ring-ref#0>"
 /local/domain/1/device/vbd/0/queue-0/event-channel = "<evtchn#0>"
 /local/domain/1/device/vbd/0/queue-1 = ""
 /local/domain/1/device/vbd/0/queue-1/ring-ref = "<ring-ref#1>"
 /local/domain/1/device/vbd/0/queue-1/event-channel = "<evtchn#1>"

So, while I after all thing I agree with you on point 1) (i.e., on
having device, or pcm-dev, or pcm, the latter being my prefernce), I
think it would be ok to manage streams like blkif manages queues, and
hence using stream-0, stream-1, etc.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 01/26/2017 01:54 PM, Dario Faggioli wrote:
> On Thu, 2017-01-26 at 13:23 +0200, Oleksandr Andrushchenko wrote:
>> On 01/26/2017 01:09 PM, Dario Faggioli wrote:
>>>> On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
>>>>> On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr
>>>>> Andrushchenko wrote:
>>>>>> + * Example for the frontend running in domain 5, instance of
>>>>>> the
>>>>>> driver
>>>>>> + * in the front is 0 (single or first PV driver), device id
>>>>>> 2,
>>>>>> + * first stream (0):
>>>>>> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
>>>>>> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
>>>>>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type =
>>>>>> "p"
>>>>> Why do you need 'device' ?
>>>>> Could not this be:
>>>>>
>>>>> /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
>>>> then one has to know that "2" stands for device.
>>>> see above, I would keep "device" here
>>>    /local/domain/5/device/vsnd/0/dev-2/stream-0/type = "p"
>>>
>>> Or, with no '-':
>>>
>>>    /local/domain/5/device/vsnd/0/dev2/stream0/type = "p"
>>>
>>> Just my 2 cents...
>> 1. Well, the only reason I have "device" here is for clarity
>> and consistency: sound card owns PCM devices, PCM device owns
>> streams
>> We could probably have "pcm-dev" instead of "device" here,
>> so we do not collide with xen device.
>>
> Sure. Or maybe even just 'pcm' (matter of taste, to large extent).
I would stick to "pcm-dev" then
>> 2. "dev-%d" or "dev%d", "stream-%d" or "stream%d"
>> IMO, we already have indices employed in xenstore,
>> e.g. "domain/5", not "domain-5" or "domain5"
>> So, is the PCM device in question any different from domain
>> from this POV? To me - not, so this is why I use "device/%d"
>>
> True. Well, actually, have both. For instance, blkif, when multiqueue
> is available are enabled, looks like this:
>
>   /local/domain/1/device/vbd/0/multi-queue-num-queues = "2"
>   /local/domain/1/device/vbd/0/queue-0 = ""
>   /local/domain/1/device/vbd/0/queue-0/ring-ref = "<ring-ref#0>"
>   /local/domain/1/device/vbd/0/queue-0/event-channel = "<evtchn#0>"
>   /local/domain/1/device/vbd/0/queue-1 = ""
>   /local/domain/1/device/vbd/0/queue-1/ring-ref = "<ring-ref#1>"
>   /local/domain/1/device/vbd/0/queue-1/event-channel = "<evtchn#1>"
Yeap, I saw this and was in doubt
> So, while I after all thing I agree with you on point 1) (i.e., on
> having device, or pcm-dev, or pcm, the latter being my prefernce), I
> think it would be ok to manage streams like blkif manages queues, and
> hence using stream-0, stream-1, etc.
Ok, then we could have a formal rule for this: the last
enumeration should follow "XXX-%d" format, e.g. "queue-%d",
"stream-%d" etc. But entries, before this enum should follow
"YYY/%d" format.
>
> Regards,
> Dario


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Thu, Jan 26, 2017 at 12:09:30PM +0100, Dario Faggioli wrote:
> On Thu, 2017-01-26 at 12:02 +0200, Oleksandr Andrushchenko wrote:
> > Hi, Konrad!
> > 
> > First of all thank you very much for the valuable comments
> > 
> > and your time!
> > 
> > The number of changes (mostly in description) is going to
> > 
> > be huge, so do you think I can publish something like
> > 
> > "RFC v16" so we can discuss the updated patch?
> > 
> Konrad's call, but why you want to introduce the 'RFC' tag now? I'd
> just go with v16...

v16 please.
> 
> > On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko
> > > wrote:
> > > > 
> > > > + * Example for the frontend running in domain 5, instance of the
> > > > driver
> > > > + * in the front is 0 (single or first PV driver), device id 2,
> > > > + * first stream (0):
> > > > + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> > > > + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> > > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
> > > Why do you need 'device' ?
> > just for clarity: the hierarchy of the sound driver would
> > be that a device may have multiple different streams.

That is kind of implied. You have an vsnd and underneath it
you can have multiplie directories with a number. Those
are the device numbers.

Not sure why you would need the 'device'?

Also if you look at both vbd and vif - they follow the same
pattern - if you have multiple devices, it is just the number
(no device).

Hence from an maintaince perspective I would rather you not
have the 'device' in there - as it clashes with the other
ring protocols formats general layout.

> > So, from readability POV I would still have "device" in place
> >  From xenstore documentation: "Data should generally be
> > human-readable for ease of management and debugging "
> > I assume this also applies to the structure as well
> >
> Perhaps:
> 
>  /local/domain/<frontend_id>/device/vsnd/<drv_idx>/dev-<dev_id>/stream-<stream_idx>/...
> 
> > > Could not this be:
> > > 
> > > /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
> > then one has to know that "2" stands for device.
> > see above, I would keep "device" here
> > > > 
> 
>  /local/domain/5/device/vsnd/0/dev-2/stream-0/type = "p"
> 
> Or, with no '-':
> 
>  /local/domain/5/device/vsnd/0/dev2/stream0/type = "p"
> 
> Just my 2 cents...
> 
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Thu, Jan 26, 2017 at 12:02:49PM +0200, Oleksandr Andrushchenko wrote:
> Hi, Konrad!
> 
> First of all thank you very much for the valuable comments
> 
> and your time!
> 
> The number of changes (mostly in description) is going to
> 
> be huge, so do you think I can publish something like
> 
> "RFC v16" so we can discuss the updated patch?

RFC sadly means folks are going to mostly ignore it.
I would prefer you did not use RFC at this stage but just
did v16.
..snip..
> > > + * Example for the frontend running in domain 5, instance of the driver
> > > + * in the front is 0 (single or first PV driver), device id 2,
> > > + * first stream (0):
> > > + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> > > + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
> > Why do you need 'device' ?
> just for clarity: the hierarchy of the sound driver would
> be that a device may have multiple different streams.

And it just occured to me that you could also imply that
each device has an stream without the 'stream' in it.

So

/local/domain/5/device/vsnd/0/2/0/type = "p"

And the format is:
/local/domain/<front-id>/device/vsnd/<instance of PV driver>/<device-id>/<stream-id>

Though I have a little of trouble with the 'instance of the
driver'. Are you suggesting you would have multiple 
PV drivers of 'vsnd'? Can't the multiple device ids fulfill this?

> So, from readability POV I would still have "device" in place
> From xenstore documentation: "Data should generally be
> human-readable for ease of management and debugging "
> I assume this also applies to the structure as well
> > 
> > Could not this be:
> > 
> > /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
> then one has to know that "2" stands for device.
> see above, I would keep "device" here
> > 
> > 
> > > + *
> > > + *------------------------------- PCM settings --------------------------------
> > > + *
> > > + * Every virtualized sound frontend has set of devices and streams, each
> > frontend or backend?
> > 
> > I would think backend since this is still the backend section?
> you are right, moved to frontend's section
> > 
> > > + * 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 driver, device or stream.
> > > + * Every underlying layer in turn can re-define some or all of them to better
> > > + * fit its needs. For example, driver 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.
> > OK, so these are more of 'global ones'?
> kind of, what is more they can be "global" to all the
> underlying levels, e.g. if device changes some of the
> parameters they become adopted by streams if not
> explicitly configured. The same is true if device changes
> parameters set at the card level.
> This way one can easily tailor particular pieces of the
> system w/o writing all settings again

Are they "global" to the "instance of the PV drivers" or all the 
device ids?

I presume the latter which is why you need to have multiple
instances of the PV drivers support?

(You may want to mention this requirement in the section talking
about the format of how the structure is exposed in the backend
and frontend).

> > > + *
> > > + * Note: if any of the values are not defined then PV driver should use
> > > + * its default values instead.
> > 
> > How is that suppose to work? What if the PV driver by default wants
> > to use say 314156 channels and the backend does not expose any? Can
> > it use that? I presume it could try.. and it would fail but that
> > hardly seems nice.
> > 
> I will probably remove this sentence and the below at all.
> If not defined this could mean an error state
> > > + *
> > > + * channels-min
> > > + *      Values:         <uint>
> > uint32_t
> > > + *
> > > + *      The minimum amount of channels that is supported.
> > > + *      Must be at least 1. If not defined then use frontend's default.
> > This is odd. This whole section is for the backend, so how can the backend
> > not have this defined?
> > 
> > Do you mean to say that this parameter is optional? Perhaps
> > then say right before 'The minimum' include '(optional)'
> > 
> > Anyhow where do you envision this optional global value to be?
> > 
> > /local/domain/0/backend/vsnd/5/channels-[min|max] ?
> > 
> > Perhaps mention that in the start of this section:
> > 
> > These global values are under the backend 'vsnd' directory, as
> > so:
> > 
> > /local/domain/0/backends/vsnd/<front-id>/channels-[min|max]
> > 
> > Thought I think there is probably a better way to
> > say this..
> well, the confusion comes from the fact that I have
> described all this under backend's section
> now when I move it to the frontend's section it should
> make more sense

OK, but do please have an 'backend' section.

> > > + *
> > > + * channels-max
> > > + *      Values:         <uint>
> > uint32_t
> uint8_t
> > > + *
> > > + *      The maximum amount of channels that is supported.
> > > + *      Must be at least <channels-min>. If not defined then use frontend's
> > > + *      default.
> > I am still having trouble understanding how the backend is suppose
> > to use the frontend's default.
> > 
> > Perhaps you want to say:
> > 
> > If this value is not exposed by the backend the frontend
> > is permitted to use its default values.
> > 
> > ?
> moved to frontend's section
> > > + *
> > > + * sample-rates
> > > + *      Values:         <list of uints>
> > .. of uint32_t
> done
> > > + *
> > > + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
> > > + *      If not defined then use frontend's default. Sample rates are expressed
> > Ahain this 'If not defined then use fronten'ds default' could be written
> > a bit differently.
> moved to frontend's section
> > > + *      as a list of decimal values w/o any ordering requirement.
> > > + *
> > > + * 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.
> > Again, this does not sound right. The backend has no clue what
> > the frontend defaults are.
> moved to frontend's section
> > > + *
> > > + * buffer-size
> > > + *      Values:         <uint>
> > uint32_t
> done
> > > + *
> > > + *      The maximum size in octets of the buffer to allocate per stream.
> > > + *
> > > + * Example configuration:
> > > + *
> > > + * Driver configuration used by all streams:
> > > + * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"
> > Huh? But this is a backend value? Don't you mean:
> > 
> > /local/domain/0/backend/vsnd/5/sample-formats = "s8;u8;s16_le;s16_be"
> > ?
> moved to frontend's section
> > > + * Stream overrides sample rates supported:
> > -EPARSE
> done
> > 
> > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates =
> > > + *        "8000;22050;44100;48000"
> > /local/domain/0/backend/vsnd/5/2/stream/0/sample-rates ="8000;22050;44100;48000"
> > 
> > But that is wrong as this section is about global values so they
> > would be more like:
> > 
> > /local/domain/0/backend/vsnd/5/sample-rates ="8000;22050;44100;48000"
> > 
> > ?
> not anymore, as I move these to frontend's configuration
> > > + *
> > > + *----------------------- Virtual sound card settings --------------------------
> > > + * short-name
> > > + *      Values:         <char[32]>
> > > + *
> > > + *      Short name of the virtual sound card. Optional.
> > > + *
> > > + * long-name
> > > + *      Values:         <char[80]>
> > > + *
> > > + *      Long name of the virtual sound card. Optional.
> > > + *
> > > + * For example,
> > > + * /local/domain/5/device/vsnd/0/short-name = "Virtual audio"
> > > + * /local/domain/5/device/vsnd/0/long-name =
> > > + *         "Virtual audio at center stack"
> > This should be inverted. That is this is on the backend
> > side:
> > 
> > /local/domain/0/backend/vsnd/5/0/2/short-name = "Virtual audio"
> > 
> > And so on.
> > 
> > Also how come your example is missing the device part?
> I will put a complete configuration example
> > Wait, why do you have an an device below but ... not here?
> > Why not group them together?
> > 
> > That is why not have 'short-name' ,'long-name' and 'name' in
> > the same directory?
> name is for a stream; short/long for the card
> > 
> > > + *
> > > + *----------------------------- Device settings --------------------------------
> > > + * name
> > > + *      Values:         <char[80]>
> > > + *
> > > + *      Name of the sound device within the virtual sound card. Optional.
> > > + *
> > > + * For example,
> > > + * /local/domain/5/device/vsnd/0/device/0/name = "General analog"
> > The example you used at start had a device value of 2. It may
> > be better to use that here, so :
> > 
> > /local/domain/0/backend/vsnd/5/2/name = "General analog"
> done
> > > + *
> > > + *----------------------------- 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/device/0/stream/0/type = "p"
> > > + *      /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"
> > OK, but again:
> > /local/domain/0/backend/vsnd/5/2/0/stream/0/type = "p"
> > /local/domain/0/backend/vsnd/5/2/0/stream/1/type = "c"
> > 
> > Do you want also an 'feature-version' to identify what version
> > the backend exposes?
> > 
> will add that
> > > + *
> > > + *****************************************************************************
> > > + *                            Frontend XenBus Nodes
> > > + *****************************************************************************
> > > + *
> > > + *----------------------- Request Transport Parameters -----------------------
> > > + *
> > > + * These are per stream.
> > .. which means they are also per device right?
> > 
> > So one can have something like this:
> > 
> > /local/domain/5/device/vsnd/2/stream/0/event-channel
> > /local/domain/5/device/vsnd/0/stream/0/event-channel
> > /local/domain/5/device/vsnd/2/stream/1/event-channel
> > /local/domain/5/device/vsnd/0/stream/0/event-channel
> yes, every stream has its own communication channel
> > > + *
> > > + * event-channel
> > > + *      Values:         <uint>
> > uint32_t
> done
> > > + *
> > > + *      The identifier of the Xen event channel used to signal activity
> > > + *      in the ring buffer.
> > > + *
> > > + * ring-ref
> > > + *      Values:         <uint>
> > uint32_t
> done
> > > + *
> > > + *      The Xen grant reference granting permission for the backend to map
> > > + *      a sole page in a single page sized ring buffer.
> > > + *
> > > + * index
> > Why not 'unique-id' ?
> makes sense, will change
> > > + *      Values:         <uint>
> > uint32_t
> done
> > > + *
> > > + *      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. This is not equal to stream_idx as the later is
> > s/stream_idx/steam-idx/
> done
> > > + *      zero based within a device, but this index is contiguous within the
> > contingous
> done
> > > + *      driver.
> > 
> > > + */
> > > +
> > > +/*
> > > + * 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
> > > + *
> > > + *                                      XenbusStateUnknown
> > > + *                                      XenbusStateClosed
> > > + *                                      XenbusStateClosing
> > > + * o Remove virtual sound device
> > > + * o Remove event channels
> > > + *              |
> > > + *              |
> > > + *              V
> > > + * XenbusStateClosed
> > > + *
> > > + */
> > > +
> > > +/*
> > > + * 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
> > > +
> > > +/*
> > > + * 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_DEVICE              "device"
> > > +#define XENSND_PATH_STREAM              "stream"
> > > +/* Field names */
> > > +#define XENSND_FIELD_VCARD_SHORT_NAME   "short-name"
> > > +#define XENSND_FIELD_VCARD_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
> > 
> > Hm, but it is uint32_t which can be much much higher than 6.
> > 
> > 4294967295
> > 
> > 11 characters?
> fixed
> > > +/* Sample format field values */
> > > +#define XENSND_SAMPLE_FORMAT_MAX_LEN    24
> > You sure? You may want to make that clear in 'sample-format'
> > section that the maximum of a string can be 24 characters.
> done
> > 
> > And also explain why 24 characters.
> no particular reason, just to fit XENSND_PCM_FORMAT_???_STR
> do you want me to change it to something else?
> > > +
> > > +#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"
> > > +
> > > +/*
> > > + * STATUS RETURN CODES.
> > > + */
> > > +/* Operation not supported. */
> > > +#define XENSND_RSP_NOTSUPP              (-2)
> > > +/* Operation failed for some unspecified reason (e. g. -EIO). */
> > Is that the -EIO going to be exposed somewhere?
> no
> >   Inside of the
> > 'struct xensnd_resp' ?
> no, XENSND_RSP_??? is used there
> > 
> > Would it make sense to have it exposed there? Is that what 'status'
> > is for?
> see above, XENSND_RSP_???
> > 
> > Why not have an 'err' and mandate that it use POSIX return values?
> well, if we agree on POSIX values then I am more than ok

I think it is a good move going forward.

For example the pciff assumed it would only need these type of
errors and 'err' would have XEN_PCI_ERR_* errors.
And the MSI-X came in and they messed it up and
'err' has now POSIX return values _and_ XEN_PCI_ERR_* depending
on the type of operation.


> > 
> > > +#define XENSND_RSP_ERROR                (-1)
> > > +/* Operation completed successfully. */
> > > +#define XENSND_RSP_OKAY                 0
> > > +
> > > +/*
> > > + * Assumptions:
> > > + *   o usage of grant reference 0 as invalid grant reference:
> > > + *     grant reference 0 is valid, but never exposed to a PV driver,
> > > + *     because of the fact it is already in use/reserved by the PV console.
> > > + *   o all references in this document to page sizes must be treated
> > > + *     as pages of size XEN_PAGE_SIZE unless  otherwise noted.
> >                                                  ^- extra space
> fixed
> > 
> > > + *
> > > + * Description of the protocol between frontend and backend driver.
> > Is this suppose to have an:
> > -----------------------------------------
> > underneath it?
> done
> > > + *
> > > + * The two halves of a Para-virtual sound driver communicate with
> > > + * each other using a shared page and an event channel.
> > shared pages and event channels?
> > 
> > Since it looks like  you can have
> > 
> > /local/domain/<frontend-id>/device/vsnd/<device-id>/stream/<stream-id>/event-channel
> > 
> > And you can have N device-id and M stream-id ?
> right, done
> > > + * Shared page contains a ring with request/response packets.
> > > + *
> > > + * All reserved fields in the structures below must be 0.
> > > + *
> > > + * All request packets have the same length (32 octets)
> > Which implies you can have at maximum 64 requests?
> > 
> > [64 bytes for the four RING_IDX along with the 48 of padding, that
> > means 4032 left, but since we need this to be modulo 2 the best
> > we can do is 2^6.
> you mean we have to pad the structures so they are all
> 64 bytes long?

No no. Just that you can only fit 64 requests on a page.

The first 64 bytes of the ring are for the producer and consumer
index values. Then after that your structures which are up to 32 bytes
are filled out.

But since the entries need to be module two that means the maximum
of these structures you can fill out is 64.

> > 
> > 
> > > + * All request packets have common header:
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                |    operation    |     stream_idx  |
> > Why the stream_idx ? Your 'ring-ref' is rooted from the '<stream-idx>' so
> > the frontend and backend already know this.
> indeed, we can probably remove this from all the structures
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                reserved                               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *   id - uint16_t, private guest value, echoed in response
> > > + *   operation - uint8_t, operation code
> > > + *   stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
> > > + *     of the stream)
> > Why do you need this duplicate information?
> it describes common header and its values

I meant the 'stream_idx'

> > 
> > > + *
> > > + *
> > > + * Request open - open a PCM stream for playback or capture:
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                reserved                               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                pcm_rate                               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |  pcm_format     |  pcm_channels   |             reserved              |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               buffer_sz                               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                         gref_directory_start                          |
> > Perhaps 'gref_list' ?
> not sure, it is the start of the page directory, its gref

Directory and list are similar enough. If you want directory keep it
but please remove the 'start'. It is implied.

> > 
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > 
> > > + *
> > > + * 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
> > .. And I presume this should be between 'channels-min' and 'channels-max'?
> > Is it OK to have to say 255 ? What are the boundary values?
> > 
> yes, it is ok to be uint8_t, IMO. I will change the
> xenstore values description as well
> > > + * buffer_sz - uint32_t, buffer size to be allocated in octets
> > Is there an maximum? Or a minimum?
> > 
> > Why have it in bytes?  Why not in the amount of grants you need?
> > Perhaps call it 'gref_nr' ? That way your computation on how
> > many grants you need is well simplified.
> there was a discussion on that:
> https://marc.info/?l=xen-devel&m=148008589320533&w=2

You want to capture that in this document (of why you choose
this way). 
> > 
> > 
> > > + * gref_directory_start - grant_ref_t, a reference to the first shared page
> > s/gref_directory_start/gref_list/ ?
> not sure
> > > + *   describing shared buffer references. At least one page exists. If shared
> > > + *   buffer size exceeds what can be addressed by this single page, then
> > s/shared buffer size/buffer_sz/ ?
> why, I am not using variable/field name here, but explaining
> I can put the name in brackets)
> > 
> > > + *   reference to the next page must be supplied (see gref_dir_next_page below)
> > Now what if grefs_nr (or buffer_sz) is say 1 page (4096).
> > Does that mean that gref_directory_start still needs to point to page
> > which only has two entries: 0, <grant ref> ?
> > 
> > Or can it be simplified and this gref_directory_start would be used for
> > data instead?
> > 
> > I think that is what you saying ("If shared .. " which would imply
> > that if "if !shared" then something else can be done?) but the start
> > says: "a reference to the first shared page describind shared buffer
> > references" which implies you do
> > need this extra indirect page even if the buffer_sz is say 4096.
> > ?
> yes, you got it right. the use-cases we have do use buffers
> bigger then 4K, so this is why we never thought of such an overhead
> > 
> > 
> > > + */
> > > +
> > > +struct xensnd_open_req {
> > s/_open_req/_req_open/
> it will be yet another flame here...
> will keep as is
> > > +    uint32_t pcm_rate; /* in Hz */
> > > +    uint8_t pcm_format;
> > > +    uint8_t pcm_channels;
> > > +    uint16_t reserved;
> > > +    uint32_t buffer_sz;
> > > +    grant_ref_t gref_directory_start;
> > > +};
> > > +
> > > +/*
> > > + * 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                           |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                gref[0]                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                gref[i]                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                gref[N -1]                             |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * gref_dir_next_page - grant_ref_t, reference to the next page describing
> > > + *   page directory. Must be 0 if no more pages in the list.
> > s/no more page/there are no more/
> done
> > 
> > > + * gref[i] - grant_ref_t, reference to a shared page of the buffer
> > > + *   allocated at XENSND_OP_OPEN
> > > + *
> > > + * Number of grant_ref_t entries in the whole page directory is not
> > > + * passed, but instead can be calculated as:
> > > + *   num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) /
> > > + *       XEN_PAGE_SIZE
> > 
> > And what are the expected errors? Woudl it make sense to define those?
> > Say:
> > 
> >   Returns:
> >   -ENOBUFS: Cannot map that many buffers.
> >   -EINVAL: Incorrect values in the requst?
> ATM, the XENSND_RSP_??? are returned
> >   -
> > > + */
> > > +
> > > +struct xensnd_page_directory {
> > > +    grant_ref_t gref_dir_next_page;
> > > +    grant_ref_t gref[1]; /* Variable length */
> > > +};
> > > +
> > > +/*
> > > + *  Request close - close an opened pcm stream:
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                | XENSND_OP_CLOSE |     stream_idx  |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + */
> > > +
> > > +/*
> > > + * Request read/write - used for read (for capture) or write (for playback):
> > Maybe say also that XENSND_OP_OPEN MUST be called before these operations
> > are permitted.
> not sure we need this: normally you have to open something
> before you can use it, e.g. a file
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                |    operation    |     stream_idx  |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                offset                                 |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                                length                                 |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
> > > + * offset - uint32_t, read or write data offset within the shared buffer
> > > + *   passed with XENSND_OP_OPEN request
> > May also want to say that this offset must be less than buffer_sz.
> > 
> > In octets? May want to say that explicitly.
> done
> > 
> > > + * length - uint32_t, read or write data length
> > In octets?
> done
> > > + */
> > > +
> > > +struct xensnd_rw_req {
> > > +    uint32_t offset;
> > > +    uint32_t len;
> > > +};
> > > +
> > > +/*
> > > + * Request set/get volume - set/get channels' volume of the stream given:
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                |    operation    |     stream_idx  |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * operation - XENSND_OP_SET_VOLUME for volume set
> > > + *   or XENSND_OP_GET_VOLUME for volume get
> > > + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
> > > + * values:
> > Oh. That means you these operation are in effect 'barrier' ones.
> > 
> > As the buffer must be flushed before hand otherwise you would be
> > overwriting data stream information.
> > 
> > You should probably mention this semantic need?
> I think this is implementation specific and shouldn't
> be in a generic protocol

How is that implementation specific? If there is something in the page
from the previous command you are overwritting those values.

> > 
> > Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
> > a similar structure to 'struct xensnd_rw_req' so that you have
> > the offset and len?
> a page can hold enough values, IMO

Let me see "640K ought to be enough"?

You are making assumptions here based on how the implementation
fills out the data structure. But the purpose of the design
is to detach oneself from the implementation and think of 
alternative ways.

To capture your words:
"
So if read/write use that buffer, and the volume and muting
> controls use it too, how do I change the volume while listening
> without disturbing the read/write?
read/write do not happen continuously, e.g. sound card fills its
internal buffers (our buffer is busy) and then until next re-fill our
buffer is free. that means that there is almost no congestion and
always a good chance to set/get volume w/o problem
> Jan
"

Well, that is implementation specific. What if some implementaiton
fills it back to back?

I would like you to add the 'offset' and 'len' so that we don't
dig a hole that we can't easily get out of.


> > > + *
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               channel[0]                              |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               channel[i]                              |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * channel[i] - sint32_t, volume of i-th channel
> > You may want to say that i is uint8_t.
> done
> > 
> > > + * Volume is expressed as a signed value in steps of 0.001 dB,
> > > + * while 0 being 0 dB.
> > > + */
> > > +
> > > +/*
> > > + * Request mute/unmute - mute/unmute stream:
> > > + *          0                 1                  2                3        octet
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                 id                |    operation    |     stream_idx  |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + * |                               reserved                                |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
> > > + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> > > + * values:
> > Again, same comments as above.
> > > + *
> > > + *                                     0                                   octet
> > > + * +-----------------------------------------------------------------------+
> > > + * |                               channel[0]                              |
> > > + * +-----------------------------------------------------------------------+
> > > + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------------------------------------------------------------+
> > > + * |                               channel[i]                              |
> > > + * +-----------------------------------------------------------------------+
> > > + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> > > + * +-----------------------------------------------------------------------+
> > > + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
> > > + * +-----------------+-----------------+-----------------+-----------------+
> > > + *
> > > + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
> > > + */
> > > +
> > > +/*
> > > + * All response packets have the same length (32 octets)
> > > + *
> > > + * 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_* - copied from request
> > > + * status - int8_t, response status (XENSND_RSP_*)
> > Could this become an 'err' and the POSIX values used for errors?
> > 
> we can discuss this, I am fine with this approach if the
> Community accepts this


You can always have _both_.

> > > + */
> > > +
> > > +struct xensnd_req {
> > > +    uint16_t id;
> > > +    uint8_t operation;
> > > +    uint8_t stream_idx;
> > > +    uint32_t reserved;
> > > +    union {
> > > +        struct xensnd_open_req open;
> > > +        struct xensnd_rw_req rw;
> > > +        uint8_t reserved[24];
> > > +    } op;
> > > +};
> > > +
> > > +struct xensnd_resp {
> > > +    uint16_t id;
> > > +    uint8_t operation;
> > > +    uint8_t stream_idx;
> > > +    int8_t status;
> > > +    uint8_t reserved[27];
> > > +};
> > > +
> > > +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req, struct xensnd_resp);
> > > +
> > > +#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */
> > > -- 
> > > 2.7.4
> > > 
> 

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

thank you for comments, please find answers below

Can we please switch to v16 discussion as v15 vs v16 is
a big change?

On 01/27/2017 05:14 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 26, 2017 at 12:02:49PM +0200, Oleksandr Andrushchenko wrote:
>> Hi, Konrad!
>>
>> First of all thank you very much for the valuable comments
>>
>> and your time!
>>
>> The number of changes (mostly in description) is going to
>>
>> be huge, so do you think I can publish something like
>>
>> "RFC v16" so we can discuss the updated patch?
> RFC sadly means folks are going to mostly ignore it.
> I would prefer you did not use RFC at this stage but just
> did v16.
> ..snip..
sure
>>>> + * Example for the frontend running in domain 5, instance of the driver
>>>> + * in the front is 0 (single or first PV driver), device id 2,
>>>> + * first stream (0):
>>>> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
>>>> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
>>>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
>>> Why do you need 'device' ?
>> just for clarity: the hierarchy of the sound driver would
>> be that a device may have multiple different streams.
> And it just occured to me that you could also imply that
> each device has an stream without the 'stream' in it.
>
> So
>
> /local/domain/5/device/vsnd/0/2/0/type = "p"
>
> And the format is:
> /local/domain/<front-id>/device/vsnd/<instance of PV driver>/<device-id>/<stream-id>
ok, so we'll end up with something like:

--------------------------------- Backend 
-----------------------------------

/local/domain/0/backend/vsnd/1/0/frontend-id = "1"
/local/domain/0/backend/vsnd/1/0/frontend = "/local/domain/1/device/vsnd/0"
/local/domain/0/backend/vsnd/1/0/state = "4"
/local/domain/0/backend/vsnd/1/0/versions = "1,2"

----------------------------- Card ----------------------------

/local/domain/1/device/vsnd/0/version = "1"
/local/domain/1/device/vsnd/0/short-name = "Card short name"
/local/domain/1/device/vsnd/0/long-name = "Card long name"
/local/domain/1/device/vsnd/0/sample-rates = "8000,32000,44100,48000,96000"
/local/domain/1/device/vsnd/0/sample-formats = "s8,u8,s16_le,s16_be"
/local/domain/1/device/vsnd/0/buffer-size = "262144"

----------------------------- PCM device 0 ----------------------------

/local/domain/1/device/vsnd/0/0/name = "General analog"
/local/domain/1/device/vsnd/0/0/channels-max = "5"

----------------------------- Stream 0, playback 
----------------------------

/local/domain/1/device/vsnd/0/0/0/type = "p"
/local/domain/1/device/vsnd/0/0/0/sample-formats = "s8,u8"
/local/domain/1/device/vsnd/0/0/0/unique-id = "0"
/local/domain/1/device/vsnd/0/0/0/ring-ref = "386"
/local/domain/1/device/vsnd/0/0/0/event-channel = "15"

------------------------------- PCM device 3 
--------------------------------

/local/domain/1/device/vsnd/0/3/name = "HDMI-0"
/local/domain/1/device/vsnd/0/3/sample-rates = "8000,32000,44100"

------------------------------ Stream 0, capture 
----------------------------

/local/domain/1/device/vsnd/0/3/0/type = "c"
/local/domain/1/device/vsnd/0/3/0/unique-id = "2"
/local/domain/1/device/vsnd/0/3/0/ring-ref = "387"
/local/domain/1/device/vsnd/0/3/0/event-channel = "151"

Is this what you would like to see?
IMO, all these values do not help understanding what it is, e.g.
this is equal to me if we have

/local/domain/1/device/vbd/51712/queue-0/ring-ref = "8"
/local/domain/1/device/vbd/51712/queue-0/event-channel = "3"
/local/domain/1/device/vbd/51712/queue-1 = ""
/local/domain/1/device/vbd/51712/queue-1/ring-ref = "9"

and then decided to go with

/local/domain/1/device/vbd/51712/0/ring-ref = "8"
/local/domain/1/device/vbd/51712/0/event-channel = "3"
/local/domain/1/device/vbd/51712/1/ring-ref = "9"

Can one easily tell what 0 or 1 after "51712/" is?

So, what is the final decision then?

> Though I have a little of trouble with the 'instance of the
> driver'. Are you suggesting you would have multiple
> PV drivers of 'vsnd'? Can't the multiple device ids fulfill this?
it is possible, but the main use-case will have a single
PV driver with multiple PCM devices/streams
>
>> So, from readability POV I would still have "device" in place
>>  From xenstore documentation: "Data should generally be
>> human-readable for ease of management and debugging "
>> I assume this also applies to the structure as well
>>> Could not this be:
>>>
>>> /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ?
>> then one has to know that "2" stands for device.
>> see above, I would keep "device" here
>>>
>>>> + *
>>>> + *------------------------------- PCM settings --------------------------------
>>>> + *
>>>> + * Every virtualized sound frontend has set of devices and streams, each
>>> frontend or backend?
>>>
>>> I would think backend since this is still the backend section?
>> you are right, moved to frontend's section
>>>> + * 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 driver, device or stream.
>>>> + * Every underlying layer in turn can re-define some or all of them to better
>>>> + * fit its needs. For example, driver 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.
>>> OK, so these are more of 'global ones'?
>> kind of, what is more they can be "global" to all the
>> underlying levels, e.g. if device changes some of the
>> parameters they become adopted by streams if not
>> explicitly configured. The same is true if device changes
>> parameters set at the card level.
>> This way one can easily tailor particular pieces of the
>> system w/o writing all settings again
> Are they "global" to the "instance of the PV drivers" or all the
> device ids?
they are global to the frontend and the settings
live on frontend's xenstore path. Please check
configuration example in v16
>
> I presume the latter which is why you need to have multiple
> instances of the PV drivers support?
>
> (You may want to mention this requirement in the section talking
> about the format of how the structure is exposed in the backend
> and frontend).
I've already put full configuration xenstore tree with
values for education purposes, so one can see how it gets
configured
>>>> + *
>>>> + * Note: if any of the values are not defined then PV driver should use
>>>> + * its default values instead.
>>> How is that suppose to work? What if the PV driver by default wants
>>> to use say 314156 channels and the backend does not expose any? Can
>>> it use that? I presume it could try.. and it would fail but that
>>> hardly seems nice.
>>>
>> I will probably remove this sentence and the below at all.
>> If not defined this could mean an error state
>>>> + *
>>>> + * channels-min
>>>> + *      Values:         <uint>
>>> uint32_t
>>>> + *
>>>> + *      The minimum amount of channels that is supported.
>>>> + *      Must be at least 1. If not defined then use frontend's default.
>>> This is odd. This whole section is for the backend, so how can the backend
>>> not have this defined?
>>>
>>> Do you mean to say that this parameter is optional? Perhaps
>>> then say right before 'The minimum' include '(optional)'
>>>
>>> Anyhow where do you envision this optional global value to be?
>>>
>>> /local/domain/0/backend/vsnd/5/channels-[min|max] ?
>>>
>>> Perhaps mention that in the start of this section:
>>>
>>> These global values are under the backend 'vsnd' directory, as
>>> so:
>>>
>>> /local/domain/0/backends/vsnd/<front-id>/channels-[min|max]
>>>
>>> Thought I think there is probably a better way to
>>> say this..
>> well, the confusion comes from the fact that I have
>> described all this under backend's section
>> now when I move it to the frontend's section it should
>> make more sense
> OK, but do please have an 'backend' section.
done,
I have clearly(?) defined both frontend and backend entries in v16
>>>> + *
>>>> + * channels-max
>>>> + *      Values:         <uint>
>>> uint32_t
>> uint8_t
>>>> + *
>>>> + *      The maximum amount of channels that is supported.
>>>> + *      Must be at least <channels-min>. If not defined then use frontend's
>>>> + *      default.
>>> I am still having trouble understanding how the backend is suppose
>>> to use the frontend's default.
>>>
>>> Perhaps you want to say:
>>>
>>> If this value is not exposed by the backend the frontend
>>> is permitted to use its default values.
>>>
>>> ?
>> moved to frontend's section
>>>> + *
>>>> + * sample-rates
>>>> + *      Values:         <list of uints>
>>> .. of uint32_t
>> done
>>>> + *
>>>> + *      List of supported sample rates separated by XENSND_LIST_SEPARATOR.
>>>> + *      If not defined then use frontend's default. Sample rates are expressed
>>> Ahain this 'If not defined then use fronten'ds default' could be written
>>> a bit differently.
>> moved to frontend's section
>>>> + *      as a list of decimal values w/o any ordering requirement.
>>>> + *
>>>> + * 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.
>>> Again, this does not sound right. The backend has no clue what
>>> the frontend defaults are.
>> moved to frontend's section
>>>> + *
>>>> + * buffer-size
>>>> + *      Values:         <uint>
>>> uint32_t
>> done
>>>> + *
>>>> + *      The maximum size in octets of the buffer to allocate per stream.
>>>> + *
>>>> + * Example configuration:
>>>> + *
>>>> + * Driver configuration used by all streams:
>>>> + * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"
>>> Huh? But this is a backend value? Don't you mean:
>>>
>>> /local/domain/0/backend/vsnd/5/sample-formats = "s8;u8;s16_le;s16_be"
>>> ?
>> moved to frontend's section
>>>> + * Stream overrides sample rates supported:
>>> -EPARSE
>> done
>>>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates =
>>>> + *        "8000;22050;44100;48000"
>>> /local/domain/0/backend/vsnd/5/2/stream/0/sample-rates ="8000;22050;44100;48000"
>>>
>>> But that is wrong as this section is about global values so they
>>> would be more like:
>>>
>>> /local/domain/0/backend/vsnd/5/sample-rates ="8000;22050;44100;48000"
>>>
>>> ?
>> not anymore, as I move these to frontend's configuration
>>>> + *
>>>> + *----------------------- Virtual sound card settings --------------------------
>>>> + * short-name
>>>> + *      Values:         <char[32]>
>>>> + *
>>>> + *      Short name of the virtual sound card. Optional.
>>>> + *
>>>> + * long-name
>>>> + *      Values:         <char[80]>
>>>> + *
>>>> + *      Long name of the virtual sound card. Optional.
>>>> + *
>>>> + * For example,
>>>> + * /local/domain/5/device/vsnd/0/short-name = "Virtual audio"
>>>> + * /local/domain/5/device/vsnd/0/long-name =
>>>> + *         "Virtual audio at center stack"
>>> This should be inverted. That is this is on the backend
>>> side:
>>>
>>> /local/domain/0/backend/vsnd/5/0/2/short-name = "Virtual audio"
>>>
>>> And so on.
>>>
>>> Also how come your example is missing the device part?
>> I will put a complete configuration example
>>> Wait, why do you have an an device below but ... not here?
>>> Why not group them together?
>>>
>>> That is why not have 'short-name' ,'long-name' and 'name' in
>>> the same directory?
>> name is for a stream; short/long for the card
>>>> + *
>>>> + *----------------------------- Device settings --------------------------------
>>>> + * name
>>>> + *      Values:         <char[80]>
>>>> + *
>>>> + *      Name of the sound device within the virtual sound card. Optional.
>>>> + *
>>>> + * For example,
>>>> + * /local/domain/5/device/vsnd/0/device/0/name = "General analog"
>>> The example you used at start had a device value of 2. It may
>>> be better to use that here, so :
>>>
>>> /local/domain/0/backend/vsnd/5/2/name = "General analog"
>> done
>>>> + *
>>>> + *----------------------------- 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/device/0/stream/0/type = "p"
>>>> + *      /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"
>>> OK, but again:
>>> /local/domain/0/backend/vsnd/5/2/0/stream/0/type = "p"
>>> /local/domain/0/backend/vsnd/5/2/0/stream/1/type = "c"
>>>
>>> Do you want also an 'feature-version' to identify what version
>>> the backend exposes?
>>>
>> will add that
>>>> + *
>>>> + *****************************************************************************
>>>> + *                            Frontend XenBus Nodes
>>>> + *****************************************************************************
>>>> + *
>>>> + *----------------------- Request Transport Parameters -----------------------
>>>> + *
>>>> + * These are per stream.
>>> .. which means they are also per device right?
>>>
>>> So one can have something like this:
>>>
>>> /local/domain/5/device/vsnd/2/stream/0/event-channel
>>> /local/domain/5/device/vsnd/0/stream/0/event-channel
>>> /local/domain/5/device/vsnd/2/stream/1/event-channel
>>> /local/domain/5/device/vsnd/0/stream/0/event-channel
>> yes, every stream has its own communication channel
>>>> + *
>>>> + * event-channel
>>>> + *      Values:         <uint>
>>> uint32_t
>> done
>>>> + *
>>>> + *      The identifier of the Xen event channel used to signal activity
>>>> + *      in the ring buffer.
>>>> + *
>>>> + * ring-ref
>>>> + *      Values:         <uint>
>>> uint32_t
>> done
>>>> + *
>>>> + *      The Xen grant reference granting permission for the backend to map
>>>> + *      a sole page in a single page sized ring buffer.
>>>> + *
>>>> + * index
>>> Why not 'unique-id' ?
>> makes sense, will change
>>>> + *      Values:         <uint>
>>> uint32_t
>> done
>>>> + *
>>>> + *      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. This is not equal to stream_idx as the later is
>>> s/stream_idx/steam-idx/
>> done
>>>> + *      zero based within a device, but this index is contiguous within the
>>> contingous
>> done
>>>> + *      driver.
>>>> + */
>>>> +
>>>> +/*
>>>> + * 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
>>>> + *
>>>> + *                                      XenbusStateUnknown
>>>> + *                                      XenbusStateClosed
>>>> + *                                      XenbusStateClosing
>>>> + * o Remove virtual sound device
>>>> + * o Remove event channels
>>>> + *              |
>>>> + *              |
>>>> + *              V
>>>> + * XenbusStateClosed
>>>> + *
>>>> + */
>>>> +
>>>> +/*
>>>> + * 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
>>>> +
>>>> +/*
>>>> + * 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_DEVICE              "device"
>>>> +#define XENSND_PATH_STREAM              "stream"
>>>> +/* Field names */
>>>> +#define XENSND_FIELD_VCARD_SHORT_NAME   "short-name"
>>>> +#define XENSND_FIELD_VCARD_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
>>> Hm, but it is uint32_t which can be much much higher than 6.
>>>
>>> 4294967295
>>>
>>> 11 characters?
>> fixed
>>>> +/* Sample format field values */
>>>> +#define XENSND_SAMPLE_FORMAT_MAX_LEN    24
>>> You sure? You may want to make that clear in 'sample-format'
>>> section that the maximum of a string can be 24 characters.
>> done
>>> And also explain why 24 characters.
>> no particular reason, just to fit XENSND_PCM_FORMAT_???_STR
>> do you want me to change it to something else?
>>>> +
>>>> +#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"
>>>> +
>>>> +/*
>>>> + * STATUS RETURN CODES.
>>>> + */
>>>> +/* Operation not supported. */
>>>> +#define XENSND_RSP_NOTSUPP              (-2)
>>>> +/* Operation failed for some unspecified reason (e. g. -EIO). */
>>> Is that the -EIO going to be exposed somewhere?
>> no
>>>    Inside of the
>>> 'struct xensnd_resp' ?
>> no, XENSND_RSP_??? is used there
>>> Would it make sense to have it exposed there? Is that what 'status'
>>> is for?
>> see above, XENSND_RSP_???
>>> Why not have an 'err' and mandate that it use POSIX return values?
>> well, if we agree on POSIX values then I am more than ok
> I think it is a good move going forward.
>
> For example the pciff assumed it would only need these type of
> errors and 'err' would have XEN_PCI_ERR_* errors.
> And the MSI-X came in and they messed it up and
> 'err' has now POSIX return values _and_ XEN_PCI_ERR_* depending
> on the type of operation.
>
good
>>>> +#define XENSND_RSP_ERROR                (-1)
>>>> +/* Operation completed successfully. */
>>>> +#define XENSND_RSP_OKAY                 0
>>>> +
>>>> +/*
>>>> + * Assumptions:
>>>> + *   o usage of grant reference 0 as invalid grant reference:
>>>> + *     grant reference 0 is valid, but never exposed to a PV driver,
>>>> + *     because of the fact it is already in use/reserved by the PV console.
>>>> + *   o all references in this document to page sizes must be treated
>>>> + *     as pages of size XEN_PAGE_SIZE unless  otherwise noted.
>>>                                                   ^- extra space
>> fixed
>>>> + *
>>>> + * Description of the protocol between frontend and backend driver.
>>> Is this suppose to have an:
>>> -----------------------------------------
>>> underneath it?
>> done
>>>> + *
>>>> + * The two halves of a Para-virtual sound driver communicate with
>>>> + * each other using a shared page and an event channel.
>>> shared pages and event channels?
>>>
>>> Since it looks like  you can have
>>>
>>> /local/domain/<frontend-id>/device/vsnd/<device-id>/stream/<stream-id>/event-channel
>>>
>>> And you can have N device-id and M stream-id ?
>> right, done
>>>> + * Shared page contains a ring with request/response packets.
>>>> + *
>>>> + * All reserved fields in the structures below must be 0.
>>>> + *
>>>> + * All request packets have the same length (32 octets)
>>> Which implies you can have at maximum 64 requests?
>>>
>>> [64 bytes for the four RING_IDX along with the 48 of padding, that
>>> means 4032 left, but since we need this to be modulo 2 the best
>>> we can do is 2^6.
>> you mean we have to pad the structures so they are all
>> 64 bytes long?
> No no. Just that you can only fit 64 requests on a page.
>
> The first 64 bytes of the ring are for the producer and consumer
> index values. Then after that your structures which are up to 32 bytes
> are filled out.
>
> But since the entries need to be module two that means the maximum
> of these structures you can fill out is 64.
have 32-octet structures in v16
>>>
>>>> + * All request packets have common header:
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                |    operation    |     stream_idx  |
>>> Why the stream_idx ? Your 'ring-ref' is rooted from the '<stream-idx>' so
>>> the frontend and backend already know this.
>> indeed, we can probably remove this from all the structures
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                reserved                               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *   id - uint16_t, private guest value, echoed in response
>>>> + *   operation - uint8_t, operation code
>>>> + *   stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>>>> + *     of the stream)
>>> Why do you need this duplicate information?
>> it describes common header and its values
> I meant the 'stream_idx'
re-worked in v16
>>>> + *
>>>> + *
>>>> + * Request open - open a PCM stream for playback or capture:
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                reserved                               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                pcm_rate                               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |  pcm_format     |  pcm_channels   |             reserved              |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               buffer_sz                               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                         gref_directory_start                          |
>>> Perhaps 'gref_list' ?
>> not sure, it is the start of the page directory, its gref
> Directory and list are similar enough. If you want directory keep it
> but please remove the 'start'. It is implied.
sounds reasonable
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * 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
>>> .. And I presume this should be between 'channels-min' and 'channels-max'?
>>> Is it OK to have to say 255 ? What are the boundary values?
>>>
>> yes, it is ok to be uint8_t, IMO. I will change the
>> xenstore values description as well
>>>> + * buffer_sz - uint32_t, buffer size to be allocated in octets
>>> Is there an maximum? Or a minimum?
>>>
>>> Why have it in bytes?  Why not in the amount of grants you need?
>>> Perhaps call it 'gref_nr' ? That way your computation on how
>>> many grants you need is well simplified.
>> there was a discussion on that:
>> https://marc.info/?l=xen-devel&m=148008589320533&w=2
> You want to capture that in this document (of why you choose
> this way).
one would need to read all the story to get to the point, because
lots of iterations were done and some of the info was in between
>>>
>>>> + * gref_directory_start - grant_ref_t, a reference to the first shared page
>>> s/gref_directory_start/gref_list/ ?
>> not sure
>>>> + *   describing shared buffer references. At least one page exists. If shared
>>>> + *   buffer size exceeds what can be addressed by this single page, then
>>> s/shared buffer size/buffer_sz/ ?
>> why, I am not using variable/field name here, but explaining
>> I can put the name in brackets)
>>>> + *   reference to the next page must be supplied (see gref_dir_next_page below)
>>> Now what if grefs_nr (or buffer_sz) is say 1 page (4096).
>>> Does that mean that gref_directory_start still needs to point to page
>>> which only has two entries: 0, <grant ref> ?
>>>
>>> Or can it be simplified and this gref_directory_start would be used for
>>> data instead?
>>>
>>> I think that is what you saying ("If shared .. " which would imply
>>> that if "if !shared" then something else can be done?) but the start
>>> says: "a reference to the first shared page describind shared buffer
>>> references" which implies you do
>>> need this extra indirect page even if the buffer_sz is say 4096.
>>> ?
>> yes, you got it right. the use-cases we have do use buffers
>> bigger then 4K, so this is why we never thought of such an overhead
>>>
>>>> + */
>>>> +
>>>> +struct xensnd_open_req {
>>> s/_open_req/_req_open/
>> it will be yet another flame here...
>> will keep as is
>>>> +    uint32_t pcm_rate; /* in Hz */
>>>> +    uint8_t pcm_format;
>>>> +    uint8_t pcm_channels;
>>>> +    uint16_t reserved;
>>>> +    uint32_t buffer_sz;
>>>> +    grant_ref_t gref_directory_start;
>>>> +};
>>>> +
>>>> +/*
>>>> + * 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                           |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                gref[0]                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                gref[i]                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                gref[N -1]                             |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * gref_dir_next_page - grant_ref_t, reference to the next page describing
>>>> + *   page directory. Must be 0 if no more pages in the list.
>>> s/no more page/there are no more/
>> done
>>>> + * gref[i] - grant_ref_t, reference to a shared page of the buffer
>>>> + *   allocated at XENSND_OP_OPEN
>>>> + *
>>>> + * Number of grant_ref_t entries in the whole page directory is not
>>>> + * passed, but instead can be calculated as:
>>>> + *   num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) /
>>>> + *       XEN_PAGE_SIZE
>>> And what are the expected errors? Woudl it make sense to define those?
>>> Say:
>>>
>>>    Returns:
>>>    -ENOBUFS: Cannot map that many buffers.
>>>    -EINVAL: Incorrect values in the requst?
>> ATM, the XENSND_RSP_??? are returned
>>>    -
>>>> + */
>>>> +
>>>> +struct xensnd_page_directory {
>>>> +    grant_ref_t gref_dir_next_page;
>>>> +    grant_ref_t gref[1]; /* Variable length */
>>>> +};
>>>> +
>>>> +/*
>>>> + *  Request close - close an opened pcm stream:
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                | XENSND_OP_CLOSE |     stream_idx  |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + */
>>>> +
>>>> +/*
>>>> + * Request read/write - used for read (for capture) or write (for playback):
>>> Maybe say also that XENSND_OP_OPEN MUST be called before these operations
>>> are permitted.
>> not sure we need this: normally you have to open something
>> before you can use it, e.g. a file
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                |    operation    |     stream_idx  |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                offset                                 |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                                length                                 |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
>>>> + * offset - uint32_t, read or write data offset within the shared buffer
>>>> + *   passed with XENSND_OP_OPEN request
>>> May also want to say that this offset must be less than buffer_sz.
>>>
>>> In octets? May want to say that explicitly.
>> done
>>>> + * length - uint32_t, read or write data length
>>> In octets?
>> done
>>>> + */
>>>> +
>>>> +struct xensnd_rw_req {
>>>> +    uint32_t offset;
>>>> +    uint32_t len;
>>>> +};
>>>> +
>>>> +/*
>>>> + * Request set/get volume - set/get channels' volume of the stream given:
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                |    operation    |     stream_idx  |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * operation - XENSND_OP_SET_VOLUME for volume set
>>>> + *   or XENSND_OP_GET_VOLUME for volume get
>>>> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
>>>> + * values:
>>> Oh. That means you these operation are in effect 'barrier' ones.
>>>
>>> As the buffer must be flushed before hand otherwise you would be
>>> overwriting data stream information.
>>>
>>> You should probably mention this semantic need?
>> I think this is implementation specific and shouldn't
>> be in a generic protocol
> How is that implementation specific? If there is something in the page
> from the previous command you are overwritting those values.
ok, all the operations are synchronous for the stream given.
it means that if there is something left in the buffer
it will be overwritten by the next req/resp, so this is expected
>
>>> Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
>>> a similar structure to 'struct xensnd_rw_req' so that you have
>>> the offset and len?
>> a page can hold enough values, IMO
> Let me see "640K ought to be enough"?
I was talking about 256 channels 4 bytes per each.
>
> You are making assumptions here based on how the implementation
> fills out the data structure. But the purpose of the design
> is to detach oneself from the implementation and think of
> alternative ways.
>
> To capture your words:
> "
> So if read/write use that buffer, and the volume and muting
>> controls use it too, how do I change the volume while listening
>> without disturbing the read/write?
> read/write do not happen continuously, e.g. sound card fills its
> internal buffers (our buffer is busy) and then until next re-fill our
> buffer is free. that means that there is almost no congestion and
> always a good chance to set/get volume w/o problem
>> Jan
> "
>
> Well, that is implementation specific. What if some implementaiton
> fills it back to back?
>
> I would like you to add the 'offset' and 'len' so that we don't
> dig a hole that we can't easily get out of.
>
ok, I will add
  * +----------------+----------------+----------------+----------------+
  * | offset                               | 12
  * +----------------+----------------+----------------+----------------+
  * | length                               | 16
  * +----------------+----------------+----------------+----------------+
to
1.Request set/get volume - set/get channels' volume of the stream given
2.Request mute/unmute - mute/unmute stream

By this change you enable a use-case when part of the shared buffer
is used for samples and part for volume/mute, right?
>>>> + *
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               channel[0]                              |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               channel[i]                              |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * channel[i] - sint32_t, volume of i-th channel
>>> You may want to say that i is uint8_t.
>> done
>>>> + * Volume is expressed as a signed value in steps of 0.001 dB,
>>>> + * while 0 being 0 dB.
>>>> + */
>>>> +
>>>> +/*
>>>> + * Request mute/unmute - mute/unmute stream:
>>>> + *          0                 1                  2                3        octet
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                 id                |    operation    |     stream_idx  |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + * |                               reserved                                |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
>>>> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
>>>> + * values:
>>> Again, same comments as above.
>>>> + *
>>>> + *                                     0                                   octet
>>>> + * +-----------------------------------------------------------------------+
>>>> + * |                               channel[0]                              |
>>>> + * +-----------------------------------------------------------------------+
>>>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------------------------------------------------------------+
>>>> + * |                               channel[i]                              |
>>>> + * +-----------------------------------------------------------------------+
>>>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>> + * +-----------------------------------------------------------------------+
>>>> + * |                channel[XENSND_OP_OPEN.pcm_channels - 1]               |
>>>> + * +-----------------+-----------------+-----------------+-----------------+
>>>> + *
>>>> + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
>>>> + */
>>>> +
>>>> +/*
>>>> + * All response packets have the same length (32 octets)
>>>> + *
>>>> + * 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_* - copied from request
>>>> + * status - int8_t, response status (XENSND_RSP_*)
>>> Could this become an 'err' and the POSIX values used for errors?
>>>
>> we can discuss this, I am fine with this approach if the
>> Community accepts this
>
> You can always have _both_.
I am with -XEN_E and int32_t
>>>> + */
>>>> +
>>>> +struct xensnd_req {
>>>> +    uint16_t id;
>>>> +    uint8_t operation;
>>>> +    uint8_t stream_idx;
>>>> +    uint32_t reserved;
>>>> +    union {
>>>> +        struct xensnd_open_req open;
>>>> +        struct xensnd_rw_req rw;
>>>> +        uint8_t reserved[24];
>>>> +    } op;
>>>> +};
>>>> +
>>>> +struct xensnd_resp {
>>>> +    uint16_t id;
>>>> +    uint8_t operation;
>>>> +    uint8_t stream_idx;
>>>> +    int8_t status;
>>>> +    uint8_t reserved[27];
>>>> +};
>>>> +
>>>> +DEFINE_RING_TYPES(xen_sndif, struct xensnd_req, struct xensnd_resp);
>>>> +
>>>> +#endif /* __XEN_PUBLIC_IO_SNDIF_H__ */
>>>> -- 
>>>> 2.7.4
>>>>


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Fri, Jan 27, 2017 at 05:50:36PM +0200, Oleksandr Andrushchenko wrote:
> thank you for comments, please find answers below
> 
> Can we please switch to v16 discussion as v15 vs v16 is
> a big change?

<shrugs>

This channel seemed appropiate to hash this out. I will
look at v16 next week (out of time for reviews for today).

See below.
> 
> On 01/27/2017 05:14 PM, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jan 26, 2017 at 12:02:49PM +0200, Oleksandr Andrushchenko wrote:
> > > Hi, Konrad!
> > > 
> > > First of all thank you very much for the valuable comments
> > > 
> > > and your time!
> > > 
> > > The number of changes (mostly in description) is going to
> > > 
> > > be huge, so do you think I can publish something like
> > > 
> > > "RFC v16" so we can discuss the updated patch?
> > RFC sadly means folks are going to mostly ignore it.
> > I would prefer you did not use RFC at this stage but just
> > did v16.
> > ..snip..
> sure
> > > > > + * Example for the frontend running in domain 5, instance of the driver
> > > > > + * in the front is 0 (single or first PV driver), device id 2,
> > > > > + * first stream (0):
> > > > > + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
> > > > > + *         device/<dev_id>/stream/<stream_idx>/type = "p"
> > > > > + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
> > > > Why do you need 'device' ?
> > > just for clarity: the hierarchy of the sound driver would
> > > be that a device may have multiple different streams.
> > And it just occured to me that you could also imply that
> > each device has an stream without the 'stream' in it.
> > 
> > So
> > 
> > /local/domain/5/device/vsnd/0/2/0/type = "p"
> > 
> > And the format is:
> > /local/domain/<front-id>/device/vsnd/<instance of PV driver>/<device-id>/<stream-id>
> ok, so we'll end up with something like:
> 
> --------------------------------- Backend
> -----------------------------------
> 
> /local/domain/0/backend/vsnd/1/0/frontend-id = "1"
> /local/domain/0/backend/vsnd/1/0/frontend = "/local/domain/1/device/vsnd/0"
> /local/domain/0/backend/vsnd/1/0/state = "4"
> /local/domain/0/backend/vsnd/1/0/versions = "1,2"


<nods>
> 
> ----------------------------- Card ----------------------------
> 
> /local/domain/1/device/vsnd/0/version = "1"
> /local/domain/1/device/vsnd/0/short-name = "Card short name"
> /local/domain/1/device/vsnd/0/long-name = "Card long name"
> /local/domain/1/device/vsnd/0/sample-rates = "8000,32000,44100,48000,96000"
> /local/domain/1/device/vsnd/0/sample-formats = "s8,u8,s16_le,s16_be"
> /local/domain/1/device/vsnd/0/buffer-size = "262144"

<nods>
> 
> ----------------------------- PCM device 0 ----------------------------
> 
> /local/domain/1/device/vsnd/0/0/name = "General analog"
> /local/domain/1/device/vsnd/0/0/channels-max = "5"

<nods>
> 
> ----------------------------- Stream 0, playback
> ----------------------------
> 
> /local/domain/1/device/vsnd/0/0/0/type = "p"
> /local/domain/1/device/vsnd/0/0/0/sample-formats = "s8,u8"
> /local/domain/1/device/vsnd/0/0/0/unique-id = "0"
> /local/domain/1/device/vsnd/0/0/0/ring-ref = "386"
> /local/domain/1/device/vsnd/0/0/0/event-channel = "15"

<nods>
> 
> ------------------------------- PCM device 3
> --------------------------------
> 
> /local/domain/1/device/vsnd/0/3/name = "HDMI-0"
> /local/domain/1/device/vsnd/0/3/sample-rates = "8000,32000,44100"

<nods>
> 
> ------------------------------ Stream 0, capture
> ----------------------------
> 
> /local/domain/1/device/vsnd/0/3/0/type = "c"
> /local/domain/1/device/vsnd/0/3/0/unique-id = "2"
> /local/domain/1/device/vsnd/0/3/0/ring-ref = "387"
> /local/domain/1/device/vsnd/0/3/0/event-channel = "151"
> 
> Is this what you would like to see?

Yes.
> IMO, all these values do not help understanding what it is, e.g.
> this is equal to me if we have
> 
> /local/domain/1/device/vbd/51712/queue-0/ring-ref = "8"
> /local/domain/1/device/vbd/51712/queue-0/event-channel = "3"
> /local/domain/1/device/vbd/51712/queue-1 = ""
> /local/domain/1/device/vbd/51712/queue-1/ring-ref = "9"
> 
> and then decided to go with
> 
> /local/domain/1/device/vbd/51712/0/ring-ref = "8"
> /local/domain/1/device/vbd/51712/0/event-channel = "3"
> /local/domain/1/device/vbd/51712/1/ring-ref = "9"
> 
> Can one easily tell what 0 or 1 after "51712/" is?

I do. But maybe my brain has been swimming in this too much?

> 
> So, what is the final decision then?

> 
> > Though I have a little of trouble with the 'instance of the
> > driver'. Are you suggesting you would have multiple
> > PV drivers of 'vsnd'? Can't the multiple device ids fulfill this?
> it is possible, but the main use-case will have a single
> PV driver with multiple PCM devices/streams

OK, so no need for this then.
.. snip..
> > > > > + * operation - XENSND_OP_SET_VOLUME for volume set
> > > > > + *   or XENSND_OP_GET_VOLUME for volume get
> > > > > + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
> > > > > + * values:
> > > > Oh. That means you these operation are in effect 'barrier' ones.
> > > > 
> > > > As the buffer must be flushed before hand otherwise you would be
> > > > overwriting data stream information.
> > > > 
> > > > You should probably mention this semantic need?
> > > I think this is implementation specific and shouldn't
> > > be in a generic protocol
> > How is that implementation specific? If there is something in the page
> > from the previous command you are overwritting those values.
> ok, all the operations are synchronous for the stream given.

Ah. I missed that. Other drivers can be asynchronous.
That is you can have multiple 'read' that are handled by
the backend - and the response to the 'read' can come
in any order.

Hence was thinking along those lines.

> it means that if there is something left in the buffer
> it will be overwritten by the next req/resp, so this is expected

Right, and that is my worry.

But you are saying that the 'response' MUST be given in the same
order as 'requests'. And that is not spelled out (it also
seems a bit limiting. Don't you want to be able to handle
in the future asynchronous responses?

> > 
> > > > Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
> > > > a similar structure to 'struct xensnd_rw_req' so that you have
> > > > the offset and len?
> > > a page can hold enough values, IMO
> > Let me see "640K ought to be enough"?
> I was talking about 256 channels 4 bytes per each.
> > 
> > You are making assumptions here based on how the implementation
> > fills out the data structure. But the purpose of the design
> > is to detach oneself from the implementation and think of
> > alternative ways.
> > 
> > To capture your words:
> > "
> > So if read/write use that buffer, and the volume and muting
> > > controls use it too, how do I change the volume while listening
> > > without disturbing the read/write?
> > read/write do not happen continuously, e.g. sound card fills its
> > internal buffers (our buffer is busy) and then until next re-fill our
> > buffer is free. that means that there is almost no congestion and
> > always a good chance to set/get volume w/o problem
> > > Jan
> > "
> > 
> > Well, that is implementation specific. What if some implementaiton
> > fills it back to back?
> > 
> > I would like you to add the 'offset' and 'len' so that we don't
> > dig a hole that we can't easily get out of.
> > 
> ok, I will add
>  * +----------------+----------------+----------------+----------------+
>  * | offset                               | 12
>  * +----------------+----------------+----------------+----------------+
>  * | length                               | 16
>  * +----------------+----------------+----------------+----------------+
> to
> 1.Request set/get volume - set/get channels' volume of the stream given
> 2.Request mute/unmute - mute/unmute stream
> 
> By this change you enable a use-case when part of the shared buffer
> is used for samples and part for volume/mute, right?

Correct.

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 01/27/2017 06:36 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 27, 2017 at 05:50:36PM +0200, Oleksandr Andrushchenko wrote:
>> thank you for comments, please find answers below
>>
>> Can we please switch to v16 discussion as v15 vs v16 is
>> a big change?
> <shrugs>
>
> This channel seemed appropiate to hash this out.
sorry about that
>   I will
> look at v16 next week (out of time for reviews for today).
thank you for your time
> See below.
>> On 01/27/2017 05:14 PM, Konrad Rzeszutek Wilk wrote:
>>> On Thu, Jan 26, 2017 at 12:02:49PM +0200, Oleksandr Andrushchenko wrote:
>>>> Hi, Konrad!
>>>>
>>>> First of all thank you very much for the valuable comments
>>>>
>>>> and your time!
>>>>
>>>> The number of changes (mostly in description) is going to
>>>>
>>>> be huge, so do you think I can publish something like
>>>>
>>>> "RFC v16" so we can discuss the updated patch?
>>> RFC sadly means folks are going to mostly ignore it.
>>> I would prefer you did not use RFC at this stage but just
>>> did v16.
>>> ..snip..
>> sure
>>>>>> + * Example for the frontend running in domain 5, instance of the driver
>>>>>> + * in the front is 0 (single or first PV driver), device id 2,
>>>>>> + * first stream (0):
>>>>>> + * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
>>>>>> + *         device/<dev_id>/stream/<stream_idx>/type = "p"
>>>>>> + * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
>>>>> Why do you need 'device' ?
>>>> just for clarity: the hierarchy of the sound driver would
>>>> be that a device may have multiple different streams.
>>> And it just occured to me that you could also imply that
>>> each device has an stream without the 'stream' in it.
>>>
>>> So
>>>
>>> /local/domain/5/device/vsnd/0/2/0/type = "p"
>>>
>>> And the format is:
>>> /local/domain/<front-id>/device/vsnd/<instance of PV driver>/<device-id>/<stream-id>
>> ok, so we'll end up with something like:
>>
>> --------------------------------- Backend
>> -----------------------------------
>>
>> /local/domain/0/backend/vsnd/1/0/frontend-id = "1"
>> /local/domain/0/backend/vsnd/1/0/frontend = "/local/domain/1/device/vsnd/0"
>> /local/domain/0/backend/vsnd/1/0/state = "4"
>> /local/domain/0/backend/vsnd/1/0/versions = "1,2"
>
> <nods>
>> ----------------------------- Card ----------------------------
>>
>> /local/domain/1/device/vsnd/0/version = "1"
>> /local/domain/1/device/vsnd/0/short-name = "Card short name"
>> /local/domain/1/device/vsnd/0/long-name = "Card long name"
>> /local/domain/1/device/vsnd/0/sample-rates = "8000,32000,44100,48000,96000"
>> /local/domain/1/device/vsnd/0/sample-formats = "s8,u8,s16_le,s16_be"
>> /local/domain/1/device/vsnd/0/buffer-size = "262144"
> <nods>
>> ----------------------------- PCM device 0 ----------------------------
>>
>> /local/domain/1/device/vsnd/0/0/name = "General analog"
>> /local/domain/1/device/vsnd/0/0/channels-max = "5"
> <nods>
>> ----------------------------- Stream 0, playback
>> ----------------------------
>>
>> /local/domain/1/device/vsnd/0/0/0/type = "p"
>> /local/domain/1/device/vsnd/0/0/0/sample-formats = "s8,u8"
>> /local/domain/1/device/vsnd/0/0/0/unique-id = "0"
>> /local/domain/1/device/vsnd/0/0/0/ring-ref = "386"
>> /local/domain/1/device/vsnd/0/0/0/event-channel = "15"
> <nods>
>> ------------------------------- PCM device 3
>> --------------------------------
>>
>> /local/domain/1/device/vsnd/0/3/name = "HDMI-0"
>> /local/domain/1/device/vsnd/0/3/sample-rates = "8000,32000,44100"
> <nods>
>> ------------------------------ Stream 0, capture
>> ----------------------------
>>
>> /local/domain/1/device/vsnd/0/3/0/type = "c"
>> /local/domain/1/device/vsnd/0/3/0/unique-id = "2"
>> /local/domain/1/device/vsnd/0/3/0/ring-ref = "387"
>> /local/domain/1/device/vsnd/0/3/0/event-channel = "151"
>>
>> Is this what you would like to see?
> Yes.
ok, then I will use 
"/local/domain/1/device/vsnd/<driver-idx>/<pcm-device-id>/<stream-idx>/"
as the pattern, no "device", "stream" or whatever
>> IMO, all these values do not help understanding what it is, e.g.
>> this is equal to me if we have
>>
>> /local/domain/1/device/vbd/51712/queue-0/ring-ref = "8"
>> /local/domain/1/device/vbd/51712/queue-0/event-channel = "3"
>> /local/domain/1/device/vbd/51712/queue-1 = ""
>> /local/domain/1/device/vbd/51712/queue-1/ring-ref = "9"
>>
>> and then decided to go with
>>
>> /local/domain/1/device/vbd/51712/0/ring-ref = "8"
>> /local/domain/1/device/vbd/51712/0/event-channel = "3"
>> /local/domain/1/device/vbd/51712/1/ring-ref = "9"
>>
>> Can one easily tell what 0 or 1 after "51712/" is?
> I do. But maybe my brain has been swimming in this too much?
I am looking at this from FAE's or integrator's POV, when one would need
to touch different parts of the system. "/0/0/0" makes me feel
sad just because either you have to keep all those numbers in mind (like 
you do)
or have documentation available (and know where it is, e.g. sources
of Xen or kernel).
I have a strong feeling that if you can change configuration without
knowing what index stands for it is always better and fail-safer then
just having numbers...
>> So, what is the final decision then?
>>> Though I have a little of trouble with the 'instance of the
>>> driver'. Are you suggesting you would have multiple
>>> PV drivers of 'vsnd'? Can't the multiple device ids fulfill this?
>> it is possible, but the main use-case will have a single
>> PV driver with multiple PCM devices/streams
> OK, so no need for this then.
> .. snip..
>>>>>> + * operation - XENSND_OP_SET_VOLUME for volume set
>>>>>> + *   or XENSND_OP_GET_VOLUME for volume get
>>>>>> + * Buffer passed with XENSND_OP_OPEN is used to exchange volume
>>>>>> + * values:
>>>>> Oh. That means you these operation are in effect 'barrier' ones.
>>>>>
>>>>> As the buffer must be flushed before hand otherwise you would be
>>>>> overwriting data stream information.
>>>>>
>>>>> You should probably mention this semantic need?
>>>> I think this is implementation specific and shouldn't
>>>> be in a generic protocol
>>> How is that implementation specific? If there is something in the page
>>> from the previous command you are overwritting those values.
>> ok, all the operations are synchronous for the stream given.
> Ah. I missed that. Other drivers can be asynchronous.
> That is you can have multiple 'read' that are handled by
> the backend - and the response to the 'read' can come
> in any order.
>
> Hence was thinking along those lines.
got you
>> it means that if there is something left in the buffer
>> it will be overwritten by the next req/resp, so this is expected
> Right, and that is my worry.
>
> But you are saying that the 'response' MUST be given in the same
> order as 'requests'. And that is not spelled out (it also
> seems a bit limiting. Don't you want to be able to handle
> in the future asynchronous responses?
if we have offset/len then we are on a safe side with async io
>>>>> Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
>>>>> a similar structure to 'struct xensnd_rw_req' so that you have
>>>>> the offset and len?
>>>> a page can hold enough values, IMO
>>> Let me see "640K ought to be enough"?
>> I was talking about 256 channels 4 bytes per each.
>>> You are making assumptions here based on how the implementation
>>> fills out the data structure. But the purpose of the design
>>> is to detach oneself from the implementation and think of
>>> alternative ways.
>>>
>>> To capture your words:
>>> "
>>> So if read/write use that buffer, and the volume and muting
>>>> controls use it too, how do I change the volume while listening
>>>> without disturbing the read/write?
>>> read/write do not happen continuously, e.g. sound card fills its
>>> internal buffers (our buffer is busy) and then until next re-fill our
>>> buffer is free. that means that there is almost no congestion and
>>> always a good chance to set/get volume w/o problem
>>>> Jan
>>> "
>>>
>>> Well, that is implementation specific. What if some implementaiton
>>> fills it back to back?
>>>
>>> I would like you to add the 'offset' and 'len' so that we don't
>>> dig a hole that we can't easily get out of.
>>>
>> ok, I will add
>>   * +----------------+----------------+----------------+----------------+
>>   * | offset                               | 12
>>   * +----------------+----------------+----------------+----------------+
>>   * | length                               | 16
>>   * +----------------+----------------+----------------+----------------+
>> to
>> 1.Request set/get volume - set/get channels' volume of the stream given
>> 2.Request mute/unmute - mute/unmute stream
>>
>> By this change you enable a use-case when part of the shared buffer
>> is used for samples and part for volume/mute, right?
> Correct.
ok, then

struct xensnd_rw_req {
     uint32_t offset;
     uint32_t len;
};
covers all the requests, but open/close
Do you want me to keep the same structure name (xensnd_rw_req)
or rename it to something like xensnd_io_req?

Thank you,
Oleksandr

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

.snip..
> I am looking at this from FAE's or integrator's POV, when one would need

FAE?

> to touch different parts of the system. "/0/0/0" makes me feel
> sad just because either you have to keep all those numbers in mind (like you
> do)
> or have documentation available (and know where it is, e.g. sources
> of Xen or kernel).
> I have a strong feeling that if you can change configuration without
> knowing what index stands for it is always better and fail-safer then
> just having numbers...

Not sure I follow that.

How would you change configuration without knowing the index?

..snip..
> ok, then
> 
> struct xensnd_rw_req {
>     uint32_t offset;
>     uint32_t len;
> };
> covers all the requests, but open/close
> Do you want me to keep the same structure name (xensnd_rw_req)
> or rename it to something like xensnd_io_req?

The name is fine.
> 
> Thank you,
> Oleksandr

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 01/27/2017 08:13 PM, Konrad Rzeszutek Wilk wrote:
> .snip..
>> I am looking at this from FAE's or integrator's POV, when one would need
> FAE?
>
Field Applications Engineer
>> to touch different parts of the system. "/0/0/0" makes me feel
>> sad just because either you have to keep all those numbers in mind (like you
>> do)
>> or have documentation available (and know where it is, e.g. sources
>> of Xen or kernel).
>> I have a strong feeling that if you can change configuration without
>> knowing what index stands for it is always better and fail-safer then
>> just having numbers...
> Not sure I follow that.
>
> How would you change configuration without knowing the index?
>
> ..snip..
if one looks at
.../pcm-dev-0/stream-1/...
most probably he/she will understand this w/o any documentation,
because it is human readable

if one looks at
.../0/1/...
well, I believe you can almost do nothing w/o looking at the documentation
>> ok, then
>>
>> struct xensnd_rw_req {
>>      uint32_t offset;
>>      uint32_t len;
>> };
>> covers all the requests, but open/close
>> Do you want me to keep the same structure name (xensnd_rw_req)
>> or rename it to something like xensnd_io_req?
> The name is fine.
>> Thank you,
>> Oleksandr


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Fri, Jan 27, 2017 at 08:38:29PM +0200, Oleksandr Andrushchenko wrote:
> 
> 
> On 01/27/2017 08:13 PM, Konrad Rzeszutek Wilk wrote:
> > .snip..
> > > I am looking at this from FAE's or integrator's POV, when one would need
> > FAE?
> > 
> Field Applications Engineer
> > > to touch different parts of the system. "/0/0/0" makes me feel
> > > sad just because either you have to keep all those numbers in mind (like you
> > > do)
> > > or have documentation available (and know where it is, e.g. sources
> > > of Xen or kernel).
> > > I have a strong feeling that if you can change configuration without
> > > knowing what index stands for it is always better and fail-safer then
> > > just having numbers...
> > Not sure I follow that.
> > 
> > How would you change configuration without knowing the index?
> > 
> > ..snip..
> if one looks at
> .../pcm-dev-0/stream-1/...
> most probably he/she will understand this w/o any documentation,
> because it is human readable
> 
> if one looks at
> .../0/1/...
> well, I believe you can almost do nothing w/o looking at the documentation

I can see the beaty of it.

I can also see the beaty of the old implied mechanism
from a maintaince perspective.

I am maintainer so I am leaning towards the second one
as having less "special" cases.

Sorry, I feel like I am mutilating your baby with this
old boring view of "maintaince" and "conform to the old
standard" :-(


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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 01/27/2017 08:57 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jan 27, 2017 at 08:38:29PM +0200, Oleksandr Andrushchenko wrote:
>>
>> On 01/27/2017 08:13 PM, Konrad Rzeszutek Wilk wrote:
>>> .snip..
>>>> I am looking at this from FAE's or integrator's POV, when one would need
>>> FAE?
>>>
>> Field Applications Engineer
>>>> to touch different parts of the system. "/0/0/0" makes me feel
>>>> sad just because either you have to keep all those numbers in mind (like you
>>>> do)
>>>> or have documentation available (and know where it is, e.g. sources
>>>> of Xen or kernel).
>>>> I have a strong feeling that if you can change configuration without
>>>> knowing what index stands for it is always better and fail-safer then
>>>> just having numbers...
>>> Not sure I follow that.
>>>
>>> How would you change configuration without knowing the index?
>>>
>>> ..snip..
>> if one looks at
>> .../pcm-dev-0/stream-1/...
>> most probably he/she will understand this w/o any documentation,
>> because it is human readable
>>
>> if one looks at
>> .../0/1/...
>> well, I believe you can almost do nothing w/o looking at the documentation
> I can see the beaty of it.
>
> I can also see the beaty of the old implied mechanism
> from a maintaince perspective.
>
> I am maintainer so I am leaning towards the second one
> as having less "special" cases.
>
> Sorry, I feel like I am mutilating your baby with this
> old boring view of "maintaince" and "conform to the old
> standard" :-(
>
np, so I will use ".../0/0/0/..." pattern here and in
displif as well. I will update "Addressing" section
so it is documented

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Konrad Rzeszutek Wilk]

On Mon, Jan 30, 2017 at 08:49:50AM +0200, Oleksandr Andrushchenko wrote:
> On 01/27/2017 08:57 PM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jan 27, 2017 at 08:38:29PM +0200, Oleksandr Andrushchenko wrote:
> > > 
> > > On 01/27/2017 08:13 PM, Konrad Rzeszutek Wilk wrote:
> > > > .snip..
> > > > > I am looking at this from FAE's or integrator's POV, when one would need
> > > > FAE?
> > > > 
> > > Field Applications Engineer
> > > > > to touch different parts of the system. "/0/0/0" makes me feel
> > > > > sad just because either you have to keep all those numbers in mind (like you
> > > > > do)
> > > > > or have documentation available (and know where it is, e.g. sources
> > > > > of Xen or kernel).
> > > > > I have a strong feeling that if you can change configuration without
> > > > > knowing what index stands for it is always better and fail-safer then
> > > > > just having numbers...
> > > > Not sure I follow that.
> > > > 
> > > > How would you change configuration without knowing the index?
> > > > 
> > > > ..snip..
> > > if one looks at
> > > .../pcm-dev-0/stream-1/...
> > > most probably he/she will understand this w/o any documentation,
> > > because it is human readable
> > > 
> > > if one looks at
> > > .../0/1/...
> > > well, I believe you can almost do nothing w/o looking at the documentation
> > I can see the beaty of it.
> > 
> > I can also see the beaty of the old implied mechanism
> > from a maintaince perspective.
> > 
> > I am maintainer so I am leaning towards the second one
> > as having less "special" cases.
> > 
> > Sorry, I feel like I am mutilating your baby with this
> > old boring view of "maintaince" and "conform to the old
> > standard" :-(
> > 
> np, so I will use ".../0/0/0/..." pattern here and in
> displif as well. I will update "Addressing" section
> so it is documented

OK, so I will ignore v16 as of right now it has the old format.
Please at your convience post v16.1 (or v17) with this change.

Thanks!

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

Re: [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Oleksandr Andrushchenko]

On 02/07/2017 09:16 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 30, 2017 at 08:49:50AM +0200, Oleksandr Andrushchenko wrote:
>> On 01/27/2017 08:57 PM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jan 27, 2017 at 08:38:29PM +0200, Oleksandr Andrushchenko wrote:
>>>> On 01/27/2017 08:13 PM, Konrad Rzeszutek Wilk wrote:
>>>>> .snip..
>>>>>> I am looking at this from FAE's or integrator's POV, when one would need
>>>>> FAE?
>>>>>
>>>> Field Applications Engineer
>>>>>> to touch different parts of the system. "/0/0/0" makes me feel
>>>>>> sad just because either you have to keep all those numbers in mind (like you
>>>>>> do)
>>>>>> or have documentation available (and know where it is, e.g. sources
>>>>>> of Xen or kernel).
>>>>>> I have a strong feeling that if you can change configuration without
>>>>>> knowing what index stands for it is always better and fail-safer then
>>>>>> just having numbers...
>>>>> Not sure I follow that.
>>>>>
>>>>> How would you change configuration without knowing the index?
>>>>>
>>>>> ..snip..
>>>> if one looks at
>>>> .../pcm-dev-0/stream-1/...
>>>> most probably he/she will understand this w/o any documentation,
>>>> because it is human readable
>>>>
>>>> if one looks at
>>>> .../0/1/...
>>>> well, I believe you can almost do nothing w/o looking at the documentation
>>> I can see the beaty of it.
>>>
>>> I can also see the beaty of the old implied mechanism
>>> from a maintaince perspective.
>>>
>>> I am maintainer so I am leaning towards the second one
>>> as having less "special" cases.
>>>
>>> Sorry, I feel like I am mutilating your baby with this
>>> old boring view of "maintaince" and "conform to the old
>>> standard" :-(
>>>
>> np, so I will use ".../0/0/0/..." pattern here and in
>> displif as well. I will update "Addressing" section
>> so it is documented
> OK, so I will ignore v16 as of right now it has the old format.
> Please at your convience post v16.1 (or v17) with this change.
v17
> Thanks!
Thank you

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

Re: [PATCH v15] sndif: add ABI for para-virtual sound

[Oleksandr Andrushchenko]

[-- Attachment #1.1: Type: text/plain, Size: 668 bytes --]

As agreed on PV call PFA pahole results


On 12/05/2016 03:05 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
>
> Hi, all!
>
> Please find the next version of the ABI for the PV sound
> after addressing review comments.
>
> Thank you,
> Oleksandr Andrushchenko
> Oleksandr Grytsov
>
> Oleksandr Andrushchenko (1):
>    This is the ABI for the two halves of a para-virtualized     sound
>      driver to communicate with each to other.
>
>   xen/include/public/io/sndif.h | 671 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 671 insertions(+)
>   create mode 100644 xen/include/public/io/sndif.h
>


[-- Attachment #1.2: Type: text/html, Size: 1181 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sndif.diff --]
[-- Type: text/x-patch; name="sndif.diff", Size: 0 bytes --]



[-- Attachment #3: sndif-x32.txt --]
[-- Type: text/plain, Size: 2063 bytes --]

struct xensnd_open_req {
	uint32_t                   pcm_rate;             /*     0     4 */
	uint8_t                    pcm_format;           /*     4     1 */
	uint8_t                    pcm_channels;         /*     5     1 */
	uint16_t                   reserved;             /*     6     2 */
	uint32_t                   buffer_sz;            /*     8     4 */
	grant_ref_t                gref_directory_start; /*    12     4 */

	/* size: 16, cachelines: 1, members: 6 */
	/* last cacheline: 16 bytes */
};
struct xensnd_page_directory {
	grant_ref_t                gref_dir_next_page;   /*     0     4 */
	grant_ref_t                gref[1];              /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};
struct xensnd_rw_req {
	uint32_t                   offset;               /*     0     4 */
	uint32_t                   len;                  /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};
struct xensnd_req {
	uint16_t                   id;                   /*     0     2 */
	uint8_t                    operation;            /*     2     1 */
	uint8_t                    stream_idx;           /*     3     1 */
	uint32_t                   reserved;             /*     4     4 */
	union {
		struct xensnd_open_req open;             /*          16 */
		struct xensnd_rw_req rw;                 /*           8 */
		uint8_t            reserved[24];         /*          24 */
	} op;                                            /*     8    24 */

	/* size: 32, cachelines: 1, members: 5 */
	/* last cacheline: 32 bytes */
};
struct xensnd_resp {
	uint16_t                   id;                   /*     0     2 */
	uint8_t                    operation;            /*     2     1 */
	uint8_t                    stream_idx;           /*     3     1 */
	int8_t                     status;               /*     4     1 */
	uint8_t                    reserved[27];         /*     5    27 */

	/* size: 32, cachelines: 1, members: 5 */
	/* last cacheline: 32 bytes */
};

[-- Attachment #4: sndif-x64.txt --]
[-- Type: text/plain, Size: 2063 bytes --]

struct xensnd_open_req {
	uint32_t                   pcm_rate;             /*     0     4 */
	uint8_t                    pcm_format;           /*     4     1 */
	uint8_t                    pcm_channels;         /*     5     1 */
	uint16_t                   reserved;             /*     6     2 */
	uint32_t                   buffer_sz;            /*     8     4 */
	grant_ref_t                gref_directory_start; /*    12     4 */

	/* size: 16, cachelines: 1, members: 6 */
	/* last cacheline: 16 bytes */
};
struct xensnd_page_directory {
	grant_ref_t                gref_dir_next_page;   /*     0     4 */
	grant_ref_t                gref[1];              /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};
struct xensnd_rw_req {
	uint32_t                   offset;               /*     0     4 */
	uint32_t                   len;                  /*     4     4 */

	/* size: 8, cachelines: 1, members: 2 */
	/* last cacheline: 8 bytes */
};
struct xensnd_req {
	uint16_t                   id;                   /*     0     2 */
	uint8_t                    operation;            /*     2     1 */
	uint8_t                    stream_idx;           /*     3     1 */
	uint32_t                   reserved;             /*     4     4 */
	union {
		struct xensnd_open_req open;             /*          16 */
		struct xensnd_rw_req rw;                 /*           8 */
		uint8_t            reserved[24];         /*          24 */
	} op;                                            /*     8    24 */

	/* size: 32, cachelines: 1, members: 5 */
	/* last cacheline: 32 bytes */
};
struct xensnd_resp {
	uint16_t                   id;                   /*     0     2 */
	uint8_t                    operation;            /*     2     1 */
	uint8_t                    stream_idx;           /*     3     1 */
	int8_t                     status;               /*     4     1 */
	uint8_t                    reserved[27];         /*     5    27 */

	/* size: 32, cachelines: 1, members: 5 */
	/* last cacheline: 32 bytes */
};

[-- Attachment #5: Type: text/plain, Size: 127 bytes --]

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