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

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

[Oleksandr Dmytryshyn]

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

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

 xen/include/public/io/sndif.h | 429 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 429 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..0118cab
--- /dev/null
+++ b/xen/include/public/io/sndif.h
@@ -0,0 +1,429 @@
+/******************************************************************************
+ * sndif.h
+ *
+ * Unified sound-device I/O interface for Xen guest OSes.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (C) 2013-2015 GlobalLogic Inc.
+ */
+
+#ifndef __XEN_PUBLIC_IO_XENSND_H__
+#define __XEN_PUBLIC_IO_XENSND_H__
+
+/*
+ * Front->back notifications: When enqueuing a new request, sending a
+ * notification can be made conditional on req_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Backends must set
+ * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()).
+ *
+ * Back->front notifications: When enqueuing a new response, sending a
+ * notification can be made conditional on rsp_event (i.e., the generic
+ * hold-off mechanism provided by the ring macros). Frontends must set
+ * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()).
+ */
+
+/*
+ * Feature and Parameter Negotiation
+ * =================================
+ * The two halves of a Para-virtual sound card driver utilize nodes within the
+ * XenStore to communicate capabilities and to negotiate operating parameters.
+ * This section enumerates these nodes which reside in the respective front and
+ * backend portions of the XenStore, following the XenBus convention.
+ *
+ * All data in the XenStore is stored as strings.  Nodes specifying numeric
+ * values are encoded in decimal.  Integer value ranges listed below are
+ * expressed as fixed sized integer types capable of storing the conversion
+ * of a properly formated node string, without loss of information.
+ *
+ *****************************************************************************
+ *                            Backend XenBus Nodes
+ *****************************************************************************
+ *
+ *------------------------- Backend Device parameters -------------------------
+ *
+ * devid
+ *      Values:         <uint32_t>
+ *
+ *      Index of the soundcard which will be created in the frontend. This
+ *      index is zero based.
+ *
+ * devcnt
+ *      Values:         <uint32_t>
+ *
+ *      PCM instances count that are created by the soundcard in the frontend.
+ *
+ *----------------------------- Streams settings ------------------------------
+ *
+ * Every virtualized device has own set of the sound streams. Each stream
+ * parameter is with index "%u" and defined as 'stream%u_???'. Stream index is
+ * zero based and should be continuous in range from 0 to 'streams_cnt' - 1.
+ *
+ * stream%u_channels
+ *      Values:         <uint32_t>
+ *
+ *      The maximum amount of channels that can be supported by this stream.
+ *      Should be from 1 to XENSND_MAX_CHANNELS_PER_STREAM.
+ *
+ * stream%u_type
+ *      Values:         "p", "c", "b"
+ *
+ *      Stream type: "p" - playback stream, "c" - capture stream,
+ *      "b" - both playback and capture stream.
+ *
+ * stream%u_bedev_p
+ *      Values:         string
+ *
+ *      Name of the playback sound device which is mapped to this stream
+ *      by the backend. Present if stream%u_type is "p" or "b"
+ *
+ * stream%u_bedev_c
+ *      Values:         string
+ *
+ *      Name of the cappture sound device which is mapped to this stream
+ *      by the backend. Present if stream%u_type is "c" or "b"
+ *
+ * stream%u_devid
+ *      Values:         <uint32_t>
+ *
+ *      Index of the PCM instance which is created by the soundcard
+ *      in the frontend.
+ *
+ *****************************************************************************
+ *                            Frontend XenBus Nodes
+ *****************************************************************************
+ *
+ *----------------------- Request Transport Parameters -----------------------
+ *
+ * event-channel
+ *      Values:         <uint32_t>
+ *
+ *      The identifier of the Xen event channel used to signal activity
+ *      in the ring buffer.
+ *
+ * ring-ref
+ *      Values:         <uint32_t>
+ *
+ *      The Xen grant reference granting permission for the backend to map
+ *      the sole page in a single page sized ring buffer.
+ */
+
+/*
+ * STATE DIAGRAMS
+ *
+ *****************************************************************************
+ *                                   Startup                                 *
+ *****************************************************************************
+ *
+ * Tool stack creates front and back nodes with state XenbusStateInitialising.
+ *
+ * Front                                Back
+ * =================================    =====================================
+ * XenbusStateInitialising              XenbusStateInitialising
+ *  o Query virtual device               o Query backend device identification
+ *    properties.                          data.
+ *  o Setup OS device instance.          o Open and validate backend device.
+ *                                       o Publish backend features and
+ *                                         transport parameters.
+ *                                                      |
+ *                                                      |
+ *                                                      V
+ *                                      XenbusStateInitWait
+ *
+ * o Query backend features and
+ *   transport parameters.
+ * o Allocate and initialize the
+ *   request ring.
+ * o Publish transport parameters
+ *   that will be in effect during
+ *   this connection.
+ *              |
+ *              |
+ *              V
+ * XenbusStateInitialised
+ *
+ *                                       o Query frontend transport parameters.
+ *                                       o Connect to the request ring and
+ *                                         event channel.
+ *                                       o Publish backend device properties.
+ *                                                      |
+ *                                                      |
+ *                                                      V
+ *                                      XenbusStateConnected
+ *
+ *  o Query backend device properties.
+ *  o Finalize OS virtual device
+ *    instance.
+ *              |
+ *              |
+ *              V
+ * XenbusStateConnected
+ *
+ * Note: Drivers that do not support any optional features, or the negotiation
+ *       of transport parameters, can skip certain states in the state machine:
+ *
+ *       o A frontend may transition to XenbusStateInitialised without
+ *         waiting for the backend to enter XenbusStateInitWait.  In this
+ *         case, default transport parameters are in effect and any
+ *         transport parameters published by the frontend must contain
+ *         their default values.
+ *
+ *       o A backend may transition to XenbusStateInitialised, bypassing
+ *         XenbusStateInitWait, without waiting for the frontend to first
+ *         enter the XenbusStateInitialised state.  In this case, default
+ *         transport parameters are in effect and any transport parameters
+ *         published by the backend must contain their default values.
+ *
+ *       Drivers that support optional features and/or transport parameter
+ *       negotiation must tolerate these additional state transition paths.
+ *       In general this means performing the work of any skipped state
+ *       transition, if it has not already been performed, in addition to the
+ *       work associated with entry into the current state.
+ */
+
+/*
+ * PCM FORMATS
+ *
+ * XENSND_PCM_FORMAT_<format>[_<endian>]
+ *
+ * format: <S/U/F><bits> or <name>
+ *     S - signed, U - unsigned, F - float
+ *     bits - 8, 16, 24, 32
+ *     name - MU_LAW, GSM, etc.
+ *
+ * endian: <LE/BE>, may be absent
+ *     LE - Little endian, BE - Big endian
+ */
+#define XENSND_PCM_FORMAT_S8            0
+#define XENSND_PCM_FORMAT_U8            1
+#define XENSND_PCM_FORMAT_S16_LE        2
+#define XENSND_PCM_FORMAT_S16_BE        3
+#define XENSND_PCM_FORMAT_U16_LE        4
+#define XENSND_PCM_FORMAT_U16_BE        5
+#define XENSND_PCM_FORMAT_S24_LE        6
+#define XENSND_PCM_FORMAT_S24_BE        7
+#define XENSND_PCM_FORMAT_U24_LE        8
+#define XENSND_PCM_FORMAT_U24_BE        9
+#define XENSND_PCM_FORMAT_S32_LE        10
+#define XENSND_PCM_FORMAT_S32_BE        11
+#define XENSND_PCM_FORMAT_U32_LE        12
+#define XENSND_PCM_FORMAT_U32_BE        13
+#define XENSND_PCM_FORMAT_F32_LE        14 /* 4-byte float, IEEE-754 32-bit, */
+#define XENSND_PCM_FORMAT_F32_BE        15 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_F64_LE        16 /* 8-byte float, IEEE-754 64-bit, */
+#define XENSND_PCM_FORMAT_F64_BE        17 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19
+#define XENSND_PCM_FORMAT_MU_LAW        20
+#define XENSND_PCM_FORMAT_A_LAW         21
+#define XENSND_PCM_FORMAT_IMA_ADPCM     22
+#define XENSND_PCM_FORMAT_MPEG          23
+#define XENSND_PCM_FORMAT_GSM           24
+#define XENSND_PCM_FORMAT_SPECIAL       31 /* Any other unspecified format */
+
+/*
+ * REQUEST CODES.
+ */
+#define XENSND_OP_OPEN                  0
+#define XENSND_OP_CLOSE                 1
+#define XENSND_OP_READ                  2
+#define XENSND_OP_WRITE                 3
+#define XENSND_OP_SET_VOLUME            4
+#define XENSND_OP_GET_VOLUME            5
+
+/*
+ * The maximum amount of shared pages which can be used in any request
+ * from the frontend driver to the backend driver
+ */
+#define XENSND_MAX_PAGES_PER_REQUEST    10
+
+/* The maximum amount of channels per virtualized stream */
+#define XENSND_MAX_CHANNELS_PER_STREAM  128
+
+/*
+ * STATUS RETURN CODES.
+ */
+ /* Operation failed for some unspecified reason (e. g. -EIO). */
+#define XENSND_RSP_ERROR                 (-1)
+ /* Operation completed successfully. */
+#define XENSND_RSP_OKAY                  0
+
+/*
+ * Description of the protocol between frontend and backend driver.
+ *
+ * The two halves of a Para-virtual sound driver communicates with
+ * each to other using an shared page and event channel.
+ * Shared page contains a ring with request/response packets.
+ * All fields within the packet are always in little-endian byte order.
+ * Almost all fields within the packet are unsigned except
+ * the field 'status' in the responses packets which is signed.
+ *
+ *
+ * All request packets have the same length (64 bytes)
+ *
+ * Request open - open an pcm stream for playback or capture:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |      stream_idx       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |      pcm_format       |      pcm_channels     |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       pcm_rate        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_OPEN
+ * stream_idx - index of the stream (from 0 to 'streams_cnt' - 1.
+ *   'streams_cnt' is read from the XenStore)
+ * pcm_format - XENSND_PCM_FORMAT_???
+ * pcm_channels - channels count in stream
+ * pcm_rate - stream data rate
+ *
+ *
+ * Request close - close an opened pcm stream:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_idx      |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_CLOSE
+ * stream_idx - index of the stream (from 0 to 'streams_cnt' - 1.
+ *   'streams_cnt' is read from the XenStore)
+ *
+ *
+ * Request read/write - used for read (for capture) or write (for playback):
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_idx      |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         length        |         gref0         |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         gref1         |         gref2         |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |          gref9        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_READ or XENSND_OP_WRITE
+ * stream_idx - index of the stream (from 0 to 'streams_cnt' - 1.
+ *   'streams_cnt' is read from the XenStore)
+ * length - read or write data length
+ * gref0 - gref9 - references to a grant entries for used pages in read/write
+ * request.
+ *
+ *
+ * Request set volume - set/get channels volume in stream:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_idx      |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         gref          |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_SET_VOLUME or XENSND_OP_GET_VOLUME
+ * stream_idx - index of the stream (from 0 to 'streams_cnt' - 1.
+ *   'streams_cnt' is read from the XenStore)
+ * gref - references to a grant entry for page with the volume values
+ *
+ *
+ * Shared page for set/get volume:
+ *
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch0        |        vol_ch1        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch2        |        vol_ch3        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       vol_ch126       |       vol_ch127       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * vol_ch0 - vol_ch127 - volume for the channel from 0 to
+ *   XENSND_MAX_CHANNELS_PER_STREAM
+ * Please, note that only first 'stream%u_channels' are used in this command,
+ *   where 'stream%u_channels' is read from the XenStore (channels count for
+ *   stream with index '%u' which equals to 'stream_idx')
+ *
+ *
+ * All response packets have the same length (64 bytes)
+ *
+ * Response for all requests:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |         status        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       stream_idx      |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - copied from request
+ * stream_idx - copied from request
+ * operation - XENSND_OP_??? - copied from request
+ * status - XENSND_RSP_???
+ */
+
+struct xensnd_request {
+    uint8_t raw[64];
+};
+
+struct xensnd_response {
+    uint8_t raw[64];
+};
+
+DEFINE_RING_TYPES(xensnd, struct xensnd_request, struct xensnd_response);
+
+#endif /* __XEN_PUBLIC_IO_XENSND_H__ */
-- 
1.9.1

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

[Ian Campbell]

On Fri, 2015-02-06 at 13:28 +0200, Oleksandr Dmytryshyn wrote:
> This is ABI for the two halves of a Para-virtual
> sound driver to communicate with each to other.
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>

This seems to have gotten rather stalled, I think because of two
factors, one is that noone has clear maintainer responsibility for
blessing the creation of new PV protocols and partly because people are
naturally a bit cautious about creating new ABIs, which must be
maintained long term, for types of device with which they are not really
familiar.

I've added Lars because perhaps there is some process we can put in
place which helps alleviate these issues. (I don't know what, but
perhaps a "staging area" for new protocols which isn't ABI stable along
with some sort of graduation process, or perhaps some sort of governance
thing which would make it clear who has to say yes to something like
this, so we can beat them with sticks until they say something).

I've also CC-d a bunch of people who commented on previous rounds in
case any of them feel neither of the factors I mentioned apply to them
and want to say Acked/Reviewed-by ;-)

Ian.

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

[Dario Faggioli]

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

On Mon, 2015-02-23 at 17:41 +0000, Ian Campbell wrote:
> On Fri, 2015-02-06 at 13:28 +0200, Oleksandr Dmytryshyn wrote:
> > This is ABI for the two halves of a Para-virtual
> > sound driver to communicate with each to other.
> > 
> > Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
> > Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> This seems to have gotten rather stalled, I think because of two
> factors, one is that noone has clear maintainer responsibility for
> blessing the creation of new PV protocols and partly because people are
> naturally a bit cautious about creating new ABIs, which must be
> maintained long term, for types of device with which they are not really
> familiar.
> 
I agree... we really don't have audio experts on the list, I think! :-/

> I've added Lars because perhaps there is some process we can put in
> place which helps alleviate these issues. (I don't know what, but
> perhaps a "staging area" for new protocols which isn't ABI stable along
> with some sort of graduation process, or perhaps some sort of governance
> thing which would make it clear who has to say yes to something like
> this, so we can beat them with sticks until they say something).
> 
+1 to the idea.

> I've also CC-d a bunch of people who commented on previous rounds in
> case any of them feel neither of the factors I mentioned apply to them
> and want to say Acked/Reviewed-by ;-)
> 
Thanks for this. Personally, I took a look at the patches and the
document, and asked Stefano to help us providing his thoughts on the
thing.

Since then, I never had much time to look into this again, but even if I
did, all you said above about being wary on new protocols/ABI and not
knowing much about audio applies. :-(

Regards,
Dario

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

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

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

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

[Lars Kurth]

> On 23 Feb 2015, at 17:41, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> On Fri, 2015-02-06 at 13:28 +0200, Oleksandr Dmytryshyn wrote:
>> This is ABI for the two halves of a Para-virtual
>> sound driver to communicate with each to other.
>> 
>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> This seems to have gotten rather stalled, I think because of two
> factors, one is that noone has clear maintainer responsibility for
> blessing the creation of new PV protocols and partly because people are
> naturally a bit cautious about creating new ABIs, which must be
> maintained long term, for types of device with which they are not really
> familiar.
> 
> I've added Lars because perhaps there is some process we can put in
> place which helps alleviate these issues. (I don't know what, but
> perhaps a "staging area" for new protocols which isn't ABI stable along
> with some sort of graduation process, or perhaps some sort of governance
> thing which would make it clear who has to say yes to something like
> this, so we can beat them with sticks until they say something).

Hi,I nearly missed this. Please make sure you forward stuff and change the headline if you want me to look into things. Otherwise I may miss it.

>From my perspective, this exactly the kind of scenario why we created the embedded / automotive subproject, with an option to store code in repos owned by the project. 

Given that the primary use-case of these drivers is embedded / automotive, my suggestion would be to
1.a) Use a repo in the embedded / automotive pv driver subproject to host the spec - but use a file system structure that matches the xen tree
1.b) I would assume there would be one back-end and several front-ends for these drivers and some would eventually appear in trees owned by the embedded / automotive pv driver subproject

In this case, the maintainer responsibility would fall to members of the embedded / automotive pv driver subproject. Once there are several implementations, and enough people with skills to review we can re-visit where the spec and drivers live. 

We can have a discussion about criteria of when to move, but I don't think that makes a lot of sense. I think the concerns that need to be addressed are:
2.a) Enough skills to review the code / protocols from different stake-holders - this should happen with time, once the spec and code are there. And of course once the embedded / automotive pv driver subproject graduates, that will also give extra weight to its maintainers in the wider community
2.b) Of course if there was a strong case that PV sound drivers are extremely useful for core data centre use-cases, I would probably suggest another approach

Maybe 2.b) needs to be checked with Intel folks - there may be some sound requirement for XenGT

Would this work as a way forward?

Regards
Lars

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

[Ian Campbell]

On Thu, 2015-03-12 at 18:14 +0000, Lars Kurth wrote:
> Hi,I nearly missed this. Please make sure you forward stuff and change
> the headline if you want me to look into things. Otherwise I may miss
> it.

Sure, I'll try and remember.

FYI before Ian J went away he mentioned that he had raised some
questions/issues (either on this or a previous version) which had not
yet been answered (or maybe not answered to his satisfaction, I'm not
sure) but that if those were addressed he would take a look with a view
to acking the interface for inclusion in xen.git.

(I've not looked in the threads for it, so I don't know the exact
state).

> From my perspective, this exactly the kind of scenario why we created
> the embedded / automotive subproject, with an option to store code in
> repos owned by the project. 
> 
> Given that the primary use-case of these drivers is embedded /
> automotive, my suggestion would be to
> 1.a) Use a repo in the embedded / automotive pv driver subproject to
> host the spec - but use a file system structure that matches the xen
> tree
> 1.b) I would assume there would be one back-end and several front-ends
> for these drivers and some would eventually appear in trees owned by
> the embedded / automotive pv driver subproject
> 
> In this case, the maintainer responsibility would fall to members of
> the embedded / automotive pv driver subproject. Once there are several
> implementations, and enough people with skills to review we can
> re-visit where the spec and drivers live. 
> 
> We can have a discussion about criteria of when to move, but I don't
> think that makes a lot of sense. I think the concerns that need to be
> addressed are:
> 2.a) Enough skills to review the code / protocols from different
> stake-holders - this should happen with time, once the spec and code
> are there. And of course once the embedded / automotive pv driver
> subproject graduates, that will also give extra weight to its
> maintainers in the wider community
> 2.b) Of course if there was a strong case that PV sound drivers are
> extremely useful for core data centre use-cases, I would probably
> suggest another approach
> 
> Maybe 2.b) needs to be checked with Intel folks - there may be some
> sound requirement for XenGT
> 
> Would this work as a way forward?

I think the main things which is missing is some decision as to the the
point at which we would consider the ABI for a PV protocol fixed, i.e.
to be maintained in a backwards compatible manner from then on. 

That's of particular importance when one end of the pair is implemented
in external projects (e.g. OS driver frontends). If the interface is not
declared stable then changes would be allowed which would invalidate
those drivers.

Ian.

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

[Lars Kurth]

> On 17 Mar 2015, at 11:40, Ian Campbell <ian.campbell@citrix.com> wrote:
> 
> On Thu, 2015-03-12 at 18:14 +0000, Lars Kurth wrote:
>> Hi,I nearly missed this. Please make sure you forward stuff and change
>> the headline if you want me to look into things. Otherwise I may miss
>> it.
> 
> Sure, I'll try and remember.
> 
> FYI before Ian J went away he mentioned that he had raised some
> questions/issues (either on this or a previous version) which had not
> yet been answered (or maybe not answered to his satisfaction, I'm not
> sure) but that if those were addressed he would take a look with a view
> to acking the interface for inclusion in xen.git.

OK. So this means there are some concrete lose ends, which need to be followed up on. I also remember that there was a discussion on how we should specify protocols, which does not appear to have fully concluded either. 

>> 
>> Would this work as a way forward?
> 
> I think the main things which is missing is some decision as to the the
> point at which we would consider the ABI for a PV protocol fixed, i.e.
> to be maintained in a backwards compatible manner from then on. 

What do we do with new APIs in such situations? It would appear that there is some commonality in how we would handle a protocols and an API. I am assuming APIs such as new hypercalls don't immediately become fixed and backwards compatible. 

> That's of particular importance when one end of the pair is implemented
> in external projects (e.g. OS driver frontends). If the interface is not
> declared stable then changes would be allowed which would invalidate
> those drivers.

I understand that external projects may have different rules to us, which may cause problems. If this was the case in this instance, this would be an argument for using a xen project tree as a temporary location until we have declared the protocol ABI stable.

Regards
Lars

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

[Stefano Stabellini]

On Tue, 17 Mar 2015, Ian Campbell wrote:
> On Thu, 2015-03-12 at 18:14 +0000, Lars Kurth wrote:
> > Hi,I nearly missed this. Please make sure you forward stuff and change
> > the headline if you want me to look into things. Otherwise I may miss
> > it.
> 
> Sure, I'll try and remember.
> 
> FYI before Ian J went away he mentioned that he had raised some
> questions/issues (either on this or a previous version) which had not
> yet been answered (or maybe not answered to his satisfaction, I'm not
> sure) but that if those were addressed he would take a look with a view
> to acking the interface for inclusion in xen.git.
> 
> (I've not looked in the threads for it, so I don't know the exact
> state).
> 
> > From my perspective, this exactly the kind of scenario why we created
> > the embedded / automotive subproject, with an option to store code in
> > repos owned by the project. 
> > 
> > Given that the primary use-case of these drivers is embedded /
> > automotive, my suggestion would be to
> > 1.a) Use a repo in the embedded / automotive pv driver subproject to
> > host the spec - but use a file system structure that matches the xen
> > tree
> > 1.b) I would assume there would be one back-end and several front-ends
> > for these drivers and some would eventually appear in trees owned by
> > the embedded / automotive pv driver subproject
> > 
> > In this case, the maintainer responsibility would fall to members of
> > the embedded / automotive pv driver subproject. Once there are several
> > implementations, and enough people with skills to review we can
> > re-visit where the spec and drivers live. 
> > 
> > We can have a discussion about criteria of when to move, but I don't
> > think that makes a lot of sense. I think the concerns that need to be
> > addressed are:
> > 2.a) Enough skills to review the code / protocols from different
> > stake-holders - this should happen with time, once the spec and code
> > are there. And of course once the embedded / automotive pv driver
> > subproject graduates, that will also give extra weight to its
> > maintainers in the wider community
> > 2.b) Of course if there was a strong case that PV sound drivers are
> > extremely useful for core data centre use-cases, I would probably
> > suggest another approach
> > 
> > Maybe 2.b) needs to be checked with Intel folks - there may be some
> > sound requirement for XenGT
> > 
> > Would this work as a way forward?
> 
> I think the main things which is missing is some decision as to the the
> point at which we would consider the ABI for a PV protocol fixed, i.e.
> to be maintained in a backwards compatible manner from then on. 
> 
> That's of particular importance when one end of the pair is implemented
> in external projects (e.g. OS driver frontends). If the interface is not
> declared stable then changes would be allowed which would invalidate
> those drivers.

I think that you are right. Declaring the interface stable or unstable
is far more important than where the code or the spec lives.

If we formally specified within the spec that the ABI is not maintained
for backward compatibility, the bar for acceptance in xen-unstable would
be far lower. Maybe the spec could even be accepted as is if nobody has
any comments?

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

[Jan Beulich]

>>> On 17.03.15 at 14:05, <lars.kurth.xen@gmail.com> wrote:
>> On 17 Mar 2015, at 11:40, Ian Campbell <ian.campbell@citrix.com> wrote:
>> I think the main things which is missing is some decision as to the the
>> point at which we would consider the ABI for a PV protocol fixed, i.e.
>> to be maintained in a backwards compatible manner from then on. 
> 
> What do we do with new APIs in such situations? It would appear that there 
> is some commonality in how we would handle a protocols and an API. I am 
> assuming APIs such as new hypercalls don't immediately become fixed and 
> backwards compatible. 

New hypercalls become set in stone as soon as they appear in any
released version, unless specifically marked as experimental or alike.
The situation is quite different for a protocol specification like this:
Here we talk about something where no code would live in xen.git at
all, only the abstract description. Hence its stability can't usefully be
tied to any released Xen version.

Jan

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

[Ian Campbell]

On Tue, 2015-03-17 at 13:05 +0000, Lars Kurth wrote:
> > On 17 Mar 2015, at 11:40, Ian Campbell <ian.campbell@citrix.com> wrote:
> > 
> > On Thu, 2015-03-12 at 18:14 +0000, Lars Kurth wrote:
> >> Hi,I nearly missed this. Please make sure you forward stuff and change
> >> the headline if you want me to look into things. Otherwise I may miss
> >> it.
> > 
> > Sure, I'll try and remember.
> > 
> > FYI before Ian J went away he mentioned that he had raised some
> > questions/issues (either on this or a previous version) which had not
> > yet been answered (or maybe not answered to his satisfaction, I'm not
> > sure) but that if those were addressed he would take a look with a view
> > to acking the interface for inclusion in xen.git.
> 
> OK. So this means there are some concrete lose ends, which need to be followed up on. I also remember that there was a discussion on how we should specify protocols, which does not appear to have fully concluded either. 
> 
> >> 
> >> Would this work as a way forward?
> > 
> > I think the main things which is missing is some decision as to the the
> > point at which we would consider the ABI for a PV protocol fixed, i.e.
> > to be maintained in a backwards compatible manner from then on. 
> 
> What do we do with new APIs in such situations?

We review then carefully and hope we get them right. We manage to get
this right at least some of the time because many of us are familiar
with the issues WRT e.g. memory management hypercalls.

This is what I was getting at with "people are naturally a bit cautious
about creating new ABIs, which must be maintained long term, for types
of device with which they are not really familiar." in my initial mail.
The "which they are not really familiar" is pretty key.

It's also (normally) not too hard to add a new hypercall fixing a
shortcoming in an existing one while retaining backwards compat,
compared with doing that for an I/O protocol (see: netchannel2).

In the I/O case adding extensions also is reasonably well understood and
something we manage, but fixing a core issue is much harder (see: the
non-uniformity of the blk protocol over different architectures, or the
ring space wastage due to various power of two requirements, neither of
which can realistically be properly fixed).

Ian.