Re: [PATCH V2] Update pvSCSI protocol description

[Juergen Gross <jgross@suse.com>]

On 08/25/2014 02:33 PM, Jan Beulich wrote:
>>>> On 25.08.14 at 14:13, <"jgross@suse.com".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