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

[Dr. David Alan Gilbert]

* Cornelia Huck (address@hidden) wrote:
> On Mon, 10 Dec 2018 12:27:56 +0000
> Peter Maydell <address@hidden> wrote:
> 
> > On Thu, 27 Sep 2018 at 09:25, Thomas Huth <address@hidden> 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 / address@hidden / Manchester, UK