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

[Thomas Huth]

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 <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?

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