Re: [qemu-s390x] [PATCH] hw/s390x/virtio-ccw.c: Don't take address of fields in packed structs

[Cornelia Huck]

On Mon, 10 Dec 2018 14:06:42 +0100
Halil Pasic <address@hidden> wrote:

> On Mon, 10 Dec 2018 12:04:36 +0000
> Peter Maydell <address@hidden> wrote:
> 
> > Taking the address of a field in a packed struct is a bad idea, because
> > it might not be actually aligned enough for that pointer type (and
> > thus cause a crash on dereference on some host architectures). Newer
> > versions of clang warn about this. Avoid the bug by not using the
> > "modify in place" byte swapping functions.
> > 
> > Patch produced with scripts/coccinelle/inplace-byteswaps.cocci
> > (with a couple of long lines manually wrapped).
> > 
> > Signed-off-by: Peter Maydell <address@hidden>  
> 
> Reviewed-by: Halil Pasic <address@hidden>
> 
> I wonder if packed was a good idea in the first place. @Connie: Do you
> have an opinion on this?

Some of the structures probably don't need the packed attribute; but
there are two structures that have a trailing 8 bit value. The problem
is that the structures are a a guest<->host interface as defined in the
virtio standard, so we can't e.g. add padding to the structures with
the isc at the end.

If I had to re-define the interface, I'd probably add tail padding; but
I was not aware of the problems the packing might cause when I defined
them, and we can't get rid of them without also getting rid of support
for older virtio versions.

Simply moving away from the modify in place byteswaps is the easiest
solution.