On Wed, Jun 18, 2014 at 05:14:17PM +0200, Alexander Graf wrote: > > On 18.06.14 17:12, Michael S. Tsirkin wrote: > >On Wed, Jun 18, 2014 at 02:35:21PM +0200, Greg Kurz wrote: > >>On Wed, 18 Jun 2014 18:38:14 +0800 > >>Stefan Hajnoczi <stefa...@gmail.com> wrote: > >> > >>>On Tue, Jun 17, 2014 at 09:40:19AM +0200, Alexander Graf wrote: > >>>>On 17.06.14 09:36, Stefan Hajnoczi wrote: > >>>>>On Fri, Jun 13, 2014 at 01:18:00PM +0200, Greg Kurz wrote: > >>>>>>This version merges the changes requested during the v7 review, remarks > >>>>>>from > >>>>>>ppc64 dump support review (yes, we talked about virtio there) and the > >>>>>>work on > >>>>>>virtio subsections migration. Also two new patches have been added: > >>>>>>- patch #1 is a preliminary fix for virtio-serial posted by Alexander > >>>>>>Graf > >>>>>>- patch #9 prepares the work on the virtio_is_big_endian() helper > >>>>>> > >>>>>>The most significant changes are: > >>>>>>- introduction of a new CPU method for virtio > >>>>>>- endianness is taken from CPU that resets the device > >>>>>>- fastpath virtio memory accessors for fixed endian targets > >>>>>>- VMState based virtio subsections (compatibility friendly) > >>>>>I'm surprised it's not enough for the virtio device to have an > >>>>>endianness field (big/little). It seems these patches make endianness > >>>>>depend on the CPUState through which the device is being accessed. > >>>>> > >>>>>Can you explain why it's necessary to check the CPUState? > >>>>They only check CPUState at the point in time of reset, as that's the only > >>>>case where we can derive the implicit endian configuration from :). > >>>What bothers me is that real hardware can't do this. Given that VIRTIO > >>>1.0 is always little-endian I guess this is just a temporary hack for > >>>ppc little-endian. Would be nice to add a comment so it's clear why > >>>this approach is being taken instead of a cleaner solution. > >>> > >>>Stefan > >>Hi Stefan, > >> > >>Previous versions of this patch set had such comments: > >> > >>"virtio data structures are defined as "target endian", which assumes > >>that's a fixed value. In fact, that actually means it's platform-specific. > >>The OASIS virtio 1.0 spec will fix this, by making all little endian. > >> > >>We need to support both implementations and we want to share as much code > >>as possible." > >> > >>but these lines got lost between v6 and v7... my bad... :-\ > >> > >>I agree all of this is a hack but: > >>- it has been on the table for nearly a year > >>- ppc LE distros are already available > >>- the memory accessors part makes sense for 1.0 > >> > >>and, speaking of 1.0, I couldn't find any clue about when QEMU would support > >>this (Cc'ing Rusty for his input), but we (IBM and distro partners) need > >>ppc LE support now. > >> > >>Cheers. > >QEMU 2.2 I think. > > > >One disadvantage of this work is that it removes some of the stimulus > >for people to work on 1.0, replacing it with hacks. Hmm. > > If you prefer I can also apply these patches and send a pull request for > them. > > > Alex
Not yet, please. v8 still got some comments, so we need to get v9. Then give people a bit of time to review. If everyone's happy it should be mergeable in about a week from now. Also there are Paolo's patches already in queue which will conflict, not sure it's a good time for you to step up and start merging virtio patches. I'm not nacking, chill :) I know you want these patches very much, but they aren't yet 100% ready anyway and I don't see why, meanwhile, I shouldn't mention the downsides for others to consider. -- MST
