Patchwork vhost: fix features ack

login
register
mail settings
Submitter Michael S. Tsirkin
Date March 31, 2010, 6:20 p.m.
Message ID <20100331182031.GA5200@redhat.com>
Download mbox | patch
Permalink /patch/49170/
State New
Headers show

Comments

Michael S. Tsirkin - March 31, 2010, 6:20 p.m.
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?


 hw/virtio-net.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
Anthony Liguori - March 31, 2010, 6:26 p.m.
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.

Regards,

Anthony Liguori

>   hw/virtio-net.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 9ddd58c..4c7c46e 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>                           (features>>  VIRTIO_NET_F_GUEST_ECN)&  1,
>                           (features>>  VIRTIO_NET_F_GUEST_UFO)&  1);
>       }
> +    if (!n->nic->nc.peer ||
> +        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> +        return;
> +    }
> +    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> +        return;
> +    }
> +    return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features);
>   }
>
>   static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>
Luiz Capitulino - March 31, 2010, 6:38 p.m.
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
"""

> 
> Regards,
> 
> Anthony Liguori
> 
> >   hw/virtio-net.c |    8 ++++++++
> >   1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 9ddd58c..4c7c46e 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
> >                           (features>>  VIRTIO_NET_F_GUEST_ECN)&  1,
> >                           (features>>  VIRTIO_NET_F_GUEST_UFO)&  1);
> >       }
> > +    if (!n->nic->nc.peer ||
> > +        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> > +        return;
> > +    }
> > +    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> > +        return;
> > +    }
> > +    return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features);
> >   }
> >
> >   static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> >    
> 
> 
>
Michael S. Tsirkin - March 31, 2010, 7:07 p.m.
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?
Anthony Liguori - March 31, 2010, 7:24 p.m.
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.

With respect to regressions, it might make sense to slow down these 
refactorings a bit and increase the amount of regression testing that is 
happening during them.

Regards,

Anthony Liguori
Michael S. Tsirkin - March 31, 2010, 7:25 p.m.
On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote:
> 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.

I'm not so sure. It's pretty low on my list of priorities. Most users only need
one target, speed of execution and/or features is likely much more important for them,
and these refactorings make code more generic and harder to extend.s
Anthony Liguori - March 31, 2010, 7:37 p.m.
On 03/31/2010 02:25 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote:
>    
>> 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.
>>      
> I'm not so sure. It's pretty low on my list of priorities. Most users only need
> one target, speed of execution and/or features is likely much more important for them,
> and these refactorings make code more generic and harder to extend.s
>    

We ought to have a set of device models that are compiled once, with 
well defined interfaces that model the actual way the various buses 
communicate.  This should all roll into a generic CPU API.  Then we 
should have a set of CPU implementations with choices including various 
TCG targets and KVM targets.

You can still compile out TCG targets that you don't care about but the 
key point is to get all of these interfaces correct.

This refactoring effort isn't really paying attention to improving 
interfaces which I think is a bit problematic.

Regards,

Anthony Liguori
Blue Swirl - March 31, 2010, 7:38 p.m.
On 3/31/10, Michael S. Tsirkin <mst@redhat.com> 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.

As has been explained many times, knowledge about CPU specific
features has no place in devices. Actively removing CPU specifics from
devices is good but preventing new bad code to be committed is better.

I don't have distro computing powers (unless some distro's compute
center only has two low power machines) and I guess some other
developers don't have either. All developers and patch submitters are
expected to compile all targets. This patch set has decreased the
number of files compiled by about 20%.

>  Maybe it makes sense to revert the compile once patches, and discuss
>  these issues before re-commit?

Maybe I'll try to remember this as your favourite problem solving
approach if I happen to dislike the changes your patches may cause in
the future. ;-)
Blue Swirl - March 31, 2010, 7:46 p.m.
On 3/31/10, Anthony Liguori <anthony@codemonkey.ws> 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.
>
>  With respect to regressions, it might make sense to slow down these
> refactorings a bit and increase the amount of regression testing that is
> happening during them.

I think there are only a few useful refactorings left. MIPS was
interesting because of fourfold savings, likewise triple savings with
PPC. Refactoring i386/x86_64 devices may be worthwhile, the rest not.
Blue Swirl - March 31, 2010, 7:54 p.m.
On 3/31/10, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/31/2010 02:25 PM, Michael S. Tsirkin wrote:
>
> > On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote:
> >
> >
> > > 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.
> > >
> > >
> > I'm not so sure. It's pretty low on my list of priorities. Most users only
> need
> > one target, speed of execution and/or features is likely much more
> important for them,
> > and these refactorings make code more generic and harder to extend.s
> >
> >
>
>  We ought to have a set of device models that are compiled once, with well
> defined interfaces that model the actual way the various buses communicate.
> This should all roll into a generic CPU API.  Then we should have a set of
> CPU implementations with choices including various TCG targets and KVM
> targets.
>
>  You can still compile out TCG targets that you don't care about but the key
> point is to get all of these interfaces correct.
>
>  This refactoring effort isn't really paying attention to improving
> interfaces which I think is a bit problematic.

I agree, but with improved memory API, the questionable byte swapping
could be eliminated from devices. Do you plan to finish your earlier
RFC patches? Were there problems with them?
Anthony Liguori - March 31, 2010, 7:55 p.m.
On 03/31/2010 02:54 PM, Blue Swirl wrote:
>
>>   This refactoring effort isn't really paying attention to improving
>> interfaces which I think is a bit problematic.
>>      
> I agree, but with improved memory API, the questionable byte swapping
> could be eliminated from devices. Do you plan to finish your earlier
> RFC patches? Were there problems with them?
>    

It's a matter of figuring out a way to do it that makes everyone happy :-)

Regards,

Anthony Liguori
Nathan Froyd - March 31, 2010, 8:06 p.m.
On Wed, Mar 31, 2010 at 10:25:53PM +0300, Michael S. Tsirkin wrote:
> On Wed, Mar 31, 2010 at 02:24:51PM -0500, Anthony Liguori wrote:
> > 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.
> 
> I'm not so sure. It's pretty low on my list of priorities. Most users only need
> one target, speed of execution and/or features is likely much more important for them,
> and these refactorings make code more generic and harder to extend.s

Depends on the user, I suppose.  Having 32-bit and 64-bit variants of
the same architecture in a single binary would be useful.  Having
big-endian and little-endian variants of an architecture supported in
a single binary would be useful (bonus points if you don't duplicate
target-specific code excessively).  Having a gigantic all-singing,
all-dancing QEMU is perhaps not so useful, but between there and where
we are now, there are definitely useful points.

-Nathan
Aurelien Jarno - March 31, 2010, 10:45 p.m.
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.
Anthony Liguori - April 1, 2010, 2:45 a.m.
On 03/31/2010 05:45 PM, Aurelien Jarno wrote:
> 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.
>    

Yeah, I'm having real trouble with the KVM regression.  I thought I had 
it fixed but linux-user really made a mess of things.  There's no simple 
solution that doesn't require quite a bit of refactoring which I'd 
rather do in a less ugly way.  We've already been discussing getting rid 
of all the kvm_enabled() stuff and I think doing that properly is going 
to be needed to handle this correctly.

I'm thinking we should back out the vl.c changes and try to clean up the 
KVM bits first.  Does that sound reasonable blueswirl or can you think 
of a cleaner way to deal with kvm?

Regards,

Anthony Liguori
Anthony Liguori - April 1, 2010, 2:46 a.m.
On 03/31/2010 09:45 PM, Anthony Liguori wrote:
> On 03/31/2010 05:45 PM, Aurelien Jarno wrote:
>> 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.
>
> Yeah, I'm having real trouble with the KVM regression.  I thought I 
> had it fixed but linux-user really made a mess of things.  There's no 
> simple solution that doesn't require quite a bit of refactoring which 
> I'd rather do in a less ugly way.  We've already been discussing 
> getting rid of all the kvm_enabled() stuff and I think doing that 
> properly is going to be needed to handle this correctly.
>
> I'm thinking we should back out the vl.c changes and try to clean up 
> the KVM bits first.  Does that sound reasonable blueswirl or can you 
> think of a cleaner way to deal with kvm?

And back out can just mean making vl.c compile per-target (along with 
the thinko fix in acpi.c).  That would be a nice temporary solution 
until we worked out the kvm_enabled() bits correctly.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori
>
Michael S. Tsirkin - April 1, 2010, 2:53 p.m.
On Wed, Mar 31, 2010 at 10:38:47PM +0300, Blue Swirl wrote:
> >  Maybe it makes sense to revert the compile once patches, and discuss
> >  these issues before re-commit?
> 
> Maybe I'll try to remember this as your favourite problem solving
> approach if I happen to dislike the changes your patches may cause in
> the future. ;-)

My patches are usually posted on list for a week or two
before commit to the main tree. So if there's anything to dislike,
you just need to post your objections, no reason to go through
commit/revert churn.
Blue Swirl - April 1, 2010, 3:54 p.m.
On 4/1/10, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/31/2010 05:45 PM, Aurelien Jarno wrote:
>
> > 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.
> >
> >
>
>  Yeah, I'm having real trouble with the KVM regression.  I thought I had it
> fixed but linux-user really made a mess of things.  There's no simple
> solution that doesn't require quite a bit of refactoring which I'd rather do
> in a less ugly way.  We've already been discussing getting rid of all the
> kvm_enabled() stuff and I think doing that properly is going to be needed to
> handle this correctly.

Strange, does linux-user use kvm.h (indirectly)?
Anthony Liguori - April 1, 2010, 4:08 p.m.
On 04/01/2010 10:54 AM, Blue Swirl wrote:
> On 4/1/10, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>    
>> On 03/31/2010 05:45 PM, Aurelien Jarno wrote:
>>
>>      
>>> 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.
>>>
>>>
>>>        
>>   Yeah, I'm having real trouble with the KVM regression.  I thought I had it
>> fixed but linux-user really made a mess of things.  There's no simple
>> solution that doesn't require quite a bit of refactoring which I'd rather do
>> in a less ugly way.  We've already been discussing getting rid of all the
>> kvm_enabled() stuff and I think doing that properly is going to be needed to
>> handle this correctly.
>>      
> Strange, does linux-user use kvm.h (indirectly)?
>    

Yes, because various things in target-i386 use kvm.h.

Regards,

Anthony Liguori
Blue Swirl - April 1, 2010, 4:08 p.m.
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.
Michael S. Tsirkin - April 4, 2010, 12:14 p.m.
On Wed, Mar 31, 2010 at 09:20:31PM +0300, 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?
> 
> 
>  hw/virtio-net.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)

Just making sure this patch does not get lost in the noise:
now that vhost net support is merged, please apply this
patch on top. Thanks!

> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 9ddd58c..4c7c46e 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>                          (features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
>                          (features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
>      }
> +    if (!n->nic->nc.peer ||
> +        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> +        return;
> +    }
> +    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> +        return;
> +    }
> +    return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features);
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -- 
> 1.7.0.2.280.gc6f05
Aurelien Jarno - April 13, 2010, 9:58 p.m.
On Wed, Mar 31, 2010 at 09:20:31PM +0300, 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?
> 

Thanks, applied.

>  hw/virtio-net.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 9ddd58c..4c7c46e 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -218,6 +218,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>                          (features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
>                          (features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
>      }
> +    if (!n->nic->nc.peer ||
> +        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
> +        return;
> +    }
> +    if (!tap_get_vhost_net(n->nic->nc.peer)) {
> +        return;
> +    }
> +    return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features);
>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> -- 
> 1.7.0.2.280.gc6f05
> 
> 
>

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 9ddd58c..4c7c46e 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -218,6 +218,14 @@  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
                         (features >> VIRTIO_NET_F_GUEST_ECN)  & 1,
                         (features >> VIRTIO_NET_F_GUEST_UFO)  & 1);
     }
+    if (!n->nic->nc.peer ||
+        n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    return vhost_net_ack_features(tap_get_vhost_net(n->nic->nc.peer), features);
 }
 
 static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,