Re: [PATCH] Update pvSCSI protocol description

[Juergen Gross <jgross@suse.com>]

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:         <uint16_t>
>
> 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