[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH 1/1] ccw: ccw payload description
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). > 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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]