[PATCH 0/2] include/public: update usbif.h

[PATCH 0/2] include/public: update usbif.h

[Juergen Gross]

Add some missing defines and documentation to the pvUSB header file.

Juergen Gross (2):
  include/public: add possible status values to usbif.h
  include/public: add better interface description to usbif.h

 xen/include/public/io/usbif.h | 171 ++++++++++++++++++++++++++++++++++
 1 file changed, 171 insertions(+)

-- 
2.26.2

[PATCH 1/2] include/public: add possible status values to usbif.h

[Juergen Gross]

The interface definition of PV USB devices is lacking the specification
of possible values of the status filed in a response. Those are
negative errno values as used in Linux, so they might differ in other
OS's. Specify them via appropriate defines.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/io/usbif.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/include/public/io/usbif.h b/xen/include/public/io/usbif.h
index c6a58639d6..fbd6f953f8 100644
--- a/xen/include/public/io/usbif.h
+++ b/xen/include/public/io/usbif.h
@@ -221,6 +221,13 @@ struct usbif_urb_response {
 	uint16_t id; /* request id */
 	uint16_t start_frame;  /* start frame (ISO) */
 	int32_t status; /* status (non-ISO) */
+#define USBIF_STATUS_OK		0
+#define USBIF_STATUS_NODEV	-19
+#define USBIF_STATUS_INVAL	-22
+#define USBIF_STATUS_STALL	-32
+#define USBIF_STATUS_IOERROR	-71
+#define USBIF_STATUS_BABBLE	-75
+#define USBIF_STATUS_SHUTDOWN	-108
 	int32_t actual_length; /* actual transfer length */
 	int32_t error_count; /* number of ISO errors */
 };
-- 
2.26.2

[PATCH 2/2] include/public: add better interface description to usbif.h

[Juergen Gross]

The PV USB protocol is poorly described. Add a more detailed
description to the usbif.h header file.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/io/usbif.h | 164 ++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/xen/include/public/io/usbif.h b/xen/include/public/io/usbif.h
index fbd6f953f8..10ff2ded58 100644
--- a/xen/include/public/io/usbif.h
+++ b/xen/include/public/io/usbif.h
@@ -32,6 +32,34 @@
 #include "../grant_table.h"
 
 /*
+ * Detailed Interface Description
+ * ==============================
+ * The pvUSB interface is using a split driver design: a frontend driver in
+ * the guest and a backend driver in a driver domain (normally dom0) having
+ * access to the physical USB device(s) being passed to the guest.
+ *
+ * The frontend and backend drivers use XenStore to initiate the connection
+ * between them, the I/O activity is handled via two shared ring pages and an
+ * event channel. As the interface between frontend and backend is at the USB
+ * host connector level, multiple (up to 31) physical USB devices can be
+ * handled by a single connection.
+ *
+ * The Xen pvUSB device name is "qusb", so the frontend's XenStore entries are
+ * to be found under "device/qusb", while the backend's XenStore entries are
+ * under "backend/<guest-dom-id>/qusb".
+ *
+ * When a new pvUSB connection is established, the frontend needs to setup the
+ * two shared ring pages for communication and the event channel. The ring
+ * pages need to be made available to the backend via the grant table
+ * interface.
+ *
+ * One of the shared ring pages is used by the backend to inform the frontend
+ * about USB device plug events (device to be added or removed). This is the
+ * "conn-ring".
+ *
+ * The other ring page is used for USB I/O communication (requests and
+ * responses). This is the "urb-ring".
+ *
  * Feature and Parameter Negotiation
  * =================================
  * The two halves of a Xen pvUSB driver utilize nodes within the XenStore to
@@ -99,6 +127,142 @@
  *      The machine ABI rules governing the format of all ring request and
  *      response structures.
  *
+ * Protocol Description
+ * ====================
+ *
+ *-------------------------- USB device plug events --------------------------
+ *
+ * USB device plug events are send via the "conn-ring" shared page. As only
+ * events are being sent, the respective requests from the frontend to the
+ * backend are just dummy ones.
+ * The events sent to the frontend have the following layout:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |    portnum     |     speed      | 4
+ * +----------------+----------------+----------------+----------------+
+ *   id - uint16_t, event id (taken from the actual frontend dummy request)
+ *   portnum - uint8_t, port number (1 ... 31)
+ *   speed - uint8_t, device USBIF_SPEED_*, USBIF_SPEED_NONE == unplug
+ *
+ * The dummy request:
+ *         0                1        octet
+ * +----------------+----------------+
+ * |               id                | 2
+ * +----------------+----------------+
+ *   id - uint16_t, guest supplied value (no need for being unique)
+ *
+ *-------------------------- USB I/O request ---------------------------------
+ *
+ * A single USB I/O request on the "urb-ring" has the following layout:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |         nr_buffer_segs          | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                               pipe                                | 8
+ * +----------------+----------------+----------------+----------------+
+ * |         transfer_flags          |          buffer_length          | 12
+ * +----------------+----------------+----------------+----------------+
+ * |                       request type specific                       | 16
+ * |                               data                                | 20
+ * +----------------+----------------+----------------+----------------+
+ * |                              seg[0]                               | 24
+ * |                               data                                | 28
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |             seg[USBIF_MAX_SEGMENTS_PER_REQUEST - 1]               | 144
+ * |                               data                                | 148
+ * +----------------+----------------+----------------+----------------+
+ * Bit field bit number 0 is always least significant bit, undefined bits must
+ * be zero.
+ *   id - uint16_t, guest supplied value
+ *   nr_buffer_segs - uint16_t, number of segment entries in seg[] array
+ *   pipe - uint32_t, bit field with multiple information:
+ *     bits 0-4: port request to send to
+ *     bit 5: unlink request with specified id (cancel I/O) if set (see below)
+ *     bit 7: direction (1 = read from device)
+ *     bits 8-14: device number on port
+ *     bits 15-18: endpoint of device
+ *     bits 30-31: request type: 00 = isochronous, 01 = interrupt,
+ *                               10 = control, 11 = bulk
+ *   transfer_flags - uint16_t, bit field with processing flags:
+ *     bit 0: less data than specified allowed
+ *   buffer_length - uint16_t, total length of data
+ *   request type specific data - 8 bytes, see below
+ *   seg[] - array with 8 byte elements, see below
+ *
+ * Request type specific data for isochronous request:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |            interval             |           start_frame           | 4
+ * +----------------+----------------+----------------+----------------+
+ * |       number_of_packets         |       nr_frame_desc_segs        | 8
+ * +----------------+----------------+----------------+----------------+
+ *   interval - uint16_t, time interval in msecs between frames
+ *   start_frame - uint16_t, start frame number
+ *   number_of_packets - uint16_t, number of packets to transfer
+ *   nr_frame_desc_segs - uint16_t number of seg[] frame descriptors elements
+ *
+ * Request type specific data for interrupt request:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |            interval             |                0                | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                                 0                                 | 8
+ * +----------------+----------------+----------------+----------------+
+ *   interval - uint16_t, time in msecs until interruption
+ *
+ * Request type specific data for control request:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |                      data of setup packet                         | 4
+ * |                                                                   | 8
+ * +----------------+----------------+----------------+----------------+
+ *
+ * Request type specific data for bulk request:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |                                 0                                 | 4
+ * |                                 0                                 | 8
+ * +----------------+----------------+----------------+----------------+
+ *
+ * Request type specific data for unlink request:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |           unlink_id             |                0                | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                                 0                                 | 8
+ * +----------------+----------------+----------------+----------------+
+ *   unlink_id - uint16_t, request id of request to terminate
+ *
+ * seg[] array element layout:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |                               gref                                | 4
+ * +----------------+----------------+----------------+----------------+
+ * |             offset              |             length              | 8
+ * +----------------+----------------+----------------+----------------+
+ *   gref - uint32_t, grant reference of buffer page
+ *   offset - uint16_t, offset of buffer start in page
+ *   length - uint16_t, length of buffer in page
+ *
+ *-------------------------- USB I/O response --------------------------------
+ *
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |          start_frame            | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                              status                               | 8
+ * +----------------+----------------+----------------+----------------+
+ * |                          actual_length                            | 12
+ * +----------------+----------------+----------------+----------------+
+ * |                           error_count                             | 16
+ * +----------------+----------------+----------------+----------------+
+ *   id - uint16_t, id of the request this response belongs to
+ *   start_frame - uint16_t, start_frame this response (iso requests only)
+ *   status - int32_t, USBIF_STATUS_* (non-iso requests)
+ *   actual_length - uint32_t, actual size of data transferred
+ *   error_count - uint32_t, number of errors (iso requests)
  */
 
 enum usb_spec_version {
-- 
2.26.2

Re: [PATCH 1/2] include/public: add possible status values to usbif.h

[Jan Beulich]

On 24.09.2021 17:04, Juergen Gross wrote:
> The interface definition of PV USB devices is lacking the specification
> of possible values of the status filed in a response. Those are

Nit: "field"?

> negative errno values as used in Linux, so they might differ in other
> OS's. Specify them via appropriate defines.

What if new errno values got used by the driver? Would we alter the
public header every time? Or is the likelihood of further values ever
getting used vanishingly small? In how far would it be possible to tie
these to Xen's public/errno.h instead?

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/include/public/io/usbif.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/include/public/io/usbif.h b/xen/include/public/io/usbif.h
> index c6a58639d6..fbd6f953f8 100644
> --- a/xen/include/public/io/usbif.h
> +++ b/xen/include/public/io/usbif.h
> @@ -221,6 +221,13 @@ struct usbif_urb_response {
>  	uint16_t id; /* request id */
>  	uint16_t start_frame;  /* start frame (ISO) */
>  	int32_t status; /* status (non-ISO) */
> +#define USBIF_STATUS_OK		0
> +#define USBIF_STATUS_NODEV	-19
> +#define USBIF_STATUS_INVAL	-22
> +#define USBIF_STATUS_STALL	-32
> +#define USBIF_STATUS_IOERROR	-71
> +#define USBIF_STATUS_BABBLE	-75
> +#define USBIF_STATUS_SHUTDOWN	-108

Nit: While probably benign for all practical uses, these negative
values nevertheless would better be parenthesized.

Jan

Re: [PATCH 1/2] include/public: add possible status values to usbif.h

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1807 bytes --]

On 27.09.21 10:13, Jan Beulich wrote:
> On 24.09.2021 17:04, Juergen Gross wrote:
>> The interface definition of PV USB devices is lacking the specification
>> of possible values of the status filed in a response. Those are
> 
> Nit: "field"?

Yes, of course.

> 
>> negative errno values as used in Linux, so they might differ in other
>> OS's. Specify them via appropriate defines.
> 
> What if new errno values got used by the driver? Would we alter the
> public header every time? Or is the likelihood of further values ever
> getting used vanishingly small? In how far would it be possible to tie
> these to Xen's public/errno.h instead?

Those are the current values returned by the backend. Other ones used
internally in the backend should IMO tried to be mapped to the values
defined in the interface specification.

> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/include/public/io/usbif.h | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/xen/include/public/io/usbif.h b/xen/include/public/io/usbif.h
>> index c6a58639d6..fbd6f953f8 100644
>> --- a/xen/include/public/io/usbif.h
>> +++ b/xen/include/public/io/usbif.h
>> @@ -221,6 +221,13 @@ struct usbif_urb_response {
>>   	uint16_t id; /* request id */
>>   	uint16_t start_frame;  /* start frame (ISO) */
>>   	int32_t status; /* status (non-ISO) */
>> +#define USBIF_STATUS_OK		0
>> +#define USBIF_STATUS_NODEV	-19
>> +#define USBIF_STATUS_INVAL	-22
>> +#define USBIF_STATUS_STALL	-32
>> +#define USBIF_STATUS_IOERROR	-71
>> +#define USBIF_STATUS_BABBLE	-75
>> +#define USBIF_STATUS_SHUTDOWN	-108
> 
> Nit: While probably benign for all practical uses, these negative
> values nevertheless would better be parenthesized.

Okay, fine with me.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/2] include/public: add possible status values to usbif.h

[Jan Beulich]

On 27.09.2021 10:20, Juergen Gross wrote:
> On 27.09.21 10:13, Jan Beulich wrote:
>> On 24.09.2021 17:04, Juergen Gross wrote:
>>> The interface definition of PV USB devices is lacking the specification
>>> of possible values of the status filed in a response. Those are
>>> negative errno values as used in Linux, so they might differ in other
>>> OS's. Specify them via appropriate defines.
>>
>> What if new errno values got used by the driver? Would we alter the
>> public header every time? Or is the likelihood of further values ever
>> getting used vanishingly small? In how far would it be possible to tie
>> these to Xen's public/errno.h instead?
> 
> Those are the current values returned by the backend. Other ones used
> internally in the backend should IMO tried to be mapped to the values
> defined in the interface specification.

In which case I'd like to reword my question: Is the set of values added
sufficiently rich? There's e.g. no ENOMEM equivalent.

One other observation: Why is this header using tab indentation? This is
by far the largest (albeit sadly not the only) of the style violators in
xen/include/public/io/.

Jan

Re: [PATCH 1/2] include/public: add possible status values to usbif.h

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1412 bytes --]

On 27.09.21 10:27, Jan Beulich wrote:
> On 27.09.2021 10:20, Juergen Gross wrote:
>> On 27.09.21 10:13, Jan Beulich wrote:
>>> On 24.09.2021 17:04, Juergen Gross wrote:
>>>> The interface definition of PV USB devices is lacking the specification
>>>> of possible values of the status filed in a response. Those are
>>>> negative errno values as used in Linux, so they might differ in other
>>>> OS's. Specify them via appropriate defines.
>>>
>>> What if new errno values got used by the driver? Would we alter the
>>> public header every time? Or is the likelihood of further values ever
>>> getting used vanishingly small? In how far would it be possible to tie
>>> these to Xen's public/errno.h instead?
>>
>> Those are the current values returned by the backend. Other ones used
>> internally in the backend should IMO tried to be mapped to the values
>> defined in the interface specification.
> 
> In which case I'd like to reword my question: Is the set of values added
> sufficiently rich? There's e.g. no ENOMEM equivalent.

Those are mapped to ESHUTDOWN today.

> One other observation: Why is this header using tab indentation? This is
> by far the largest (albeit sadly not the only) of the style violators in
> xen/include/public/io/.

Good question. This seems to date back to its introduction back in
2009.

I can add a style fixup patch to the series.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]