| Message ID | 20180309223355.21222-1-steve@sk2.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > +/* > + * SCSI command sizes are as follows, in bytes, for fixed size commands, per > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > + * determine its group. > + * The size table is encoded into a 32-bit value by subtracting each value > + * from 16, resulting in a value of 1715488362 > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). > + * Command group 3 is reserved and should never be used. > + */ > +#define COMMAND_SIZE(opcode) \ > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) To me this seems hard to read and hard to verify. Could this have been written as a combination of ternary expressions, e.g. using a gcc statement expression to ensure that opcode is evaluated once? Thanks, Bart.
Hi Bart, On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > > +/* > > + * SCSI command sizes are as follows, in bytes, for fixed size commands, > > per > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > > + * determine its group. > > + * The size table is encoded into a 32-bit value by subtracting each > > value > > + * from 16, resulting in a value of 1715488362 > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + > > 10). > > + * Command group 3 is reserved and should never be used. > > + */ > > +#define COMMAND_SIZE(opcode) \ > > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) > > To me this seems hard to read and hard to verify. Could this have been > written as a combination of ternary expressions, e.g. using a gcc statement > expression to ensure that opcode is evaluated once? That’s what I’d tried initially, e.g. #define COMMAND_SIZE(opcode) ({ \ int index = ((opcode) >> 5) & 7; \ index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : 10); \ }) But gcc still reckons that results in a VLA, defeating the initial purpose of the exercise. Does it help if I make the magic value construction clearer? #define SCSI_COMMAND_SIZE_TBL ( \ (16 - 6) \ + ((16 - 10) << 4) \ + ((16 - 10) << 8) \ + ((16 - 12) << 12) \ + ((16 - 16) << 16) \ + ((16 - 12) << 20) \ + ((16 - 10) << 24) \ + ((16 - 10) << 28)) #define COMMAND_SIZE(opcode) \ (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & 7))))) Regards, Stephen
On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote: > Hi Bart, > > On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd > c.com> > wrote: > > > > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > > > > > > +/* > > > + * SCSI command sizes are as follows, in bytes, for fixed size > > > commands, > > > per > > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of > > > an opcode > > > + * determine its group. > > > + * The size table is encoded into a 32-bit value by subtracting > > > each > > > value > > > + * from 16, resulting in a value of 1715488362 > > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 > > > << 4 + > > > 10). > > > + * Command group 3 is reserved and should never be used. > > > + */ > > > +#define COMMAND_SIZE(opcode) \ > > > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & > > > 7))))) > > > > To me this seems hard to read and hard to verify. Could this have > > been > > written as a combination of ternary expressions, e.g. using a gcc > > statement > > expression to ensure that opcode is evaluated once? > > That’s what I’d tried initially, e.g. > > #define COMMAND_SIZE(opcode) ({ \ > int index = ((opcode) >> 5) & 7; \ > index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : > 10); \ > }) > > But gcc still reckons that results in a VLA, defeating the initial > purpose of > the exercise. > > Does it help if I make the magic value construction clearer? > > #define SCSI_COMMAND_SIZE_TBL ( \ > (16 - 6) \ > + ((16 - 10) << 4) \ > + ((16 - 10) << 8) \ > + ((16 - 12) << 12) \ > + ((16 - 16) << 16) \ > + ((16 - 12) << 20) \ > + ((16 - 10) << 24) \ > + ((16 - 10) << 28)) > > #define COMMAND_SIZE(opcode) > \ > (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & > 7))))) Couldn't we do the less clever thing of making the array a static const and moving it to a header? That way the compiler should be able to work it out at compile time. James
On 2018-03-10 03:49 PM, James Bottomley wrote: > On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote: >> Hi Bart, >> >> On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd >> c.com> >> wrote: >>> >>> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: >>>> >>>> +/* >>>> + * SCSI command sizes are as follows, in bytes, for fixed size >>>> commands, >>>> per >>>> + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of >>>> an opcode >>>> + * determine its group. >>>> + * The size table is encoded into a 32-bit value by subtracting >>>> each >>>> value >>>> + * from 16, resulting in a value of 1715488362 >>>> + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 >>>> << 4 + >>>> 10). >>>> + * Command group 3 is reserved and should never be used. >>>> + */ >>>> +#define COMMAND_SIZE(opcode) \ >>>> + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & >>>> 7))))) >>> >>> To me this seems hard to read and hard to verify. Could this have >>> been >>> written as a combination of ternary expressions, e.g. using a gcc >>> statement >>> expression to ensure that opcode is evaluated once? >> >> That’s what I’d tried initially, e.g. >> >> #define COMMAND_SIZE(opcode) ({ \ >> int index = ((opcode) >> 5) & 7; \ >> index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : >> 10); \ >> }) >> >> But gcc still reckons that results in a VLA, defeating the initial >> purpose of >> the exercise. >> >> Does it help if I make the magic value construction clearer? >> >> #define SCSI_COMMAND_SIZE_TBL ( \ >> (16 - 6) \ >> + ((16 - 10) << 4) \ >> + ((16 - 10) << 8) \ >> + ((16 - 12) << 12) \ >> + ((16 - 16) << 16) \ >> + ((16 - 12) << 20) \ >> + ((16 - 10) << 24) \ >> + ((16 - 10) << 28)) >> >> #define COMMAND_SIZE(opcode) >> \ >> (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & >> 7))))) > > Couldn't we do the less clever thing of making the array a static const > and moving it to a header? That way the compiler should be able to > work it out at compile time. And maybe add a comment that as of now (SPC-5 rev 19), COMMAND_SIZE is not valid for opcodes 0x7e and 0x7f plus everything above and including 0xc0. The latter ones are vendor specific and are loosely constrained, probably all even numbered lengths in the closed range: [6,260]. If the SCSI command sets want to keep up with NVMe, they may want to think about how they can gainfully use cdb_s that are > 64 bytes long. WRITE SCATTERED got into SBC-4 but READ GATHERED didn't, due to lack of interest. The READ GATHERED proposed was a bidi command, but it could have been a a simpler data-in command with a looong cdb (holding LBA, number_of_blocks pairs). Doug Gilbert
On Sat, 10 Mar 2018 12:49:17 -0800, James Bottomley <jejb@linux.vnet.ibm.com> wrote: > On Sat, 2018-03-10 at 14:29 +0100, Stephen Kitt wrote: > > On Fri, 9 Mar 2018 22:47:12 +0000, Bart Van Assche <Bart.VanAssche@wd > > c.com> > > wrote: > > > On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote: > > > > +/* > > > > + * SCSI command sizes are as follows, in bytes, for fixed size > > > > commands, > > > > per > > > > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of > > > > an opcode > > > > + * determine its group. > > > > + * The size table is encoded into a 32-bit value by subtracting > > > > each > > > > value > > > > + * from 16, resulting in a value of 1715488362 > > > > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 > > > > << 4 + > > > > 10). > > > > + * Command group 3 is reserved and should never be used. > > > > + */ > > > > +#define COMMAND_SIZE(opcode) \ > > > > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & > > > > 7))))) > > > > > > To me this seems hard to read and hard to verify. Could this have > > > been > > > written as a combination of ternary expressions, e.g. using a gcc > > > statement > > > expression to ensure that opcode is evaluated once? > > > > That’s what I’d tried initially, e.g. > > > > #define COMMAND_SIZE(opcode) ({ \ > > int index = ((opcode) >> 5) & 7; \ > > index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : > > 10); \ > > }) > > > > But gcc still reckons that results in a VLA, defeating the initial > > purpose of > > the exercise. > > > > Does it help if I make the magic value construction clearer? > > > > #define SCSI_COMMAND_SIZE_TBL ( \ > > (16 - 6) \ > > + ((16 - 10) << 4) \ > > + ((16 - 10) << 8) \ > > + ((16 - 12) << 12) \ > > + ((16 - 16) << 16) \ > > + ((16 - 12) << 20) \ > > + ((16 - 10) << 24) \ > > + ((16 - 10) << 28)) > > > > #define > > COMMAND_SIZE(opcode) \ > > (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & > > 7))))) > > Couldn't we do the less clever thing of making the array a static const > and moving it to a header? That way the compiler should be able to > work it out at compile time. I hoped so too, but const-ifying a variable doesn’t make it a compile-time constant (even though the optimiser can resolve it at build-time), so GCC still considers uses such as u8 cdb[COMMAND_SIZE(MAINTENANCE_IN)]; as variable-length arrays, which C90 forbids (and which we’re trying to eliminate here). Regards, Stephen
From: Stephen Kitt > Sent: 09 March 2018 22:34 > > COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c. > A number of device_handler functions use this to initialise arrays, > and this is flagged by -Wvla. > > This patch replaces COMMAND_SIZE with a variant using a formula which > can be resolved at compile time in cases where the opcode is fixed, > resolving the array size and avoiding the VLA. The downside is that > the code is less maintainable and that some call sites end up having > more complex generated code. > > Since scsi_command_size_tbl is dropped, we can remove the dependency > on BLK_SCSI_REQUEST from drivers/target/Kconfig. > > This was prompted by https://lkml.org/lkml/2018/3/7/621 > > Signed-off-by: Stephen Kitt <steve@sk2.org> > --- > block/scsi_ioctl.c | 8 -------- > drivers/target/Kconfig | 1 - > include/scsi/scsi_common.h | 13 +++++++++++-- > 3 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index 60b471f8621b..b9e176e537be 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -41,14 +41,6 @@ struct blk_cmd_filter { > > static struct blk_cmd_filter blk_default_cmd_filter; > > -/* Command group 3 is reserved and should never be used. */ > -const unsigned char scsi_command_size_tbl[8] = > -{ > - 6, 10, 10, 12, > - 16, 12, 10, 10 > -}; > -EXPORT_SYMBOL(scsi_command_size_tbl); > - > #include <scsi/sg.h> > > static int sg_get_version(int __user *p) > diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig > index 4c44d7bed01a..b5f05b60cf06 100644 > --- a/drivers/target/Kconfig > +++ b/drivers/target/Kconfig > @@ -4,7 +4,6 @@ menuconfig TARGET_CORE > depends on SCSI && BLOCK > select CONFIGFS_FS > select CRC_T10DIF > - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl.. > select SGL_ALLOC > default n > help > diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h > index 731ac09ed231..48d950666376 100644 > --- a/include/scsi/scsi_common.h > +++ b/include/scsi/scsi_common.h > @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr) > return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; > } > > -extern const unsigned char scsi_command_size_tbl[8]; > -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] > +/* > + * SCSI command sizes are as follows, in bytes, for fixed size commands, per > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode > + * determine its group. > + * The size table is encoded into a 32-bit value by subtracting each value > + * from 16, resulting in a value of 1715488362 > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). > + * Command group 3 is reserved and should never be used. > + */ > +#define COMMAND_SIZE(opcode) \ > + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) Why not: (6 + (15 & (0x446a6440u .... Specifying the constant in hex makes it more obvious. It is a shame about the 16, but an offset is easier than the negate. David
diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 60b471f8621b..b9e176e537be 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -41,14 +41,6 @@ struct blk_cmd_filter { static struct blk_cmd_filter blk_default_cmd_filter; -/* Command group 3 is reserved and should never be used. */ -const unsigned char scsi_command_size_tbl[8] = -{ - 6, 10, 10, 12, - 16, 12, 10, 10 -}; -EXPORT_SYMBOL(scsi_command_size_tbl); - #include <scsi/sg.h> static int sg_get_version(int __user *p) diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig index 4c44d7bed01a..b5f05b60cf06 100644 --- a/drivers/target/Kconfig +++ b/drivers/target/Kconfig @@ -4,7 +4,6 @@ menuconfig TARGET_CORE depends on SCSI && BLOCK select CONFIGFS_FS select CRC_T10DIF - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl.. select SGL_ALLOC default n help diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h index 731ac09ed231..48d950666376 100644 --- a/include/scsi/scsi_common.h +++ b/include/scsi/scsi_common.h @@ -15,8 +15,17 @@ scsi_varlen_cdb_length(const void *hdr) return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; } -extern const unsigned char scsi_command_size_tbl[8]; -#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] +/* + * SCSI command sizes are as follows, in bytes, for fixed size commands, per + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode + * determine its group. + * The size table is encoded into a 32-bit value by subtracting each value + * from 16, resulting in a value of 1715488362 + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 + 10). + * Command group 3 is reserved and should never be used. + */ +#define COMMAND_SIZE(opcode) \ + (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7))))) static inline unsigned scsi_command_size(const unsigned char *cmnd)
COMMAND_SIZE currently uses an array of values in block/scsi_ioctl.c. A number of device_handler functions use this to initialise arrays, and this is flagged by -Wvla. This patch replaces COMMAND_SIZE with a variant using a formula which can be resolved at compile time in cases where the opcode is fixed, resolving the array size and avoiding the VLA. The downside is that the code is less maintainable and that some call sites end up having more complex generated code. Since scsi_command_size_tbl is dropped, we can remove the dependency on BLK_SCSI_REQUEST from drivers/target/Kconfig. This was prompted by https://lkml.org/lkml/2018/3/7/621 Signed-off-by: Stephen Kitt <steve@sk2.org> --- block/scsi_ioctl.c | 8 -------- drivers/target/Kconfig | 1 - include/scsi/scsi_common.h | 13 +++++++++++-- 3 files changed, 11 insertions(+), 11 deletions(-)