Re: [qemu-s390x] [PATCH 2/3] hw/s390x/css: Remove QEMU_PACKED from struct SenseId

[David Hildenbrand]

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 <address@hidden>
> ---
>  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