Patchwork qemu/virtio: move features to an inline function

login
register
mail settings
Submitter Michael S. Tsirkin
Date Aug. 10, 2009, 7:28 p.m.
Message ID <20090810192809.GA16800@redhat.com>
Download mbox | patch
Permalink /patch/31103/
State Superseded
Headers show

Comments

Michael S. Tsirkin - Aug. 10, 2009, 7:28 p.m.
devices should have the final say over which virtio features they
support. E.g. indirect entries may or may not make sense in the context
of virtio-console.  Move the common bits from virtio-pci to an inline
function and let each device call it.

No functional changes.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

This is part of my vhost work, but IMO the patch makes sense separately
as well: the common features are not related to pci at all.

 hw/virtio-balloon.c |    2 +-
 hw/virtio-blk.c     |    2 +-
 hw/virtio-console.c |    2 +-
 hw/virtio-net.c     |    2 +-
 hw/virtio-pci.c     |    3 ---
 hw/virtio.h         |   10 ++++++++++
 6 files changed, 14 insertions(+), 7 deletions(-)
Anthony Liguori - Aug. 10, 2009, 7:37 p.m.
Michael S. Tsirkin wrote:
> devices should have the final say over which virtio features they
> support. E.g. indirect entries may or may not make sense in the context
> of virtio-console.  Move the common bits from virtio-pci to an inline
> function and let each device call it.
>   

What drove this in vhost?

Normally, the common features are transport features and the devices 
should have absolutely no knowledge of transport feature (since they're 
transport dependent).

IOW, VIRTIO_RING_F_INDIRECT_DESC is meaningless to virtio-console 
because virtio-console has no idea what the ring implementation is that 
it sits on top of.

Regards,

Anthony Liguori
Michael S. Tsirkin - Aug. 10, 2009, 7:47 p.m.
On Mon, Aug 10, 2009 at 02:37:20PM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> devices should have the final say over which virtio features they
>> support. E.g. indirect entries may or may not make sense in the context
>> of virtio-console.  Move the common bits from virtio-pci to an inline
>> function and let each device call it.
>>   
>
> What drove this in vhost?

vhost does not support indirect ring entries and notify on empty yet.

> Normally, the common features are transport features and the devices  
> should have absolutely no knowledge of transport feature (since they're  
> transport dependent).

Good point. But

1. note that with my patch they don't. They call
virtio_get_common_features and that's all.

2. some features may not make sense for some devices. For example, it is
quite possible that indirect ring entries feature improves performance
on block but hurts on net, as net has a similar merged buffers feature.
Someone should try benchmarking with it disabled, and it becomes
possible with my patch.

> IOW, VIRTIO_RING_F_INDIRECT_DESC is meaningless to virtio-console  
> because virtio-console has no idea what the ring implementation is that  
> it sits on top of.

At some level.  Same can be said to be true for virtio-pci: it sits
below the ring implementation. However, it is not *completely*
meaningless: some devices may benefit from indirect entries, others may
not, or may benefit from disabling them.

> Regards,
>
> Anthony Liguori
>
Anthony Liguori - Aug. 10, 2009, 8:33 p.m.
Michael S. Tsirkin wrote:
>> Normally, the common features are transport features and the devices  
>> should have absolutely no knowledge of transport feature (since they're  
>> transport dependent).
>>     
>
> Good point. But
>
> 1. note that with my patch they don't. They call
> virtio_get_common_features and that's all.
>
> 2. some features may not make sense for some devices. For example, it is
> quite possible that indirect ring entries feature improves performance
> on block but hurts on net, as net has a similar merged buffers feature.
> Someone should try benchmarking with it disabled, and it becomes
> possible with my patch.
>   

I don't necessarily disagree but I think your patch goes about it the 
wrong way.

There ought to be a way to layer qdev properties that achieves this goal 
so that when you create a virtio-pci-block device, you have the ability 
to turn off indirect sg without virtio-block having to know what that is.

For your use-case, I wonder if you're integrating at the wrong level.  
If you implement a ring in-kernel, maybe the thing to do is introduce 
more layering in qemu like we have in the kernel so that you can easily 
add a new ring backend type.  At any rate, see if you can achieve the 
same goal with qdev properties.  If you could, you should be able to 
hack something up easily to disable this for vhost without completely 
overhauling qemu's virtio implementation.

Regards,

Anthony Liguori
Michael S. Tsirkin - Aug. 10, 2009, 10:19 p.m.
On Mon, Aug 10, 2009 at 03:33:59PM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>>> Normally, the common features are transport features and the devices  
>>> should have absolutely no knowledge of transport feature (since 
>>> they're  transport dependent).
>>>     
>>
>> Good point. But
>>
>> 1. note that with my patch they don't. They call
>> virtio_get_common_features and that's all.
>>
>> 2. some features may not make sense for some devices. For example, it is
>> quite possible that indirect ring entries feature improves performance
>> on block but hurts on net, as net has a similar merged buffers feature.
>> Someone should try benchmarking with it disabled, and it becomes
>> possible with my patch.
>>   
>
> I don't necessarily disagree but I think your patch goes about it the  
> wrong way.
>
> There ought to be a way to layer qdev properties that achieves this goal  
> so that when you create a virtio-pci-block device, you have the ability  
> to turn off indirect sg without virtio-block having to know what that is.

I don't understand, sorry. Why do you insist on involving pci here?
ring layout has nothing to do with pci, does it?  With my patch,
virtio-block does not know what indirect sg is. It just does
enable/disable.

virtio net has device-specific feature that overlaps with
indirect entries. So insisting that devices should just ignore
transport does not make sense, to me.

> For your use-case, I wonder if you're integrating at the wrong level.   

Forget about that for now. Let's solve the generic problem.

> Regards,
>
> Anthony Liguori
Anthony Liguori - Aug. 10, 2009, 10:35 p.m.
Michael S. Tsirkin wrote:
> On Mon, Aug 10, 2009 at 03:33:59PM -0500, Anthony Liguori wrote:
>   
>> There ought to be a way to layer qdev properties that achieves this goal  
>> so that when you create a virtio-pci-block device, you have the ability  
>> to turn off indirect sg without virtio-block having to know what that is.
>>     
>
> I don't understand, sorry. Why do you insist on involving pci here?
> ring layout has nothing to do with pci, does it?

What I'm saying is that virtio-blk-pci, which is the qdev instantiation 
of virtio-pci + virtio-blk, should be able to have a set of qdev 
properties that is composed of a combination of at least two sets of 
properties: virtio-blk's qdev properties and virtio-pci's qdev properties.

Right now, all of the properties are defined in virtio-pci.c, so you 
could add a property that was DEFINE_PROP_BOOL("indirect-sg", ...), that 
you could then use to selectively enable/disable indirect sg on 
virtio-blk-pci devices without ever having to involve virtio-blk.c.

Ideally, we wouldn't have the properties centralized in virtio-pci.c.  
Rather, it would be nice if virtio-blk.c could have a set of properties 
and virtio-pci.c could just add those properties to it's own set of 
properties.

Today, we don't have a concept of a ring abstraction.  If we did, then 
virtio-ring.c could have it's own set of properties.

N.B. I expect that the in-kernel virtio-net device is going to be 
separate qdev device than virtio-net-pci.  It can have an identical 
guest interface but within qemu, it should be a separate device.  This 
is how we handle the in-kernel PIT and it's how we should handle the 
in-kernel APIC.

Regards,

Anthony Liguori
Michael S. Tsirkin - Aug. 11, 2009, 8:48 a.m.
On Mon, Aug 10, 2009 at 05:35:13PM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On Mon, Aug 10, 2009 at 03:33:59PM -0500, Anthony Liguori wrote:
>>   
>>> There ought to be a way to layer qdev properties that achieves this 
>>> goal  so that when you create a virtio-pci-block device, you have the 
>>> ability  to turn off indirect sg without virtio-block having to know 
>>> what that is.
>>>     
>>
>> I don't understand, sorry. Why do you insist on involving pci here?
>> ring layout has nothing to do with pci, does it?
>
> What I'm saying is that virtio-blk-pci, which is the qdev instantiation  
> of virtio-pci + virtio-blk, should be able to have a set of qdev  
> properties that is composed of a combination of at least two sets of  
> properties: virtio-blk's qdev properties and virtio-pci's qdev 
> properties.

Yea. But indirect ring entries is not virtio-pci property.  And even
with virtio-pci properies, such as MSI, specific device should
have control over number of vectors.

> Right now, all of the properties are defined in virtio-pci.c, so you  
> could add a property that was DEFINE_PROP_BOOL("indirect-sg", ...), that  
> you could then use to selectively enable/disable indirect sg on  
> virtio-blk-pci devices without ever having to involve virtio-blk.c.

Me as a user? We can't expect the user to tweak such low level stuff for
each device.  So devices such as block should have a say in which ring
format options are used, in a way optimal for the specific usage.  My
example is that virtio net has merged buffers so indirect ring is
probably just useless.  And again, pci seems to have nothing to do with
it, so why drag it in?

> Ideally, we wouldn't have the properties centralized in virtio-pci.c.   
> Rather, it would be nice if virtio-blk.c could have a set of properties  
> and virtio-pci.c could just add those properties to it's own set of  
> properties.

That's what my patch did. think of feature bits as properties.  The set
of properties can not currently be controlled by user, but this can be
added if there is need.


> Today, we don't have a concept of a ring abstraction.  If we did, then  
> virtio-ring.c could have it's own set of properties.

Yes but devices might and do care about which of these properties
get used.

> N.B. I expect that the in-kernel virtio-net device is going to be  
> separate qdev device than virtio-net-pci.  It can have an identical  
> guest interface but within qemu, it should be a separate device.  This  
> is how we handle the in-kernel PIT and it's how we should handle the  
> in-kernel APIC.

Ugh. What advantages does this have?  This would break things like
migrating between userspace and kernel virtio (something that I
support).  IMO, this should work like MSI, detect capability and just
have virtio go faster, with a disable flag for troubleshooting purposes.

Can migration between in-kernel and userspace PIT work?
If yes we would be better off changing that, as well.

Separate devices should be for things that have guest-visible
differences. Don't try to encode random information into the device
name.

> Regards,
>
> Anthony Liguori
Anthony Liguori - Aug. 11, 2009, 1:15 p.m.
Michael S. Tsirkin wrote:
> On Mon, Aug 10, 2009 at 05:35:13PM -0500, Anthony Liguori wrote:
>   
>> What I'm saying is that virtio-blk-pci, which is the qdev instantiation  
>> of virtio-pci + virtio-blk, should be able to have a set of qdev  
>> properties that is composed of a combination of at least two sets of  
>> properties: virtio-blk's qdev properties and virtio-pci's qdev 
>> properties.
>>     
>
> Yea. But indirect ring entries is not virtio-pci property.

It's a ring feature and the ring implementation is currently in the 
generic virtio code.  Ring features really have no home today so 
virtio-pci seems logical.

>   And ev
> with virtio-pci properies, such as MSI, specific device should
> have control over number of vectors.
>   

Devices, or instantiation of the devices?  The later is what I'm suggesting.

Let's say we supported virtio-vbus along with virtio-pci.  What does 
virtio_blk_get_features() do to mask out sg_indirect?  For all 
virtio-blk knows, it could be on top of virtio-vbus.

> Me as a user? We can't expect the user to tweak such low level stuff for
> each device.  So devices such as block should have a say in which ring
> format options are used, in a way optimal for the specific usage.  My
> example is that virtio net has merged buffers so indirect ring is
> probably just useless.  And again, pci seems to have nothing to do with
> it, so why drag it in?
>   

If you want to tweak such thing, why not use default property values for 
virtio-blk-pci's definition in virtio-pci.c?  That keeps it out of 
virtio-blk.c.

>> separate qdev device than virtio-net-pci.  It can have an identical  
>> guest interface but within qemu, it should be a separate device.  This  
>> is how we handle the in-kernel PIT and it's how we should handle the  
>> in-kernel APIC.
>>     
>
> Ugh. What advantages does this have?

It keeps a clean separate of the two devices.  It actually ends up 
making things a lot easier to understand because it's clear what 
portions of code are not being used for the in-kernel device models.

>   This would break things like
> migrating between userspace and kernel virtio (something that I
> support).

The PIT uses a common state structure and common code for save/restore.  
This makes migration compatible.

>   IMO, this should work like MSI, detect capability and just
> have virtio go faster, with a disable flag for troubleshooting purposes.
>
> Can migration between in-kernel and userspace PIT work?
> If yes we would be better off changing that, as well.
>   

Take a look at i8524{,-kvm.c} in qemu-kvm and how it's instantiated in 
pc.c.  It ends up being really straight forward.

> Separate devices should be for things that have guest-visible
> differences. Don't try to encode random information into the device
> name.
>   

In this case, it's two separate implementations of the same device.  I 
think it makes sense for them to be separate devices.

Regards,

Anthony Liguori
Michael S. Tsirkin - Aug. 11, 2009, 1:43 p.m.
On Tue, Aug 11, 2009 at 08:15:23AM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On Mon, Aug 10, 2009 at 05:35:13PM -0500, Anthony Liguori wrote:
>>   
>>> What I'm saying is that virtio-blk-pci, which is the qdev 
>>> instantiation  of virtio-pci + virtio-blk, should be able to have a 
>>> set of qdev  properties that is composed of a combination of at least 
>>> two sets of  properties: virtio-blk's qdev properties and 
>>> virtio-pci's qdev properties.
>>>     
>>
>> Yea. But indirect ring entries is not virtio-pci property.
>
> It's a ring feature and the ring implementation is currently in the  
> generic virtio code.  Ring features really have no home today so  
> virtio-pci seems logical.

Why is this logical? Let's put the feature in generic virtio code,
where ring itself it. That's what my patch does.

>>   And ev
>> with virtio-pci properies, such as MSI, specific device should
>> have control over number of vectors.
>>   
>
> Devices, or instantiation of the devices?  The later is what I'm suggesting.

The former would be my guess. Hard to see why different instances
of blk would behave much differently.

> Let's say we supported virtio-vbus along with virtio-pci.  What does  
> virtio_blk_get_features() do to mask out sg_indirect?  For all  
> virtio-blk knows, it could be on top of virtio-vbus.

So? VIRTIO_RING_F_INDIRECT_DESC applies to all transports.
Just clear this bit.

>> Me as a user? We can't expect the user to tweak such low level stuff for
>> each device.  So devices such as block should have a say in which ring
>> format options are used, in a way optimal for the specific usage.  My
>> example is that virtio net has merged buffers so indirect ring is
>> probably just useless.  And again, pci seems to have nothing to do with
>> it, so why drag it in?
>>   
>
> If you want to tweak such thing, why not use default property values for  
> virtio-blk-pci's definition in virtio-pci.c?  That keeps it out of  
> virtio-blk.c.

Me as a user? I don't want to know what it is, much less tweak it.
Me as a developer? Using different ring formats depending on
transport is possible, but creates extra testing/benchmarking burden,
reduces coverage.

>>> separate qdev device than virtio-net-pci.  It can have an identical   
>>> guest interface but within qemu, it should be a separate device.  
>>> This  is how we handle the in-kernel PIT and it's how we should 
>>> handle the  in-kernel APIC.
>>>     
>>
>> Ugh. What advantages does this have?
>
> It keeps a clean separate of the two devices.  It actually ends up  
> making things a lot easier to understand because it's clear what  
> portions of code are not being used for the in-kernel device models.

Ah, I see what you mean now.  That separation is a wish I can't grant,
sorry.  See below.

>>   This would break things like
>> migrating between userspace and kernel virtio (something that I
>> support).
>
> The PIT uses a common state structure and common code for save/restore.   
> This makes migration compatible.

Isn't device name put in the machine config, which presumably is
send along as well?

>>   IMO, this should work like MSI, detect capability and just
>> have virtio go faster, with a disable flag for troubleshooting purposes.
>>
>> Can migration between in-kernel and userspace PIT work?
>> If yes we would be better off changing that, as well.
>>   
>
> Take a look at i8524{,-kvm.c} in qemu-kvm and how it's instantiated in  
> pc.c.  It ends up being really straight forward.
>
>> Separate devices should be for things that have guest-visible
>> differences. Don't try to encode random information into the device
>> name.
>>   
>
> In this case, it's two separate implementations of the same device.  I  
> think it makes sense for them to be separate devices.
>
> Regards,
>
> Anthony Liguori

Hmm, I see what you mean. But kernel virtio is harder. Unlike
PIT/APIC, it is not a separate codepath.  It still needs
all of userspace virtio to support live migration and non-MSI guests.
Really, it's the same device that switches between kernel and userspace
modes on the fly.

This will become clearer from code when I implement migration for vhost,
but basically you switch to userspace when you start migration, and
back to kernel if migration fails. You also switch to kernel when MSI
is enabled and back to userspace when it is disabled.
Anthony Liguori - Aug. 11, 2009, 4:08 p.m.
Michael S. Tsirkin wrote:
>> Let's say we supported virtio-vbus along with virtio-pci.  What does  
>> virtio_blk_get_features() do to mask out sg_indirect?  For all  
>> virtio-blk knows, it could be on top of virtio-vbus.
>>     
>
> So? VIRTIO_RING_F_INDIRECT_DESC applies to all transports.
> Just clear this bit.
>   

You can have many layers with virtio.  device + transport + ring

virtio-vbus would have a different transport and a different ring 
implementation.  So no, VIRTIO_RING_F_INDIRECT_DESC wouldn't apply to 
virtio-vbus.

>>>   This would break things like
>>> migrating between userspace and kernel virtio (something that I
>>> support).
>>>       
>> The PIT uses a common state structure and common code for save/restore.   
>> This makes migration compatible.
>>     
>
> Isn't device name put in the machine config, which presumably is
> send along as well?
>   

Good question.  I don't know the best way to resolve this.

Maybe migration between devices isn't such a good idea.  It's 
conceivable that vhost will require some state that isn't present in the 
userspace virtio-net.  I think this requires some thought.

>> In this case, it's two separate implementations of the same device.  I  
>> think it makes sense for them to be separate devices.
>>
>> Regards,
>>
>> Anthony Liguori
>>     
>
> Hmm, I see what you mean. But kernel virtio is harder. Unlike
> PIT/APIC, it is not a separate codepath.  It still needs
> all of userspace virtio to support live migration and non-MSI guests.
> Really, it's the same device that switches between kernel and userspace
> modes on the fly.
>
> This will become clearer from code when I implement migration for vhost,
> but basically you switch to userspace when you start migration, and
> back to kernel if migration fails. You also switch to kernel when MSI
> is enabled and back to userspace when it is disabled.
>   

Why bother switching to userspace for migration?  Can't you just have 
get/set ioctls for the state?

Regards,

Anthony Liguori
Michael S. Tsirkin - Aug. 11, 2009, 4:25 p.m.
On Tue, Aug 11, 2009 at 11:08:32AM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>>> Let's say we supported virtio-vbus along with virtio-pci.  What does  
>>> virtio_blk_get_features() do to mask out sg_indirect?  For all   
>>> virtio-blk knows, it could be on top of virtio-vbus.
>>>     
>>
>> So? VIRTIO_RING_F_INDIRECT_DESC applies to all transports.
>> Just clear this bit.
>>   
>
> You can have many layers with virtio.  device + transport + ring
>
> virtio-vbus would have a different transport and a different ring  
> implementation.  So no, VIRTIO_RING_F_INDIRECT_DESC wouldn't apply to  
> virtio-vbus.
>
>>>>   This would break things like
>>>> migrating between userspace and kernel virtio (something that I
>>>> support).
>>>>       
>>> The PIT uses a common state structure and common code for 
>>> save/restore.   This makes migration compatible.
>>>     
>>
>> Isn't device name put in the machine config, which presumably is
>> send along as well?
>>   
>
> Good question.  I don't know the best way to resolve this.
>
> Maybe migration between devices isn't such a good idea.  It's  
> conceivable that vhost will require some state that isn't present in the  
> userspace virtio-net.

It can't. It switches to userspace before migration.

>  I think this requires some thought.
>
>>> In this case, it's two separate implementations of the same device.  
>>> I  think it makes sense for them to be separate devices.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>     
>>
>> Hmm, I see what you mean. But kernel virtio is harder. Unlike
>> PIT/APIC, it is not a separate codepath.  It still needs
>> all of userspace virtio to support live migration and non-MSI guests.
>> Really, it's the same device that switches between kernel and userspace
>> modes on the fly.
>>
>> This will become clearer from code when I implement migration for vhost,
>> but basically you switch to userspace when you start migration, and
>> back to kernel if migration fails. You also switch to kernel when MSI
>> is enabled and back to userspace when it is disabled.
>>   
>
> Why bother switching to userspace for migration?  Can't you just have  
> get/set ioctls for the state?

I have these. But live migration requires dirty page logging.
I do not want to implement it in kernel.

> Regards,
>
> Anthony Liguori
>
Anthony Liguori - Aug. 12, 2009, 7:18 p.m.
Michael S. Tsirkin wrote:
> On
>   
>> Why bother switching to userspace for migration?  Can't you just have  
>> get/set ioctls for the state?
>>     
>
> I have these. But live migration requires dirty page logging.
> I do not want to implement it in kernel.
>   

Is it really that difficult?  I think it would be better to just do that.

I wonder though if mmu notifiers can be used to make it transparent...

Regards,

Anthony Liguori

>> Regards,
>>
>> Anthony Liguori
>>
>>
Michael S. Tsirkin - Aug. 13, 2009, 6:15 a.m.
On Wed, Aug 12, 2009 at 02:18:20PM -0500, Anthony Liguori wrote:
> Michael S. Tsirkin wrote:
>> On
>>   
>>> Why bother switching to userspace for migration?  Can't you just have 
>>>  get/set ioctls for the state?
>>>     
>>
>> I have these. But live migration requires dirty page logging.
>> I do not want to implement it in kernel.
>>   
>
> Is it really that difficult?  I think it would be better to just do that.

The idea is if it can be in userspace, let's keep it there.

> I wonder though if mmu notifiers can be used to make it transparent...

Maybe they can, but that decision belongs to KVM.
Avi, what do you think?

> Regards,
>
> Anthony Liguori
>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>>>
Avi Kivity - Aug. 13, 2009, 9:28 a.m.
On 08/13/2009 09:15 AM, Michael S. Tsirkin wrote:
>
>> I wonder though if mmu notifiers can be used to make it transparent...
>>      
>
> Maybe they can, but that decision belongs to KVM.
> Avi, what do you think?
>
>    

I don't see how mmu notifiers help.  You can use mmu notifiers to sync 
an external mmu to the linux pagetables, but that's not the case here.

I see the following options:

- mprotect() guest memory, trap faults, mprotect() back

Will work, but exceedingly slowly and wastefully

- add a generic user visible dirty bit tracking based on linux ptes

A lot of work, not sure if useful beyond kvm

- implement vhost dirty bit tracking

Not too difficult; not sure if it's worth the effort though

- reuse the kvm dirty bitmap

Not too difficult but introduces tight coupling between vhost and kvm 
which might slow down development

- drop to userspace for live migration

Reuse qemu code, lose some performance
Michael S. Tsirkin - Aug. 13, 2009, 9:51 a.m.
On Thu, Aug 13, 2009 at 12:28:52PM +0300, Avi Kivity wrote:
> On 08/13/2009 09:15 AM, Michael S. Tsirkin wrote:
>>
>>> I wonder though if mmu notifiers can be used to make it transparent...
>>>      
>>
>> Maybe they can, but that decision belongs to KVM.
>> Avi, what do you think?
>>
>>    
>
> I don't see how mmu notifiers help.  You can use mmu notifiers to sync  
> an external mmu to the linux pagetables, but that's not the case here.
>
> I see the following options:
>
> - mprotect() guest memory, trap faults, mprotect() back
>
> Will work, but exceedingly slowly and wastefully

I think this is what Anthony had in mind.

> - add a generic user visible dirty bit tracking based on linux ptes
>
> A lot of work, not sure if useful beyond kvm
>
> - implement vhost dirty bit tracking
>
> Not too difficult; not sure if it's worth the effort though
>
> - reuse the kvm dirty bitmap
>
> Not too difficult but introduces tight coupling between vhost and kvm  
> which might slow down development
>
> - drop to userspace for live migration
>
> Reuse qemu code, lose some performance

This is what I planned.  Note that ability to drop to userspace is
required for non-MSI mode, anyway.

> -- 
> error compiling committee.c: too many arguments to function

Patch

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 7ca783e..15b50bb 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -127,7 +127,7 @@  static void virtio_balloon_set_config(VirtIODevice *vdev,
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev)
 {
-    return 0;
+    return virtio_common_features();
 }
 
 static ram_addr_t virtio_balloon_to_target(void *opaque, ram_addr_t target)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 2beff52..7d33fee 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -364,7 +364,7 @@  static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
     if (strcmp(s->serial_str, "0"))
         features |= 1 << VIRTIO_BLK_F_IDENTIFY;
 
-    return features;
+    return features | virtio_common_features();
 }
 
 static void virtio_blk_save(QEMUFile *f, void *opaque)
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 663c8b9..ac25499 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -53,7 +53,7 @@  static void virtio_console_handle_input(VirtIODevice *vdev, VirtQueue *vq)
 
 static uint32_t virtio_console_get_features(VirtIODevice *vdev)
 {
-    return 0;
+    return virtio_common_features();
 }
 
 static int vcon_can_read(void *opaque)
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ef9e7ff..2e51a6a 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -154,7 +154,7 @@  static uint32_t virtio_net_get_features(VirtIODevice *vdev)
     }
 #endif
 
-    return features;
+    return features | virtio_common_features();
 }
 
 static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3b9bfd1..90f51be 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -232,9 +232,6 @@  static uint32_t virtio_ioport_read(VirtIOPCIProxy *proxy, uint32_t addr)
     switch (addr) {
     case VIRTIO_PCI_HOST_FEATURES:
         ret = vdev->get_features(vdev);
-        ret |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
-        ret |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
-        ret |= (1 << VIRTIO_F_BAD_FEATURE);
         break;
     case VIRTIO_PCI_GUEST_FEATURES:
         ret = vdev->features;
diff --git a/hw/virtio.h b/hw/virtio.h
index aa55677..de620a7 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -166,4 +166,14 @@  VirtIODevice *virtio_net_init(DeviceState *dev);
 VirtIODevice *virtio_console_init(DeviceState *dev);
 VirtIODevice *virtio_balloon_init(DeviceState *dev);
 
+static inline uint32_t virtio_common_features(void)
+{
+    uint32_t features = 0;
+    features |= (1 << VIRTIO_F_NOTIFY_ON_EMPTY);
+    features |= (1 << VIRTIO_RING_F_INDIRECT_DESC);
+    features |= (1 << VIRTIO_F_BAD_FEATURE);
+
+    return features;
+}
+
 #endif