Re: [Qemu-devel] [PATCH v8 00/20] virtio endian-ambivalent target

[Michael S. Tsirkin]

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 <address@hidden> 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