[PATCH] srp.h: avoid padding of structs

[PATCH] srp.h: avoid padding of structs

[Arne Redlich]

Several structs in srp.h are padded by the compiler on some architectures, causing problems e.g. in ib_srp:

				i386	x86_64	(SRP spec)

sizeof(struct indirect_buf)	20	24	(20)
sizeof(struct srp_login_rsp)	52	56	(52)
sizeof(struct srp_rsp)		36	40	(36)

The patch below addresses this issue by packing these three structs, as well as the remaining ones.

Signed-off-by: Arne Redlich <arne.redlich@xiranet.com>

diff -prauN a/include/scsi/srp.h b/include/scsi/srp.h
--- a/include/scsi/srp.h	2006-04-16 19:12:27.347924280 +0200
+++ b/include/scsi/srp.h	2006-04-16 19:16:47.789331168 +0200
@@ -91,18 +91,13 @@ struct srp_direct_buf {
 	__be64	va;
 	__be32	key;
 	__be32  len;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
- */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
 	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,
@@ -120,7 +115,7 @@ struct srp_login_req {
 	u8	reserved3[5];
 	u8	initiator_port_id[16];
 	u8	target_port_id[16];
-};
+} __attribute__((packed));
 
 struct srp_login_rsp {
 	u8	opcode;
@@ -132,7 +127,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;
@@ -142,13 +137,13 @@ struct srp_login_rej {
 	u8	reserved2[8];
 	__be16	buf_fmt;
 	u8	reserved3[6];
-};
+} __attribute__((packed));
 
 struct srp_i_logout {
 	u8	opcode;
 	u8	reserved[7];
 	u64	tag;
-};
+} __attribute__((packed));
 
 struct srp_t_logout {
 	u8	opcode;
@@ -156,30 +151,22 @@ struct srp_t_logout {
 	u8	reserved[2];
 	__be32	reason;
 	u64	tag;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_tsk_mgmt {
 	u8	opcode;
 	u8	sol_not;
 	u8	reserved1[6];
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3[2];
 	u8	tsk_mgmt_func;
 	u8	reserved4;
 	u64	task_tag;
 	u8	reserved5[8];
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_cmd {
 	u8	opcode;
 	u8	sol_not;
@@ -189,14 +176,14 @@ struct srp_cmd {
 	u8	data_in_desc_cnt;
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3;
 	u8	task_attr;
 	u8	reserved4;
 	u8	add_cdb_len;
 	u8	cdb[16];
 	u8	add_data[0];
-};
+} __attribute__((packed));
 
 enum {
 	SRP_RSP_FLAG_RSPVALID = 1 << 0,
@@ -221,6 +208,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */

Re: [PATCH] srp.h: avoid padding of structs

[Arne Redlich]

On Sun, Apr 16, 2006 at 09:34:43PM +0200, Arne Redlich wrote:
> Several structs in srp.h are padded by the compiler on some architectures, causing problems e.g. in ib_srp:
> 
> 				i386	x86_64	(SRP spec)
> 
> sizeof(struct indirect_buf)	20	24	(20)
> sizeof(struct srp_login_rsp)	52	56	(52)
> sizeof(struct srp_rsp)		36	40	(36)
> 
> The patch below addresses this issue by packing these three structs, as well as the remaining ones.
> 

Scratch the previous version - I've mistakenly left an old and broken
"__attribute__((packed))" in struct srp_indirect_buf. Corrected below.

Signed-off-by: Arne Redlich <arne.redlich@xiranet.com>

diff -prauN a/include/scsi/srp.h b/include/scsi/srp.h
--- a/include/scsi/srp.h	2006-04-16 19:12:27.347924280 +0200
+++ b/include/scsi/srp.h	2006-04-17 11:04:24.333322224 +0200
@@ -91,18 +91,13 @@ struct srp_direct_buf {
 	__be64	va;
 	__be32	key;
 	__be32  len;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
- */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
-	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+	struct srp_direct_buf	desc_list[0];
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,
@@ -120,7 +115,7 @@ struct srp_login_req {
 	u8	reserved3[5];
 	u8	initiator_port_id[16];
 	u8	target_port_id[16];
-};
+} __attribute__((packed));
 
 struct srp_login_rsp {
 	u8	opcode;
@@ -132,7 +127,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;
@@ -142,13 +137,13 @@ struct srp_login_rej {
 	u8	reserved2[8];
 	__be16	buf_fmt;
 	u8	reserved3[6];
-};
+} __attribute__((packed));
 
 struct srp_i_logout {
 	u8	opcode;
 	u8	reserved[7];
 	u64	tag;
-};
+} __attribute__((packed));
 
 struct srp_t_logout {
 	u8	opcode;
@@ -156,30 +151,22 @@ struct srp_t_logout {
 	u8	reserved[2];
 	__be32	reason;
 	u64	tag;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_tsk_mgmt {
 	u8	opcode;
 	u8	sol_not;
 	u8	reserved1[6];
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3[2];
 	u8	tsk_mgmt_func;
 	u8	reserved4;
 	u64	task_tag;
 	u8	reserved5[8];
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_cmd {
 	u8	opcode;
 	u8	sol_not;
@@ -189,14 +176,14 @@ struct srp_cmd {
 	u8	data_in_desc_cnt;
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3;
 	u8	task_attr;
 	u8	reserved4;
 	u8	add_cdb_len;
 	u8	cdb[16];
 	u8	add_data[0];
-};
+} __attribute__((packed));
 
 enum {
 	SRP_RSP_FLAG_RSPVALID = 1 << 0,
@@ -221,6 +208,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */

[PATCH] srp.h: avoid padding of structs

[Roland Dreier]

Thanks for pointing out this problem.  However I don't think we should
pack every structure in the file, because packing structures that
don't need it leads to gcc generating horrible code on architectures
like ia64.  I'd prefer something like the following.

James, can you queue this up?  I'd be happy to merge it through my
tree but it seems more appropriate for it to go through the SCSI tree.
It's probably 2.6.17 material at this point in the cycle.

Thanks,
  Roland



Several structs in <scsi/srp.h> get padded to a multiple of 8 bytes on
64-bit architectures and end up with a size that does not match the
definition in the SRP spec:

                                     SRP spec     64-bit
    sizeof (struct indirect_buf)        20          24
    sizeof (struct srp_login_rsp)       52          56
    sizeof (struct srp_rsp)             36          40

Fix this by adding __attribute__((packed)) to the offending structs.

Problem pointed out by Arne Redlich <arne.redlich@xiranet.com>.

Signed-off-by: Roland Dreier <rolandd@cisco.com>

diff --git a/include/scsi/srp.h b/include/scsi/srp.h
index 6c2681d..637f77e 100644
--- a/include/scsi/srp.h
+++ b/include/scsi/srp.h
@@ -95,14 +95,15 @@ struct srp_direct_buf {
 
 /*
  * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
+ * descriptors at an offset of 20, which is not aligned to the size of
+ * struct srp_direct_buf.  The whole structure must be packed to avoid
+ * having the 20-byte structure padded to 24 bytes on 64-bit architectures.
  */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
-	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+	struct srp_direct_buf	desc_list[0];
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,
@@ -122,6 +123,11 @@ struct srp_login_req {
 	u8	target_port_id[16];
 };
 
+/*
+ * The SRP spec defines the size of the LOGIN_RSP structure to be 52
+ * bytes, so it needs to be packed to avoid having it padded to 56
+ * bytes on 64-bit architectures.
+ */
 struct srp_login_rsp {
 	u8	opcode;
 	u8	reserved1[3];
@@ -132,7 +138,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;
@@ -207,6 +213,11 @@ enum {
 	SRP_RSP_FLAG_DIUNDER  = 1 << 5
 };
 
+/*
+ * The SRP spec defines the size of the RSP structure to be 36 bytes,
+ * so it needs to be packed to avoid having it padded to 40 bytes on
+ * 64-bit architectures.
+ */
 struct srp_rsp {
 	u8	opcode;
 	u8	sol_not;
@@ -221,6 +232,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */

Re: [PATCH] srp.h: avoid padding of structs

[Arne Redlich]

Am Dienstag, den 18.04.2006, 09:05 -0700 schrieb Roland Dreier:
> Thanks for pointing out this problem.  However I don't think we should
> pack every structure in the file, because packing structures that
> don't need it leads to gcc generating horrible code on architectures
> like ia64.  I'd prefer something like the following.

Sure - I wasn't aware that gcc will produce different code even if it
doesn't have to insert padding bytes.

Thanks,
Arne
-- 
Arne Redlich
Xiranet Communications GmbH

Re: [PATCH] srp.h: avoid padding of structs

[Roland Dreier]

    Arne> Sure - I wasn't aware that gcc will produce different code
    Arne> even if it doesn't have to insert padding bytes.

Yes, it gets paranoid about alignment.  For example, if you build the
following on ia64:

	struct foo { int a; };
	struct bar { int b; } __attribute__((packed));

	int c(struct foo *x) { return x->a; }
	int d(struct bar *x) { return x->b; }

Then c() compiles to:

      0:       13 40 00 40 10 10       [MBB]       ld4 r8=[r32]
      6:       00 00 00 00 10 80                   nop.b 0x0
      c:       08 00 84 00                         br.ret.sptk.many b0;;

and d() compiles to:

     10:       09 70 00 40 00 21       [MMI]       mov r14=r32
     16:       f0 10 80 00 42 00                   adds r15=2,r32
     1c:       34 00 01 84                         adds r32=3,r32;;
     20:       19 80 04 1c 00 14       [MMB]       ld1 r16=[r14],1
     26:       f0 00 3c 00 20 00                   ld1 r15=[r15]
     2c:       00 00 00 20                         nop.b 0x0;;
     30:       09 70 00 1c 00 10       [MMI]       ld1 r14=[r14]
     36:       80 00 80 00 20 e0                   ld1 r8=[r32]
     3c:       f1 78 bd 53                         shl r15=r15,16;;
     40:       01 00 00 00 01 00       [MII]       nop.m 0x0
     46:       e0 70 dc ee 29 00                   shl r14=r14,8
     4c:       81 38 9d 53                         shl r8=r8,24;;
     50:       0b 70 40 1c 0e 20       [MMI]       or r14=r16,r14;;
     56:       f0 70 3c 1c 40 00                   or r15=r14,r15
     5c:       00 00 04 00                         nop.i 0x0;;
     60:       11 00 00 00 01 00       [MIB]       nop.m 0x0
     66:       80 78 20 1c 40 80                   or r8=r15,r8
     6c:       08 00 84 00                         br.ret.sptk.many b0;;

We should use __attribute__((packed)) only when it's really needed.

 - R.

Re: [PATCH] srp.h: avoid padding of structs

[Arne Redlich]

Am Dienstag, den 18.04.2006, 09:05 -0700 schrieb Roland Dreier:
> Thanks for pointing out this problem.  However I don't think we should
> pack every structure in the file, because packing structures that
> don't need it leads to gcc generating horrible code on architectures
> like ia64.  I'd prefer something like the following.
> 
> James, can you queue this up?  I'd be happy to merge it through my
> tree but it seems more appropriate for it to go through the SCSI tree.
> It's probably 2.6.17 material at this point in the cycle.
> 
> Thanks,
>   Roland
> 
> 
> 
> Several structs in <scsi/srp.h> get padded to a multiple of 8 bytes on
> 64-bit architectures and end up with a size that does not match the
> definition in the SRP spec:
> 
>                                      SRP spec     64-bit
>     sizeof (struct indirect_buf)        20          24
>     sizeof (struct srp_login_rsp)       52          56
>     sizeof (struct srp_rsp)             36          40
> 
> Fix this by adding __attribute__((packed)) to the offending structs.
> 
> Problem pointed out by Arne Redlich <arne.redlich@xiranet.com>.
> 
> Signed-off-by: Roland Dreier <rolandd@cisco.com>

James,

since this one isn't in scsi-rc-fixes-2.6 yet and you didn't comment on
it, either, I assume you missed this one:

http://marc.theaimsgroup.com/?l=linux-scsi&m=114537635902712&w=2

Arne
-- 
Arne Redlich
Xiranet Communications GmbH