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 <th...@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