From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH] Update pvSCSI protocol description Date: Mon, 25 Aug 2014 13:13:44 +0200 Message-ID: <53FB1A68.1070904@suse.com> References: <1408943152-6244-1-git-send-email-jgross@suse.com> <53FB0B88020000780002D0D1@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FB0B88020000780002D0D1@mail.emea.novell.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, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, david.vrabel@citrix.com List-Id: xen-devel@lists.xenproject.org On 08/25/2014 10:10 AM, Jan Beulich wrote: >>>> On 25.08.14 at 07:05, <"jgross@suse.com".non-mime.internet> wrote: >> Update the protocol description of the pvSCSI framework used to pass through >> SCSI devices to a guest (pv or hvm). >> >> The main changes are: >> - added comments >> - adapt to Linux style guide > > This is a Xen header, so no, Linux style isn't what we want here (it > is going to remain an exercise of changing the style when carrying > changes to this header over to the individual consuming OSes). Okay. > >> - add support for larger SG-lists by putting them in an own granted page >> - remove stale definitions > > With the significant amount of formatting changes it doesn't really > become clear which ones these are. If, after reverting the Linux > style changes, this still ends up being hard to spot, please enumerate > them here. Yep. > >> @@ -30,11 +33,144 @@ >> #include "ring.h" >> #include "../grant_table.h" >> >> -/* commands between backend and frontend */ >> -#define VSCSIIF_ACT_SCSI_CDB 1 /* SCSI CDB command */ >> -#define VSCSIIF_ACT_SCSI_ABORT 2 /* SCSI Device(Lun) Abort*/ >> -#define VSCSIIF_ACT_SCSI_RESET 3 /* SCSI Device(Lun) Reset*/ >> -#define VSCSIIF_ACT_SCSI_SG_PRESET 4 /* Preset SG elements */ >> +/* >> + * Front->back notifications: When enqueuing a new request, sending a >> + * notification can be made conditional on req_event (i.e., the generic >> + * hold-off mechanism provided by the ring macros). Backends must set >> + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()). >> + * >> + * Back->front notifications: When enqueuing a new response, sending a >> + * notification can be made conditional on rsp_event (i.e., the generic >> + * hold-off mechanism provided by the ring macros). Frontends must set >> + * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()). >> + */ > > I don't think this belongs here; a referral to ring.h would be the most > to be put here (but I think the mere fact that this header includes > ring.h is enough of a reference). I just copied blkif.h - I'll remove that section again. > >> +/* >> + * Feature and Parameter Negotiation >> + * ================================= >> + * The two halves of a Xen pvSCSI driver utilize nodes within the XenStore to >> + * communicate capabilities and to negotiate operating parameters. This >> + * section enumerates these nodes which reside in the respective front and >> + * backend portions of the XenStore, following the XenBus convention. >> + * >> + * All data in the XenStore is stored as strings. Nodes specifying numeric >> + * values are encoded in decimal. Integer value ranges listed below are >> + * expressed as fixed sized integer types capable of storing the conversion >> + * of a properly formated node string, without loss of information. > > Mostly the same very likely goes for this paragraph. Okay. > >> + * feature-sg-grant >> + * Values: > > Since when can XenStore encode fixed-width values? Same further > down - they're all plain (perhaps unsigned) integer values at this > layer. This documents the supported value range - I'd rather keep it. > >> - >> -#endif /*__XEN__PUBLIC_IO_SCSI_H__*/ >> -/* >> - * Local variables: >> - * mode: C >> - * c-file-style: "BSD" >> - * c-basic-offset: 4 >> - * tab-width: 4 >> - * indent-tabs-mode: nil >> - * End: >> - */ >> +#endif /*__XEN__PUBLIC_IO_SCSI_H__*/ > > These markers shouldn't get removed either. Okay. Juergen