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

[PATCH v12] 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 | 709 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 709 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 v12] 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__
---
---
 xen/include/public/io/sndif.h | 709 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 709 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..2d7784e
--- /dev/null
+++ b/xen/include/public/io/sndif.h
@@ -0,0 +1,709 @@
+/******************************************************************************
+ * 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
+
+/*
+ * Description of the protocol between frontend and backend driver.
+ *
+ * The two halves of a Para-virtual sound driver communicates with
+ * each other using a shared page and an event channel.
+ * Shared page contains a ring with request/response packets.
+ *
+ *
+ * All request packets have the same length (64 octets)
+ *
+ *
+ * Request open - open a PCM stream for playback or capture:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                pcm_rate                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |  pcm_format     |  pcm_channels   |             reserved              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                         gref_directory_start                          |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_OPEN
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * pcm_rate - uint32_t, stream data rate, Hz
+ * pcm_format - uint8_t, XENSND_PCM_FORMAT_XXX value
+ * pcm_channels - uint8_t, number of channels of this stream
+ * gref_directory_start - grant_ref_t, a reference to the first shared page
+ *   describing shared buffer references. At least one page exists. If shared
+ *   buffer size exceeds what can be addressed by this single page, then
+ *   reference to the next page must be supplied (gref_dir_next_page below
+ *   is not NULL)
+ */
+
+struct xensnd_open_req {
+    uint32_t pcm_rate; /* in Hz */
+    uint8_t pcm_format;
+    uint8_t pcm_channels;
+    uint16_t reserved;
+    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                           |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                num_grefs                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[0]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[1]                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                gref[N -1]                             |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * gref_dir_next_page - grant_ref_t, reference to the next page describing
+ *   page directory
+ * num_grefs - number of grefs in this page
+ * gref[i] - grant_ref_t, reference to a shared page of the buffer
+ *   allocated at XENSND_OP_OPEN
+ */
+
+struct xensnd_page_directory {
+    grant_ref_t gref_dir_next_page;
+    uint32_t num_grefs;
+    /* zero-sized arrays are not allowed */
+    grant_ref_t gref[1] /* Variable length */
+};
+
+/*
+ *  Request close - close an opened pcm stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_CLOSE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ */
+
+struct xensnd_close_req {
+    /* place holder, remove if changing the structure */
+    uint8_t placeholder;
+};
+
+/*
+ * Request read/write - used for read (for capture) or write (for playback):
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                offset                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                length                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_READ/XENSND_OP_WRITE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * offset - uint32_t, read or write data offset within the shared buffer
+ *   passed with XENSND_OP_OPEN request
+ * length - uint32_t, read or write data length
+ */
+
+struct xensnd_write_req {
+    uint32_t offset;
+    uint32_t len;
+};
+
+struct xensnd_read_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  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_SET_VOLUME or XENSND_OP_GET_VOLUME
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * Buffer passed with XENSND_OP_OPEN is used to exchange volume
+ * values:
+ *
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[0]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               channel[1]                              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ *                  channel[XENSND_OP_OPEN.pcm_channels - 1]               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * channel[i] - sint32_t, volume of i-th channel
+ * Volume is expressed as a signed value in steps of 0.001 dB,
+ * while 0 being 0 dB.
+ */
+
+struct xensnd_get_vol_req {
+    /* place holder, remove if changing the structure */
+    uint8_t placeholder;
+};
+
+struct xensnd_set_vol_req {
+    /* place holder, remove if changing the structure */
+    uint8_t placeholder;
+};
+
+/*
+ * Request mute/unmute - mute/unmute stream:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_MUTE/XENSND_OP_UNMUTE
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
+ * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
+ * values:
+ *
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
+ * Number of channels passed is equal to XENSND_OP_OPEN request pcm_channels
+ * field
+ */
+
+struct xensnd_mute_req {
+    /* place holder, remove if changing the structure */
+    uint8_t placeholder;
+};
+
+struct xensnd_unmute_req {
+    /* place holder, remove if changing the structure */
+    uint8_t placeholder;
+};
+
+/*
+ * All response packets have the same length (64 bytes)
+ *
+ * Response for all requests:
+ *          0                 1                  2                3        octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |      status     |                      reserved                       |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                              reserved                                 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
+ *
+ * id - uint16_t, copied from the request
+ * stream_idx - uint8_t, copied from request
+ * operation - uint8_t, XENSND_OP_XXX - copied from request
+ * status - int8_t, response status (XENSND_RSP_???)
+ */
+
+union xensnd_req {
+    struct {
+        uint16_t id;
+        uint8_t operation;
+        uint8_t stream_idx;
+        uint32_t padding;
+        union {
+            struct xensnd_open_req open;
+            struct xensnd_close_req close;
+            struct xensnd_write_req write;
+            struct xensnd_read_req read;
+            struct xensnd_get_vol_req get_vol;
+            struct xensnd_set_vol_req set_vol;
+            struct xensnd_mute_req mute;
+            struct xensnd_unmute_req unmute;
+        } op;
+    } data;
+    uint8_t raw[64];
+};
+
+union xensnd_resp {
+    struct {
+        uint16_t id;
+        uint8_t operation;
+        uint8_t stream_idx;
+        int8_t status;
+        uint8_t padding[3];
+    } data;
+    uint8_t raw[64];
+};
+
+DEFINE_RING_TYPES(xen_sndif, union xensnd_req, union 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 v12] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.

[Jan Beulich]

>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
> + * Request open - open a PCM stream for playback or capture:
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |                 id                |    operation    |     stream_idx  |
> + * +-----------------+-----------------+-----------------+-----------------+

I think individual requests would better spell out their values, instead
of all saying "operation".

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

Please explain the common header fields just once, instead of for
each of the commands.

> + * gref_directory_start - grant_ref_t, a reference to the first shared page
> + *   describing shared buffer references. At least one page exists. If shared
> + *   buffer size exceeds what can be addressed by this single page, then
> + *   reference to the next page must be supplied (gref_dir_next_page below
> + *   is not NULL)

NULL is a pointer value. You mean zero (or one of its synonyms).

> +struct xensnd_page_directory {
> +    grant_ref_t gref_dir_next_page;
> +    uint32_t num_grefs;

You can't fit that many requests on one page anyway, so I think it
would be better to limit this to 16 bits, and leave 16 bits reserved
for future use (specified as must-be-zero). (Which made me look
back at other reserved fields: You them "padding" in the ASCII art,
but "reserved" in the structure definitions, and I don't think I've
found a place stating that they're required to be zero filled, again
for future extensibility.)

> +    /* zero-sized arrays are not allowed */
> +    grant_ref_t gref[1] /* Variable length */

The second comment is fine; I once again don't think the former
(describing C language aspects again) is needed.

> +struct xensnd_write_req {
> +    uint32_t offset;
> +    uint32_t len;
> +};
> +
> +struct xensnd_read_req {
> +    uint32_t offset;
> +    uint32_t len;
> +};

There should be just one of them, possibly named xensnd_rw_req.

> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
> + * values:
> + *
> + *          0                 1                  2                3        octet
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
> + * +-----------------+-----------------+-----------------+-----------------+
> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
> + * +-----------------+-----------------+-----------------+-----------------+

There's no i defined anywhere here. Did you mean the last one to
be XENSND_OP_OPEN.pcm_channels - 1 (just like for the two volume
operations)?

> +union xensnd_req {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        uint32_t padding;

reserved (I don't see why you'd need padding here)

> +        union {
> +            struct xensnd_open_req open;
> +            struct xensnd_close_req close;
> +            struct xensnd_write_req write;
> +            struct xensnd_read_req read;
> +            struct xensnd_get_vol_req get_vol;
> +            struct xensnd_set_vol_req set_vol;
> +            struct xensnd_mute_req mute;
> +            struct xensnd_unmute_req unmute;
> +        } op;
> +    } data;
> +    uint8_t raw[64];

This is still misplaced imo, and belongs into the inner union, with the
array size suitably reduced. After all the initial part is mean to be
common for all verbs aiui, i.e. including future ones.

> +union xensnd_resp {
> +    struct {
> +        uint16_t id;
> +        uint8_t operation;
> +        uint8_t stream_idx;
> +        int8_t status;
> +        uint8_t padding[3];
> +    } data;
> +    uint8_t raw[64];

And here you don't need the field at all - just make the padding field
large enough.

Jan

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

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

[Oleksandr Andrushchenko]

On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>> + * Request open - open a PCM stream for playback or capture:
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |                 id                |    operation    |     stream_idx  |
>> + * +-----------------+-----------------+-----------------+-----------------+
> I think individual requests would better spell out their values, instead
> of all saying "operation".
done, but for those operations like read/write I'll still have operation 
field
in the structure and have description like:
  * operation is equal to XENSND_OP_READ for read or XENSND_OP_WRITE for 
write
>> + * id - uint16_t, private guest value, echoed in response
>> + * operation - uint8_t, XENSND_OP_OPEN
>> + * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
>> + *   of the stream)
> Please explain the common header fields just once, instead of for
> each of the commands.
done
>> + * gref_directory_start - grant_ref_t, a reference to the first shared page
>> + *   describing shared buffer references. At least one page exists. If shared
>> + *   buffer size exceeds what can be addressed by this single page, then
>> + *   reference to the next page must be supplied (gref_dir_next_page below
>> + *   is not NULL)
> NULL is a pointer value. You mean zero (or one of its synonyms).
"is not 0"
>
>> +struct xensnd_page_directory {
>> +    grant_ref_t gref_dir_next_page;
>> +    uint32_t num_grefs;
> You can't fit that many requests on one page anyway, so I think it
> would be better to limit this to 16 bits, and leave 16 bits reserved
> for future use (specified as must-be-zero).
these are not for requests, but for buffers. you can imagine the size of 
this
buffer if we play something like 8 channels 192kHz...
> (Which made me look
> back at other reserved fields: You them "padding" in the ASCII art,
> but "reserved" in the structure definitions, and I don't think I've
> found a place stating that they're required to be zero filled, again
> for future extensibility.)
done
>> +    /* zero-sized arrays are not allowed */
>> +    grant_ref_t gref[1] /* Variable length */
> The second comment is fine; I once again don't think the former
> (describing C language aspects again) is needed.
done
>> +struct xensnd_write_req {
>> +    uint32_t offset;
>> +    uint32_t len;
>> +};
>> +
>> +struct xensnd_read_req {
>> +    uint32_t offset;
>> +    uint32_t len;
>> +};
> There should be just one of them, possibly named xensnd_rw_req.
agree
>> + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
>> + * values:
>> + *
>> + *          0                 1                  2                3        octet
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>> + * +-----------------+-----------------+-----------------+-----------------+
>> + * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
>> + * +-----------------+-----------------+-----------------+-----------------+
> There's no i defined anywhere here. Did you mean the last one to
> be XENSND_OP_OPEN.pcm_channels - 1 (just like for the two volume
> operations)?
fixed
>> +union xensnd_req {
>> +    struct {
>> +        uint16_t id;
>> +        uint8_t operation;
>> +        uint8_t stream_idx;
>> +        uint32_t padding;
> reserved (I don't see why you'd need padding here)
done
>> +        union {
>> +            struct xensnd_open_req open;
>> +            struct xensnd_close_req close;
>> +            struct xensnd_write_req write;
>> +            struct xensnd_read_req read;
>> +            struct xensnd_get_vol_req get_vol;
>> +            struct xensnd_set_vol_req set_vol;
>> +            struct xensnd_mute_req mute;
>> +            struct xensnd_unmute_req unmute;
>> +        } op;
>> +    } data;
>> +    uint8_t raw[64];
BTW, I missed a change of 64 to 32 as discussed and agreed before
> This is still misplaced imo, and belongs into the inner union, with the
> array size suitably reduced. After all the initial part is mean to be
> common for all verbs aiui, i.e. including future ones.
if we put this in then you have to find the biggest structure and 
calculate its size so
you know how many bytes need to be in that padding array
if you change something you have to redo the same again and beware
of the fact that this is needed
>> +union xensnd_resp {
>> +    struct {
>> +        uint16_t id;
>> +        uint8_t operation;
>> +        uint8_t stream_idx;
>> +        int8_t status;
>> +        uint8_t padding[3];
>> +    } data;
>> +    uint8_t raw[64];
> And here you don't need the field at all - just make the padding field
> large enough.
I can do it, but my concern is just like above
So, I would have the raw arrays where they are and only change their 
sizes to 32
>
> Jan


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

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

[Jan Beulich]

>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>> +struct xensnd_page_directory {
>>> +    grant_ref_t gref_dir_next_page;
>>> +    uint32_t num_grefs;
>> You can't fit that many requests on one page anyway, so I think it
>> would be better to limit this to 16 bits, and leave 16 bits reserved
>> for future use (specified as must-be-zero).
> these are not for requests, but for buffers. you can imagine the size of 
> this
> buffer if we play something like 8 channels 192kHz...

I don't understand: You have one such entry per page, so how
can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?

>>> +union xensnd_req {
>>> +    struct {
>>> +        uint16_t id;
>>> +        uint8_t operation;
>>> +        uint8_t stream_idx;
>>> +        uint32_t padding;
>>> +        union {
>>> +            struct xensnd_open_req open;
>>> +            struct xensnd_close_req close;
>>> +            struct xensnd_write_req write;
>>> +            struct xensnd_read_req read;
>>> +            struct xensnd_get_vol_req get_vol;
>>> +            struct xensnd_set_vol_req set_vol;
>>> +            struct xensnd_mute_req mute;
>>> +            struct xensnd_unmute_req unmute;
>>> +        } op;
>>> +    } data;
>>> +    uint8_t raw[64];
>> This is still misplaced imo, and belongs into the inner union, with the
>> array size suitably reduced. After all the initial part is mean to be
>> common for all verbs aiui, i.e. including future ones.
> if we put this in then you have to find the biggest structure and 
> calculate its size so
> you know how many bytes need to be in that padding array
> if you change something you have to redo the same again and beware
> of the fact that this is needed

No, that's why I've asked for it to be a member of the inner union.

Jan


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

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

[Oleksandr Andrushchenko]

On 11/25/2016 04:12 PM, Jan Beulich wrote:
>>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
>> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>>> +struct xensnd_page_directory {
>>>> +    grant_ref_t gref_dir_next_page;
>>>> +    uint32_t num_grefs;
>>> You can't fit that many requests on one page anyway, so I think it
>>> would be better to limit this to 16 bits, and leave 16 bits reserved
>>> for future use (specified as must-be-zero).
>> these are not for requests, but for buffers. you can imagine the size of
>> this
>> buffer if we play something like 8 channels 192kHz...
> I don't understand: You have one such entry per page, so how
> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
we employ the idea of a linked list here, e.g. gref_dir_next_page points
to the next page of the same structure.
if there are no more pages with grefs then num_grefs will not be equal to
(PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) / 
sizeof(grant_ref_t),
but will contain the remaining number of grefs in this page
this way we can flexibly allocate and reference buffers of any requested 
size
>
>>>> +union xensnd_req {
>>>> +    struct {
>>>> +        uint16_t id;
>>>> +        uint8_t operation;
>>>> +        uint8_t stream_idx;
>>>> +        uint32_t padding;
>>>> +        union {
>>>> +            struct xensnd_open_req open;
>>>> +            struct xensnd_close_req close;
>>>> +            struct xensnd_write_req write;
>>>> +            struct xensnd_read_req read;
>>>> +            struct xensnd_get_vol_req get_vol;
>>>> +            struct xensnd_set_vol_req set_vol;
>>>> +            struct xensnd_mute_req mute;
>>>> +            struct xensnd_unmute_req unmute;
>>>> +        } op;
>>>> +    } data;
>>>> +    uint8_t raw[64];
>>> This is still misplaced imo, and belongs into the inner union, with the
>>> array size suitably reduced. After all the initial part is mean to be
>>> common for all verbs aiui, i.e. including future ones.
>> if we put this in then you have to find the biggest structure and
>> calculate its size so
>> you know how many bytes need to be in that padding array
>> if you change something you have to redo the same again and beware
>> of the fact that this is needed
> No, that's why I've asked for it to be a member of the inner union.
got you
>
> Jan
>


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

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

[Jan Beulich]

>>> On 25.11.16 at 15:27, <andr2000@gmail.com> wrote:
> On 11/25/2016 04:12 PM, Jan Beulich wrote:
>>>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
>>> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>>>> +struct xensnd_page_directory {
>>>>> +    grant_ref_t gref_dir_next_page;
>>>>> +    uint32_t num_grefs;
>>>> You can't fit that many requests on one page anyway, so I think it
>>>> would be better to limit this to 16 bits, and leave 16 bits reserved
>>>> for future use (specified as must-be-zero).
>>> these are not for requests, but for buffers. you can imagine the size of
>>> this
>>> buffer if we play something like 8 channels 192kHz...
>> I don't understand: You have one such entry per page, so how
>> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
> we employ the idea of a linked list here, e.g. gref_dir_next_page points
> to the next page of the same structure.
> if there are no more pages with grefs then num_grefs will not be equal to
> (PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) / 
> sizeof(grant_ref_t),
> but will contain the remaining number of grefs in this page
> this way we can flexibly allocate and reference buffers of any requested 
> size

This in no way addresses my original or my subsequent replies.
(Apart from that elsewhere you state that a zero gref indicates
the end of the list. And the, am I reading this most recent reply
of yours correctly that you mean for this list to be volatile? If
so, how do frontend and backend synchronize accesses?)

Jan


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

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

[Oleksandr Andrushchenko]

On 11/25/2016 04:45 PM, Jan Beulich wrote:
>>>> On 25.11.16 at 15:27, <andr2000@gmail.com> wrote:
>> On 11/25/2016 04:12 PM, Jan Beulich wrote:
>>>>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
>>>> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>>>>> +struct xensnd_page_directory {
>>>>>> +    grant_ref_t gref_dir_next_page;
>>>>>> +    uint32_t num_grefs;
>>>>> You can't fit that many requests on one page anyway, so I think it
>>>>> would be better to limit this to 16 bits, and leave 16 bits reserved
>>>>> for future use (specified as must-be-zero).
got you
>>>> these are not for requests, but for buffers. you can imagine the size of
>>>> this
>>>> buffer if we play something like 8 channels 192kHz...
>>> I don't understand: You have one such entry per page, so how
>>> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
>> we employ the idea of a linked list here, e.g. gref_dir_next_page points
>> to the next page of the same structure.
>> if there are no more pages with grefs then num_grefs will not be equal to
>> (PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) /
>> sizeof(grant_ref_t),
>> but will contain the remaining number of grefs in this page
>> this way we can flexibly allocate and reference buffers of any requested
>> size
> This in no way addresses my original or my subsequent replies.
sorry for that, got what you want
agree
> (Apart from that elsewhere you state that a zero gref indicates
> the end of the list.
"If shared
  *   buffer size exceeds what can be addressed by this single page, then
  *   reference to the next page must be supplied (gref_dir_next_page below
  *   is not 0)"
>   And the, am I reading this most recent reply
> of yours correctly that you mean for this list to be volatile? If
> so, how do frontend and backend synchronize accesses?)
this list is only passed from front to back on buffer allocation,
e.g. open command and destroyed on close.
so, read/write happens on already established buffer
>
> Jan
>


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

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

[Jan Beulich]

>>> On 25.11.16 at 15:50, <andr2000@gmail.com> wrote:
> On 11/25/2016 04:45 PM, Jan Beulich wrote:
>>>>> On 25.11.16 at 15:27, <andr2000@gmail.com> wrote:
>>> On 11/25/2016 04:12 PM, Jan Beulich wrote:
>>>>>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
>>>>> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>>>>>> +struct xensnd_page_directory {
>>>>>>> +    grant_ref_t gref_dir_next_page;
>>>>>>> +    uint32_t num_grefs;
>>>>>> You can't fit that many requests on one page anyway, so I think it
>>>>>> would be better to limit this to 16 bits, and leave 16 bits reserved
>>>>>> for future use (specified as must-be-zero).
> got you
>>>>> these are not for requests, but for buffers. you can imagine the size of
>>>>> this
>>>>> buffer if we play something like 8 channels 192kHz...
>>>> I don't understand: You have one such entry per page, so how
>>>> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
>>> we employ the idea of a linked list here, e.g. gref_dir_next_page points
>>> to the next page of the same structure.
>>> if there are no more pages with grefs then num_grefs will not be equal to
>>> (PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) /
>>> sizeof(grant_ref_t),
>>> but will contain the remaining number of grefs in this page
>>> this way we can flexibly allocate and reference buffers of any requested
>>> size
>> This in no way addresses my original or my subsequent replies.
> sorry for that, got what you want
> agree
>> (Apart from that elsewhere you state that a zero gref indicates
>> the end of the list.
> "If shared
>   *   buffer size exceeds what can be addressed by this single page, then
>   *   reference to the next page must be supplied (gref_dir_next_page below
>   *   is not 0)"

Okay, in that case - what's the count for in the first place?
Couldn't you simply fill all the rest of the page with grefs, as
it would be clear from the specified buffer size how many there
are?

>>   And the, am I reading this most recent reply
>> of yours correctly that you mean for this list to be volatile? If
>> so, how do frontend and backend synchronize accesses?)
> this list is only passed from front to back on buffer allocation,
> e.g. open command and destroyed on close.
> so, read/write happens on already established buffer

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?

Jan


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

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

[Oleksandr Andrushchenko]

On 11/25/2016 04:56 PM, Jan Beulich wrote:
>>>> On 25.11.16 at 15:50, <andr2000@gmail.com> wrote:
>> On 11/25/2016 04:45 PM, Jan Beulich wrote:
>>>>>> On 25.11.16 at 15:27, <andr2000@gmail.com> wrote:
>>>> On 11/25/2016 04:12 PM, Jan Beulich wrote:
>>>>>>>> On 25.11.16 at 14:59, <andr2000@gmail.com> wrote:
>>>>>> On 11/25/2016 02:54 PM, Jan Beulich wrote:
>>>>>>>>>> On 25.11.16 at 12:57, <andr2000@gmail.com> wrote:
>>>>>>>> +struct xensnd_page_directory {
>>>>>>>> +    grant_ref_t gref_dir_next_page;
>>>>>>>> +    uint32_t num_grefs;
>>>>>>> You can't fit that many requests on one page anyway, so I think it
>>>>>>> would be better to limit this to 16 bits, and leave 16 bits reserved
>>>>>>> for future use (specified as must-be-zero).
>> got you
>>>>>> these are not for requests, but for buffers. you can imagine the size of
>>>>>> this
>>>>>> buffer if we play something like 8 channels 192kHz...
>>>>> I don't understand: You have one such entry per page, so how
>>>>> can it be larger than (or even equal to) PAGE_SIZE / sizeof(grant_ref_t)?
>>>> we employ the idea of a linked list here, e.g. gref_dir_next_page points
>>>> to the next page of the same structure.
>>>> if there are no more pages with grefs then num_grefs will not be equal to
>>>> (PAGE_SIZE - sizeof(gref_dir_next_page) - sizeof(num_grefs)) /
>>>> sizeof(grant_ref_t),
>>>> but will contain the remaining number of grefs in this page
>>>> this way we can flexibly allocate and reference buffers of any requested
>>>> size
>>> This in no way addresses my original or my subsequent replies.
>> sorry for that, got what you want
>> agree
>>> (Apart from that elsewhere you state that a zero gref indicates
>>> the end of the list.
>> "If shared
>>    *   buffer size exceeds what can be addressed by this single page, then
>>    *   reference to the next page must be supplied (gref_dir_next_page below
>>    *   is not 0)"
> Okay, in that case - what's the count for in the first place?
> Couldn't you simply fill all the rest of the page with grefs, as
> it would be clear from the specified buffer size how many there
> are?
yes, this can be done this way. I will remove num_grefs and put a comment
on how to determine when to stop
>>>    And the, am I reading this most recent reply
>>> of yours correctly that you mean for this list to be volatile? If
>>> so, how do frontend and backend synchronize accesses?)
>> this list is only passed from front to back on buffer allocation,
>> e.g. open command and destroyed on close.
>> so, read/write happens on already established buffer
> 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
>


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

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

[Julien Grall]

Hi Oleksandr,

May I ask you to wait a bit more between two version of series?

It gives the opportunity to gather more feedback from people and
potentially arguing on some changes.

Regards,

On 25/11/16 11:57, 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 | 709 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 709 insertions(+)
>  create mode 100644 xen/include/public/io/sndif.h
>

-- 
Julien Grall

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

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

[Oleksandr Andrushchenko]

On 11/25/2016 02:02 PM, Julien Grall wrote:
> Hi Oleksandr,
>
> May I ask you to wait a bit more between two version of series?
>
> It gives the opportunity to gather more feedback from people and
> potentially arguing on some changes.
>
> Regards,
>
of course, sorry about this
> On 25/11/16 11:57, 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 | 709 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 709 insertions(+)
>>  create mode 100644 xen/include/public/io/sndif.h
>>
>


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

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

[Jan Beulich]

>>> On 25.11.16 at 13:02, <julien.grall@arm.com> wrote:
> May I ask you to wait a bit more between two version of series?

Indeed I was about to ask for the same.

Jan


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