[PATCH 0/3] kvmtool: fix vhost-net support

[PATCH 0/3] kvmtool: fix vhost-net support

[Andre Przywara]

Hi,

this mini-series is a spin-off of my ITS emulation series, so those
patches have been on the list before [1].
I repost them separately here to get them merged, because patch 2/3
fixes a real issue:
Vhost-net support seems to be broken (on x86). In my experiments
(-n mode=tap,script=/etc/qemu-ifup,vhost=1) the guest hangs because
it waits for an virtio IRQ to trigger. But the MSI never makes it
through, because virtio-pci changes the MSI configuration _after_
having set up the IRQ routing for this interrupt. So we need to update
the IRQ routing in case PCI config space accesses change the MSIs.

I put the move of the IRQ routing code into generic code here as 1/3,
since we need this anyway later and swapping the two patches looks
like pain, which we would also need to redo later anyway. Also it
should help to consolidate other IRQ related patches.
Finally we deny vhost-net on any bi-endian architecture at the moment,
which is unnecessary as long as host and guest use the same endianess.
Patch 3/3 fixes this.

Please have a look and apply!

Cheers,
Andre.

[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357884.html

Andre Przywara (3):
  irq: move IRQ routing into irq.c
  MSI-X: update GSI routing after changed MSI-X configuration
  virtio: fix endianness check for vhost support

 Makefile             |   4 +-
 arm/irq.c            |   9 ----
 hw/pci-shmem.c       |   2 +
 include/kvm/irq.h    |   6 +++
 include/kvm/virtio.h |   9 +++-
 irq.c                | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
 mips/irq.c           |  10 -----
 powerpc/irq.c        |  31 --------------
 virtio/net.c         |   2 +-
 virtio/pci.c         |  54 ++++++++++++++++++++----
 x86/irq.c            |  45 +++-----------------
 11 files changed, 181 insertions(+), 105 deletions(-)
 delete mode 100644 arm/irq.c
 delete mode 100644 mips/irq.c
 delete mode 100644 powerpc/irq.c

-- 
2.6.4

[PATCH 1/3] irq: move IRQ routing into irq.c

[Andre Przywara]

The current IRQ routing code in x86/irq.c is mostly implementing a
generic KVM interface which other architectures may use too.
Move the code to set up an MSI route into the generic irq.c file and
guard it with the KVM_CAP_IRQ_ROUTING capability to return an error
if the kernel does not support interrupt routing.
This also removes the dummy implementations for all other
architectures and only leaves the x86 specific code in x86/irq.c.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile          |  4 +--
 arm/irq.c         |  9 ------
 hw/pci-shmem.c    |  2 ++
 include/kvm/irq.h |  5 ++++
 irq.c             | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mips/irq.c        | 10 -------
 powerpc/irq.c     | 31 ---------------------
 virtio/pci.c      | 18 ++++++++----
 x86/irq.c         | 45 ++++--------------------------
 9 files changed, 108 insertions(+), 99 deletions(-)
 delete mode 100644 arm/irq.c
 delete mode 100644 mips/irq.c
 delete mode 100644 powerpc/irq.c

diff --git a/Makefile b/Makefile
index 59622c3..bed0714 100644
--- a/Makefile
+++ b/Makefile
@@ -137,7 +137,6 @@ ifeq ($(ARCH), powerpc)
 	DEFINES += -DCONFIG_PPC
 	OBJS	+= powerpc/boot.o
 	OBJS	+= powerpc/ioport.o
-	OBJS	+= powerpc/irq.o
 	OBJS	+= powerpc/kvm.o
 	OBJS	+= powerpc/cpu_info.o
 	OBJS	+= powerpc/kvm-cpu.o
@@ -152,7 +151,7 @@ ifeq ($(ARCH), powerpc)
 endif
 
 # ARM
-OBJS_ARM_COMMON		:= arm/fdt.o arm/gic.o arm/ioport.o arm/irq.o \
+OBJS_ARM_COMMON		:= arm/fdt.o arm/gic.o arm/ioport.o \
 			   arm/kvm.o arm/kvm-cpu.o arm/pci.o arm/timer.o
 HDRS_ARM_COMMON		:= arm/include
 ifeq ($(ARCH), arm)
@@ -184,7 +183,6 @@ ifeq ($(ARCH),mips)
 	ARCH_INCLUDE	:= mips/include
 	OBJS		+= mips/kvm.o
 	OBJS		+= mips/kvm-cpu.o
-	OBJS		+= mips/irq.o
 endif
 ###
 
diff --git a/arm/irq.c b/arm/irq.c
deleted file mode 100644
index d8f44df..0000000
--- a/arm/irq.c
+++ /dev/null
@@ -1,9 +0,0 @@
-#include "kvm/irq.h"
-#include "kvm/kvm.h"
-#include "kvm/util.h"
-
-int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
-{
-	die(__FUNCTION__);
-	return 0;
-}
diff --git a/hw/pci-shmem.c b/hw/pci-shmem.c
index a1c5ab7..7ce98cb 100644
--- a/hw/pci-shmem.c
+++ b/hw/pci-shmem.c
@@ -136,6 +136,8 @@ int pci_shmem__get_local_irqfd(struct kvm *kvm)
 
 		if (pci_shmem_pci_device.msix.ctrl & cpu_to_le16(PCI_MSIX_FLAGS_ENABLE)) {
 			gsi = irq__add_msix_route(kvm, &msix_table[0].msg);
+			if (gsi < 0)
+				return gsi;
 		} else {
 			gsi = pci_shmem_pci_device.irq_line;
 		}
diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index 8a78e43..bb71521 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -10,11 +10,16 @@
 
 struct kvm;
 
+extern struct kvm_irq_routing *irq_routing;
+extern int next_gsi;
+
 int irq__alloc_line(void);
 int irq__get_nr_allocated_lines(void);
 
 int irq__init(struct kvm *kvm);
 int irq__exit(struct kvm *kvm);
+
+int irq__allocate_routing_entry(void);
 int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg);
 
 #endif
diff --git a/irq.c b/irq.c
index 71eaa05..1aee478 100644
--- a/irq.c
+++ b/irq.c
@@ -1,7 +1,19 @@
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <linux/types.h>
+#include <linux/kvm.h>
+#include <errno.h>
+
+#include "kvm/kvm.h"
 #include "kvm/irq.h"
 #include "kvm/kvm-arch.h"
 
 static u8 next_line = KVM_IRQ_OFFSET;
+static int allocated_gsis = 0;
+
+int next_gsi;
+
+struct kvm_irq_routing *irq_routing = NULL;
 
 int irq__alloc_line(void)
 {
@@ -12,3 +24,74 @@ int irq__get_nr_allocated_lines(void)
 {
 	return next_line - KVM_IRQ_OFFSET;
 }
+
+int irq__allocate_routing_entry(void)
+{
+	size_t table_size = sizeof(struct kvm_irq_routing);
+	int nr_entries = 0;
+
+	if (irq_routing)
+		nr_entries = irq_routing->nr;
+
+	if (nr_entries < allocated_gsis)
+		return 0;
+
+	allocated_gsis = ALIGN(nr_entries + 1, 32);
+	table_size += sizeof(struct kvm_irq_routing_entry) * allocated_gsis;
+	irq_routing = realloc(irq_routing, table_size);
+
+	if (irq_routing == NULL)
+		return ENOMEM;
+
+	irq_routing->nr = nr_entries;
+
+	return 0;
+}
+
+static bool check_for_irq_routing(struct kvm *kvm)
+{
+	static int has_irq_routing = 0;
+
+	if (has_irq_routing == 0) {
+		if (kvm__supports_extension(kvm, KVM_CAP_IRQ_ROUTING))
+			has_irq_routing = 1;
+		else
+			has_irq_routing = -1;
+	}
+
+	return has_irq_routing > 0;
+}
+
+int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
+{
+	int r;
+
+	if (!check_for_irq_routing(kvm))
+		return -EINVAL;
+
+	r = irq__allocate_routing_entry();
+	if (r)
+		return r;
+
+	irq_routing->entries[irq_routing->nr++] =
+		(struct kvm_irq_routing_entry) {
+			.gsi = next_gsi,
+			.type = KVM_IRQ_ROUTING_MSI,
+			.u.msi.address_hi = msg->address_hi,
+			.u.msi.address_lo = msg->address_lo,
+			.u.msi.data = msg->data,
+		};
+
+	r = ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
+	if (r)
+		return r;
+
+	return next_gsi++;
+}
+
+int __attribute__((weak)) irq__exit(struct kvm *kvm)
+{
+	free(irq_routing);
+	return 0;
+}
+dev_base_exit(irq__exit);
diff --git a/mips/irq.c b/mips/irq.c
deleted file mode 100644
index c1ff6bb..0000000
--- a/mips/irq.c
+++ /dev/null
@@ -1,10 +0,0 @@
-#include "kvm/irq.h"
-#include "kvm/kvm.h"
-
-#include <stdlib.h>
-
-int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
-{
-	pr_warning("irq__add_msix_route");
-	return 1;
-}
diff --git a/powerpc/irq.c b/powerpc/irq.c
deleted file mode 100644
index 03f2fe7..0000000
--- a/powerpc/irq.c
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
- * PPC64 IRQ routines
- *
- * Copyright 2011 Matt Evans <matt@ozlabs.org>, IBM Corporation.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 as published
- * by the Free Software Foundation.
- */
-
-#include "kvm/devices.h"
-#include "kvm/irq.h"
-#include "kvm/kvm.h"
-#include "kvm/util.h"
-
-#include <linux/types.h>
-#include <linux/rbtree.h>
-#include <linux/list.h>
-#include <linux/kvm.h>
-#include <sys/ioctl.h>
-
-#include <stddef.h>
-#include <stdlib.h>
-
-#include "kvm/pci.h"
-
-int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
-{
-	die(__FUNCTION__);
-	return 0;
-}
diff --git a/virtio/pci.c b/virtio/pci.c
index 90fcd64..70c93ce 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -166,21 +166,27 @@ static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *v
 			if (vec == VIRTIO_MSI_NO_VECTOR)
 				break;
 
-			gsi = irq__add_msix_route(kvm, &vpci->msix_table[vec].msg);
-
-			vpci->config_gsi = gsi;
+			gsi = irq__add_msix_route(kvm,
+						  &vpci->msix_table[vec].msg);
+			if (gsi >= 0)
+				vpci->config_gsi = gsi;
 			break;
 		case VIRTIO_MSI_QUEUE_VECTOR:
-			vec = vpci->vq_vector[vpci->queue_selector] = ioport__read16(data);
+			vec = ioport__read16(data);
+			vpci->vq_vector[vpci->queue_selector] = vec;
 
 			if (vec == VIRTIO_MSI_NO_VECTOR)
 				break;
 
-			gsi = irq__add_msix_route(kvm, &vpci->msix_table[vec].msg);
+			gsi = irq__add_msix_route(kvm,
+						  &vpci->msix_table[vec].msg);
+			if (gsi < 0)
+				break;
 			vpci->gsis[vpci->queue_selector] = gsi;
 			if (vdev->ops->notify_vq_gsi)
 				vdev->ops->notify_vq_gsi(kvm, vpci->dev,
-							vpci->queue_selector, gsi);
+							 vpci->queue_selector,
+							 gsi);
 			break;
 		};
 
diff --git a/x86/irq.c b/x86/irq.c
index 72177e7..49b2e90 100644
--- a/x86/irq.c
+++ b/x86/irq.c
@@ -11,20 +11,15 @@
 #include <stddef.h>
 #include <stdlib.h>
 
-#define IRQ_MAX_GSI			64
 #define IRQCHIP_MASTER			0
 #define IRQCHIP_SLAVE			1
 #define IRQCHIP_IOAPIC			2
 
-/* First 24 GSIs are routed between IRQCHIPs and IOAPICs */
-static u32 gsi = 24;
-
-struct kvm_irq_routing *irq_routing;
-
 static int irq__add_routing(u32 gsi, u32 type, u32 irqchip, u32 pin)
 {
-	if (gsi >= IRQ_MAX_GSI)
-		return -ENOSPC;
+	int r = irq__allocate_routing_entry();
+	if (r)
+		return r;
 
 	irq_routing->entries[irq_routing->nr++] =
 		(struct kvm_irq_routing_entry) {
@@ -41,11 +36,6 @@ int irq__init(struct kvm *kvm)
 {
 	int i, r;
 
-	irq_routing = calloc(sizeof(struct kvm_irq_routing) +
-			IRQ_MAX_GSI * sizeof(struct kvm_irq_routing_entry), 1);
-	if (irq_routing == NULL)
-		return -ENOMEM;
-
 	/* Hook first 8 GSIs to master IRQCHIP */
 	for (i = 0; i < 8; i++)
 		if (i != 2)
@@ -69,33 +59,8 @@ int irq__init(struct kvm *kvm)
 		return errno;
 	}
 
-	return 0;
-}
-dev_base_init(irq__init);
+	next_gsi = 24;
 
-int irq__exit(struct kvm *kvm)
-{
-	free(irq_routing);
 	return 0;
 }
-dev_base_exit(irq__exit);
-
-int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
-{
-	int r;
-
-	irq_routing->entries[irq_routing->nr++] =
-		(struct kvm_irq_routing_entry) {
-			.gsi = gsi,
-			.type = KVM_IRQ_ROUTING_MSI,
-			.u.msi.address_hi = msg->address_hi,
-			.u.msi.address_lo = msg->address_lo,
-			.u.msi.data = msg->data,
-		};
-
-	r = ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
-	if (r)
-		return r;
-
-	return gsi++;
-}
+dev_base_init(irq__init);
-- 
2.6.4

Re: [PATCH 1/3] irq: move IRQ routing into irq.c

[Will Deacon]

Hi Andre,

On Tue, Mar 01, 2016 at 04:49:36PM +0000, Andre Przywara wrote:
> The current IRQ routing code in x86/irq.c is mostly implementing a
> generic KVM interface which other architectures may use too.
> Move the code to set up an MSI route into the generic irq.c file and
> guard it with the KVM_CAP_IRQ_ROUTING capability to return an error
> if the kernel does not support interrupt routing.
> This also removes the dummy implementations for all other
> architectures and only leaves the x86 specific code in x86/irq.c.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Makefile          |  4 +--
>  arm/irq.c         |  9 ------
>  hw/pci-shmem.c    |  2 ++
>  include/kvm/irq.h |  5 ++++
>  irq.c             | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  mips/irq.c        | 10 -------
>  powerpc/irq.c     | 31 ---------------------
>  virtio/pci.c      | 18 ++++++++----
>  x86/irq.c         | 45 ++++--------------------------
>  9 files changed, 108 insertions(+), 99 deletions(-)
>  delete mode 100644 arm/irq.c
>  delete mode 100644 mips/irq.c
>  delete mode 100644 powerpc/irq.c

[...]

> diff --git a/x86/irq.c b/x86/irq.c
> index 72177e7..49b2e90 100644
> --- a/x86/irq.c
> +++ b/x86/irq.c
> @@ -11,20 +11,15 @@
>  #include <stddef.h>
>  #include <stdlib.h>
>  
> -#define IRQ_MAX_GSI			64
>  #define IRQCHIP_MASTER			0
>  #define IRQCHIP_SLAVE			1
>  #define IRQCHIP_IOAPIC			2
>  
> -/* First 24 GSIs are routed between IRQCHIPs and IOAPICs */
> -static u32 gsi = 24;
> -
> -struct kvm_irq_routing *irq_routing;
> -
>  static int irq__add_routing(u32 gsi, u32 type, u32 irqchip, u32 pin)
>  {
> -	if (gsi >= IRQ_MAX_GSI)
> -		return -ENOSPC;
> +	int r = irq__allocate_routing_entry();
> +	if (r)
> +		return r;
>  
>  	irq_routing->entries[irq_routing->nr++] =
>  		(struct kvm_irq_routing_entry) {
> @@ -41,11 +36,6 @@ int irq__init(struct kvm *kvm)
>  {
>  	int i, r;
>  
> -	irq_routing = calloc(sizeof(struct kvm_irq_routing) +
> -			IRQ_MAX_GSI * sizeof(struct kvm_irq_routing_entry), 1);
> -	if (irq_routing == NULL)
> -		return -ENOMEM;
> -
>  	/* Hook first 8 GSIs to master IRQCHIP */
>  	for (i = 0; i < 8; i++)
>  		if (i != 2)
> @@ -69,33 +59,8 @@ int irq__init(struct kvm *kvm)
>  		return errno;
>  	}
>  
> -	return 0;
> -}
> -dev_base_init(irq__init);
> +	next_gsi = 24;

Can we not just have an arch-specific initialiser that defaults to zero,
like we do for wired interrupts? (e.g. KVM_MSI_OFFSET). That way, we can
keep next_gsi private to the common irq routing code.

Will

[PATCH 2/3] MSI-X: update GSI routing after changed MSI-X configuration

[Andre Przywara]

When we set up GSI routing to map MSIs to KVM's GSI numbers, we
write the current device's MSI setup into the kernel routing table.
However the device driver in the guest can use PCI configuration space
accesses to change the MSI configuration (address and/or payload data).
Whenever this happens after we have setup the routing table already,
we must amend the previously sent data.
So when MSI-X PCI config space accesses write address or payload,
find the associated GSI number and the matching routing table entry
and update the kernel routing table (only if the data has changed).

This fixes vhost-net, where the queue's IRQFD was setup before the
MSI vectors.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/irq.h |  1 +
 irq.c             | 31 +++++++++++++++++++++++++++++++
 virtio/pci.c      | 36 +++++++++++++++++++++++++++++++++---
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/include/kvm/irq.h b/include/kvm/irq.h
index bb71521..f35eb7e 100644
--- a/include/kvm/irq.h
+++ b/include/kvm/irq.h
@@ -21,5 +21,6 @@ int irq__exit(struct kvm *kvm);
 
 int irq__allocate_routing_entry(void);
 int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg);
+void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg);
 
 #endif
diff --git a/irq.c b/irq.c
index 1aee478..25ac8d7 100644
--- a/irq.c
+++ b/irq.c
@@ -89,6 +89,37 @@ int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
 	return next_gsi++;
 }
 
+static bool update_data(u32 *ptr, u32 newdata)
+{
+	if (*ptr == newdata)
+		return false;
+
+	*ptr = newdata;
+	return true;
+}
+
+void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg)
+{
+	struct kvm_irq_routing_msi *entry;
+	unsigned int i;
+	bool changed;
+
+	for (i = 0; i < irq_routing->nr; i++)
+		if (gsi == irq_routing->entries[i].gsi)
+			break;
+	if (i == irq_routing->nr)
+		return;
+
+	entry = &irq_routing->entries[i].u.msi;
+
+	changed  = update_data(&entry->address_hi, msg->address_hi);
+	changed |= update_data(&entry->address_lo, msg->address_lo);
+	changed |= update_data(&entry->data, msg->data);
+
+	if (changed)
+		ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
+}
+
 int __attribute__((weak)) irq__exit(struct kvm *kvm)
 {
 	free(irq_routing);
diff --git a/virtio/pci.c b/virtio/pci.c
index 70c93ce..625fec0 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -152,6 +152,30 @@ static bool virtio_pci__io_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 p
 	return ret;
 }
 
+static void update_msix_map(struct virtio_pci *vpci,
+			    struct msix_table *msix_entry, u32 vecnum)
+{
+	u32 gsi, i;
+
+	/* Find the GSI number used for that vector */
+	if (vecnum == vpci->config_vector) {
+		gsi = vpci->config_gsi;
+	} else {
+		for (i = 0; i < VIRTIO_PCI_MAX_VQ; i++)
+			if (vpci->vq_vector[i] == vecnum)
+				break;
+		if (i == VIRTIO_PCI_MAX_VQ)
+			return;
+		gsi = vpci->gsis[i];
+	}
+
+	if (gsi == 0)
+		return;
+
+	msix_entry = &msix_entry[vecnum];
+	irq__update_msix_route(vpci->kvm, gsi, &msix_entry->msg);
+}
+
 static bool virtio_pci__specific_io_out(struct kvm *kvm, struct virtio_device *vdev, u16 port,
 					void *data, int size, int offset)
 {
@@ -269,10 +293,16 @@ static void virtio_pci__msix_mmio_callback(struct kvm_cpu *vcpu,
 		offset	= vpci->msix_io_block;
 	}
 
-	if (is_write)
-		memcpy(table + addr - offset, data, len);
-	else
+	if (!is_write) {
 		memcpy(data, table + addr - offset, len);
+		return;
+	}
+
+	memcpy(table + addr - offset, data, len);
+
+	/* Did we just update the address or payload? */
+	if (addr % 0x10 < 0xc)
+		update_msix_map(vpci, table, (addr - offset) / 16);
 }
 
 static void virtio_pci__signal_msi(struct kvm *kvm, struct virtio_pci *vpci, int vec)
-- 
2.6.4

Re: [PATCH 2/3] MSI-X: update GSI routing after changed MSI-X configuration

[Will Deacon]

On Tue, Mar 01, 2016 at 04:49:37PM +0000, Andre Przywara wrote:
> When we set up GSI routing to map MSIs to KVM's GSI numbers, we
> write the current device's MSI setup into the kernel routing table.
> However the device driver in the guest can use PCI configuration space
> accesses to change the MSI configuration (address and/or payload data).
> Whenever this happens after we have setup the routing table already,
> we must amend the previously sent data.
> So when MSI-X PCI config space accesses write address or payload,
> find the associated GSI number and the matching routing table entry
> and update the kernel routing table (only if the data has changed).
> 
> This fixes vhost-net, where the queue's IRQFD was setup before the
> MSI vectors.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/irq.h |  1 +
>  irq.c             | 31 +++++++++++++++++++++++++++++++
>  virtio/pci.c      | 36 +++++++++++++++++++++++++++++++++---
>  3 files changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/irq.h b/include/kvm/irq.h
> index bb71521..f35eb7e 100644
> --- a/include/kvm/irq.h
> +++ b/include/kvm/irq.h
> @@ -21,5 +21,6 @@ int irq__exit(struct kvm *kvm);
>  
>  int irq__allocate_routing_entry(void);
>  int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg);
> +void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg);
>  
>  #endif
> diff --git a/irq.c b/irq.c
> index 1aee478..25ac8d7 100644
> --- a/irq.c
> +++ b/irq.c
> @@ -89,6 +89,37 @@ int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
>  	return next_gsi++;
>  }
>  
> +static bool update_data(u32 *ptr, u32 newdata)
> +{
> +	if (*ptr == newdata)
> +		return false;
> +
> +	*ptr = newdata;
> +	return true;
> +}
> +
> +void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg)
> +{
> +	struct kvm_irq_routing_msi *entry;
> +	unsigned int i;
> +	bool changed;
> +
> +	for (i = 0; i < irq_routing->nr; i++)
> +		if (gsi == irq_routing->entries[i].gsi)
> +			break;
> +	if (i == irq_routing->nr)
> +		return;
> +
> +	entry = &irq_routing->entries[i].u.msi;
> +
> +	changed  = update_data(&entry->address_hi, msg->address_hi);
> +	changed |= update_data(&entry->address_lo, msg->address_lo);
> +	changed |= update_data(&entry->data, msg->data);
> +
> +	if (changed)
> +		ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
> +}

What goes wrong if you just call the ioctl every time? Is this actually
a fast path in practice?

Will

Re: [PATCH 2/3] MSI-X: update GSI routing after changed MSI-X configuration

[André Przywara]

Hi,

On 02/03/16 01:16, Will Deacon wrote:
> On Tue, Mar 01, 2016 at 04:49:37PM +0000, Andre Przywara wrote:
>> When we set up GSI routing to map MSIs to KVM's GSI numbers, we
>> write the current device's MSI setup into the kernel routing table.
>> However the device driver in the guest can use PCI configuration space
>> accesses to change the MSI configuration (address and/or payload data).
>> Whenever this happens after we have setup the routing table already,
>> we must amend the previously sent data.
>> So when MSI-X PCI config space accesses write address or payload,
>> find the associated GSI number and the matching routing table entry
>> and update the kernel routing table (only if the data has changed).
>>
>> This fixes vhost-net, where the queue's IRQFD was setup before the
>> MSI vectors.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  include/kvm/irq.h |  1 +
>>  irq.c             | 31 +++++++++++++++++++++++++++++++
>>  virtio/pci.c      | 36 +++++++++++++++++++++++++++++++++---
>>  3 files changed, 65 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/kvm/irq.h b/include/kvm/irq.h
>> index bb71521..f35eb7e 100644
>> --- a/include/kvm/irq.h
>> +++ b/include/kvm/irq.h
>> @@ -21,5 +21,6 @@ int irq__exit(struct kvm *kvm);
>>  
>>  int irq__allocate_routing_entry(void);
>>  int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg);
>> +void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg);
>>  
>>  #endif
>> diff --git a/irq.c b/irq.c
>> index 1aee478..25ac8d7 100644
>> --- a/irq.c
>> +++ b/irq.c
>> @@ -89,6 +89,37 @@ int irq__add_msix_route(struct kvm *kvm, struct msi_msg *msg)
>>  	return next_gsi++;
>>  }
>>  
>> +static bool update_data(u32 *ptr, u32 newdata)
>> +{
>> +	if (*ptr == newdata)
>> +		return false;
>> +
>> +	*ptr = newdata;
>> +	return true;
>> +}
>> +
>> +void irq__update_msix_route(struct kvm *kvm, u32 gsi, struct msi_msg *msg)
>> +{
>> +	struct kvm_irq_routing_msi *entry;
>> +	unsigned int i;
>> +	bool changed;
>> +
>> +	for (i = 0; i < irq_routing->nr; i++)
>> +		if (gsi == irq_routing->entries[i].gsi)
>> +			break;
>> +	if (i == irq_routing->nr)
>> +		return;
>> +
>> +	entry = &irq_routing->entries[i].u.msi;
>> +
>> +	changed  = update_data(&entry->address_hi, msg->address_hi);
>> +	changed |= update_data(&entry->address_lo, msg->address_lo);
>> +	changed |= update_data(&entry->data, msg->data);
>> +
>> +	if (changed)
>> +		ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
>> +}
> 
> What goes wrong if you just call the ioctl every time? Is this actually
> a fast path in practice?

I guess nothing, it's just a lot of needless churn in the kernel. We
trap on every word access to the MSI data region and I have seen so many
non-updates in there. For instance if the guest updates the 	payload, it
writes the unchanged address parts also and we trap that.
Also please note that this ioctl updates the _whole table_ every time.
If you now look at what virt/kvm/kvm_main.c actually does (kmalloc,
copy_from_user, kmalloc again, update each entry (with kmallocs), RCU
switch over to the new table, free the old table, free, free), I hope
you agree that his little extra code in userland is
surely worth the effort. I had debug messages in the kernel to chase the
bug and the output was huge every time for actually no change at all
most of the times.

Cheers,
Andre.

[PATCH 3/3] virtio: fix endianness check for vhost support

[Andre Przywara]

Currently we deny any VHOST_* functionality if the architecture
supports guests with different endianness than the host. Most of the
time even on those architectures the endianness of guest and host are
the same, though, so we are denying the glory of VHOST needlessly.
Switch from compile time determination to a run time scheme, which
takes the actual endianness of the guest into account.
For this we change the semantics of VIRTIO_ENDIAN_HOST to return the
actual endianness of the host (the endianness of kvmtool at compile
time, really). The actual check in vhost_net now compares this against
the guest endianness.
This enables vhost support on ARM and ARM64.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 include/kvm/virtio.h | 9 +++++++--
 virtio/net.c         | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
index 768ee96..66530fd 100644
--- a/include/kvm/virtio.h
+++ b/include/kvm/virtio.h
@@ -17,10 +17,15 @@
 #define VIRTIO_PCI_O_CONFIG	0
 #define VIRTIO_PCI_O_MSIX	1
 
-#define VIRTIO_ENDIAN_HOST	0
 #define VIRTIO_ENDIAN_LE	(1 << 0)
 #define VIRTIO_ENDIAN_BE	(1 << 1)
 
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define VIRTIO_ENDIAN_HOST VIRTIO_ENDIAN_LE
+#else
+#define VIRTIO_ENDIAN_HOST VIRTIO_ENDIAN_BE
+#endif
+
 struct virt_queue {
 	struct vring	vring;
 	u32		pfn;
@@ -40,7 +45,7 @@ struct virt_queue {
 #define VIRTIO_RING_ENDIAN VIRTIO_ENDIAN_HOST
 #endif
 
-#if (VIRTIO_RING_ENDIAN & (VIRTIO_ENDIAN_LE | VIRTIO_ENDIAN_BE))
+#if VIRTIO_RING_ENDIAN != VIRTIO_ENDIAN_HOST
 
 static inline __u16 __virtio_g2h_u16(u16 endian, __u16 val)
 {
diff --git a/virtio/net.c b/virtio/net.c
index 6d1be65..e94e37a 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -531,7 +531,7 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 	}
 
 	if (queue->endian != VIRTIO_ENDIAN_HOST)
-		die_perror("VHOST requires VIRTIO_ENDIAN_HOST");
+		die_perror("VHOST requires the same endianness in guest and host");
 
 	state.num = queue->vring.num;
 	r = ioctl(ndev->vhost_fd, VHOST_SET_VRING_NUM, &state);
-- 
2.6.4

Re: [PATCH 3/3] virtio: fix endianness check for vhost support

[Will Deacon]

On Tue, Mar 01, 2016 at 04:49:38PM +0000, Andre Przywara wrote:
> Currently we deny any VHOST_* functionality if the architecture
> supports guests with different endianness than the host. Most of the
> time even on those architectures the endianness of guest and host are
> the same, though, so we are denying the glory of VHOST needlessly.
> Switch from compile time determination to a run time scheme, which
> takes the actual endianness of the guest into account.
> For this we change the semantics of VIRTIO_ENDIAN_HOST to return the
> actual endianness of the host (the endianness of kvmtool at compile
> time, really). The actual check in vhost_net now compares this against
> the guest endianness.
> This enables vhost support on ARM and ARM64.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  include/kvm/virtio.h | 9 +++++++--
>  virtio/net.c         | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h
> index 768ee96..66530fd 100644
> --- a/include/kvm/virtio.h
> +++ b/include/kvm/virtio.h
> @@ -17,10 +17,15 @@
>  #define VIRTIO_PCI_O_CONFIG	0
>  #define VIRTIO_PCI_O_MSIX	1
>  
> -#define VIRTIO_ENDIAN_HOST	0
>  #define VIRTIO_ENDIAN_LE	(1 << 0)
>  #define VIRTIO_ENDIAN_BE	(1 << 1)
>  
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define VIRTIO_ENDIAN_HOST VIRTIO_ENDIAN_LE
> +#else
> +#define VIRTIO_ENDIAN_HOST VIRTIO_ENDIAN_BE
> +#endif

pci.h already uses __BYTE_ORDER and __LITTLE_ENDIAN, so we should at least
be consistent here.

Will