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

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

[Michael S. Tsirkin]

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(-)

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

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

[Anthony Liguori]

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,
>    

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

[Luiz Capitulino]

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,
> >    
> 
> 
> 

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

[Michael S. Tsirkin]

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?

-- 
MST

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

[Anthony Liguori]

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

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

[Michael S. Tsirkin]

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

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

[Anthony Liguori]

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

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

[Blue Swirl]

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?

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

[Anthony Liguori]

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

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

[Nathan Froyd]

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

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

[Blue Swirl]

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.

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

[Aurelien Jarno]

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.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

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

[Anthony Liguori]

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

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

[Anthony Liguori]

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
>

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

[Blue Swirl]

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)?

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

[Anthony Liguori]

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

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

[Blue Swirl]

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.

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

[Blue Swirl]

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. ;-)

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

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On 03/31/2010 02:38 PM, Blue Swirl wrote:
> 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%.
>    

BTW, this seems to do it.  I'll commit after some testing.

Regards,

Anthony Liguori

[-- Attachment #2: 0001-Fix-enable-kvm.patch --]
[-- Type: text/plain, Size: 4387 bytes --]

From 7c1920d330fcf7ed5b477bc8e22ca4f7e5e2b343 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Wed, 31 Mar 2010 14:20:11 -0500
Subject: [PATCH] Fix -enable-kvm

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

Introduced a regression in -enable-kvm because it left CONFIG_KVM in kvm.h
while allowing compiled-once objects to use kvm.h.  CONFIG_KVM is set on a
per-target basis.

commit 53b67b3052f39b049bc7c79ae1ce132c90098c6c
Author: Blue Swirl <blauwirbel@gmail.com>
Date:   Mon Mar 29 19:23:52 2010 +0000

    Compile acpi only once

Also regressed -enable-kvm because of a thinko.

Once we make these changes, the if(0) magic of kvm_enabled() no longer works
so we need empty stub functions.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/acpi.c     |    3 +-
 kvm-generic.c |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 kvm.h         |    9 ++--
 3 files changed, 139 insertions(+), 6 deletions(-)
 create mode 100644 kvm-generic.c

diff --git a/hw/acpi.c b/hw/acpi.c
index 33c6bc8..5c01c2e 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -529,7 +529,8 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 
     register_ioport_write(ACPI_DBG_IO_ADDR, 4, 4, acpi_dbg_writel, s);
 
-    if (kvm_enabled) {
+    s->kvm_enabled = kvm_enabled;
+    if (s->kvm_enabled) {
         /* Mark SMM as already inited to prevent SMM from running.  KVM does not
          * support SMM mode. */
         pci_conf[0x5B] = 0x02;
diff --git a/kvm-generic.c b/kvm-generic.c
new file mode 100644
index 0000000..c67fb55
--- /dev/null
+++ b/kvm-generic.c
@@ -0,0 +1,133 @@
+/*
+ * QEMU KVM support
+ *
+ * Copyright IBM, Corp. 2010
+ *           Red Hat, Inc. 2008
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Glauber Costa     <gcosta@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "cpu.h"
+#include "kvm.h"
+
+int kvm_init(int smp_cpus)
+{
+    return -ENOSYS;
+}
+
+int kvm_init_vcpu(CPUState *env)
+{
+    return -ENOSYS;
+}
+
+int kvm_cpu_exec(CPUState *env)
+{
+    return -ENOSYS;
+}
+
+int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_sync_mmu(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_vcpu_events(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_has_robust_singlestep(void)
+{
+    return -ENOSYS;
+}
+
+void kvm_setup_guest_memory(void *start, size_t size)
+{
+}
+
+int kvm_coalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+int kvm_uncoalesce_mmio_region(target_phys_addr_t start, ram_addr_t size)
+{
+    return -ENOSYS;
+}
+
+void kvm_flush_coalesced_mmio_buffer(void)
+{
+}
+
+int kvm_insert_breakpoint(CPUState *current_env, target_ulong addr,
+                          target_ulong len, int type)
+{
+    return -ENOSYS;
+}
+
+int kvm_remove_breakpoint(CPUState *current_env, target_ulong addr,
+                          target_ulong len, int type)
+{
+    return -ENOSYS;
+}
+
+void kvm_remove_all_breakpoints(CPUState *current_env)
+{
+}
+
+int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
+{
+    return -ENOSYS;
+}
+
+#ifndef _WIN32
+int kvm_set_signal_mask(CPUState *env, const sigset_t *sigset)
+{
+    return -ENOSYS;
+}
+#endif
+
+int kvm_pit_in_kernel(void)
+{
+    return -ENOSYS;
+}
+
+int kvm_irqchip_in_kernel(void)
+{
+    return -ENOSYS;
+}
+
+void kvm_cpu_synchronize_state(CPUState *env)
+{
+}
+
+void kvm_cpu_synchronize_post_reset(CPUState *env)
+{
+}
+
+void kvm_cpu_synchronize_post_init(CPUState *env)
+{
+}
+
+/* This is evil but the KVM PPC code needs refactoring */
+void kvmppc_init(void);
+
+void kvmppc_init(void)
+{
+}
diff --git a/kvm.h b/kvm.h
index 4f77188..9f57fd5 100644
--- a/kvm.h
+++ b/kvm.h
@@ -19,11 +19,10 @@
 
 extern int kvm_allowed;
 
-#ifdef CONFIG_KVM
-#define kvm_enabled() (kvm_allowed)
-#else
-#define kvm_enabled() (0)
-#endif
+static inline int kvm_enabled(void)
+{
+    return kvm_allowed;
+}
 
 struct kvm_run;
 
-- 
1.6.5.2

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

[Blue Swirl]

On 3/31/10, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/31/2010 02:38 PM, Blue Swirl wrote:
>
> > 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%.
> >
> >
>
>  BTW, this seems to do it.  I'll commit after some testing.

But this makes kvm_enabled() check dynamic and code that was
eliminated by compiler for !CONFIG_KVM will now be generated.

Wouldn't adding CONFIG_KVM to config-host.h also solve the problem?

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

[Paul Brook]

> But this makes kvm_enabled() check dynamic and code that was
> eliminated by compiler for !CONFIG_KVM will now be generated.
> 
> Wouldn't adding CONFIG_KVM to config-host.h also solve the problem?

CONFIG_KVM is (and should be) a per-target decision.

Paul

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

[Michael S. Tsirkin]

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.

-- 
MST

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

[Michael S. Tsirkin]

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

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

[Aurelien Jarno]

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
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net