[PATCH 1/4] kvm tools: Use GSI routing

[PATCH 1/4] kvm tools: Use GSI routing

[Sasha Levin]

Map GSIs manually when starting the guest.
This will allow us mapping new GSIs for MSIX in the future.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/builtin-run.c     |    3 ++
 tools/kvm/include/kvm/irq.h |    3 ++
 tools/kvm/irq.c             |   75 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index 941129c..f9c3e7e 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -35,6 +35,7 @@
 #include <kvm/vnc.h>
 #include <kvm/sdl.h>
 #include <kvm/framebuffer.h>
+#include <kvm/irq.h>
 
 /* header files for gitish interface  */
 #include <kvm/builtin-run.h>
@@ -573,6 +574,8 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 
 	kvm = kvm__init(kvm_dev, ram_size, guest_name);
 
+	irq__init();
+
 	kvm->single_step = single_step;
 
 	ioeventfd__init();
diff --git a/tools/kvm/include/kvm/irq.h b/tools/kvm/include/kvm/irq.h
index 7a75a0c..2f00b18 100644
--- a/tools/kvm/include/kvm/irq.h
+++ b/tools/kvm/include/kvm/irq.h
@@ -21,4 +21,7 @@ int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line);
 
 struct rb_node *irq__get_pci_tree(void);
 
+void irq__init(void);
+u32 irq__add_msix_route(u32 low, u32 high, u32 data);
+
 #endif
diff --git a/tools/kvm/irq.c b/tools/kvm/irq.c
index 15f4702..fc5d800 100644
--- a/tools/kvm/irq.c
+++ b/tools/kvm/irq.c
@@ -1,16 +1,47 @@
 #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>
 
+#define IRQ_MAX_GSI			64
+#define IRQCHIP_MASTER			0
+#define IRQCHIP_SLAVE			1
+#define IRQCHIP_IOAPIC			2
+
 static u8		next_line	= 3;
 static u8		next_dev	= 1;
 static struct rb_root	pci_tree	= RB_ROOT;
 
+/* First 24 GSIs are routed between IRQCHIPs and IOAPICs */
+static u32 gsi = 24;
+
+extern struct kvm *kvm;
+
+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;
+
+	irq_routing->entries[irq_routing->nr++] = (struct kvm_irq_routing_entry) {
+		.gsi = gsi,
+		.type = type,
+		.u.irqchip.irqchip = irqchip,
+		.u.irqchip.pin = pin,
+	};
+
+	return 0;
+}
+
 static struct pci_dev *search(struct rb_root *root, u32 id)
 {
 	struct rb_node *node = root->rb_node;
@@ -106,6 +137,50 @@ int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)
 	return -1;
 }
 
+void irq__init(void)
+{
+	int i, r;
+
+	irq_routing = malloc(sizeof(struct kvm_irq_routing) +
+			IRQ_MAX_GSI * sizeof(struct kvm_irq_routing_entry));
+
+	/* Hook first 8 GSIs to master IRQCHIP */
+	for (i = 0; i < 8; i++)
+		irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_MASTER, i);
+
+	/* Hook next 8 GSIs to slave IRQCHIP */
+	for (i = 8; i < 16; i++)
+		irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_SLAVE, 8-i);
+
+	/* Last but not least, IOAPIC */
+	for (i = 0; i < 24; i++) {
+		if (i == 0)
+			irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_IOAPIC, 2);
+		else
+			irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_IOAPIC, i);
+	}
+
+	r = ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
+	if (r)
+		die("Failed setting GSI routes");
+}
+
+u32 irq__add_msix_route(u32 low, u32 high, u32 data)
+{
+	irq_routing->entries[irq_routing->nr++] = (struct kvm_irq_routing_entry) {
+		.gsi = gsi,
+		.type = KVM_IRQ_ROUTING_MSI,
+		.u.msi.address_lo = low,
+		.u.msi.address_hi = high,
+		.u.msi.data = data,
+	};
+
+	if (ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing))
+		return -1;
+
+	return gsi++;
+}
+
 struct rb_node *irq__get_pci_tree(void)
 {
 	return rb_first(&pci_tree);
-- 
1.7.6

[PATCH 2/4] kvm tools: Fix PCI probing

[Sasha Levin]

PCI BAR probing is done in four steps:

 1. Read address (and flags).
 2. Mask BAR.
 3. Read BAR again - Now the expected result is the size of the BAR.
 4. Mask BAR with address.

So far, we have only took care of the first step. This means that the kernel
was using address as the size, causing a PCI allocation blunder.

This patch fixes the issue by passing a proper size after masking.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/pci.h |    1 +
 tools/kvm/pci.c             |   57 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/tools/kvm/include/kvm/pci.h b/tools/kvm/include/kvm/pci.h
index 6ad4426..a7532e3 100644
--- a/tools/kvm/include/kvm/pci.h
+++ b/tools/kvm/include/kvm/pci.h
@@ -51,5 +51,6 @@ struct pci_device_header {
 
 void pci__init(void);
 void pci__register(struct pci_device_header *dev, u8 dev_num);
+u32 pci_get_io_space_block(void);
 
 #endif /* KVM__PCI_H */
diff --git a/tools/kvm/pci.c b/tools/kvm/pci.c
index a1ad8ba..799536e3 100644
--- a/tools/kvm/pci.c
+++ b/tools/kvm/pci.c
@@ -5,11 +5,23 @@
 #include <assert.h>
 
 #define PCI_MAX_DEVICES			256
+#define PCI_IO_SIZE			0x100
 
 static struct pci_device_header		*pci_devices[PCI_MAX_DEVICES];
 
 static struct pci_config_address	pci_config_address;
 
+/* This is within our PCI gap */
+static u32 io_space_blocks		= 0xE1000000;
+
+u32 pci_get_io_space_block(void)
+{
+	u32 block = io_space_blocks;
+	io_space_blocks += PCI_IO_SIZE;
+
+	return block;
+}
+
 static void *pci_config_address_ptr(u16 port)
 {
 	unsigned long offset;
@@ -44,11 +56,6 @@ static struct ioport_operations pci_config_address_ops = {
 	.io_out		= pci_config_address_out,
 };
 
-static bool pci_config_data_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
-{
-	return true;
-}
-
 static bool pci_device_exists(u8 bus_number, u8 device_number, u8 function_number)
 {
 	struct pci_device_header *dev;
@@ -67,6 +74,46 @@ static bool pci_device_exists(u8 bus_number, u8 device_number, u8 function_numbe
 	return dev != NULL;
 }
 
+static bool pci_config_data_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
+{
+	unsigned long start;
+	u8 dev_num;
+
+	/*
+	 * If someone accesses PCI configuration space offsets that are not
+	 * aligned to 4 bytes, it uses ioports to signify that.
+	 */
+	start = port - PCI_CONFIG_DATA;
+
+	dev_num		= pci_config_address.device_number;
+
+	if (pci_device_exists(0, dev_num, 0)) {
+		unsigned long offset;
+
+		offset = start + (pci_config_address.register_number << 2);
+		if (offset < sizeof(struct pci_device_header)) {
+			void *p = pci_devices[dev_num];
+			u32 sz = PCI_IO_SIZE;
+
+			/*
+			 * If the kernel masks the BAR it would expect to find the
+			 * size of the BAR there next time it reads from it.
+			 * When the kernel got the size it would write the address
+			 * back.
+			 */
+			if ((offset >= offsetof(struct pci_device_header, bar[0])) &&
+			(offset <= offsetof(struct pci_device_header, bar[6]))) {
+				if (*(u32 *)(p + offset))
+					memcpy(p + offset, &sz, sizeof(sz));
+			} else if (*(u32 *)(p + offset)) {
+				memcpy(p + offset, data, size);
+			}
+		}
+	}
+
+	return true;
+}
+
 static bool pci_config_data_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
 {
 	unsigned long start;
-- 
1.7.6

[PATCH 3/4] kvm tools: Add a void ptr to be passed to mmio callback

[Sasha Levin]

This makes MMIO callback similar to it's PIO counterpart by passing
a void* value provided in the registration to the callback function.

This allows to keep context within the MMIO callback function.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/kvm.h |    2 +-
 tools/kvm/mmio.c            |    8 +++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index eeb55b6..5f3cbbf 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -64,7 +64,7 @@ void kvm__irq_line(struct kvm *kvm, int irq, int level);
 bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int size, u32 count);
 bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_write);
 void kvm__register_mem(struct kvm *kvm, u64 guest_phys, u64 size, void *userspace_addr);
-bool kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write));
+bool kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr), void *ptr);
 bool kvm__deregister_mmio(struct kvm *kvm, u64 phys_addr);
 void kvm__pause(void);
 void kvm__continue(void);
diff --git a/tools/kvm/mmio.c b/tools/kvm/mmio.c
index 64bef37..9a7f84a 100644
--- a/tools/kvm/mmio.c
+++ b/tools/kvm/mmio.c
@@ -14,7 +14,8 @@
 
 struct mmio_mapping {
 	struct rb_int_node	node;
-	void			(*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write);
+	void			(*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
+	void			*ptr;
 };
 
 static struct rb_root mmio_tree = RB_ROOT;
@@ -55,7 +56,7 @@ static const char *to_direction(u8 is_write)
 	return "read";
 }
 
-bool kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write))
+bool kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr), void *ptr)
 {
 	struct mmio_mapping *mmio;
 	struct kvm_coalesced_mmio_zone zone;
@@ -68,6 +69,7 @@ bool kvm__register_mmio(struct kvm *kvm, u64 phys_addr, u64 phys_addr_len, void
 	*mmio = (struct mmio_mapping) {
 		.node = RB_INT_INIT(phys_addr, phys_addr + phys_addr_len),
 		.kvm_mmio_callback_fn = kvm_mmio_callback_fn,
+		.ptr	= ptr,
 	};
 
 	zone = (struct kvm_coalesced_mmio_zone) {
@@ -120,7 +122,7 @@ bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_
 	mmio = mmio_search(&mmio_tree, phys_addr, len);
 
 	if (mmio)
-		mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write);
+		mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write, mmio->ptr);
 	else
 		fprintf(stderr, "Warning: Ignoring MMIO %s at %016llx (length %u)\n",
 			to_direction(is_write), phys_addr, len);
-- 
1.7.6

[PATCH 4/4] kvm tools: Implement MSI-X for virtio-rng

[Sasha Levin]

This patch implements basic MSI-X support for virtio-rng.

The device uses the virtio preferred method of working with MSI-X by
creating one vector for configuration and one vector for each vq in the
device.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/pci.h |   16 +++++++++++++
 tools/kvm/virtio/rng.c      |   53 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/tools/kvm/include/kvm/pci.h b/tools/kvm/include/kvm/pci.h
index a7532e3..27fa349 100644
--- a/tools/kvm/include/kvm/pci.h
+++ b/tools/kvm/include/kvm/pci.h
@@ -24,6 +24,21 @@ struct pci_config_address {
 	unsigned	enable_bit	: 1;		/* 31       */
 };
 
+struct msix_table {
+	u32 low;
+	u32 high;
+	u32 data;
+	u32 ctrl;
+};
+
+struct msix_cap {
+	u8 cap;
+	u8 next;
+	u16 table_size;
+	u32 table_offset;
+	struct msix_table table[3 * PCI_MSIX_ENTRY_SIZE];
+};
+
 struct pci_device_header {
 	u16		vendor_id;
 	u16		device_id;
@@ -47,6 +62,7 @@ struct pci_device_header {
 	u8		irq_pin;
 	u8		min_gnt;
 	u8		max_lat;
+	struct msix_cap msix;
 };
 
 void pci__init(void);
diff --git a/tools/kvm/virtio/rng.c b/tools/kvm/virtio/rng.c
index 1a7f569..6841623 100644
--- a/tools/kvm/virtio/rng.c
+++ b/tools/kvm/virtio/rng.c
@@ -39,6 +39,8 @@ struct rng_dev {
 	u8			isr;
 	u16			config_vector;
 	int			fd;
+	u32			vq_vector[NUM_VIRT_QUEUES];
+	u32			msix_io_block;
 
 	/* virtio queue */
 	u16			queue_selector;
@@ -81,6 +83,9 @@ static bool virtio_rng_pci_io_in(struct ioport *ioport, struct kvm *kvm, u16 por
 	case VIRTIO_MSI_CONFIG_VECTOR:
 		ioport__write16(data, rdev->config_vector);
 		break;
+	case VIRTIO_MSI_QUEUE_VECTOR:
+		ioport__write16(data, rdev->vq_vector[rdev->queue_selector]);
+		break;
 	default:
 		ret		= false;
 		break;
@@ -109,10 +114,10 @@ static void virtio_rng_do_io(struct kvm *kvm, void *param)
 	struct virt_queue *vq = job->vq;
 	struct rng_dev *rdev = job->rdev;
 
-	while (virt_queue__available(vq)) {
+	while (virt_queue__available(vq))
 		virtio_rng_do_io_request(kvm, rdev, vq);
-		virt_queue__trigger_irq(vq, rdev->pci_hdr.irq_line, &rdev->isr, kvm);
-	}
+
+	kvm__irq_line(kvm, rdev->pci_hdr.irq_line, VIRTIO_IRQ_HIGH);
 }
 
 static bool virtio_rng_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
@@ -125,7 +130,6 @@ static bool virtio_rng_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 po
 	offset = port - rdev->base_addr;
 
 	switch (offset) {
-	case VIRTIO_MSI_QUEUE_VECTOR:
 	case VIRTIO_PCI_GUEST_FEATURES:
 		break;
 	case VIRTIO_PCI_QUEUE_PFN: {
@@ -163,8 +167,21 @@ static bool virtio_rng_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 po
 		rdev->status		= ioport__read8(data);
 		break;
 	case VIRTIO_MSI_CONFIG_VECTOR:
-		rdev->config_vector	= VIRTIO_MSI_NO_VECTOR;
+		rdev->config_vector	= ioport__read16(data);
 		break;
+	case VIRTIO_MSI_QUEUE_VECTOR:
+	{
+		u32 gsi;
+		u32 vec;
+
+		vec = rdev->vq_vector[rdev->queue_selector] = ioport__read16(data);
+
+		gsi = irq__add_msix_route(rdev->pci_hdr.msix.table[vec].low,
+					  rdev->pci_hdr.msix.table[vec].high,
+					  rdev->pci_hdr.msix.table[vec].data);
+		rdev->pci_hdr.irq_line = gsi;
+		break;
+	}
 	default:
 		ret			= false;
 		break;
@@ -185,6 +202,16 @@ static void ioevent_callback(struct kvm *kvm, void *param)
 	thread_pool__do_job(&job->job_id);
 }
 
+static void callback_mmio(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr)
+{
+	struct rng_dev *rdev = ptr;
+	void *table = &rdev->pci_hdr.msix.table;
+	if (is_write)
+		memcpy(table + addr - rdev->msix_io_block, data, len);
+	else
+		memcpy(data, table + addr - rdev->msix_io_block, len);
+}
+
 void virtio_rng__init(struct kvm *kvm)
 {
 	u8 pin, line, dev, i;
@@ -196,7 +223,10 @@ void virtio_rng__init(struct kvm *kvm)
 	if (rdev == NULL)
 		return;
 
+	rdev->msix_io_block = pci_get_io_space_block();
+
 	rdev_base_addr = ioport__register(IOPORT_EMPTY, &virtio_rng_io_ops, IOPORT_SIZE, rdev);
+	kvm__register_mmio(kvm, rdev->msix_io_block, 0x100, callback_mmio, rdev);
 
 	rdev->pci_hdr = (struct pci_device_header) {
 		.vendor_id		= PCI_VENDOR_ID_REDHAT_QUMRANET,
@@ -207,8 +237,21 @@ void virtio_rng__init(struct kvm *kvm)
 		.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
 		.subsys_id		= VIRTIO_ID_RNG,
 		.bar[0]			= rdev_base_addr | PCI_BASE_ADDRESS_SPACE_IO,
+		.bar[1]			= rdev->msix_io_block |
+					PCI_BASE_ADDRESS_SPACE_MEMORY |
+					PCI_BASE_ADDRESS_MEM_TYPE_64,
+		/* bar[2] is the continuation of bar[1] for 64bit addressing */
+		.bar[2]			= 0,
+		.status			= PCI_STATUS_CAP_LIST,
+		.capabilities		= (void *)&rdev->pci_hdr.msix - (void *)&rdev->pci_hdr,
 	};
 
+	rdev->pci_hdr.msix.cap = PCI_CAP_ID_MSIX;
+	rdev->pci_hdr.msix.next = 0;
+	rdev->pci_hdr.msix.table_size = (NUM_VIRT_QUEUES + 1) | PCI_MSIX_FLAGS_ENABLE;
+	rdev->pci_hdr.msix.table_offset = 1; /* Use BAR 1 */
+
+	rdev->config_vector = 0;
 	rdev->base_addr = rdev_base_addr;
 	rdev->fd = open("/dev/urandom", O_RDONLY);
 	if (rdev->fd < 0)
-- 
1.7.6

Re: [PATCH 1/4] kvm tools: Use GSI routing

[Pekka Enberg]

Hi Sasha,

On Thu, Jul 28, 2011 at 12:01 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> Map GSIs manually when starting the guest.
> This will allow us mapping new GSIs for MSIX in the future.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/builtin-run.c     |    3 ++
>  tools/kvm/include/kvm/irq.h |    3 ++
>  tools/kvm/irq.c             |   75 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 0 deletions(-)
>
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index 941129c..f9c3e7e 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -35,6 +35,7 @@
>  #include <kvm/vnc.h>
>  #include <kvm/sdl.h>
>  #include <kvm/framebuffer.h>
> +#include <kvm/irq.h>
>
>  /* header files for gitish interface  */
>  #include <kvm/builtin-run.h>
> @@ -573,6 +574,8 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
>
>        kvm = kvm__init(kvm_dev, ram_size, guest_name);
>
> +       irq__init();
> +
>        kvm->single_step = single_step;
>
>        ioeventfd__init();
> diff --git a/tools/kvm/include/kvm/irq.h b/tools/kvm/include/kvm/irq.h
> index 7a75a0c..2f00b18 100644
> --- a/tools/kvm/include/kvm/irq.h
> +++ b/tools/kvm/include/kvm/irq.h
> @@ -21,4 +21,7 @@ int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line);
>
>  struct rb_node *irq__get_pci_tree(void);
>
> +void irq__init(void);
> +u32 irq__add_msix_route(u32 low, u32 high, u32 data);
> +
>  #endif
> diff --git a/tools/kvm/irq.c b/tools/kvm/irq.c
> index 15f4702..fc5d800 100644
> --- a/tools/kvm/irq.c
> +++ b/tools/kvm/irq.c
> @@ -1,16 +1,47 @@
>  #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>
>
> +#define IRQ_MAX_GSI                    64
> +#define IRQCHIP_MASTER                 0
> +#define IRQCHIP_SLAVE                  1
> +#define IRQCHIP_IOAPIC                 2
> +
>  static u8              next_line       = 3;
>  static u8              next_dev        = 1;
>  static struct rb_root  pci_tree        = RB_ROOT;
>
> +/* First 24 GSIs are routed between IRQCHIPs and IOAPICs */
> +static u32 gsi = 24;
> +
> +extern struct kvm *kvm;

I'd prefer you actually pass this as an argument to irq__init() and
irq__add_msix_route().

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

EINVAL is more appropriate, I think.

> +
> +       irq_routing->entries[irq_routing->nr++] = (struct kvm_irq_routing_entry) {
> +               .gsi = gsi,
> +               .type = type,
> +               .u.irqchip.irqchip = irqchip,
> +               .u.irqchip.pin = pin,

Please indent the assignments.

> +       };
> +
> +       return 0;
> +}
> +
>  static struct pci_dev *search(struct rb_root *root, u32 id)
>  {
>        struct rb_node *node = root->rb_node;
> @@ -106,6 +137,50 @@ int irq__register_device(u32 dev, u8 *num, u8 *pin, u8 *line)
>        return -1;
>  }
>
> +void irq__init(void)
> +{
> +       int i, r;
> +
> +       irq_routing = malloc(sizeof(struct kvm_irq_routing) +
> +                       IRQ_MAX_GSI * sizeof(struct kvm_irq_routing_entry));

We usually die() on out-of-memory. Maybe it doesn't make sense here. Dunno.

> +
> +       /* Hook first 8 GSIs to master IRQCHIP */
> +       for (i = 0; i < 8; i++)
> +               irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_MASTER, i);
> +
> +       /* Hook next 8 GSIs to slave IRQCHIP */
> +       for (i = 8; i < 16; i++)
> +               irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_SLAVE, 8-i);
> +
> +       /* Last but not least, IOAPIC */
> +       for (i = 0; i < 24; i++) {
> +               if (i == 0)
> +                       irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_IOAPIC, 2);
> +               else
> +                       irq__add_routing(i, KVM_IRQ_ROUTING_IRQCHIP, IRQCHIP_IOAPIC, i);
> +       }
> +
> +       r = ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing);
> +       if (r)
> +               die("Failed setting GSI routes");
> +}
> +
> +u32 irq__add_msix_route(u32 low, u32 high, u32 data)
> +{
> +       irq_routing->entries[irq_routing->nr++] = (struct kvm_irq_routing_entry) {
> +               .gsi = gsi,
> +               .type = KVM_IRQ_ROUTING_MSI,
> +               .u.msi.address_lo = low,
> +               .u.msi.address_hi = high,
> +               .u.msi.data = data,

Please indent the assignments.

> +       };
> +
> +       if (ioctl(kvm->vm_fd, KVM_SET_GSI_ROUTING, irq_routing))
> +               return -1;

How will that work when we have 'u32' as the return value type?

> +
> +       return gsi++;
> +}
> +
>  struct rb_node *irq__get_pci_tree(void)
>  {
>        return rb_first(&pci_tree);
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/4] kvm tools: Fix PCI probing

[Pekka Enberg]

On Thu, Jul 28, 2011 at 12:01 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> PCI BAR probing is done in four steps:
>
>  1. Read address (and flags).
>  2. Mask BAR.
>  3. Read BAR again - Now the expected result is the size of the BAR.
>  4. Mask BAR with address.
>
> So far, we have only took care of the first step. This means that the kernel
> was using address as the size, causing a PCI allocation blunder.
>
> This patch fixes the issue by passing a proper size after masking.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/include/kvm/pci.h |    1 +
>  tools/kvm/pci.c             |   57 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/tools/kvm/include/kvm/pci.h b/tools/kvm/include/kvm/pci.h
> index 6ad4426..a7532e3 100644
> --- a/tools/kvm/include/kvm/pci.h
> +++ b/tools/kvm/include/kvm/pci.h
> @@ -51,5 +51,6 @@ struct pci_device_header {
>
>  void pci__init(void);
>  void pci__register(struct pci_device_header *dev, u8 dev_num);
> +u32 pci_get_io_space_block(void);

s/pci_get_io_space_block/pci__get_io_space_block/

>
>  #endif /* KVM__PCI_H */
> diff --git a/tools/kvm/pci.c b/tools/kvm/pci.c
> index a1ad8ba..799536e3 100644
> --- a/tools/kvm/pci.c
> +++ b/tools/kvm/pci.c
> @@ -5,11 +5,23 @@
>  #include <assert.h>
>
>  #define PCI_MAX_DEVICES                        256
> +#define PCI_IO_SIZE                    0x100
>
>  static struct pci_device_header                *pci_devices[PCI_MAX_DEVICES];
>
>  static struct pci_config_address       pci_config_address;
>
> +/* This is within our PCI gap */
> +static u32 io_space_blocks             = 0xE1000000;

The magic number wants to be a constant. Preferably somewhere near
where we specify the PCI gap in.

> +
> +u32 pci_get_io_space_block(void)
> +{
> +       u32 block = io_space_blocks;
> +       io_space_blocks += PCI_IO_SIZE;
> +
> +       return block;
> +}
> +
>  static void *pci_config_address_ptr(u16 port)
>  {
>        unsigned long offset;
> @@ -44,11 +56,6 @@ static struct ioport_operations pci_config_address_ops = {
>        .io_out         = pci_config_address_out,
>  };
>
> -static bool pci_config_data_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
> -{
> -       return true;
> -}
> -
>  static bool pci_device_exists(u8 bus_number, u8 device_number, u8 function_number)
>  {
>        struct pci_device_header *dev;
> @@ -67,6 +74,46 @@ static bool pci_device_exists(u8 bus_number, u8 device_number, u8 function_numbe
>        return dev != NULL;
>  }
>
> +static bool pci_config_data_out(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
> +{
> +       unsigned long start;
> +       u8 dev_num;
> +
> +       /*
> +        * If someone accesses PCI configuration space offsets that are not
> +        * aligned to 4 bytes, it uses ioports to signify that.
> +        */
> +       start = port - PCI_CONFIG_DATA;
> +
> +       dev_num         = pci_config_address.device_number;
> +
> +       if (pci_device_exists(0, dev_num, 0)) {
> +               unsigned long offset;
> +
> +               offset = start + (pci_config_address.register_number << 2);
> +               if (offset < sizeof(struct pci_device_header)) {
> +                       void *p = pci_devices[dev_num];
> +                       u32 sz = PCI_IO_SIZE;
> +
> +                       /*
> +                        * If the kernel masks the BAR it would expect to find the
> +                        * size of the BAR there next time it reads from it.
> +                        * When the kernel got the size it would write the address
> +                        * back.
> +                        */
> +                       if ((offset >= offsetof(struct pci_device_header, bar[0])) &&
> +                       (offset <= offsetof(struct pci_device_header, bar[6]))) {

Maybe add a helper for the bar offset checks?

> +                               if (*(u32 *)(p + offset))
> +                                       memcpy(p + offset, &sz, sizeof(sz));
> +                       } else if (*(u32 *)(p + offset)) {
> +                               memcpy(p + offset, data, size);
> +                       }

That if-else maze is pretty scary and needs to be simplified.

> +               }
> +       }
> +
> +       return true;
> +}
> +
>  static bool pci_config_data_in(struct ioport *ioport, struct kvm *kvm, u16 port, void *data, int size, u32 count)
>  {
>        unsigned long start;
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/4] kvm tools: Implement MSI-X for virtio-rng

[Pekka Enberg]

On Thu, Jul 28, 2011 at 12:01 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> @@ -163,8 +167,21 @@ static bool virtio_rng_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 po
>                rdev->status            = ioport__read8(data);
>                break;
>        case VIRTIO_MSI_CONFIG_VECTOR:
> -               rdev->config_vector     = VIRTIO_MSI_NO_VECTOR;
> +               rdev->config_vector     = ioport__read16(data);
>                break;
> +       case VIRTIO_MSI_QUEUE_VECTOR:
> +       {

Nit: the curly brace should be on the same line as 'case' keyword.

> +               u32 gsi;
> +               u32 vec;
> +

Re: [PATCH 2/4] kvm tools: Fix PCI probing

[Cyrill Gorcunov]

On Thu, Jul 28, 2011 at 12:31:51PM +0300, Pekka Enberg wrote:
> On Thu, Jul 28, 2011 at 12:01 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > PCI BAR probing is done in four steps:
> >
> >  1. Read address (and flags).
> >  2. Mask BAR.
> >  3. Read BAR again - Now the expected result is the size of the BAR.
> >  4. Mask BAR with address.
> >
> > So far, we have only took care of the first step. This means that the kernel
> > was using address as the size, causing a PCI allocation blunder.
> >
> > This patch fixes the issue by passing a proper size after masking.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> >  tools/kvm/include/kvm/pci.h |    1 +
> >  tools/kvm/pci.c             |   57 +++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/kvm/include/kvm/pci.h b/tools/kvm/include/kvm/pci.h
> > index 6ad4426..a7532e3 100644
> > --- a/tools/kvm/include/kvm/pci.h
> > +++ b/tools/kvm/include/kvm/pci.h
> > @@ -51,5 +51,6 @@ struct pci_device_header {
> >
> >  void pci__init(void);
> >  void pci__register(struct pci_device_header *dev, u8 dev_num);
> > +u32 pci_get_io_space_block(void);
> 
> s/pci_get_io_space_block/pci__get_io_space_block/
> 

Pekka, can we drop this idea with double underscopes? iirc perf is about
to drop them too.

Re: [PATCH 3/4] kvm tools: Add a void ptr to be passed to mmio callback

[Cyrill Gorcunov]

On Thu, Jul 28, 2011 at 12:01:54PM +0300, Sasha Levin wrote:
...
>  
>  struct mmio_mapping {
>  	struct rb_int_node	node;
> -	void			(*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write);
> +	void			(*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
> +	void			*ptr;
>  };

I guess no need to name it *that* long, probably simple

struct mmio_mapping {
	struct rb_int_node	node;
	void			(*mmio_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
	void			*ptr;
};
...
>  
>  	if (mmio)
> -		mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write);
> +		mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write, mmio->ptr);

So this would be

	if (mmio)
		mmio->mmio_fn(phys_addr, data, len, is_write, mmio->ptr);

no?

	Cyrill

Re: [PATCH 2/4] kvm tools: Fix PCI probing

[Pekka Enberg]

On Thu, Jul 28, 2011 at 12:38 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > @@ -51,5 +51,6 @@ struct pci_device_header {
>> >
>> >  void pci__init(void);
>> >  void pci__register(struct pci_device_header *dev, u8 dev_num);
>> > +u32 pci_get_io_space_block(void);
>>
>> s/pci_get_io_space_block/pci__get_io_space_block/
>
> Pekka, can we drop this idea with double underscopes? iirc perf is about
> to drop them too.

Ingo, is that so?

I any case, we're not doing that in *this* patch so I'd really prefer
that the inconsistency is fixed. :-)

                        Pekka

Re: [PATCH 1/4] kvm tools: Use GSI routing

[Cyrill Gorcunov]

On Thu, Jul 28, 2011 at 12:01:52PM +0300, Sasha Levin wrote:
> Map GSIs manually when starting the guest.
> This will allow us mapping new GSIs for MSIX in the future.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---

Other than a few nits the series looks good to me, thanks Sasha!

Re: [PATCH 3/4] kvm tools: Add a void ptr to be passed to mmio callback

[Pekka Enberg]

On Thu, Jul 28, 2011 at 12:42 PM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 12:01:54PM +0300, Sasha Levin wrote:
> ...
>>
>>  struct mmio_mapping {
>>       struct rb_int_node      node;
>> -     void                    (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write);
>> +     void                    (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
>> +     void                    *ptr;
>>  };
>
> I guess no need to name it *that* long, probably simple
>
> struct mmio_mapping {
>        struct rb_int_node      node;
>        void                    (*mmio_fn)(u64 addr, u8 *data, u32 len, u8 is_write, void *ptr);
>        void                    *ptr;
> };
> ...
>>
>>       if (mmio)
>> -             mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write);
>> +             mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write, mmio->ptr);
>
> So this would be
>
>        if (mmio)
>                mmio->mmio_fn(phys_addr, data, len, is_write, mmio->ptr);
>
> no?

Makes sense, yes.