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" <> wrote:
>
> > On Thu, Jan 30, 2014 at 10:22:16AM +1030, Rusty Russell wrote:
> > > Cornelia Huck <> writes:
> > > > On Tue, 21 Jan 2014 10:37:26 +1030
> > > > Rusty Russell <> wrote:
> > > >
> > > >> Cornelia Huck <> writes:
> > > >> > On Mon, 20 Jan 2014 11:17:17 +0200
> > > >> > "Michael S. Tsirkin" <> 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).
> 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.
Take a look at virtio drivers that Rusty wrote:
ring layouts *don't* use le tags because sometimes
they are used with be.
--
MST
