[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V2] Update pvSCSI protocol description

On 08/25/2014 02:33 PM, Jan Beulich wrote:
On 25.08.14 at 14:13, <"jgross@xxxxxxxx".non-mime.internet> wrote:
+ * feature-sg-grant
+ *      Values:         <uint16_t>

Hmm, you said "Yes, that's better" on my suggestion on how to change
this, but it's still saying uint16_t here?

Hmm, "stg refresh" after changing the file seems to be a good idea.

  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 */
+    uint8_t cmd_len;        /* valid CDB bytes */
+    uint8_t cmnd[VSCSIIF_MAX_COMMAND_SIZE]; /* the CDB */
+    uint16_t timeout_per_command;

I admit the original comment on this field wasn't very helpful, but
couldn't you make this a useful comment rather than simply dropping

Oh, you are right. In my scsiback re-implementation the timeout isn't
necessary any more, as the default timeouts are handled in the core
target infrastructure. I'll add an appropriate comment.

+    uint16_t channel, id, lun;      /* (virtual) device specification */
+    uint16_t ref_rqid;              /* command abort reference */

This field rename should also be mentioned in the description; I think
renaming the original padding field is fine, but this shouldn't be done


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

Is that really accurate? If said value is to go into nr_segments,
all such requests would have to have exactly 128 "grant pages
with SG elements".

Have you read the comment just above the #define of VSCSIIF_ACT_SCSI_CDB
describing the interface?

-#define VSCSIIF_SG_LIST_SIZE ((sizeof(vscsiif_request_t) - 4) \
-                              / sizeof(vscsiif_segment_t))
-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];
-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 */
+    uint16_t rqid;          /* identifies request */
+    uint8_t padding;
      uint8_t sense_len;
      uint8_t sense_buffer[VSCSIIF_SENSE_BUFFERSIZE];
      int32_t rslt;

You can't just drop these SG preset definitions. You can call them
deprecated, just like you do for the action code, but they have to
remain here.



Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.