Re: [Qemu-devel] [PATCH v3 0/3] Fix migration problems of s390x guests on Sparc hosts

Re: [Qemu-devel] [PATCH v3 0/3] Fix migration problems of s390x guests on Sparc hosts

[Cornelia Huck]

On Thu, 27 Sep 2018 10:23:32 +0200
Thomas Huth <thuth@redhat.com> wrote:

> The new migration test uncovered some alignment problems in the s390x
> code:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg03012.html
> 
> Here are some patches to fix these issues (only tested with
> clang and -fsanitize=undefined, since I do not have access to
> a Sparc machine, but I hope that covers the issues there, too).
> 
> v3:
>  - Fix description of the first patch
>  - Add a comment before copy_sense_id_to_guest() in the 2nd patch
> 
> v2:
>  - Use static assert with offsetof in the first patch instead of comments
>  - Use stw_be_p in the second patch and add a comment about SA22-7204
> 
> Thomas Huth (3):
>   hw/s390x/ipl: Fix alignment problems of S390IPLState members
>   hw/s390x/css: Remove QEMU_PACKED from struct SenseId
>   hw/s390x/ioinst: Fix alignment problem in struct SubchDev
> 
>  hw/s390x/css.c            | 38 ++++++++++++++++++++++----------------
>  hw/s390x/ipl.h            |  5 +++--
>  include/hw/s390x/css.h    |  6 +++---
>  include/hw/s390x/ioinst.h | 21 ++++++++++++++-------
>  4 files changed, 42 insertions(+), 28 deletions(-)
> 

Thanks, applied.

Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev

[Cornelia Huck]

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.

Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev

[Peter Maydell]

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