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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

>  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
it?

> +    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
sliently.

> +    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".

> -#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.

Jan


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