Re: [Qemu-devel] Re: [PATCH] vhost: fix features ack

[Blue Swirl <blauwirbel@gmail.com>]

On 4/1/10, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote:
>  > On 03/31/2010 02:07 PM, Michael S. Tsirkin wrote:
>  >> On Wed, Mar 31, 2010 at 03:38:05PM -0300, Luiz Capitulino wrote:
>  >>
>  >>> On Wed, 31 Mar 2010 13:26:23 -0500
>  >>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>  >>>
>  >>>
>  >>>> On 03/31/2010 01:20 PM, Michael S. Tsirkin wrote:
>  >>>>
>  >>>>> From: David L Stevens<dlstevens@us.ibm.com>
>  >>>>>
>  >>>>> vhost driver in qemu didn't ack features, and this happens
>  >>>>> to work because we don't really require any features. However,
>  >>>>> it's better not to rely on this. This patch passes features to
>  >>>>> vhost as guest acks them.
>  >>>>>
>  >>>>> Signed-off-by: David L Stevens<dlstevens@us.ibm.com>
>  >>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>  >>>>> ---
>  >>>>>
>  >>>>> Anthony, here's a fixup patch to address an issue in vhost
>  >>>>> patches. Incidentially, what's the status of the vhost patchset?
>  >>>>>
>  >>>>>
>  >>>> http://repo.or.cz/w/qemu/aliguori-queue.git vhost
>  >>>>
>  >>>> Is what I'm currently testing.  With vhost disabled,  the following seg
>  >>>> faults:
>  >>>>
>  >>>> qemu-system-x86_64 -hda ~/images/linux.img -net tap -net
>  >>>> nic,model=virtio -enable-kvm
>  >>>>
>  >>>> But not when using TCG.  I'm not sure that it's your patches at fault
>  >>>> and I'm attempting to bisect now to figure that out.
>  >>>>
>  >>>   Probably this is the same segfault I'm getting right now in master,
>  >>> bisect says it's:
>  >>>
>  >>> """
>  >>> commit ad96090a01d848df67d70c5259ed8aa321fa8716
>  >>> Author: Blue Swirl<blauwirbel@gmail.com>
>  >>> Date:   Mon Mar 29 19:23:52 2010 +0000
>  >>>
>  >>>      Refactor target specific handling, compile vl.c only once
>  >>> """
>  >>>
>  >> Why are the compile once patches helpful? They seem to introduce
>  >> churn and bugs, they actively make it harder to extend qemu as you can't use
>  >> target-specific code in code that is compiled once, they might have
>  >> performance penalty - and what do we gain? Any given user is unlikely to
>  >> need to build on more than one target, distros have enough computing
>  >> power to build in parallel.
>  >>
>  >> Maybe it makes sense to revert the compile once patches, and discuss
>  >> these issues before re-commit?
>  >>
>  >
>  > Compiling objects once is certainly useful.  Long term, I think most of
>  > us want to see a single qemu executable that works for all architectures
>  > and compiling once is an important step in that direction.
>  >
>
>
> While it probably make sense to achieve this goal, it doesn't mean it
>  should be done the dirty way.
>
>  For example it is known for a lot of time that the solution for the
>  bswap in the device code is to add a bus model doing the byteswapping.
>  Removing the #ifdef by deciding "this device will only be big/little
>  endian" doesn't seem to go in the right direction.

That was not the goal of the patch, so it doesn't go in the right
direction with regards to bus model. The bus model will need a lot of
changes for the devices, but since we are not there yet, we don't know
what would be the right changes either.

My guess is that the need for byte swapping just disappears for
devices. Then the changes done now may or may not be useful. Where I
made separate byte swapping and non-swapping versions, perhaps passing
the endianness flag and the swapping version will be deleted. The
endianness flag may still be useful at board level to decide whether
the byte twisting bus will be installed or not. In PPC cases, the
bswap lines will be deleted. For the devices that I haven't touched,
there are a few #ifdef lines more to be deleted.