[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem i
From: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev |
Date: |
Thu, 27 Sep 2018 10:40:10 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 27/09/2018 10:23, Thomas Huth wrote:
> struct SubchDev embeds several other structures which are marked with
> QEMU_PACKED. This causes the compiler to not care for proper alignment
> of these structures. When we later pass around pointers to the unaligned
> struct members during migration, this causes problems on host architectures
> like Sparc that can not do unaligned memory access.
>
> Most of the structs in ioinst.h are naturally aligned, so we can fix
> most of the problem by removing the QEMU_PACKED statements (and use
> QEMU_BUILD_BUG_MSG() statements instead to make sure that there is no
> padding). However, for the struct SCHIB, we have to keep the QEMU_PACKED
> since the compiler adds some padding here otherwise. Move this struct
> to the beginning of struct SubchDev instead to fix the alignment problem
> here, too.
>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
> include/hw/s390x/css.h | 4 ++--
> include/hw/s390x/ioinst.h | 21 ++++++++++++++-------
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index bec82d0..aae19c4 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -118,11 +118,12 @@ typedef enum IOInstEnding {
> typedef struct SubchDev SubchDev;
> struct SubchDev {
> /* channel-subsystem related things: */
> + SCHIB curr_status; /* Needs alignment and thus must come first
> */
> + ORB orb;
> uint8_t cssid;
> uint8_t ssid;
> uint16_t schid;
> uint16_t devno;
> - SCHIB curr_status;
> uint8_t sense_data[32];
> hwaddr channel_prog;
> CCW1 last_cmd;
> @@ -131,7 +132,6 @@ struct SubchDev {
> bool thinint_active;
> uint8_t ccw_no_data_cnt;
> uint16_t migrated_schid; /* used for missmatch detection */
> - ORB orb;
> CcwDataStream cds;
> /* transport-provided data: */
> int (*ccw_cb) (SubchDev *, CCW1);
> diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
> index 5f2db69..c6737a3 100644
> --- a/include/hw/s390x/ioinst.h
> +++ b/include/hw/s390x/ioinst.h
> @@ -25,7 +25,8 @@ typedef struct SCSW {
> uint8_t dstat;
> uint8_t cstat;
> uint16_t count;
> -} QEMU_PACKED SCSW;
> +} SCSW;
> +QEMU_BUILD_BUG_MSG(sizeof(SCSW) != 12, "size of SCSW is wrong");
>
> #define SCSW_FLAGS_MASK_KEY 0xf000
> #define SCSW_FLAGS_MASK_SCTL 0x0800
> @@ -94,7 +95,8 @@ typedef struct PMCW {
> uint8_t pam;
> uint8_t chpid[8];
> uint32_t chars;
> -} QEMU_PACKED PMCW;
> +} PMCW;
> +QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong");
>
> #define PMCW_FLAGS_MASK_QF 0x8000
> #define PMCW_FLAGS_MASK_W 0x4000
> @@ -127,7 +129,8 @@ typedef struct IRB {
> uint32_t esw[5];
> uint32_t ecw[8];
> uint32_t emw[8];
> -} QEMU_PACKED IRB;
> +} IRB;
> +QEMU_BUILD_BUG_MSG(sizeof(IRB) != 96, "size of IRB is wrong");
>
> /* operation request block */
> typedef struct ORB {
> @@ -136,7 +139,8 @@ typedef struct ORB {
> uint8_t lpm;
> uint8_t ctrl1;
> uint32_t cpa;
> -} QEMU_PACKED ORB;
> +} ORB;
> +QEMU_BUILD_BUG_MSG(sizeof(ORB) != 12, "size of ORB is wrong");
>
> #define ORB_CTRL0_MASK_KEY 0xf000
> #define ORB_CTRL0_MASK_SPND 0x0800
> @@ -165,7 +169,8 @@ typedef struct CCW0 {
> uint8_t flags;
> uint8_t reserved;
> uint16_t count;
> -} QEMU_PACKED CCW0;
> +} CCW0;
> +QEMU_BUILD_BUG_MSG(sizeof(CCW0) != 8, "size of CCW0 is wrong");
>
> /* channel command word (type 1) */
> typedef struct CCW1 {
> @@ -173,7 +178,8 @@ typedef struct CCW1 {
> uint8_t flags;
> uint16_t count;
> uint32_t cda;
> -} QEMU_PACKED CCW1;
> +} CCW1;
> +QEMU_BUILD_BUG_MSG(sizeof(CCW1) != 8, "size of CCW1 is wrong");
>
> #define CCW_FLAG_DC 0x80
> #define CCW_FLAG_CC 0x40
> @@ -192,7 +198,8 @@ typedef struct CCW1 {
> typedef struct CRW {
> uint16_t flags;
> uint16_t rsid;
> -} QEMU_PACKED CRW;
> +} CRW;
> +QEMU_BUILD_BUG_MSG(sizeof(CRW) != 4, "size of CRW is wrong");
>
> #define CRW_FLAGS_MASK_S 0x4000
> #define CRW_FLAGS_MASK_R 0x2000
>
Reviewed-by: David Hildenbrand <address@hidden>
--
Thanks,
David / dhildenb