Re: [PATCH RFC] virtio-pci: new config layout: using memory BAR

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, May 29, 2013 at 02:28:32PM +0200, Paolo Bonzini wrote:
> Il 29/05/2013 14:16, Michael S. Tsirkin ha scritto:
> >>>> > >> If you really want to use offsetof like this you're
> >>>> > >> going to need to decorate the structs with QEMU_PACKED.
> >> > 
> >>> > > Nope.
> >>> > > These structs are carefully designed not to have any padding.
> >> > 
> >> > ...on every compiler and OS combination that QEMU builds for?
> > Yes. All the way back to EGCS and before.
> > GCC has been like this for many many years.
> 
> I would still prefer to have QEMU_PACKED (or __attribute((__packed__))
> in the kernel).
> 
> >>> > > And if there was a bug and there was some padding, we still
> >>> > > can't fix it with PACKED because this structure
> >>> > > is used to interact with the guest code which does not
> >>> > > have the packed attribute.
> >> > 
> >> > The guest code has to use a set of structure offsets and
> >> > sizes which is fixed by the virtio ABI -- how it implements
> >> > this is up to the guest (and if it misimplements it that is
> >> > a guest bug and not our problem).
> 
> On the other hand, encouraging both userspace and guest to reuse a
> single set of headers means that the bad offset becomes a spec bug more
> than a guest bug.

Heh

> > Deviating from driver in random ways is an endless source
> > of hard to debug issues, and it's a very practical
> > consideration because virtio spec is constantly
> > being extended (unlike hardware which is mostly fixed).
> 
> Agreed---but the driver should use __attribute__((__packed__)) too.
> 
> Paolo

For these specific structures I don't mind, we never dereference them
anyway. If you do dereference a structure, using packed is generally
a mistake. In particular because GCC generates code that is
much slower.

You can also get nasty surprises e.g.

struct foo {
	char a;
	int b;
} __attribute__((packed));

struct foo foo;
int *a = &foo.a;

Last line compiles fine but isn't legal C.

So I think there are two cases with packed:
1. it does not change logical behaviour but results in bad code
   generated
2. it does change logical behaviour and leads to unsafe code

There aren't many good reasons to use packed: if you
must treat unaligned data, best to use constants.

-- 
MST