virtio: export virtqueue structure

vhost needs physical addresses for
ring so expose that structure.

Signed-off-by Michael S. Tsirkin<[email protected]>

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  kvm-all.c |   24 ++++++++++++++++++++++++
  kvm.h     |    3 +++
  2 files changed, 27 insertions(+), 0 deletions(-)

kvm-all.c +8 -0

index a312654..aa00119 100644
@@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env)
  {
  }
  #endif /* !KVM_CAP_SET_GUEST_DEBUG */
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = data,
+        .addr = addr,
+        .len = 2,

Add binding API to set iofd/irqfd support.
Will be used by vhost.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  hw/virtio.c |   13 ++++++++++---
  hw/virtio.h |    4 ++++
  2 files changed, 14 insertions(+), 3 deletions(-)

hw/virtio.c +10 -3

index 8e3c9ad..b9ec863 100644
@@ -572,6 +572,12 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
      return&vdev->vq[i];
  }
 
+void virtio_irq(VirtIODevice *vdev, VirtQueue *vq)
+{
+    vdev->isr |= 0x01;
+    virtio_notify_vector(vdev, vq->vector);
+}
+
  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
  {
      /* Make sure used ring is updated before we check NO_INTERRUPT. */

@@ -582,8 +588,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
           (vq->inuse || vring_avail_idx(vq) != vq->last_avail_idx)))
          return;
 
-    vdev->isr |= 0x01;
-    virtio_notify_vector(vdev, vq->vector);
+    virtio_irq(vdev, vq);
  }
 
  void virtio_notify_config(VirtIODevice *vdev)

@@ -696,8 +701,10 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
      vdev->queue_sel = 0;
      vdev->config_vector = VIRTIO_NO_VECTOR;
      vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
-    for(i = 0; i<  VIRTIO_PCI_QUEUE_MAX; i++)
+    for(i = 0; i<  VIRTIO_PCI_QUEUE_MAX; i++) {
          vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+        vdev->vq[i].vdev = vdev;
+    }
 
      vdev->name = name;
      vdev->config_len = config_size;

hw/virtio.h +0 -0

index ca840e1..193b3f9 100644

Looks like order got mixed up: vhost_net header
is added by a follow-up patch. Will be fixed
in the next revision.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  net.c           |    8 ++++++++
  net/tap.c       |   29 +++++++++++++++++++++++++++++
  qemu-options.hx |    4 +++-
  3 files changed, 40 insertions(+), 1 deletions(-)

net.c +0 -0

index 6ef93e6..b942d03 100644
@@ -976,6 +976,14 @@ static struct {
                  .name = "vnet_hdr",
                  .type = QEMU_OPT_BOOL,
                  .help = "enable the IFF_VNET_HDR flag on the tap interface"

Looks like order got mixed up: vhost_net header
is added by a follow-up patch. Will be fixed
in the next revision.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  net.c           |    8 ++++++++
  net/tap.c       |   29 +++++++++++++++++++++++++++++
  qemu-options.hx |    4 +++-
  3 files changed, 40 insertions(+), 1 deletions(-)

net.c +8 -0

index 6ef93e6..b942d03 100644
@@ -976,6 +976,14 @@ static struct {
                  .name = "vnet_hdr",
                  .type = QEMU_OPT_BOOL,
                  .help = "enable the IFF_VNET_HDR flag on the tap interface"
+            }, {
+                .name = "vhost",
+                .type = QEMU_OPT_BOOL,
+                .help = "enable vhost-net network accelerator",
+            }, {
+                .name = "vhostfd",
+                .type = QEMU_OPT_STRING,
+                .help = "file descriptor of an already opened vhost net device",
              },
  #endif /* _WIN32 */
              { /* end of list */ }

net/tap.c +11 -0

index 7e9ca79..d9f2e41 100644
@@ -41,6 +41,8 @@
 
  #include "net/tap-linux.h"
 
+#include "hw/vhost_net.h"
+
  /* Maximum GSO packet size (64k) plus plenty of room for
   * the ethernet and virtio_net headers
   */

@@ -57,6 +59,7 @@ typedef struct TAPState {
      unsigned int has_vnet_hdr : 1;
      unsigned int using_vnet_hdr : 1;
      unsigned int has_ufo: 1;
+    struct vhost_net *vhost_net;
  } TAPState;
 
  static int launch_script(const char *setup_script, const char *ifname, int fd);

@@ -252,6 +255,10 @@ static void tap_cleanup(VLANClientState *nc)
  {
      TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
+    if (s->vhost_net) {
+        vhost_net_cleanup(s->vhost_net);
+    }
+
      qemu_purge_queued_packets(nc);
 
      if (s->down_script[0])

@@ -307,6 +314,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
      s->has_ufo = tap_probe_has_ufo(s->fd);
      tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
      tap_read_poll(s, 1);
+    s->vhost_net = NULL;
      return s;
  }
 

@@ -456,6 +464,27 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
          }
      }
 
+    if (qemu_opt_get_bool(opts, "vhost", 0)) {
+        int vhostfd, r;
+        if (qemu_opt_get(opts, "vhostfd")) {

This adds vhost net support in qemu. Will be tied to tap device and virtio later. Raw backend is currently missing, will be worked on/submitted separately.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  Makefile.target |    1 +
  hw/vhost.c      |  349 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  hw/vhost.h      |   33 +++++
  hw/vhost_net.c  |  145 +++++++++++++++++++++++
  hw/vhost_net.h  |   20 +++
  5 files changed, 548 insertions(+), 0 deletions(-)

create mode 100644 hw/vhost.c
create mode 100644 hw/vhost.h
create mode 100644 hw/vhost_net.c
create mode 100644 hw/vhost_net.h

Makefile.target +1 -0

index 7c1f30c..61b7148 100644
@@ -157,6 +157,7 @@ obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o gdbstub.o
  # virtio has to be here due to weird dependency between PCI and virtio-net.
  # need to fix this properly
  obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
+obj-y += vhost_net.o vhost.o
  obj-$(CONFIG_KVM) += kvm.o kvm-all.o
  obj-$(CONFIG_ISA_MMIO) += isa_mmio.o
  LIBS+=-lz

hw/vhost.c new file +128 -0

new file mode 100644
index 0000000..d23d94c
@@ -0,0 +1,349 @@
+#include "linux/vhost.h"
+#include<sys/ioctl.h>
+#include<sys/eventfd.h>
+#include "vhost.h"
+#include "hw/hw.h"
+/* For range_get_last */
+#include "pci.h"
+
+static void vhost_dev_unassign_memory(struct vhost_dev *dev,
+				      struct vhost_memory *mem,
+				      uint64_t start_addr,
+				      uint64_t size)
+{
+	int from, to;
+	for (from = 0, to = 0; from<  dev->mem->nregions; ++from, ++to) {
+		struct vhost_memory_region *reg = mem->regions + to;
+		uint64_t reglast;
+		uint64_t memlast;
+		uint64_t change;
+
+		/* clone old region */
+		memcpy(reg, dev->mem->regions + from, sizeof *reg);
+
+		/* No overlap is simple */
+		if (!ranges_overlap(reg->guest_phys_addr, reg->memory_size,
+				    start_addr, size)) {
+			continue;
+		}
+		reglast = range_get_last(reg->guest_phys_addr, reg->memory_size);
+		memlast = range_get_last(start_addr, size);
+
+		/* Remove whole region */
+		if (start_addr<= reg->guest_phys_addr&&  memlast>= reglast) {
+			--to;
+			continue;
+		}
+
+		/* Shrink region */
+		if (memlast>= reglast) {
+			reg->memory_size = start_addr - reg->guest_phys_addr;
+			continue;
+		}
+
+		/* Shift region */
+		if (start_addr<= reg->guest_phys_addr) {
+			change = memlast + 1 - reg->guest_phys_addr;
+			reg->memory_size -= change;
+			reg->guest_phys_addr += change;
+			reg->userspace_addr += change;
+			continue;
+		}
+
+		/* Split region: shrink first part, shift second part. */
+		memcpy(reg + 1, reg, sizeof *reg);
+		reg[0].memory_size = start_addr - reg->guest_phys_addr;
+		change = memlast + 1 - reg->guest_phys_addr;
+		reg[1].memory_size -= change;
+		reg[1].guest_phys_addr += change;
+		reg[1].userspace_addr += change;
+		++to;
+	}
+	mem->nregions = to;
+}
+
+/* Called after unassign, so no regions overlap the given range. */
+static void vhost_dev_assign_memory(struct vhost_dev *dev,
+				    struct vhost_memory *mem,
+				    uint64_t start_addr,
+				    uint64_t size,
+				    uint64_t uaddr)
+{
+	int from, to;
+	struct vhost_memory_region *merged = NULL;
+	for (from = 0, to = 0; from<  dev->mem->nregions; ++from, ++to) {
+		struct vhost_memory_region *reg = mem->regions + to;
+		uint64_t prlast, urlast;
+		uint64_t pmlast, umlast;
+		uint64_t s, e, u;
+
+		/* clone old region */
+		memcpy(reg, dev->mem->regions + from, sizeof *reg);
+		prlast = range_get_last(reg->guest_phys_addr, reg->memory_size);
+		pmlast = range_get_last(start_addr, size);
+		urlast = range_get_last(reg->userspace_addr, reg->memory_size);
+		umlast = range_get_last(uaddr, size);
+
+		/* Not an adjecent region - do not merge. */
+		if ((prlast + 1 != start_addr || urlast + 1 != uaddr)&&
+		    (pmlast + 1 != reg->guest_phys_addr ||
+		     umlast + 1 != reg->userspace_addr)) {
+			continue;
+		}
+
+		if (!merged) {
+			--to;
+		} else {
+			merged = reg;
+		}
+		u = MIN(uaddr, reg->userspace_addr);
+		s = MIN(start_addr, reg->guest_phys_addr);
+		e = MAX(pmlast, prlast);
+		uaddr = merged->userspace_addr = u;
+		start_addr = merged->guest_phys_addr = s;
+		size = merged->memory_size = e - s + 1;
+	}
+
+	if (!merged) {
+		struct vhost_memory_region *reg = mem->regions + to;
+		reg->memory_size = size;
+		reg->guest_phys_addr = start_addr;
+		reg->userspace_addr = uaddr;
+		++to;
+	}
+	mem->nregions = to;
+}
+
+static void vhost_client_set_memory(CPUPhysMemoryClient *client,
+				    target_phys_addr_t start_addr,
+				    ram_addr_t size,
+				    ram_addr_t phys_offset)
+{
+	struct vhost_dev *dev = container_of(client, struct vhost_dev, client);
+	ram_addr_t flags = phys_offset&  ~TARGET_PAGE_MASK;
+	int s = offsetof(struct vhost_memory, regions) +
+		(dev->mem->nregions + 1)* sizeof dev->mem->regions[0];
+	struct vhost_memory *mem = qemu_malloc(s);
+	memcpy(mem, dev->mem, s);
+

new file mode 100644
index 0000000..9f82b42

hw/vhost.h new file +4 -0

@@ -0,0 +1,33 @@
+#ifndef VHOST_H
+#define VHOST_H
+
+#include "hw/hw.h"

Quoted 53 lines · Michael S. Tsirkin

Add binding API to set iofd/irqfd support.
Will be used by vhost.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  hw/virtio.c |   13 ++++++++++---
  hw/virtio.h |    4 ++++
  2 files changed, 14 insertions(+), 3 deletions(-)

hw/virtio.c +10 -3

index 8e3c9ad..b9ec863 100644
@@ -572,6 +572,12 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
      return&vdev->vq[i];
  }
 
+void virtio_irq(VirtIODevice *vdev, VirtQueue *vq)
+{
+    vdev->isr |= 0x01;
+    virtio_notify_vector(vdev, vq->vector);
+}
+
  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
  {
      /* Make sure used ring is updated before we check NO_INTERRUPT. */

@@ -582,8 +588,7 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
           (vq->inuse || vring_avail_idx(vq) != vq->last_avail_idx)))
          return;
 
-    vdev->isr |= 0x01;
-    virtio_notify_vector(vdev, vq->vector);
+    virtio_irq(vdev, vq);
  }
 
  void virtio_notify_config(VirtIODevice *vdev)

@@ -696,8 +701,10 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id,
      vdev->queue_sel = 0;
      vdev->config_vector = VIRTIO_NO_VECTOR;
      vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX);
-    for(i = 0; i<  VIRTIO_PCI_QUEUE_MAX; i++)
+    for(i = 0; i<  VIRTIO_PCI_QUEUE_MAX; i++) {
          vdev->vq[i].vector = VIRTIO_NO_VECTOR;
+        vdev->vq[i].vdev = vdev;
+    }
 
      vdev->name = name;
      vdev->config_len = config_size;

hw/virtio.h +0 -0

index ca840e1..193b3f9 100644

hw/virtio.h typedef struct { · line 93

Quoted 6 lines

hw/virtio.h +2 -0

@@ -88,6 +88,8 @@ typedef struct {
      int (*load_config)(void * opaque, QEMUFile *f);
      int (*load_queue)(void * opaque, int n, QEMUFile *f);
      unsigned (*get_features)(void * opaque);
+    int (*set_irqfd)(void * opaque, int n, int fd, bool assigned);
+    int (*set_queuefd)(void * opaque, int n, int fd, bool assigned);
 

I'd like to see us abstract things like irqfd a bit more. It should be a first class object with an method to raise/lower. In fact, you can probably just use qemu_irq.

Regards,

Anthony Liguori

Quoted 24 lines · Michael S. Tsirkin

Looks like order got mixed up: vhost_net header
is added by a follow-up patch. Will be fixed
in the next revision.

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  net.c           |    8 ++++++++
  net/tap.c       |   29 +++++++++++++++++++++++++++++
  qemu-options.hx |    4 +++-
  3 files changed, 40 insertions(+), 1 deletions(-)

net.c +5 -0

index 6ef93e6..b942d03 100644
@@ -976,6 +976,14 @@ static struct {
                  .name = "vnet_hdr",
                  .type = QEMU_OPT_BOOL,
                  .help = "enable the IFF_VNET_HDR flag on the tap interface"
+            }, {
+                .name = "vhost",
+                .type = QEMU_OPT_BOOL,
+                .help = "enable vhost-net network accelerator",
+            }, {

net.c static struct {

Quoted 5 lines

net.c +3 -0

+                .name = "vhostfd",
+                .type = QEMU_OPT_STRING,
+                .help = "file descriptor of an already opened vhost net device",
              },
 

Semantically, I think making vhost it's own backend makes more sense from a user perspective.

I don't think it's a significant code change.

Regards,

Anthony Liguori

Quoted 3 lines · Michael S. Tsirkin

vhost needs physical addresses for
ring so expose that structure.

Quoted 2 lines

Signed-off-by Michael S. Tsirkin<[email protected]>

I think accessor functions might make more sense.

Regards,

Anthony Liguori

Quoted 23 lines · Michael S. Tsirkin

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  kvm-all.c |   24 ++++++++++++++++++++++++
  kvm.h     |    3 +++
  2 files changed, 27 insertions(+), 0 deletions(-)

kvm-all.c +8 -0

index a312654..aa00119 100644
@@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env)
  {
  }
  #endif /* !KVM_CAP_SET_GUEST_DEBUG */
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = data,
+        .addr = addr,
+        .len = 2,

kvm-all.c void kvm_remove_all_breakpoints(CPUState *current_env) · line 1133

Quoted 10 lines

kvm-all.c +10 -0

+        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+        .fd = fd,
+    };
+    if (!assigned)
+        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick);
+    if (r<  0)
+        return r;
+    return 0;
+}

I think we really ought to try to avoid having global state used here. That means either we need to pass a CPUState to this function or we need to have a ioeventfd be allocated as a structure that is then passed around that can store a copy of the kvm_state fetched through CPUState.

I think the later is best. We really don't want ioeventfd scattered throughout the code.

Regards,

Anthony Liguori

Quoted 5 lines · Anthony Liguori

Quoted 3 lines · Michael S. Tsirkin

vhost needs physical addresses for
ring so expose that structure.

Quoted 9 lines

Quoted 3 lines

Signed-off-by Michael S. Tsirkin<[email protected]>

I think accessor functions might make more sense.

Regards,

Anthony Liguori

Well, take a look:

typedef struct VRing

{
     unsigned int num;
     target_phys_addr_t desc;
     target_phys_addr_t avail;
     target_phys_addr_t used;
} VRing;

struct VirtQueue

{
     VRing vring;
     target_phys_addr_t pa;
     uint16_t last_avail_idx;
     int inuse;
     uint16_t vector;
     void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq);
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;
}

(Notifiers are added by patches I will shortly post). We need at least:

     unsigned int num;
     target_phys_addr_t desc;
     target_phys_addr_t avail;
     target_phys_addr_t used;
     VRing vring;
     target_phys_addr_t pa;

     uint16_t vector;
     VirtIODevice *vdev;
     EventNotifier guest_notifier;
     EventNotifier host_notifier;

Quoted 44 lines · Anthony Liguori

Quoted 24 lines · Michael S. Tsirkin

Signed-off-by Michael S. Tsirkin<[email protected]>

---
   kvm-all.c |   24 ++++++++++++++++++++++++
   kvm.h     |    3 +++
   2 files changed, 27 insertions(+), 0 deletions(-)

kvm-all.c +8 -0

index a312654..aa00119 100644
@@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env)
   {
   }
   #endif /* !KVM_CAP_SET_GUEST_DEBUG */
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = data,
+        .addr = addr,
+        .len = 2,

kvm-all.c void kvm_remove_all_breakpoints(CPUState *current_env) · line 1133

Quoted 10 lines

kvm-all.c +10 -0

+        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+        .fd = fd,
+    };
+    if (!assigned)
+        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick);
+    if (r<   0)
+        return r;
+    return 0;
+}

I think we really ought to try to avoid having global state used here. That means either we need to pass a CPUState to this function or we need to have a ioeventfd be allocated as a structure that is then passed around that can store a copy of the kvm_state fetched through CPUState.

I think the later is best. We really don't want ioeventfd scattered throughout the code.

Quoted 4 lines

Regards,

Anthony Liguori

I don't really understand this comment. I have no idea where to get CPUState in a device and how to get kvm state from there.

And all other kvm-all functions use kvm_state already, let's fix them first if we want to get rid of global state?

Avi, what's your take on this patch?

Quoted 46 lines · Michael S. Tsirkin

Quoted 44 lines · Anthony Liguori

Quoted 24 lines · Michael S. Tsirkin

Signed-off-by Michael S. Tsirkin<[email protected]>

---
   kvm-all.c |   24 ++++++++++++++++++++++++
   kvm.h     |    3 +++
   2 files changed, 27 insertions(+), 0 deletions(-)

kvm-all.c +8 -0

index a312654..aa00119 100644
@@ -1113,3 +1113,27 @@ void kvm_remove_all_breakpoints(CPUState *current_env)
   {
   }
   #endif /* !KVM_CAP_SET_GUEST_DEBUG */
+
+#ifdef KVM_IOEVENTFD
+int kvm_set_ioeventfd(uint16_t addr, uint16_t data, int fd, bool assigned)
+{
+    struct kvm_ioeventfd kick = {
+        .datamatch = data,
+        .addr = addr,
+        .len = 2,

kvm-all.c void kvm_remove_all_breakpoints(CPUState *current_env) · line 1133

Quoted 10 lines

kvm-all.c +10 -0

+        .flags = KVM_IOEVENTFD_FLAG_DATAMATCH | KVM_IOEVENTFD_FLAG_PIO,
+        .fd = fd,
+    };
+    if (!assigned)
+        kick.flags |= KVM_IOEVENTFD_FLAG_DEASSIGN;
+    int r = kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD,&kick);
+    if (r<   0)
+        return r;
+    return 0;
+}

I think we really ought to try to avoid having global state used here. That means either we need to pass a CPUState to this function or we need to have a ioeventfd be allocated as a structure that is then passed around that can store a copy of the kvm_state fetched through CPUState.

I think the later is best. We really don't want ioeventfd scattered throughout the code.

Quoted 9 lines

Quoted 4 lines

Regards,

Anthony Liguori

I don't really understand this comment. I have no idea where to get CPUState in a device and how to get kvm state from there.

This function doesn't seem to get called in the current patches so it's hard to evaluate what the right thing to do is.

Quoted 4 lines

And all other kvm-all functions use kvm_state already, let's fix them first if we want to get rid of global state?

Any time an operation is tied to a CPU in some way, we should not rely on global state. Based on the name and the fact that it references PIO, it strongly suggests to me that it's somehow related to a CPU but since it's not used in the current code it's hard to validate that.

Regards,

Anthony Liguori

Quoted 4 lines

Avi, what's your take on this patch?

It is called in the binding. Pls take a look. The function does not reference a specific CPU: it binds an eventfd descriptor to a specific address/data pair for all CPUs.

start/stop backend on driver start/stop

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
  1 files changed, 40 insertions(+), 0 deletions(-)

hw/virtio-net.c +27 -0

index c2a389f..99169e1 100644
@@ -17,6 +17,7 @@
  #include "net/tap.h"
  #include "qemu-timer.h"
  #include "virtio-net.h"
+#include "vhost_net.h"
 
  #define VIRTIO_NET_VM_VERSION    11
 

@@ -47,6 +48,7 @@ typedef struct VirtIONet
      uint8_t nomulti;
      uint8_t nouni;
      uint8_t nobcast;
+    uint8_t vhost_started;
      struct {
          int in_use;
          int first_multi;

@@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
      n->nomulti = 0;
      n->nouni = 0;
      n->nobcast = 0;
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
 
      /* Flush any MAC and VLAN filter table state */
      n->mac_table.in_use = 0;

@@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
      .link_status_changed = virtio_net_set_link_status,
  };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    if (!!n->vhost_started == !!(vdev->status&  VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+    if (vdev->status&  VIRTIO_CONFIG_S_DRIVER_OK) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        if (r<  0) {
+            fprintf(stderr, "unable to start vhost net: "
+                    "falling back on userspace virtio\n");

hw/virtio-net.c +0 -0

  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
  {

      VirtIONet *n;

hw/virtio-net.c +4 -0

@@ -835,6 +871,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
      n->vdev.set_features = virtio_net_set_features;
      n->vdev.bad_features = virtio_net_bad_features;
      n->vdev.reset = virtio_net_reset;
+    n->vdev.set_status = virtio_net_set_status;
      n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
      n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
      n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);

@@ -864,6 +901,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
  void virtio_net_exit(VirtIODevice *vdev)
  {
      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+    }
 
      qemu_purge_queued_packets(&n->nic->nc);
 

Quoted 63 lines · Michael S. Tsirkin

start/stop backend on driver start/stop

Signed-off-by Michael S. Tsirkin<[email protected]>

---
  hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
  1 files changed, 40 insertions(+), 0 deletions(-)

hw/virtio-net.c +27 -0

index c2a389f..99169e1 100644
@@ -17,6 +17,7 @@
  #include "net/tap.h"
  #include "qemu-timer.h"
  #include "virtio-net.h"
+#include "vhost_net.h"
 
  #define VIRTIO_NET_VM_VERSION    11
 

@@ -47,6 +48,7 @@ typedef struct VirtIONet
      uint8_t nomulti;
      uint8_t nouni;
      uint8_t nobcast;
+    uint8_t vhost_started;
      struct {
          int in_use;
          int first_multi;

@@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
      n->nomulti = 0;
      n->nouni = 0;
      n->nobcast = 0;
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
 
      /* Flush any MAC and VLAN filter table state */
      n->mac_table.in_use = 0;

@@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
      .link_status_changed = virtio_net_set_link_status,
  };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    if (!!n->vhost_started == !!(vdev->status&  VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+    if (vdev->status&  VIRTIO_CONFIG_S_DRIVER_OK) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        if (r<  0) {
+            fprintf(stderr, "unable to start vhost net: "
+                    "falling back on userspace virtio\n");

hw/virtio-net.c static NetClientInfo net_virtio_info = { · line 859

Quoted 10 lines

hw/virtio-net.c +9 -0

+        } else {
+            n->vhost_started = 1;
+        }
+    } else {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
+}
+
 

This function does a number of bad things. It makes virtio-net have specific knowledge of backends (like tap) and then has virtio-net pass device specific state (vdev) to a network backend.

Ultimately, the following things need to happen:

1) when a driver is ready to begin operating:
a) virtio-net needs to tell vhost the location of the ring in physical
memory
b) virtio-net needs to tell vhost about any notification it receives
(allowing kvm to shortcut userspace)
c) virtio-net needs to tell vhost about which irq is being used
(allowing kvm to shortcut userspace)

What this suggests is that we need an API for the network backends to do the following:

1) probe whether ring passthrough is supported
2) set the *virtual* address of the ring elements
3) hand the backend some sort of notification object for sending and
receiving notifications
4) stop ring passthrough

vhost should not need any direct knowledge of the device. This interface should be enough to communicating the required data. I think the only bit that is a little fuzzy and perhaps non-obvious for the current patches is the notification object. I would expect it look something like:

typedef struct IOEvent {
int type;
void (*notify)(IOEvent *);
void (*on_notify)(IOEvent *, void (*cb)(IOEvent *, void *));
} IOEvent;
And then we would have

typedef struct KVMIOEvent {
IOEvent event = {.type = KVM_IO_EVENT};
int fd;
} KVMIOEvent;

if (kvm_enabled()) in virtio-net, we would allocate a KVMIOEvent for the PIO notification and hand that to vhost. vhost would check event.type and if it's KVM_IOEVENT, downcast and use that to get at the file descriptor.

The KVMIOEvent should be created, not in the set status callback, but in the PCI map callback. And in the PCI map callback, cpu_single_env should be passed to a kvm specific function to create this KVMIOEvent object that contains the created eventfd() that's handed to kvm via ioctl.

It doesn't have to be exactly like this, but the point of all of this is that these KVM specific mechanism (which are really implementation details) should not be pervasive throughout the QEMU interfaces.

There
should also be strong separation between the vhost-net code and the virtio-net device.

Regards,

Anthony Liguori

Quoted 23 lines

hw/virtio-net.c +0 -0

  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
  {

      VirtIONet *n;

hw/virtio-net.c +4 -0

@@ -835,6 +871,7 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
      n->vdev.set_features = virtio_net_set_features;
      n->vdev.bad_features = virtio_net_bad_features;
      n->vdev.reset = virtio_net_reset;
+    n->vdev.set_status = virtio_net_set_status;
      n->rx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_rx);
      n->tx_vq = virtio_add_queue(&n->vdev, 256, virtio_net_handle_tx);
      n->ctrl_vq = virtio_add_queue(&n->vdev, 64, virtio_net_handle_ctrl);

@@ -864,6 +901,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf)
  void virtio_net_exit(VirtIODevice *vdev)
  {
      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+    }
 
      qemu_purge_queued_packets(&n->nic->nc);
 

Quoted 90 lines · Anthony Liguori

Quoted 64 lines · Michael S. Tsirkin

start/stop backend on driver start/stop

Signed-off-by Michael S. Tsirkin<[email protected]>

---
   hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
   1 files changed, 40 insertions(+), 0 deletions(-)

hw/virtio-net.c +28 -0

index c2a389f..99169e1 100644
@@ -17,6 +17,7 @@
   #include "net/tap.h"
   #include "qemu-timer.h"
   #include "virtio-net.h"
+#include "vhost_net.h"
 
   #define VIRTIO_NET_VM_VERSION    11
 

@@ -47,6 +48,7 @@ typedef struct VirtIONet
       uint8_t nomulti;
       uint8_t nouni;
       uint8_t nobcast;
+    uint8_t vhost_started;
       struct {
           int in_use;
           int first_multi;

@@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
       n->nomulti = 0;
       n->nouni = 0;
       n->nobcast = 0;
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
 
       /* Flush any MAC and VLAN filter table state */
       n->mac_table.in_use = 0;

@@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
       .link_status_changed = virtio_net_set_link_status,
   };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    if (!!n->vhost_started == !!(vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+    if (vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        if (r<   0) {
+            fprintf(stderr, "unable to start vhost net: "
+                    "falling back on userspace virtio\n");
+        } else {

hw/virtio-net.c static NetClientInfo net_virtio_info = { · line 860

Quoted 10 lines

hw/virtio-net.c +8 -0

+            n->vhost_started = 1;
+        }
+    } else {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
+}
+
 
 

This function does a number of bad things. It makes virtio-net have specific knowledge of backends (like tap) and then has virtio-net pass device specific state (vdev) to a network backend.

Ultimately, the following things need to happen:

1) when a driver is ready to begin operating:
a) virtio-net needs to tell vhost the location of the ring in physical
memory
b) virtio-net needs to tell vhost about any notification it receives
(allowing kvm to shortcut userspace)
c) virtio-net needs to tell vhost about which irq is being used
(allowing kvm to shortcut userspace)

Quoted 9 lines

What this suggests is that we need an API for the network backends to do the following:

1) probe whether ring passthrough is supported
2) set the *virtual* address of the ring elements
3) hand the backend some sort of notification object for sending and
receiving notifications
4) stop ring passthrough

Look at how vnet_hdr is setup: frontend probes backend, and has 'if (backend has vnet header) blabla' vhost is really very similar, so I would like to do it in the same way.

Generally I do not believe in abstractions that only have one implementation behind it: you only know how to abstract interface after you have more than one implementation. So whoever writes another frontend that can use vhost will be in a better position to add infrastructure to abstract both that new thing and virtio.

Quoted 18 lines

vhost should not need any direct knowledge of the device. This interface should be enough to communicating the required data. I think the only bit that is a little fuzzy and perhaps non-obvious for the current patches is the notification object. I would expect it look something like:

typedef struct IOEvent {
   int type;
   void (*notify)(IOEvent *);
   void (*on_notify)(IOEvent *, void (*cb)(IOEvent *, void *));
} IOEvent;

And then we would have

typedef struct KVMIOEvent {
   IOEvent event = {.type = KVM_IO_EVENT};
   int fd;
} KVMIOEvent;

Quoted 5 lines

if (kvm_enabled()) in virtio-net, we would allocate a KVMIOEvent for the PIO notification and hand that to vhost. vhost would check event.type and if it's KVM_IOEVENT, downcast and use that to get at the file descriptor.

Since we don't have any other IOEvents, I just put the fd in the generic structure directly. If other types surface we'll see how to generalize it.

Quoted 6 lines

The KVMIOEvent should be created, not in the set status callback, but in the PCI map callback. And in the PCI map callback, cpu_single_env should be passed to a kvm specific function to create this KVMIOEvent object that contains the created eventfd() that's handed to kvm via ioctl.

So this specific thing does not work very well because with irqchip, we want to bypass notification and send irq directly from kernel. So I created a structure but it does not have callbacks, only the fd.

Quoted 6 lines

There
should also be strong separation between the vhost-net code and the virtio-net device.

Regards,

Quoted 3 lines

Anthony Liguori

I don't see the point of this last idea. vhost is virtio accelerator, not a generic network backend. Whoever wants to use vhost for non-virtio frontends will have to add infrastructure for this separation, but I do not believe it's practical or desirable.

Quoted 101 lines · Michael S. Tsirkin

Quoted 90 lines · Anthony Liguori

Quoted 64 lines · Michael S. Tsirkin

start/stop backend on driver start/stop

Signed-off-by Michael S. Tsirkin<[email protected]>

---
   hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
   1 files changed, 40 insertions(+), 0 deletions(-)

hw/virtio-net.c +28 -0

index c2a389f..99169e1 100644
@@ -17,6 +17,7 @@
   #include "net/tap.h"
   #include "qemu-timer.h"
   #include "virtio-net.h"
+#include "vhost_net.h"
 
   #define VIRTIO_NET_VM_VERSION    11
 

@@ -47,6 +48,7 @@ typedef struct VirtIONet
       uint8_t nomulti;
       uint8_t nouni;
       uint8_t nobcast;
+    uint8_t vhost_started;
       struct {
           int in_use;
           int first_multi;

@@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
       n->nomulti = 0;
       n->nouni = 0;
       n->nobcast = 0;
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
 
       /* Flush any MAC and VLAN filter table state */
       n->mac_table.in_use = 0;

@@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
       .link_status_changed = virtio_net_set_link_status,
   };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    if (!!n->vhost_started == !!(vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+    if (vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        if (r<   0) {
+            fprintf(stderr, "unable to start vhost net: "
+                    "falling back on userspace virtio\n");
+        } else {

hw/virtio-net.c static NetClientInfo net_virtio_info = { · line 860

Quoted 10 lines

hw/virtio-net.c +8 -0

+            n->vhost_started = 1;
+        }
+    } else {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
+}
+
 
 

This function does a number of bad things. It makes virtio-net have specific knowledge of backends (like tap) and then has virtio-net pass device specific state (vdev) to a network backend.

Ultimately, the following things need to happen:

1) when a driver is ready to begin operating:
a) virtio-net needs to tell vhost the location of the ring in physical
memory
b) virtio-net needs to tell vhost about any notification it receives
(allowing kvm to shortcut userspace)
c) virtio-net needs to tell vhost about which irq is being used
(allowing kvm to shortcut userspace)

Quoted 9 lines

What this suggests is that we need an API for the network backends to do the following:

1) probe whether ring passthrough is supported
2) set the *virtual* address of the ring elements
3) hand the backend some sort of notification object for sending and
receiving notifications
4) stop ring passthrough

Quoted 4 lines

Look at how vnet_hdr is setup: frontend probes backend, and has 'if (backend has vnet header) blabla' vhost is really very similar, so I would like to do it in the same way.

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

We don't have to repeat this with vhost-net though because unlike vnet_hdr, we don't have to expose anything to the guest.

Quoted 6 lines

Generally I do not believe in abstractions that only have one implementation behind it: you only know how to abstract interface after you have more than one implementation. So whoever writes another frontend that can use vhost will be in a better position to add infrastructure to abstract both that new thing and virtio.

I agree with you, but at the same time, I also believe that layering violations should be avoided. I'm not suggesting that you need to make anything other than the vhost-net + virtio-net case work. Just make the interfaces abstract enough that you don't need to hand a vdev to vhost-net and such that you don't have to pass kvm specific data structure (ioeventfd) in what are supposed to be generic interfaces.

Quoted 18 lines

Quoted 18 lines

vhost should not need any direct knowledge of the device. This interface should be enough to communicating the required data. I think the only bit that is a little fuzzy and perhaps non-obvious for the current patches is the notification object. I would expect it look something like:

typedef struct IOEvent {
   int type;
   void (*notify)(IOEvent *);
   void (*on_notify)(IOEvent *, void (*cb)(IOEvent *, void *));
} IOEvent;

And then we would have

typedef struct KVMIOEvent {
   IOEvent event = {.type = KVM_IO_EVENT};
   int fd;
} KVMIOEvent;

Quoted 9 lines

Quoted 5 lines

if (kvm_enabled()) in virtio-net, we would allocate a KVMIOEvent for the PIO notification and hand that to vhost. vhost would check event.type and if it's KVM_IOEVENT, downcast and use that to get at the file descriptor.

Since we don't have any other IOEvents, I just put the fd in the generic structure directly. If other types surface we'll see how to generalize it.

I'd feel a whole lot better if we didn't pass the fd around and instead passed around a structure. With just a tiny bit of layering, we can even avoid making that structure KVM specific :-)

Quoted 11 lines

Quoted 6 lines

The KVMIOEvent should be created, not in the set status callback, but in the PCI map callback. And in the PCI map callback, cpu_single_env should be passed to a kvm specific function to create this KVMIOEvent object that contains the created eventfd() that's handed to kvm via ioctl.

So this specific thing does not work very well because with irqchip, we want to bypass notification and send irq directly from kernel. So I created a structure but it does not have callbacks, only the fd.

Your structure is an int, right? I understand the limits due to lack of in-kernel irqchip but I still think passing around an fd is a mistake.

Quoted 6 lines

Quoted 6 lines

There
should also be strong separation between the vhost-net code and the virtio-net device.

Regards,

Quoted 8 lines

Quoted 3 lines

Anthony Liguori

I don't see the point of this last idea. vhost is virtio accelerator, not a generic network backend. Whoever wants to use vhost for non-virtio frontends will have to add infrastructure for this separation, but I do not believe it's practical or desirable.

vhost is a userspace interface to inject packets into a network device just like a raw socket or a tun/tap device is. It happens to have some interesting features like it supports remapping of physical addresses and it implements interfaces for notifications that can conveniently be used by KVM to bypass userspace in the fast paths.

We should think of it this way for the same reason that vhost-net doesn't live in kvm.ko. If it's easy to isolate something and make it more generic, it's almost always the right thing to do. In this case, isolating vhost-net from virtio-net in qemu just involves passing some information in a function call verses passing a non-public data structure that is then accessed directly. Regardless of whether you agree with my view of vhost-net, the argument alone to avoid making a non-public structure public should be enough of an argument.

Regards,

Anthony Liguori

Quoted 101 lines · Michael S. Tsirkin

Quoted 90 lines · Anthony Liguori

Quoted 64 lines · Michael S. Tsirkin

start/stop backend on driver start/stop

Signed-off-by Michael S. Tsirkin<[email protected]>

---
   hw/virtio-net.c |   40 ++++++++++++++++++++++++++++++++++++++++
   1 files changed, 40 insertions(+), 0 deletions(-)

hw/virtio-net.c +28 -0

index c2a389f..99169e1 100644
@@ -17,6 +17,7 @@
   #include "net/tap.h"
   #include "qemu-timer.h"
   #include "virtio-net.h"
+#include "vhost_net.h"
 
   #define VIRTIO_NET_VM_VERSION    11
 

@@ -47,6 +48,7 @@ typedef struct VirtIONet
       uint8_t nomulti;
       uint8_t nouni;
       uint8_t nobcast;
+    uint8_t vhost_started;
       struct {
           int in_use;
           int first_multi;

@@ -114,6 +116,10 @@ static void virtio_net_reset(VirtIODevice *vdev)
       n->nomulti = 0;
       n->nouni = 0;
       n->nobcast = 0;
+    if (n->vhost_started) {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
 
       /* Flush any MAC and VLAN filter table state */
       n->mac_table.in_use = 0;

@@ -820,6 +826,36 @@ static NetClientInfo net_virtio_info = {
       .link_status_changed = virtio_net_set_link_status,
   };
 
+static void virtio_net_set_status(struct VirtIODevice *vdev)
+{
+    VirtIONet *n = to_virtio_net(vdev);
+    if (!n->nic->nc.peer) {
+        return;
+    }
+    if (n->nic->nc.peer->info->type != NET_CLIENT_TYPE_TAP) {
+        return;
+    }
+
+    if (!tap_get_vhost_net(n->nic->nc.peer)) {
+        return;
+    }
+    if (!!n->vhost_started == !!(vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+    if (vdev->status&   VIRTIO_CONFIG_S_DRIVER_OK) {
+        int r = vhost_net_start(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        if (r<   0) {
+            fprintf(stderr, "unable to start vhost net: "
+                    "falling back on userspace virtio\n");
+        } else {

hw/virtio-net.c static NetClientInfo net_virtio_info = { · line 860

Quoted 10 lines

hw/virtio-net.c +8 -0

+            n->vhost_started = 1;
+        }
+    } else {
+        vhost_net_stop(tap_get_vhost_net(n->nic->nc.peer), vdev);
+        n->vhost_started = 0;
+    }
+}
+
 
 

This function does a number of bad things. It makes virtio-net have specific knowledge of backends (like tap) and then has virtio-net pass device specific state (vdev) to a network backend.

Ultimately, the following things need to happen:

1) when a driver is ready to begin operating:
a) virtio-net needs to tell vhost the location of the ring in physical
memory
b) virtio-net needs to tell vhost about any notification it receives
(allowing kvm to shortcut userspace)
c) virtio-net needs to tell vhost about which irq is being used
(allowing kvm to shortcut userspace)

Quoted 9 lines

What this suggests is that we need an API for the network backends to do the following:

1) probe whether ring passthrough is supported
2) set the *virtual* address of the ring elements
3) hand the backend some sort of notification object for sending and
receiving notifications
4) stop ring passthrough

Quoted 4 lines

Look at how vnet_hdr is setup: frontend probes backend, and has 'if (backend has vnet header) blabla' vhost is really very similar, so I would like to do it in the same way.

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

We don't have to repeat this with vhost-net though because unlike vnet_hdr, we don't have to expose anything to the guest.

Quoted 6 lines

Generally I do not believe in abstractions that only have one implementation behind it: you only know how to abstract interface after you have more than one implementation. So whoever writes another frontend that can use vhost will be in a better position to add infrastructure to abstract both that new thing and virtio.

I agree with you, but at the same time, I also believe that layering violations should be avoided. I'm not suggesting that you need to make anything other than the vhost-net + virtio-net case work. Just make the interfaces abstract enough that you don't need to hand a vdev to vhost-net and such that you don't have to pass kvm specific data structure (ioeventfd) in what are supposed to be generic interfaces.

Quoted 18 lines

Quoted 18 lines

vhost should not need any direct knowledge of the device. This interface should be enough to communicating the required data. I think the only bit that is a little fuzzy and perhaps non-obvious for the current patches is the notification object. I would expect it look something like:

typedef struct IOEvent {
   int type;
   void (*notify)(IOEvent *);
   void (*on_notify)(IOEvent *, void (*cb)(IOEvent *, void *));
} IOEvent;

And then we would have

typedef struct KVMIOEvent {
   IOEvent event = {.type = KVM_IO_EVENT};
   int fd;
} KVMIOEvent;

Quoted 9 lines

Quoted 5 lines

if (kvm_enabled()) in virtio-net, we would allocate a KVMIOEvent for the PIO notification and hand that to vhost. vhost would check event.type and if it's KVM_IOEVENT, downcast and use that to get at the file descriptor.

Since we don't have any other IOEvents, I just put the fd in the generic structure directly. If other types surface we'll see how to generalize it.

I'd feel a whole lot better if we didn't pass the fd around and instead passed around a structure. With just a tiny bit of layering, we can even avoid making that structure KVM specific :-)

Quoted 6 lines

Quoted 6 lines

The KVMIOEvent should be created, not in the set status callback, but in the PCI map callback. And in the PCI map callback, cpu_single_env should be passed to a kvm specific function to create this KVMIOEvent object that contains the created eventfd() that's handed to kvm via ioctl.

Quoted 5 lines

So this specific thing does not work very well because with irqchip, we want to bypass notification and send irq directly from kernel. So I created a structure but it does not have callbacks, only the fd.

Your structure is an int, right?

I understand the limits due to lack of
in-kernel irqchip but I still think passing around an fd is a mistake.

Quoted 6 lines

Quoted 6 lines

There
should also be strong separation between the vhost-net code and the virtio-net device.

Regards,

Quoted 8 lines

Quoted 3 lines

Anthony Liguori

I don't see the point of this last idea. vhost is virtio accelerator, not a generic network backend. Whoever wants to use vhost for non-virtio frontends will have to add infrastructure for this separation, but I do not believe it's practical or desirable.

vhost is a userspace interface to inject packets into a network device just like a raw socket or a tun/tap device is. It happens to have some interesting features like it supports remapping of physical addresses and it implements interfaces for notifications that can conveniently be used by KVM to bypass userspace in the fast paths.

We should think of it this way for the same reason that vhost-net doesn't live in kvm.ko. If it's easy to isolate something and make it more generic, it's almost always the right thing to do. In this case, isolating vhost-net from virtio-net in qemu just involves passing some information in a function call verses passing a non-public data structure that is then accessed directly. Regardless of whether you agree with my view of vhost-net, the argument alone to avoid making a non-public structure public should be enough of an argument.

Regards,

Anthony Liguori

it *has* an int:

	struct EventNotifier {
		int fd;
	};

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

Quoted 8 lines

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

Or make virtio-net pass this on to the guest, and have that deal with the problem.

If you implement software GSO, then devices can assume it's always present and don't need to probe.

Paul

Quoted 6 lines

Quoted 6 lines

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

Quoted 6 lines

Quoted 3 lines

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

Or make virtio-net pass this on to the guest, and have that deal with the problem.

This is exacly what we do, via feature bits.

Quoted 11 lines · Paul Brook

Quoted 10 lines

Quoted 9 lines

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

Quoted 4 lines

Quoted 2 lines

Or make virtio-net pass this on to the guest, and have that deal with the problem.

This is exacly what we do, via feature bits.

AFAIK we only have static feature bits. There aren't useful for anything that the user may change on the fly (or via migration).

If you don't have a fallback implementation then the guest must be able to cope with this feature disappearing without warning.

If we do have a software
fallback then the feature bit is just for backwards compatibility, and should be enabled unconditionally (on current machine types).

Paul

Quoted 3 lines

If we do have a software
fallback then the feature bit is just for backwards compatibility, and should be enabled unconditionally (on current machine types).

Software fallback might turn out to be slower than disabling the feature in the guest. For example, and extra pass over packet might cause extra CPU cache thrashing. In that case, it's not obvious whether enabling it unconditionally will turn out to be a good idea. But we'll have to have that code to be able to tell.

Quoted 4 lines

Quoted 3 lines

If we do have a software
fallback then the feature bit is just for backwards compatibility, and should be enabled unconditionally (on current machine types).

Quoted 5 lines

Software fallback might turn out to be slower than disabling the feature in the guest. For example, and extra pass over packet might cause extra CPU cache thrashing. In that case, it's not obvious whether enabling it unconditionally will turn out to be a good idea. But we'll have to have that code to be able to tell.

IMO once you accept that these things can change, consistency is more important than trying to guess what the "best" option may be.

Starting qemu on machine A and migrating to machine B should give the same guest environment as starting on machine B.

Paul

Quoted 6 lines

vnet_hdr is IMHO a really bad example to copy from.

vnet_hdr leaks into the migration state via n->has_vnet_hdr. What this means is that if you want to migrate from -net tap -net nic,model=virtio to -net user -net nic,model=virtio, it will fail.

Quoted 3 lines

This is a hard problem to solve in qemu though because it would require that we implement software GSO which so far, no one has stepped up to do.

Or make virtio-net pass this on to the guest, and have that deal with the problem. If you implement software GSO, then devices can assume it's always present and don't need to probe.

Paul

Ah, you mean telling the guest to switch features on and off: natureally this would require guest driver changes. This might be also non-trivial to implement. Consider a request that has been posted without checksum, suddenly we disable checksum support. Guest will need a way to handle that. Guest OSes might also not be prepared to handle device features going away.