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

[Juergen Gross <jgross@xxxxxxxx>]

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
> it?
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
> sliently.
Okay.

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


Juergen

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