From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38753) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gWLm9-0006Ie-6I for qemu-devel@nongnu.org; Mon, 10 Dec 2018 08:39:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gWLfU-000413-Q8 for qemu-devel@nongnu.org; Mon, 10 Dec 2018 08:32:21 -0500 Date: Mon, 10 Dec 2018 13:32:06 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20181210133205.GC2372@work-vm> References: <1538036615-32542-1-git-send-email-thuth@redhat.com> <1538036615-32542-4-git-send-email-thuth@redhat.com> <20181210141607.0ef48273.cohuck@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181210141607.0ef48273.cohuck@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 3/3] hw/s390x/ioinst: Fix alignment problem in struct SubchDev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 wrote: > > > On Thu, 27 Sep 2018 at 09:25, 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. > > > > 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