The uint16_t member cu_type of struct SenseId is not naturally aligned,
and since the struct is marked with QEMU_PACKED, this can lead to
unaligned memory accesses - which does not work on architectures like
Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
byte by byte when we do copy_sense_id_to_guest().

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/css.c         | 35 +++++++++++++++++++----------------
 include/hw/s390x/css.h |  2 +-
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5a9fe45..aaa2efa 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -750,20 +750,23 @@ static void sch_handle_halt_func(SubchDev *sch)
 
 }
 
-static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
+static void copy_sense_id_to_guest(uint8_t *dest, SenseId *src)
 {
     int i;
 
-    dest->reserved = src->reserved;
-    dest->cu_type = cpu_to_be16(src->cu_type);
-    dest->cu_model = src->cu_model;
-    dest->dev_type = cpu_to_be16(src->dev_type);
-    dest->dev_model = src->dev_model;
-    dest->unused = src->unused;
-    for (i = 0; i < ARRAY_SIZE(dest->ciw); i++) {
-        dest->ciw[i].type = src->ciw[i].type;
-        dest->ciw[i].command = src->ciw[i].command;
-        dest->ciw[i].count = cpu_to_be16(src->ciw[i].count);
+    dest[0] = src->reserved;
+    dest[1] = src->cu_type >> 8;
+    dest[2] = src->cu_type & 0xff;
+    dest[3] = src->cu_model;
+    dest[4] = src->dev_type >> 8;
+    dest[5] = src->dev_type & 0xff;
+    dest[6] = src->dev_model;
+    dest[7] = src->unused;
+    for (i = 0; i < ARRAY_SIZE(src->ciw); i++) {
+        dest[8 + i * 4] = src->ciw[i].type;
+        dest[9 + i * 4] = src->ciw[i].command;
+        dest[10 + i * 4] = src->ciw[i].count >> 8;
+        dest[11 + i * 4] = src->ciw[i].count & 0xff;
     }
 }
 
@@ -1044,9 +1047,9 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
         break;
     case CCW_CMD_SENSE_ID:
     {
-        SenseId sense_id;
+        uint8_t sense_id[256];
 
-        copy_sense_id_to_guest(&sense_id, &sch->id);
+        copy_sense_id_to_guest(sense_id, &sch->id);
         /* Sense ID information is device specific. */
         if (check_len) {
             if (ccw.count != sizeof(sense_id)) {
@@ -1060,11 +1063,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
          * have enough place to store at least bytes 0-3.
          */
         if (len >= 4) {
-            sense_id.reserved = 0xff;
+            sense_id[0] = 0xff;
         } else {
-            sense_id.reserved = 0;
+            sense_id[0] = 0;
         }
-        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
+        ccw_dstream_write_buf(&sch->cds, sense_id, len);
         sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
         ret = 0;
         break;
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 9da5912..bec82d0 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -48,7 +48,7 @@ typedef struct SenseId {
     uint8_t unused;          /* padding byte */
     /* extended part */
     CIW ciw[MAX_CIWS];       /* variable # of CIWs */
-} QEMU_PACKED SenseId;
+} SenseId;                   /* Note: No QEMU_PACKED due to unaligned members */
 
 /* Channel measurements, from linux/drivers/s390/cio/cmf.c. */
 typedef struct CMB {
-- 
1.8.3.1

On Tue, 25 Sep 2018 17:20:08 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The uint16_t member cu_type of struct SenseId is not naturally aligned,
> and since the struct is marked with QEMU_PACKED, this can lead to
> unaligned memory accesses - which does not work on architectures like
> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
> byte by byte when we do copy_sense_id_to_guest().

Hm... would it look nicer if we chopped up {cu,dev}_type into a hi/lo
byte and keep the QEMU_PACKED? (The ciws should be ok, I think.)

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/css.c         | 35 +++++++++++++++++++----------------
>  include/hw/s390x/css.h |  2 +-
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 5a9fe45..aaa2efa 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -750,20 +750,23 @@ static void sch_handle_halt_func(SubchDev *sch)
>  
>  }
>  
> -static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
> +static void copy_sense_id_to_guest(uint8_t *dest, SenseId *src)
>  {
>      int i;
>  
> -    dest->reserved = src->reserved;
> -    dest->cu_type = cpu_to_be16(src->cu_type);
> -    dest->cu_model = src->cu_model;
> -    dest->dev_type = cpu_to_be16(src->dev_type);
> -    dest->dev_model = src->dev_model;
> -    dest->unused = src->unused;
> -    for (i = 0; i < ARRAY_SIZE(dest->ciw); i++) {
> -        dest->ciw[i].type = src->ciw[i].type;
> -        dest->ciw[i].command = src->ciw[i].command;
> -        dest->ciw[i].count = cpu_to_be16(src->ciw[i].count);
> +    dest[0] = src->reserved;
> +    dest[1] = src->cu_type >> 8;
> +    dest[2] = src->cu_type & 0xff;
> +    dest[3] = src->cu_model;
> +    dest[4] = src->dev_type >> 8;
> +    dest[5] = src->dev_type & 0xff;
> +    dest[6] = src->dev_model;
> +    dest[7] = src->unused;
> +    for (i = 0; i < ARRAY_SIZE(src->ciw); i++) {
> +        dest[8 + i * 4] = src->ciw[i].type;
> +        dest[9 + i * 4] = src->ciw[i].command;
> +        dest[10 + i * 4] = src->ciw[i].count >> 8;
> +        dest[11 + i * 4] = src->ciw[i].count & 0xff;

/me gets a headache while trying to understand the endianness here...

>      }
>  }
>  
> @@ -1044,9 +1047,9 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>          break;
>      case CCW_CMD_SENSE_ID:
>      {
> -        SenseId sense_id;
> +        uint8_t sense_id[256];

If we go with this approach, maybe add a #define here? The
documentation ("Common I/O Device Commands") defines the sense id data
to be at most 256 bytes, so that would fit nicely.

>  
> -        copy_sense_id_to_guest(&sense_id, &sch->id);
> +        copy_sense_id_to_guest(sense_id, &sch->id);
>          /* Sense ID information is device specific. */
>          if (check_len) {
>              if (ccw.count != sizeof(sense_id)) {
> @@ -1060,11 +1063,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>           * have enough place to store at least bytes 0-3.
>           */
>          if (len >= 4) {
> -            sense_id.reserved = 0xff;
> +            sense_id[0] = 0xff;
>          } else {
> -            sense_id.reserved = 0;
> +            sense_id[0] = 0;
>          }
> -        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> +        ccw_dstream_write_buf(&sch->cds, sense_id, len);
>          sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>          ret = 0;
>          break;
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 9da5912..bec82d0 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -48,7 +48,7 @@ typedef struct SenseId {
>      uint8_t unused;          /* padding byte */
>      /* extended part */
>      CIW ciw[MAX_CIWS];       /* variable # of CIWs */
> -} QEMU_PACKED SenseId;
> +} SenseId;                   /* Note: No QEMU_PACKED due to unaligned members */
>  
>  /* Channel measurements, from linux/drivers/s390/cio/cmf.c. */
>  typedef struct CMB {

On 25 September 2018 at 17:14, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, 25 Sep 2018 17:20:08 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> The uint16_t member cu_type of struct SenseId is not naturally aligned,
>> and since the struct is marked with QEMU_PACKED, this can lead to
>> unaligned memory accesses - which does not work on architectures like
>> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
>> byte by byte when we do copy_sense_id_to_guest().
>
> Hm... would it look nicer if we chopped up {cu,dev}_type into a hi/lo
> byte and keep the QEMU_PACKED? (The ciws should be ok, I think.)

>> -static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
>> +static void copy_sense_id_to_guest(uint8_t *dest, SenseId *src)
>>  {
>>      int i;
>>
>> -    dest->reserved = src->reserved;
>> -    dest->cu_type = cpu_to_be16(src->cu_type);
>> -    dest->cu_model = src->cu_model;
>> -    dest->dev_type = cpu_to_be16(src->dev_type);
>> -    dest->dev_model = src->dev_model;
>> -    dest->unused = src->unused;
>> -    for (i = 0; i < ARRAY_SIZE(dest->ciw); i++) {
>> -        dest->ciw[i].type = src->ciw[i].type;
>> -        dest->ciw[i].command = src->ciw[i].command;
>> -        dest->ciw[i].count = cpu_to_be16(src->ciw[i].count);
>> +    dest[0] = src->reserved;
>> +    dest[1] = src->cu_type >> 8;
>> +    dest[2] = src->cu_type & 0xff;
>> +    dest[3] = src->cu_model;
>> +    dest[4] = src->dev_type >> 8;
>> +    dest[5] = src->dev_type & 0xff;
>> +    dest[6] = src->dev_model;
>> +    dest[7] = src->unused;
>> +    for (i = 0; i < ARRAY_SIZE(src->ciw); i++) {
>> +        dest[8 + i * 4] = src->ciw[i].type;
>> +        dest[9 + i * 4] = src->ciw[i].command;
>> +        dest[10 + i * 4] = src->ciw[i].count >> 8;
>> +        dest[11 + i * 4] = src->ciw[i].count & 0xff;
>
> /me gets a headache while trying to understand the endianness here...

Possibly easier to understand written as:
    stw_be_p(dest + 10 + i * 4, src->ciw[i].count);

(There is also an stb_p(ptr, value) if you want all the filling
in of the dest buffer here to look consistent, though it doesn't
do anything more magical than *(uint8_t *)ptr = value.)

thanks
-- PMM

On 2018-09-25 18:14, Cornelia Huck wrote:
> On Tue, 25 Sep 2018 17:20:08 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The uint16_t member cu_type of struct SenseId is not naturally aligned,
>> and since the struct is marked with QEMU_PACKED, this can lead to
>> unaligned memory accesses - which does not work on architectures like
>> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
>> byte by byte when we do copy_sense_id_to_guest().
> 
> Hm... would it look nicer if we chopped up {cu,dev}_type into a hi/lo
> byte and keep the QEMU_PACKED? (The ciws should be ok, I think.)

That will cause some ugly extra logic for the

  VMSTATE_UINT16(cu_type, SenseId)

in css.c instead. I'd like to avoid that, but if you insist, I can have
a try...

 Thomas

On Tue, 25 Sep 2018 18:43:22 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 2018-09-25 18:14, Cornelia Huck wrote:
> > On Tue, 25 Sep 2018 17:20:08 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> The uint16_t member cu_type of struct SenseId is not naturally aligned,
> >> and since the struct is marked with QEMU_PACKED, this can lead to
> >> unaligned memory accesses - which does not work on architectures like
> >> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
> >> byte by byte when we do copy_sense_id_to_guest().  
> > 
> > Hm... would it look nicer if we chopped up {cu,dev}_type into a hi/lo
> > byte and keep the QEMU_PACKED? (The ciws should be ok, I think.)  
> 
> That will cause some ugly extra logic for the
> 
>   VMSTATE_UINT16(cu_type, SenseId)
> 
> in css.c instead. I'd like to avoid that, but if you insist, I can have
> a try...

That's why I asked "would it look nicer?" :) If it doesn't (ugly
vmstate stuff), just disregard my idea.

On 25/09/2018 17:20, Thomas Huth wrote:
> The uint16_t member cu_type of struct SenseId is not naturally aligned,
> and since the struct is marked with QEMU_PACKED, this can lead to
> unaligned memory accesses - which does not work on architectures like
> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
> byte by byte when we do copy_sense_id_to_guest().
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/css.c         | 35 +++++++++++++++++++----------------
>  include/hw/s390x/css.h |  2 +-
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 5a9fe45..aaa2efa 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -750,20 +750,23 @@ static void sch_handle_halt_func(SubchDev *sch)
>  
>  }
>  
> -static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
> +static void copy_sense_id_to_guest(uint8_t *dest, SenseId *src)
>  {
>      int i;
>  
> -    dest->reserved = src->reserved;
> -    dest->cu_type = cpu_to_be16(src->cu_type);
> -    dest->cu_model = src->cu_model;
> -    dest->dev_type = cpu_to_be16(src->dev_type);
> -    dest->dev_model = src->dev_model;
> -    dest->unused = src->unused;
> -    for (i = 0; i < ARRAY_SIZE(dest->ciw); i++) {
> -        dest->ciw[i].type = src->ciw[i].type;
> -        dest->ciw[i].command = src->ciw[i].command;
> -        dest->ciw[i].count = cpu_to_be16(src->ciw[i].count);
> +    dest[0] = src->reserved;
> +    dest[1] = src->cu_type >> 8;
> +    dest[2] = src->cu_type & 0xff;
> +    dest[3] = src->cu_model;
> +    dest[4] = src->dev_type >> 8;
> +    dest[5] = src->dev_type & 0xff;
> +    dest[6] = src->dev_model;
> +    dest[7] = src->unused;
> +    for (i = 0; i < ARRAY_SIZE(src->ciw); i++) {
> +        dest[8 + i * 4] = src->ciw[i].type;
> +        dest[9 + i * 4] = src->ciw[i].command;
> +        dest[10 + i * 4] = src->ciw[i].count >> 8;
> +        dest[11 + i * 4] = src->ciw[i].count & 0xff;
>      }
>  }
>  
> @@ -1044,9 +1047,9 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>          break;
>      case CCW_CMD_SENSE_ID:
>      {
> -        SenseId sense_id;
> +        uint8_t sense_id[256];
>  
> -        copy_sense_id_to_guest(&sense_id, &sch->id);
> +        copy_sense_id_to_guest(sense_id, &sch->id);
>          /* Sense ID information is device specific. */
>          if (check_len) {
>              if (ccw.count != sizeof(sense_id)) {
> @@ -1060,11 +1063,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>           * have enough place to store at least bytes 0-3.
>           */
>          if (len >= 4) {
> -            sense_id.reserved = 0xff;
> +            sense_id[0] = 0xff;
>          } else {
> -            sense_id.reserved = 0;
> +            sense_id[0] = 0;
>          }
> -        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
> +        ccw_dstream_write_buf(&sch->cds, sense_id, len);
>          sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>          ret = 0;
>          break;
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index 9da5912..bec82d0 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -48,7 +48,7 @@ typedef struct SenseId {
>      uint8_t unused;          /* padding byte */
>      /* extended part */
>      CIW ciw[MAX_CIWS];       /* variable # of CIWs */
> -} QEMU_PACKED SenseId;
> +} SenseId;                   /* Note: No QEMU_PACKED due to unaligned members */
>  
>  /* Channel measurements, from linux/drivers/s390/cio/cmf.c. */
>  typedef struct CMB {
> 

Dumb idea: as migration under spark never worked, add some #idef
alignment fix for sparc only?

-- 

Thanks,

David / dhildenb

On 2018-09-25 18:52, David Hildenbrand wrote:
> On 25/09/2018 17:20, Thomas Huth wrote:
>> The uint16_t member cu_type of struct SenseId is not naturally aligned,
>> and since the struct is marked with QEMU_PACKED, this can lead to
>> unaligned memory accesses - which does not work on architectures like
>> Sparc. Thus remove the QEMU_PACKED here and rather copy the struct
>> byte by byte when we do copy_sense_id_to_guest().
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/s390x/css.c         | 35 +++++++++++++++++++----------------
>>  include/hw/s390x/css.h |  2 +-
>>  2 files changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
>> index 5a9fe45..aaa2efa 100644
>> --- a/hw/s390x/css.c
>> +++ b/hw/s390x/css.c
>> @@ -750,20 +750,23 @@ static void sch_handle_halt_func(SubchDev *sch)
>>  
>>  }
>>  
>> -static void copy_sense_id_to_guest(SenseId *dest, SenseId *src)
>> +static void copy_sense_id_to_guest(uint8_t *dest, SenseId *src)
>>  {
>>      int i;
>>  
>> -    dest->reserved = src->reserved;
>> -    dest->cu_type = cpu_to_be16(src->cu_type);
>> -    dest->cu_model = src->cu_model;
>> -    dest->dev_type = cpu_to_be16(src->dev_type);
>> -    dest->dev_model = src->dev_model;
>> -    dest->unused = src->unused;
>> -    for (i = 0; i < ARRAY_SIZE(dest->ciw); i++) {
>> -        dest->ciw[i].type = src->ciw[i].type;
>> -        dest->ciw[i].command = src->ciw[i].command;
>> -        dest->ciw[i].count = cpu_to_be16(src->ciw[i].count);
>> +    dest[0] = src->reserved;
>> +    dest[1] = src->cu_type >> 8;
>> +    dest[2] = src->cu_type & 0xff;
>> +    dest[3] = src->cu_model;
>> +    dest[4] = src->dev_type >> 8;
>> +    dest[5] = src->dev_type & 0xff;
>> +    dest[6] = src->dev_model;
>> +    dest[7] = src->unused;
>> +    for (i = 0; i < ARRAY_SIZE(src->ciw); i++) {
>> +        dest[8 + i * 4] = src->ciw[i].type;
>> +        dest[9 + i * 4] = src->ciw[i].command;
>> +        dest[10 + i * 4] = src->ciw[i].count >> 8;
>> +        dest[11 + i * 4] = src->ciw[i].count & 0xff;
>>      }
>>  }
>>  
>> @@ -1044,9 +1047,9 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>          break;
>>      case CCW_CMD_SENSE_ID:
>>      {
>> -        SenseId sense_id;
>> +        uint8_t sense_id[256];
>>  
>> -        copy_sense_id_to_guest(&sense_id, &sch->id);
>> +        copy_sense_id_to_guest(sense_id, &sch->id);
>>          /* Sense ID information is device specific. */
>>          if (check_len) {
>>              if (ccw.count != sizeof(sense_id)) {
>> @@ -1060,11 +1063,11 @@ static int css_interpret_ccw(SubchDev *sch, hwaddr ccw_addr,
>>           * have enough place to store at least bytes 0-3.
>>           */
>>          if (len >= 4) {
>> -            sense_id.reserved = 0xff;
>> +            sense_id[0] = 0xff;
>>          } else {
>> -            sense_id.reserved = 0;
>> +            sense_id[0] = 0;
>>          }
>> -        ccw_dstream_write_buf(&sch->cds, &sense_id, len);
>> +        ccw_dstream_write_buf(&sch->cds, sense_id, len);
>>          sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>>          ret = 0;
>>          break;
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index 9da5912..bec82d0 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
>> @@ -48,7 +48,7 @@ typedef struct SenseId {
>>      uint8_t unused;          /* padding byte */
>>      /* extended part */
>>      CIW ciw[MAX_CIWS];       /* variable # of CIWs */
>> -} QEMU_PACKED SenseId;
>> +} SenseId;                   /* Note: No QEMU_PACKED due to unaligned members */
>>  
>>  /* Channel measurements, from linux/drivers/s390/cio/cmf.c. */
>>  typedef struct CMB {
> 
> Dumb idea: as migration under spark never worked, add some #idef
> alignment fix for sparc only?

Better not, #ifdefs even make the code even more uglier and harder to
test (e.g. currently I still can check the alignment issues with
-fsanitize=undefined ... with an #ifdef it's not possible anymore).

 Thomas