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

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Thu, 30 Jan 2014 09:21:42 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 30, 2014 at 10:22:16AM +1030, Rusty Russell wrote:
> > Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > > On Tue, 21 Jan 2014 10:37:26 +1030
> > > Rusty Russell <rusty@au1.ibm.com> wrote:
> > >
> > >> Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > >> > On Mon, 20 Jan 2014 11:17:17 +0200
> > >> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >> >
> > >> >> On Mon, Jan 20, 2014 at 02:28:00PM +1030, Rusty Russell wrote:
> > >> >> > We'll just have to live with the lack of padding, and let's try not to
> > >> >> > add any more packed-requiring structures.  If we deprecate
> > >> >> > virtio-balloon, the rest will be contained in CCW.
> > >> >> > 
> > >> >> > Cheers,
> > >> >> > Rusty.
> > >> >> 
> > >> >> For CCW, how about my idea of adding optional padding explicitly?
> > >> >> In the compatibility section, we can explicitly specify the
> > >> >> the length.
> > >> >> Drivers can use offsetof(padding) and avoid packed attribute.
> > >> >
> > >> > Since Rusty does not like the tables, this looks like a good idea.
> > >> 
> > >> Surely making the padding optional makes it more complex, for no real
> > >> gain?  It's not that much simpler in practice, is it?
> > >> 
> > >> This patch just removes the __attribute__((packed)) where it's useless,
> > >> and notes explicitly where it's required:
> > >
> > > (...)
> > >
> > > Do we want to go the route of this patch now, together with my
> > > clarifications in
> > > https://lists.oasis-open.org/archives/virtio-comment/201401/msg00034.html ?
> > 
> > It's not quite a trivial change, and I know there's some debate.  So
> > I've created an issue for it (VIRTIO-56), and we can discuss next
> > meeting.
> > 
> > Thanks,
> > Rusty.
> 
> I agree. For the record I agree with this change.

Great, I'll prepare a patch for VIRTIO-56.

> 
> There's something that's bothering me.
> We have re-written in-memory structures to say "le32" etc everywhere.
> But it's wrong for the legacy interface.
> 
> Also, it now says "le" everywhere, so this isn't very
> informative.

Except for some places in the ccw transport that are explicitly be...

> 
> For the new interface, everything is le so there's real no need
> to note this at each point.
> But for the old interface, some fields are LE and some are
> native.
> 
> So why don't we mark fields leXX if they are little endian in both
> interfaces, and uXX if they are little endian in new one and
> native in old one?

I would find that a bit confusing.

> 
> Alternatively we could invent our own type for "virtio endian".
> It seemed like a nice idea to me originally but I had a hard time
> coming up with with a nice name for it.
> 
> Thoughts?
> 

I think what makes this complicated is that we specify datastructures
that are supposed to work for both legacy and non-legacy, when they
are in fact different (at least on be).

How about adding "A note on legacy interfaces" for each of these cases,
where we specify the data structures with native-endian where
appropriate, and keep the explicit le notation for the standard? We'd
end up with some duplication, but we'd be very clear on what is
expected.