* Re: [Qemu-devel] [PATCH v3 0/3] Fix migration problems of s390x guests on Sparc hosts [not found] <1538036615-32542-1-git-send-email-thuth@redhat.com> @ 2018-10-01 12:28 ` Cornelia Huck [not found] ` <1538036615-32542-4-git-send-email-thuth@redhat.com> 1 sibling, 0 replies; 5+ messages in thread From: Cornelia Huck @ 2018-10-01 12:28 UTC (permalink / raw) To: Thomas Huth Cc: qemu-s390x, qemu-devel, Christian Borntraeger, David Hildenbrand, Dr. David Alan Gilbert 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <1538036615-32542-4-git-send-email-thuth@redhat.com>]
* Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev [not found] ` <1538036615-32542-4-git-send-email-thuth@redhat.com> @ 2018-12-10 12:27 ` Peter Maydell 2018-12-10 13:16 ` Cornelia Huck 0 siblings, 1 reply; 5+ messages in thread From: Peter Maydell @ 2018-12-10 12:27 UTC (permalink / raw) To: Thomas Huth Cc: qemu-s390x, Cornelia Huck, Christian Borntraeger, QEMU Developers, Dr. David Alan Gilbert, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev 2018-12-10 12:27 ` [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev Peter Maydell @ 2018-12-10 13:16 ` Cornelia Huck 2018-12-10 13:32 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 5+ messages in thread From: Cornelia Huck @ 2018-12-10 13:16 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, qemu-s390x, Christian Borntraeger, QEMU Developers, Dr. David Alan Gilbert, David Hildenbrand 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev 2018-12-10 13:16 ` Cornelia Huck @ 2018-12-10 13:32 ` Dr. David Alan Gilbert 2018-12-10 13:47 ` Peter Maydell 0 siblings, 1 reply; 5+ messages in thread From: Dr. David Alan Gilbert @ 2018-12-10 13:32 UTC (permalink / raw) To: Cornelia Huck Cc: Peter Maydell, Thomas Huth, qemu-s390x, Christian Borntraeger, QEMU Developers, David Hildenbrand * 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev 2018-12-10 13:32 ` Dr. David Alan Gilbert @ 2018-12-10 13:47 ` Peter Maydell 0 siblings, 0 replies; 5+ messages in thread From: Peter Maydell @ 2018-12-10 13:47 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Cornelia Huck, Thomas Huth, qemu-s390x, Christian Borntraeger, QEMU Developers, David Hildenbrand 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-10 13:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1538036615-32542-1-git-send-email-thuth@redhat.com>
2018-10-01 12:28 ` [Qemu-devel] [PATCH v3 0/3] Fix migration problems of s390x guests on Sparc hosts Cornelia Huck
[not found] ` <1538036615-32542-4-git-send-email-thuth@redhat.com>
2018-12-10 12:27 ` [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev Peter Maydell
2018-12-10 13:16 ` Cornelia Huck
2018-12-10 13:32 ` Dr. David Alan Gilbert
2018-12-10 13:47 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).