All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Update pvSCSI protocol description
@ 2014-08-25  5:05 Juergen Gross
  2014-08-25  8:10 ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2014-08-25  5:05 UTC (permalink / raw)
  To: xen-devel, david.vrabel, ian.campbell, ian.jackson, jbeulich, keir, tim
  Cc: Juergen Gross

Update the protocol description of the pvSCSI framework used to pass through
SCSI devices to a guest (pv or hvm).

The main changes are:
- added comments
- adapt to Linux style guide
- add support for larger SG-lists by putting them in an own granted page
- remove stale definitions

This update is related to the rework of the pvSCSI backend and frontend drivers
in the Linux kernel. This interface version is included in that rework, too.

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

diff --git a/xen/include/public/io/vscsiif.h b/xen/include/public/io/vscsiif.h
index a50f980..ed22480 100644
--- a/xen/include/public/io/vscsiif.h
+++ b/xen/include/public/io/vscsiif.h
@@ -1,8 +1,11 @@
 /******************************************************************************
  * vscsiif.h
- * 
+ *
  * Based on the blkif.h code.
- * 
+ *
+ * This interface is to be regarded as a stable API between XEN domains
+ * running potentially different Linux kernel versions.
+ *
  * 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
@@ -30,11 +33,144 @@
 #include "ring.h"
 #include "../grant_table.h"
 
-/* commands between backend and frontend */
-#define VSCSIIF_ACT_SCSI_CDB         1    /* SCSI CDB command */
-#define VSCSIIF_ACT_SCSI_ABORT       2    /* SCSI Device(Lun) Abort*/
-#define VSCSIIF_ACT_SCSI_RESET       3    /* SCSI Device(Lun) Reset*/
-#define VSCSIIF_ACT_SCSI_SG_PRESET   4    /* Preset SG elements */
+/*
+ * 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 Xen pvSCSI 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.
+ *
+ * Any specified default value is in effect if the corresponding XenBus node
+ * is not present in the XenStore.
+ *
+ * XenStore nodes in sections marked "PRIVATE" are solely for use by the
+ * driver side whose XenBus tree contains them.
+ *
+ *****************************************************************************
+ *                            Backend XenBus Nodes
+ *****************************************************************************
+ *
+ *------------------ Backend Device Identification (PRIVATE) ------------------
+ *
+ * p-devname
+ *      Values:         string
+ *
+ *      A free string used to identify the physical device (e.g. a disk name).
+ *
+ * p-dev
+ *      Values:         string
+ *
+ *      A string specifying the backend device: either a 4-tuple "h:c:t:l"
+ *      (host, controller, target, lun, all integers), or a WWN (e.g.
+ *      "naa.60014054ac780582").
+ *
+ * v-dev
+ *      Values:         string
+ *
+ *      A string specifying the frontend device in form of a 4-tuple "h:c:t:l"
+ *      (host, controller, target, lun, all integers).
+ *
+ *--------------------------------- Features ---------------------------------
+ *
+ * feature-sg-grant
+ *      Values:         <uint16_t>
+ *      Default Value:  0
+ *
+ *      Specifies the maximum number of scatter/gather elements in grant pages
+ *      supported. If not set, the backend supports up to VSCSIIF_SG_TABLESIZE
+ *      SG elements specified directly in the request.
+ *
+ *****************************************************************************
+ *                            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.
+ *
+ * protocol
+ *      Values:         string (XEN_IO_PROTO_ABI_*)
+ *      Default Value:  XEN_IO_PROTO_ABI_NATIVE
+ *
+ *      The machine ABI rules governing the format of all ring request and
+ *      response structures.
+ */
+
+/* Requests from the frontend to the backend */
+
+/*
+ * Request a SCSI operation specified via a CDB in vscsiif_request.cmnd.
+ * The target is specified via channel, id and lun.
+ * The operation to be performed is specified via a CDB in cmnd[], the length
+ * of the CDB is in cmd_len. sc_data_direction specifies the direction of data
+ * (to the device, from the device, or none at all).
+ * If data is to be transferred to or from the device the buffer(s) in the
+ * guest memory is/are specified via one or multiple scsiif_request_segment
+ * descriptors each specifying a memory page via a grant_ref_t, a offset into
+ * the page and the length of the area in that page. All scsiif_request_segment
+ * areas concatenated form the resulting data buffer used by the operation.
+ * If the number of scsiif_request_segment areas is not too large (less than
+ * or equal VSCSIIF_SG_TABLESIZE) the areas can be specified directly in the
+ * seg[] array and the number of valid scsiif_request_segment elements is to be
+ * set in nr_segments.
+ * If "feature-sg-grant" in the Xenstore is set it is possible to specify more
+ * than VSCSIIF_SG_TABLESIZE scsiif_request_segment elements via indirection.
+ * The maximum number of allowed scsiif_request_segment elements is the value
+ * of the "feature-sg-grant" entry from Xenstore. When using indirection the
+ * seg[] array doesn't contain specifications of the data buffers, but
+ * references to scsiif_request_segment arrays, which in turn reference the
+ * data buffers. While nr_segments holds the number of populated seg[] entries
+ * (plus the set VSCSIIF_SG_GRANT bit), the number of scsiif_request_segment
+ * elements referencing the target data buffers is calculated from the lengths
+ * of the seg[] elements (the sum of all valid seg[].length divided by the
+ * size of one scsiif_request_segment structure).
+ */
+#define VSCSIIF_ACT_SCSI_CDB		1
+
+/*
+ * Request abort of a running operation for the specified target given by
+ * channel, id, lun and the operation's rqid in ref_rqid.
+ */
+#define VSCSIIF_ACT_SCSI_ABORT		2
+
+/*
+ * Request a device reset of the specified target (channel and id).
+ */
+#define VSCSIIF_ACT_SCSI_RESET		3
+
+/*
+ * Preset scatter/gather elements for a following request. Deprecated.
+ * Keeping the define only to avoid usage of the value "4" for other actions.
+ */
+#define VSCSIIF_ACT_SCSI_SG_PRESET	4
 
 /*
  * Maximum scatter/gather segments per request.
@@ -42,76 +178,64 @@
  * Considering balance between allocating at least 16 "vscsiif_request"
  * structures on one page (4096 bytes) and the number of scatter/gather
  * elements needed, we decided to use 26 as a magic number.
+ *
+ * If "feature-sg-grant" is set, more scatter/gather elements can be specified
+ * by placing them in one or more (up to VSCSIIF_SG_TABLESIZE) granted pages.
+ * In this case the vscsiif_request seg elements don't contain references to
+ * the user data, but to the SG elements referencing the user data.
  */
-#define VSCSIIF_SG_TABLESIZE             26
+#define VSCSIIF_SG_TABLESIZE		26
 
 /*
- * based on Linux kernel 2.6.18
+ * based on Linux kernel 2.6.18, still valid
+ * Changing these values requires support of multiple protocols via the rings
+ * as "old clients" will blindly use these values and the resulting structure
+ * sizes.
  */
-#define VSCSIIF_MAX_COMMAND_SIZE         16
-#define VSCSIIF_SENSE_BUFFERSIZE         96
+#define VSCSIIF_MAX_COMMAND_SIZE	16
+#define VSCSIIF_SENSE_BUFFERSIZE	96
 
 struct scsiif_request_segment {
-    grant_ref_t gref;
-    uint16_t offset;
-    uint16_t length;
+	grant_ref_t gref;
+	uint16_t offset;
+	uint16_t length;
 };
-typedef struct scsiif_request_segment vscsiif_segment_t;
 
+/* Size of one request is 252 bytes */
 struct vscsiif_request {
-    uint16_t rqid;          /* private guest value, echoed in resp  */
-    uint8_t act;            /* command between backend and frontend */
-    uint8_t cmd_len;
-
-    uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE];
-    uint16_t timeout_per_command;     /* The command is issued by twice 
-                                         the value in Backend. */
-    uint16_t channel, id, lun;
-    uint16_t padding;
-    uint8_t sc_data_direction;        /* for DMA_TO_DEVICE(1)
-                                         DMA_FROM_DEVICE(2)
-                                         DMA_NONE(3) requests  */
-    uint8_t nr_segments;              /* Number of pieces of scatter-gather */
-
-    vscsiif_segment_t seg[VSCSIIF_SG_TABLESIZE];
-    uint32_t reserved[3];
-};
-typedef struct vscsiif_request vscsiif_request_t;
+	uint16_t rqid;		/* private guest value, echoed in resp  */
+	uint8_t act;		/* command between backend and frontend */
+	uint8_t cmd_len;	/* valid CDB bytes */
 
-#define VSCSIIF_SG_LIST_SIZE ((sizeof(vscsiif_request_t) - 4) \
-                              / sizeof(vscsiif_segment_t))
+	uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE];	/* the CDB */
+	uint16_t timeout_per_command;
+	uint16_t channel, id, lun;	/* (virtual) device specification */
+	uint16_t ref_rqid;		/* command abort reference */
+	uint8_t sc_data_direction;	/* for DMA_TO_DEVICE(1)
+					   DMA_FROM_DEVICE(2)
+					   DMA_NONE(3) requests */
+	uint8_t nr_segments;		/* Number of pieces of scatter-gather */
+#define VSCSIIF_SG_GRANT	0x80	/* flag: SG elements via grant page */
+					/* nr_segments counts grant pages with
+					   SG elements.
+					   usable if "feature-sg-grant" set */
 
-struct vscsiif_sg_list {
-    /* First two fields must match struct vscsiif_request! */
-    uint16_t rqid;          /* private guest value, must match main req */
-    uint8_t act;            /* VSCSIIF_ACT_SCSI_SG_PRESET */
-    uint8_t nr_segments;    /* Number of pieces of scatter-gather */
-    vscsiif_segment_t seg[VSCSIIF_SG_LIST_SIZE];
+	struct scsiif_request_segment seg[VSCSIIF_SG_TABLESIZE];
+	uint32_t reserved[3];
 };
-typedef struct vscsiif_sg_list vscsiif_sg_list_t;
 
+/* Size of one response is 252 bytes */
 struct vscsiif_response {
-    uint16_t rqid;
-    uint8_t act;               /* valid only when backend supports SG_PRESET */
-    uint8_t sense_len;
-    uint8_t sense_buffer[VSCSIIF_SENSE_BUFFERSIZE];
-    int32_t rslt;
-    uint32_t residual_len;     /* request bufflen - 
-                                  return the value from physical device */
-    uint32_t reserved[36];
+	uint16_t rqid;		/* identifies request */
+	uint8_t padding;
+	uint8_t sense_len;
+	uint8_t sense_buffer[VSCSIIF_SENSE_BUFFERSIZE];
+	int32_t rslt;
+	uint32_t residual_len;	/* request bufflen -
+				   return the value from physical device */
+	uint32_t reserved[36];
 };
-typedef struct vscsiif_response vscsiif_response_t;
 
 DEFINE_RING_TYPES(vscsiif, struct vscsiif_request, struct vscsiif_response);
 
-
-#endif  /*__XEN__PUBLIC_IO_SCSI_H__*/
-/*
- * Local variables:
- * mode: C
- * c-file-style: "BSD"
- * c-basic-offset: 4
- * tab-width: 4
- * indent-tabs-mode: nil
- * End:
- */
+#endif /*__XEN__PUBLIC_IO_SCSI_H__*/
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Update pvSCSI protocol description
  2014-08-25  5:05 [PATCH] Update pvSCSI protocol description Juergen Gross
@ 2014-08-25  8:10 ` Jan Beulich
  2014-08-25 11:13   ` Juergen Gross
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2014-08-25  8:10 UTC (permalink / raw)
  To: Juergen Gross
  Cc: keir, ian.campbell, ian.jackson, tim, xen-devel, david.vrabel

>>> On 25.08.14 at 07:05, <"jgross@suse.com".non-mime.internet> wrote:
> Update the protocol description of the pvSCSI framework used to pass through
> SCSI devices to a guest (pv or hvm).
> 
> The main changes are:
> - added comments
> - adapt to Linux style guide

This is a Xen header, so no, Linux style isn't what we want here (it
is going to remain an exercise of changing the style when carrying
changes to this header over to the individual consuming OSes).

> - add support for larger SG-lists by putting them in an own granted page
> - remove stale definitions

With the significant amount of formatting changes it doesn't really
become clear which ones these are. If, after reverting the Linux
style changes, this still ends up being hard to spot, please enumerate
them here.

> @@ -30,11 +33,144 @@
>  #include "ring.h"
>  #include "../grant_table.h"
>  
> -/* commands between backend and frontend */
> -#define VSCSIIF_ACT_SCSI_CDB         1    /* SCSI CDB command */
> -#define VSCSIIF_ACT_SCSI_ABORT       2    /* SCSI Device(Lun) Abort*/
> -#define VSCSIIF_ACT_SCSI_RESET       3    /* SCSI Device(Lun) Reset*/
> -#define VSCSIIF_ACT_SCSI_SG_PRESET   4    /* Preset SG elements */
> +/*
> + * 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()).
> + */

I don't think this belongs here; a referral to ring.h would be the most
to be put here (but I think the mere fact that this header includes
ring.h is enough of a reference).

> +/*
> + * Feature and Parameter Negotiation
> + * =================================
> + * The two halves of a Xen pvSCSI 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.

Mostly the same very likely goes for this paragraph.

> + * feature-sg-grant
> + *      Values:         <uint16_t>

Since when can XenStore encode fixed-width values? Same further
down - they're all plain (perhaps unsigned) integer values at this
layer.

> -
> -#endif  /*__XEN__PUBLIC_IO_SCSI_H__*/
> -/*
> - * Local variables:
> - * mode: C
> - * c-file-style: "BSD"
> - * c-basic-offset: 4
> - * tab-width: 4
> - * indent-tabs-mode: nil
> - * End:
> - */
> +#endif /*__XEN__PUBLIC_IO_SCSI_H__*/

These markers shouldn't get removed either.

Jan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Update pvSCSI protocol description
  2014-08-25  8:10 ` Jan Beulich
@ 2014-08-25 11:13   ` Juergen Gross
  2014-08-25 11:21     ` Jan Beulich
  0 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2014-08-25 11:13 UTC (permalink / raw)
  To: Jan Beulich; +Cc: keir, ian.campbell, tim, ian.jackson, xen-devel, david.vrabel

On 08/25/2014 10:10 AM, Jan Beulich wrote:
>>>> On 25.08.14 at 07:05, <"jgross@suse.com".non-mime.internet> wrote:
>> Update the protocol description of the pvSCSI framework used to pass through
>> SCSI devices to a guest (pv or hvm).
>>
>> The main changes are:
>> - added comments
>> - adapt to Linux style guide
>
> This is a Xen header, so no, Linux style isn't what we want here (it
> is going to remain an exercise of changing the style when carrying
> changes to this header over to the individual consuming OSes).

Okay.

>
>> - add support for larger SG-lists by putting them in an own granted page
>> - remove stale definitions
>
> With the significant amount of formatting changes it doesn't really
> become clear which ones these are. If, after reverting the Linux
> style changes, this still ends up being hard to spot, please enumerate
> them here.

Yep.

>
>> @@ -30,11 +33,144 @@
>>   #include "ring.h"
>>   #include "../grant_table.h"
>>
>> -/* commands between backend and frontend */
>> -#define VSCSIIF_ACT_SCSI_CDB         1    /* SCSI CDB command */
>> -#define VSCSIIF_ACT_SCSI_ABORT       2    /* SCSI Device(Lun) Abort*/
>> -#define VSCSIIF_ACT_SCSI_RESET       3    /* SCSI Device(Lun) Reset*/
>> -#define VSCSIIF_ACT_SCSI_SG_PRESET   4    /* Preset SG elements */
>> +/*
>> + * 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()).
>> + */
>
> I don't think this belongs here; a referral to ring.h would be the most
> to be put here (but I think the mere fact that this header includes
> ring.h is enough of a reference).

I just copied blkif.h - I'll remove that section again.

>
>> +/*
>> + * Feature and Parameter Negotiation
>> + * =================================
>> + * The two halves of a Xen pvSCSI 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.
>
> Mostly the same very likely goes for this paragraph.

Okay.

>
>> + * feature-sg-grant
>> + *      Values:         <uint16_t>
>
> Since when can XenStore encode fixed-width values? Same further
> down - they're all plain (perhaps unsigned) integer values at this
> layer.

This documents the supported value range - I'd rather keep it.

>
>> -
>> -#endif  /*__XEN__PUBLIC_IO_SCSI_H__*/
>> -/*
>> - * Local variables:
>> - * mode: C
>> - * c-file-style: "BSD"
>> - * c-basic-offset: 4
>> - * tab-width: 4
>> - * indent-tabs-mode: nil
>> - * End:
>> - */
>> +#endif /*__XEN__PUBLIC_IO_SCSI_H__*/
>
> These markers shouldn't get removed either.

Okay.


Juergen

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Update pvSCSI protocol description
  2014-08-25 11:13   ` Juergen Gross
@ 2014-08-25 11:21     ` Jan Beulich
  2014-08-25 11:32       ` Juergen Gross
  0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2014-08-25 11:21 UTC (permalink / raw)
  To: Juergen Gross
  Cc: keir, ian.campbell, ian.jackson, tim, xen-devel, david.vrabel

>>> On 25.08.14 at 13:13, <JGross@suse.com> wrote:
> On 08/25/2014 10:10 AM, Jan Beulich wrote:
>>>>> On 25.08.14 at 07:05, <"jgross@suse.com".non-mime.internet> wrote:
>>> + * feature-sg-grant
>>> + *      Values:         <uint16_t>
>>
>> Since when can XenStore encode fixed-width values? Same further
>> down - they're all plain (perhaps unsigned) integer values at this
>> layer.
> 
> This documents the supported value range - I'd rather keep it.

64k grants? And even if indeed so, how would you express a
range constraint of 0...15 this way, or even one not starting at
zero? I think this should say "<integer> (valid range 0...LIMIT)"
or some such.

Jan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Update pvSCSI protocol description
  2014-08-25 11:21     ` Jan Beulich
@ 2014-08-25 11:32       ` Juergen Gross
  0 siblings, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2014-08-25 11:32 UTC (permalink / raw)
  To: Jan Beulich; +Cc: keir, ian.campbell, tim, ian.jackson, xen-devel, david.vrabel

On 08/25/2014 01:21 PM, Jan Beulich wrote:
>>>> On 25.08.14 at 13:13, <JGross@suse.com> wrote:
>> On 08/25/2014 10:10 AM, Jan Beulich wrote:
>>>>>> On 25.08.14 at 07:05, <"jgross@suse.com".non-mime.internet> wrote:
>>>> + * feature-sg-grant
>>>> + *      Values:         <uint16_t>
>>>
>>> Since when can XenStore encode fixed-width values? Same further
>>> down - they're all plain (perhaps unsigned) integer values at this
>>> layer.
>>
>> This documents the supported value range - I'd rather keep it.
>
> 64k grants? And even if indeed so, how would you express a
> range constraint of 0...15 this way, or even one not starting at
> zero? I think this should say "<integer> (valid range 0...LIMIT)"
> or some such.

Yes, that's better.

Juergen

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2014-08-25 11:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-25  5:05 [PATCH] Update pvSCSI protocol description Juergen Gross
2014-08-25  8:10 ` Jan Beulich
2014-08-25 11:13   ` Juergen Gross
2014-08-25 11:21     ` Jan Beulich
2014-08-25 11:32       ` Juergen Gross

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.