[PATCHv4 0/6] qemu-kvm: vhost net support

[PATCHv4 0/6] qemu-kvm: vhost net support

[Michael S. Tsirkin]

This adds support for vhost-net virtio kernel backend.

This is not intented for merge.  See vhost net patch description for
details.  This applies on top of commit
47e465f031fc43c53ea8f08fa55cc3482c6435c8 in Avi's tree.  It won't apply
to tree tip. TODO: rebase.

The patchset also includes raw socket backend since I find it useful for
testing vhost. When we get to merging, there's no reason to merge these
features together, and dependency between patches is minimal.

Changes since v3:
- changed option syntax
- filled in feature negotiation
Changes since v2:
- minor fixes
- added patch to build on RHEL5.3
Changes since v1:
- rebased on top of 9dc275d9d660fe1cd64d36102d600885f9fdb88a

Michael S. Tsirkin (5):
  qemu/virtio: move features to an inline function
  qemu/net: routines to get tap fd
  qemu/net: move typedef to qemu-common.h
  qemu/raw: add API to get raw socket
  qemu-kvm: vhost-net implementation

Or Gerlitz (1):
  qemu/net: add raw backend

 Makefile.target           |    3 +-
 hw/vhost_net.c            |  251 +++++++++++++++++++++++++++++++++++++++++++++
 hw/vhost_net.h            |   38 +++++++
 hw/virtio-balloon.c       |    2 +-
 hw/virtio-blk.c           |    2 +-
 hw/virtio-console.c       |    2 +-
 hw/virtio-net.c           |   68 ++++++++++--
 hw/virtio-pci.c           |   43 +++++++-
 hw/virtio.c               |   19 ----
 hw/virtio.h               |   38 ++++++-
 kvm/include/linux/vhost.h |  126 +++++++++++++++++++++++
 net.c                     |  217 +++++++++++++++++++++++++++++++++++++++
 net.h                     |    6 +-
 qemu-common.h             |    1 +
 qemu-kvm.c                |    8 --
 qemu-kvm.h                |    9 ++
 qemu-options.hx           |    4 +
 17 files changed, 787 insertions(+), 50 deletions(-)
 create mode 100644 hw/vhost_net.c
 create mode 100644 hw/vhost_net.h
 create mode 100644 kvm/include/linux/vhost.h

Re: [PATCHv4 0/6] qemu-kvm: vhost net support

[Anthony Liguori]

Hi Michael,

I'll reserve individual patch review until they're in a mergable state, 
but I do have some comments about the overall integration architecture.

Generally speaking, I think the integration unnecessarily invasive.  It 
adds things to the virtio infrastructure that shouldn't be there like 
the irqfd/queuefd bindings.  It also sneaks in things like raw backend 
support which really isn't needed.

I think we can do better.  Here's what I suggest:

The long term goal should be to have a NetDevice interface that looks 
very much like virtio-net but as an API, not an ABI.  Roughly, it would 
look something like:

struct NetDevice {
   int add_xmit(NetDevice *dev, struct iovec *iov, int iovcnt, void *token);
   int add recv(NetDevice *dev, struct iovec *iov, int iovcnt, void *token);

   void *get_xmit(NetDevice *dev);
   void *get_recv(NetDevice *dev);

   void kick(NetDevice *dev);

   ...
};

That gives us a better API for use with virtio-net, e1000, etc.

Assuming we had this interface, I think a natural extension would be:

int add_ring(NetDevice *dev, void *address);
int add_kickfd(NetDevice *dev, int fd);

For slot management, it really should happen outside of the NetDevice 
structure.  We'll need a slot notifier mechanism such that we can keep 
this up to date as things change.

vhost-net because a NetDevice.  It can support things like the e1000 by 
doing ring translation behind the scenes. virtio-net can be fast pathed 
in the case that we're using KVM but otherwise, it would also rely on 
the ring translation.  N.B. in the case vhost-net is fast pathed, it 
requires a different device in QEMU that uses a separate virtio 
transport.  We should reuse as much code as possible obviously.  It 
doesn't make sense to have all of the virtio-pci code and virtio-net 
code in place when we aren't using it.

All this said, I'm *not* suggesting you have to implement all of this to 
get vhost-net merged.  Rather, I'm suggesting that we should try to 
structure the current vhost-net implementation to complement this 
architecture assuming we all agree this is the sane thing to do.  That 
means I would make the following changes to your series:

 - move vhost-net support to a VLANClientState backend.
 - do not introduce a raw socket backend
 - if for some reason you want to back to tap and raw, those should be 
options to the vhost-net backend.
 - when fast pathing with vhost-net, we should introduce interfaces to 
VLANClientState similar to add_ring and add_kickfd.  They'll be very 
specific to vhost-net for now, but that's okay.
 - sort out the layering of vhost-net within the virtio infrastructure.  
vhost-net should really be it's own qdev device.  I don't see very much 
code reuse happening right now so I don't understand why it's not that 
way currently.

Regards,

Anthony Liguori

Re: [PATCHv4 0/6] qemu-kvm: vhost net support

[Michael S. Tsirkin]

On Mon, Nov 02, 2009 at 04:58:39PM -0600, Anthony Liguori wrote:
> Hi Michael,
>
> I'll reserve individual patch review until they're in a mergable state,  
> but I do have some comments about the overall integration architecture.
>
> Generally speaking, I think the integration unnecessarily invasive.  It  
> adds things to the virtio infrastructure that shouldn't be there like  
> the irqfd/queuefd bindings.  It also sneaks in things like raw backend  
> support which really isn't needed.
>
> I think we can do better.  Here's what I suggest:
>
> The long term goal should be to have a NetDevice interface that looks  
> very much like virtio-net but as an API, not an ABI.  Roughly, it would  
> look something like:
>
> struct NetDevice {
>   int add_xmit(NetDevice *dev, struct iovec *iov, int iovcnt, void *token);
>   int add recv(NetDevice *dev, struct iovec *iov, int iovcnt, void *token);
>
>   void *get_xmit(NetDevice *dev);
>   void *get_recv(NetDevice *dev);
>
>   void kick(NetDevice *dev);
>
>   ...
> };
>
> That gives us a better API for use with virtio-net, e1000, etc.

This is not much different from what we have now with VLANClientState,
is it?

> Assuming we had this interface, I think a natural extension would be:
>
> int add_ring(NetDevice *dev, void *address);
> int add_kickfd(NetDevice *dev, int fd);
>
> For slot management, it really should happen outside of the NetDevice  
> structure.  We'll need a slot notifier mechanism such that we can keep  
> this up to date as things change.

Yes.

> vhost-net because a NetDevice.  It can support things like the e1000 by  
> doing ring translation behind the scenes.

And the point would be?

> virtio-net can be fast pathed  
> in the case that we're using KVM but otherwise, it would also rely on  
> the ring translation.

Won't it be easier to just keep using existing code?

>  N.B. in the case vhost-net is fast pathed, it  requires a different
>  device in QEMU that uses a separate virtio  transport.  We should
>  reuse as much code as possible obviously.  It  doesn't make sense to
>  have all of the virtio-pci code and virtio-net  code in place when we
>  aren't using it.

Note that all of virtio-pci and setup parts of virtio-net are reused.
The only things we are *not* re-using are send/receive and callbacks in
virtio-net.

> All this said, I'm *not* suggesting you have to implement all of this to  
> get vhost-net merged.  Rather, I'm suggesting that we should try to  
> structure the current vhost-net implementation to complement this  
> architecture assuming we all agree this is the sane thing to do.  That  
> means I would make the following changes to your series:
>
> - move vhost-net support to a VLANClientState backend.
> - do not introduce a raw socket backend
> - if for some reason you want to back to tap and raw, those should be  
> options to the vhost-net backend.
> - when fast pathing with vhost-net, we should introduce interfaces to  
> VLANClientState similar to add_ring and add_kickfd.  They'll be very  
> specific to vhost-net for now, but that's okay.
> - sort out the layering of vhost-net within the virtio infrastructure.   
> vhost-net should really be it's own qdev device.
>  I don't see very much  
> code reuse happening right now so I don't understand why it's not that  
> way currently.
> Regards,
>
> Anthony Liguori

What you propose short-term is workable.  So basically, vhost would be
an option supported by backends.  virtio net would go ahead and activate
it if available and other frontends will ignore it and just keep
injecting packets through regular interfaces.

-- 
MST