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 <thuth@redhat.com>
---
 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
-- 
1.8.3.1

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 <thuth@redhat.com>
> ---
>  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 <david@redhat.com>

-- 

Thanks,

David / dhildenb

On Thu, 27 Sep 2018 at 09:25, Thomas Huth <thuth@redhat.com> 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.

Unfortunately clang does not like the struct SCHIB being still
marked packed:

/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
                        ^~~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
                                     ^~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_scsw_to_guest(&dest->scsw, &src->scsw);
                        ^~~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_scsw_to_guest(&dest->scsw, &src->scsw);
                                     ^~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
                          ^~~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
warning: taking address of packed member 'pmcw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
                                       ^~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_scsw_from_guest(&dest->scsw, &src->scsw);
                          ^~~~~~~~~~
/home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
warning: taking address of packed member 'scsw' of class or structure
'SCHIB' may result in an unaligned pointer value
[-Waddress-of-packed-member]
    copy_scsw_from_guest(&dest->scsw, &src->scsw);
                                       ^~~~~~~~~


Not sure how best to address this. A couple of ideas that I had:

(1) make the 'uint64_t mba' field in the SCHIB struct into
two uint32_t fields, adjusting all the code which needs
to access it accordingly; then we could drop the packed
annotation from the struct

(2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
macros, so we can do them inline in the copy_schib_{to,from}_guest()
function and thus operate directly on src->pmcw.foo &c
fields rather than ever having to take the address of any
of the fields in src or dest

thanks
-- PMM

On Mon, 10 Dec 2018 12:27:56 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 27 Sep 2018 at 09:25, Thomas Huth <thuth@redhat.com> 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.  
> 
> Unfortunately clang does not like the struct SCHIB being still
> marked packed:
> 
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
>                         ^~~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
>                                      ^~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_scsw_to_guest(&dest->scsw, &src->scsw);
>                         ^~~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_scsw_to_guest(&dest->scsw, &src->scsw);
>                                      ^~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
>                           ^~~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
> warning: taking address of packed member 'pmcw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
>                                        ^~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_scsw_from_guest(&dest->scsw, &src->scsw);
>                           ^~~~~~~~~~
> /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
> warning: taking address of packed member 'scsw' of class or structure
> 'SCHIB' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>     copy_scsw_from_guest(&dest->scsw, &src->scsw);
>                                        ^~~~~~~~~

That's really annoying :(

> Not sure how best to address this. A couple of ideas that I had:
> 
> (1) make the 'uint64_t mba' field in the SCHIB struct into
> two uint32_t fields, adjusting all the code which needs
> to access it accordingly; then we could drop the packed
> annotation from the struct

This would mean some annoying gymnastics, but fortunately that field is
not accessed in many places.

> 
> (2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
> macros, so we can do them inline in the copy_schib_{to,from}_guest()
> function and thus operate directly on src->pmcw.foo &c
> fields rather than ever having to take the address of any
> of the fields in src or dest

I'm not really a fan of using macros, but if it stays readable...

Not sure what the best option is here; this is why I haven't done
anything yet to fix it, as no idea was really appealing.

* Cornelia Huck (cohuck@redhat.com) wrote:
> On Mon, 10 Dec 2018 12:27:56 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On Thu, 27 Sep 2018 at 09:25, Thomas Huth <thuth@redhat.com> 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.  
> > 
> > Unfortunately clang does not like the struct SCHIB being still
> > marked packed:
> > 
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:25:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> >                         ^~~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1294:38:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
> >                                      ^~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:25:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_scsw_to_guest(&dest->scsw, &src->scsw);
> >                         ^~~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1295:38:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_scsw_to_guest(&dest->scsw, &src->scsw);
> >                                      ^~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:27:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> >                           ^~~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1343:40:
> > warning: taking address of packed member 'pmcw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
> >                                        ^~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:27:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_scsw_from_guest(&dest->scsw, &src->scsw);
> >                           ^~~~~~~~~~
> > /home/petmay01/linaro/qemu-from-laptop/qemu/hw/s390x/css.c:1344:40:
> > warning: taking address of packed member 'scsw' of class or structure
> > 'SCHIB' may result in an unaligned pointer value
> > [-Waddress-of-packed-member]
> >     copy_scsw_from_guest(&dest->scsw, &src->scsw);
> >                                        ^~~~~~~~~
> 
> That's really annoying :(

Is the problem here that the field could actually be misaligned (on
any conceivable build) or is it just a matter of convincing clang it's
safe?

Dave

> > Not sure how best to address this. A couple of ideas that I had:
> > 
> > (1) make the 'uint64_t mba' field in the SCHIB struct into
> > two uint32_t fields, adjusting all the code which needs
> > to access it accordingly; then we could drop the packed
> > annotation from the struct
> 
> This would mean some annoying gymnastics, but fortunately that field is
> not accessed in many places.
> 
> > 
> > (2) have the guts of copy_{pmcw,scsw}_{to,from}_guest() be
> > macros, so we can do them inline in the copy_schib_{to,from}_guest()
> > function and thus operate directly on src->pmcw.foo &c
> > fields rather than ever having to take the address of any
> > of the fields in src or dest
> 
> I'm not really a fan of using macros, but if it stays readable...
> 
> Not sure what the best option is here; this is why I haven't done
> anything yet to fix it, as no idea was really appealing.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On Mon, 10 Dec 2018 at 13:32, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> Is the problem here that the field could actually be misaligned (on
> any conceivable build) or is it just a matter of convincing clang it's
> safe?

This is mostly a "clang doesn't know that the struct field
will actually always be 4 aligned and the function being
called only requires 4 alignment" thing. But there is an
actual-in-theory problem too: because the SCHIB struct is
marked QEMU_PACKED it has no alignment requirements at all.
So in for instance css_do_msch() the compiler is entitled
to align the local "SCHIB schib" at any alignment it likes,
which may not be the 4-alignment that copy_pmcw_from_guest()
assumes.

An option (3) which I hadn't previously thought of:
copy the pmcw and scsw fields into and out of locals
which are guaranteed to be correctly aligned:

--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1290,9 +1290,15 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src)
 static void copy_schib_to_guest(SCHIB *dest, const SCHIB *src)
 {
     int i;
+    PMCW srcpmcw, destpmcw;
+    SCSW srcscsw, destscsw;

-    copy_pmcw_to_guest(&dest->pmcw, &src->pmcw);
-    copy_scsw_to_guest(&dest->scsw, &src->scsw);
+    srcpmcw = src->pmcw;
+    copy_pmcw_to_guest(&destpmcw, &srcpmcw);
+    dest->pmcw = destpmcw;
+    srcscsw = src->scsw;
+    copy_scsw_to_guest(&destscsw, &srcscsw);
+    dest->scsw = destscsw;
     dest->mba = cpu_to_be64(src->mba);
     for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
         dest->mda[i] = src->mda[i];
@@ -1339,9 +1345,15 @@ static void copy_scsw_from_guest(SCSW *dest,
const SCSW *src)
 static void copy_schib_from_guest(SCHIB *dest, const SCHIB *src)
 {
     int i;
+    PMCW srcpmcw, destpmcw;
+    SCSW srcscsw, destscsw;

-    copy_pmcw_from_guest(&dest->pmcw, &src->pmcw);
-    copy_scsw_from_guest(&dest->scsw, &src->scsw);
+    srcpmcw = src->pmcw;
+    copy_pmcw_from_guest(&destpmcw, &srcpmcw);
+    dest->pmcw = destpmcw;
+    srcscsw = src->scsw;
+    copy_scsw_from_guest(&destscsw, &srcscsw);
+    dest->scsw = destscsw;
     dest->mba = be64_to_cpu(src->mba);
     for (i = 0; i < ARRAY_SIZE(dest->mda); i++) {
         dest->mda[i] = src->mda[i];

thanks
-- PMM