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

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

On Thu, 30 Jan 2014 15:13:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jan 30, 2014 at 01:10:29PM +0100, Cornelia Huck wrote:
> > 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).
> 
> So how about something like attribute((virtio_endian)) (a bit too long)
> or ve32 (slightly ugly).

vexx might be slightly ugly, but I think it's fine if defined properly.

> 
> > 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.
> 
> In practice, this will make it harder to write transitional
> drivers. Driver authors will have to reverse-engineer field
> layouts and note that offsets happen to match.
> That's not being nice to readers.

You have a point.

> 
> Take a look at virtio drivers that Rusty wrote:
> ring layouts *don't* use le tags because sometimes
> they are used with be.
>