From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH V2] Update pvSCSI protocol description Date: Mon, 25 Aug 2014 14:51:50 +0200 Message-ID: <53FB3166.1070903@suse.com> References: <1408968810-19172-1-git-send-email-jgross@suse.com> <53FB4927020000780002D367@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FB4927020000780002D367@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir@xen.org, ian.campbell@citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org, david.vrabel@citrix.com List-Id: xen-devel@lists.xenproject.org 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: > > 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