OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-dev] Re: [virtio-comment] [PATCH 1/1] ccw: ccw payload description

James Bottomley <jbottomley@parallels.com> writes:
> On Tue, 2014-01-21 at 14:34 +0200, Michael S. Tsirkin wrote:
>> > > What does this add to what I wrote above?
>> > 
>> > I'd like to be able to drop the __attribute__((packed)), and just make
>> > that the default.
>> To me __attribute__((packed)) seems helpful, and harmless.
>> If someone wants to pack everything in driver and bloat up
>> code unnecessarily, one always has the option.
> To me as well.  As a driver writer, I start to worry when I *don't* see
> an __attribute__((packed)) on any data structure shared between the
> driver and the device (whether or not it's a virtual device).
> Conversely, relying on the C compiler to do standard padding is
> dangerous (and error prone).  For instance, the padding requirements for
> 64 bit words on x86 is 32 bits, but it's 64 bits on every other
> architecture, so 

Agreed, but just to be clear:

- We always use explicit length types in the spec.

- We have only one case where we have padding, and it's explicit
    (virtio_blk's "unused" field).

- CCW has two places (and virtio_balloon one case) where the struct is
  unnaturally *short*: these must be explicitly called out IMHO.

I'm happy with Cornelia's explicit verbiage around the different types,
though I can't say it's confused anyone yet.

BTW, there are several suspect uses of "u16" in the spec, in particular:

	struct virtio_rev_info {
	       __u16 revision;
	       __u16 length;
	       __u8 data[];

(I'll address the VIRTIO_NET_F_MQ ones in a separate patch).


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]