[PATCH 0/5] kvmtool: Misc fixes

[PATCH 0/5] kvmtool: Misc fixes

[Andreas Herrmann]

Hi Will,

Following some patches to fix misc issues found when testing the
standalone kvmtool version.

Please apply.


Thanks,

Andreas

[PATCH 1/5] kvmtool: Fix compile error on MIPS

[Andreas Herrmann]

When including asm-generic/types.h instead of asm/types.h it results
in conflicting types for __s64 et al (at least with my
toolchain). Other header files are including asm/types.h
(e.g. include/kvm/ioport.h) and types defined there don't necessarily
match the types defined in asm-generic/types.h.

Signed-off-by: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>
---
 include/linux/types.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/types.h b/include/linux/types.h
index 17807b8..5e20f10 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -3,7 +3,7 @@
 
 #include <kvm/compiler.h>
 #define __SANE_USERSPACE_TYPES__	/* For PPC64, to get LL64 types */
-#include <asm-generic/types.h>
+#include <asm/types.h>
 
 typedef __u64 u64;
 typedef __s64 s64;
-- 
1.7.9.5

[PATCH 2/5] kvmtool: Fix regression introduced with d2a7ddff4

[Andreas Herrmann]

Since commit d2a7ddff4 (Add minimal support for macvtap) opening
of tap device might fail. lkvm shows

  Warning: Config tap device error. Are you root?

virtio_net_request_tap passed wrong pointer for struct ifreq to
TUNSETIFF ioctl.

Signed-off-by: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>
---
 virtio/net.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virtio/net.c b/virtio/net.c
index d59a0d5..122a9e2 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -287,7 +287,7 @@ static int virtio_net_request_tap(struct net_dev *ndev, struct ifreq *ifr,
 	if (tapname)
 		strncpy(ifr->ifr_name, tapname, sizeof(ifr->ifr_name));
 
-	ret = ioctl(ndev->tap_fd, TUNSETIFF, &ifr);
+	ret = ioctl(ndev->tap_fd, TUNSETIFF, ifr);
 
 	if (ret >= 0)
 		strncpy(ndev->tap_name, ifr->ifr_name, sizeof(ndev->tap_name));
-- 
1.7.9.5

[PATCH 3/5] kvmtool: Register each guest memory bank as vhost_memory_region

[Andreas Herrmann]

Otherwise vhost does not work if a virtio descriptor is used that was
allocated from a guest memory bank not registered as
vhost_memory_region.

Signed-off-by: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>
---
 virtio/net.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/virtio/net.c b/virtio/net.c
index 122a9e2..9784520 100644
--- a/virtio/net.c
+++ b/virtio/net.c
@@ -639,23 +639,28 @@ static struct virtio_ops net_dev_virtio_ops = (struct virtio_ops) {
 
 static void virtio_net__vhost_init(struct kvm *kvm, struct net_dev *ndev)
 {
+	struct kvm_mem_bank *bank;
 	struct vhost_memory *mem;
-	int r;
+	int r, i;
 
 	ndev->vhost_fd = open("/dev/vhost-net", O_RDWR);
 	if (ndev->vhost_fd < 0)
 		die_perror("Failed openning vhost-net device");
 
-	mem = calloc(1, sizeof(*mem) + sizeof(struct vhost_memory_region));
+	mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
 	if (mem == NULL)
 		die("Failed allocating memory for vhost memory map");
 
-	mem->nregions = 1;
-	mem->regions[0] = (struct vhost_memory_region) {
-		.guest_phys_addr	= 0,
-		.memory_size		= kvm->ram_size,
-		.userspace_addr		= (unsigned long)kvm->ram_start,
-	};
+	i = 0;
+	list_for_each_entry(bank, &kvm->mem_banks, list) {
+		mem->regions[i] = (struct vhost_memory_region) {
+			.guest_phys_addr = bank->guest_phys_addr,
+			.memory_size	 = bank->size,
+			.userspace_addr	 = (unsigned long)bank->host_addr,
+		};
+		i++;
+	}
+	mem->nregions = i;
 
 	r = ioctl(ndev->vhost_fd, VHOST_SET_OWNER);
 	if (r != 0)
-- 
1.7.9.5

[PATCH 4/5] kvmtool: Save datamatch as little endian in {add,del}_event

[Andreas Herrmann]

W/o dedicated endianess it's impossible to find reliably a match
e.g. in kernel/virt/kvm/eventfd.c ioeventfd_in_range.

Signed-off-by: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>
---
 ioeventfd.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ioeventfd.c b/ioeventfd.c
index bce6861..a724baa 100644
--- a/ioeventfd.c
+++ b/ioeventfd.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/kvm.h>
 #include <linux/types.h>
+#include <linux/byteorder.h>
 
 #include "kvm/ioeventfd.h"
 #include "kvm/kvm.h"
@@ -140,7 +141,7 @@ int ioeventfd__add_event(struct ioevent *ioevent, int flags)
 	kvm_ioevent = (struct kvm_ioeventfd) {
 		.addr		= ioevent->io_addr,
 		.len		= ioevent->io_len,
-		.datamatch	= ioevent->datamatch,
+		.datamatch	= cpu_to_le64(ioevent->datamatch),
 		.fd		= event,
 		.flags		= KVM_IOEVENTFD_FLAG_DATAMATCH,
 	};
@@ -199,7 +200,7 @@ int ioeventfd__del_event(u64 addr, u64 datamatch)
 	kvm_ioevent = (struct kvm_ioeventfd) {
 		.addr			= ioevent->io_addr,
 		.len			= ioevent->io_len,
-		.datamatch		= ioevent->datamatch,
+		.datamatch		= cpu_to_le64(ioevent->datamatch),
 		.flags			= KVM_IOEVENTFD_FLAG_PIO
 					| KVM_IOEVENTFD_FLAG_DEASSIGN
 					| KVM_IOEVENTFD_FLAG_DATAMATCH,
-- 
1.7.9.5

Re: [PATCH 4/5] kvmtool: Save datamatch as little endian in {add,del}_event

[Will Deacon]

On Mon, Jun 15, 2015 at 12:49:45PM +0100, Andreas Herrmann wrote:
> W/o dedicated endianess it's impossible to find reliably a match
> e.g. in kernel/virt/kvm/eventfd.c ioeventfd_in_range.

Hmm, but shouldn't this be the endianness of the guest, rather than just
forcing things to little-endian?

Will

Re: [PATCH 4/5] kvmtool: Save datamatch as little endian in {add,del}_event

[Andreas Herrmann]

On Tue, Jun 16, 2015 at 06:17:14PM +0100, Will Deacon wrote:
> On Mon, Jun 15, 2015 at 12:49:45PM +0100, Andreas Herrmann wrote:
> > W/o dedicated endianess it's impossible to find reliably a match
> > e.g. in kernel/virt/kvm/eventfd.c ioeventfd_in_range.
> 
> Hmm, but shouldn't this be the endianness of the guest, rather than just
> forcing things to little-endian?

With my patch and following adaption to
ioeventfd_in_range (in virt/kvm/eventfd.c):

        switch (len) {
	case 1:
                _val = *(u8 *)val;
                break;
        case 2:
                _val = le16_to_cpu(*(u16 *)val);
                break;
	case 4:
                _val = le32_to_cpu(*(u32 *)val);
                break;
        case 8:
                _val = le64_to_cpu(*(u64 *)val);
		        break;
        default:
                return false;
		}

        return _val == le64_to_cpu(p->datamatch) ? true : false;

datamatch is properly evaluated on either endianess.

The current code in ioeventfd_in_range looks fragile to me (for big
endian systems) and didn't work with kvmtool:

        switch (len) {
        case 1:
                _val = *(u8 *)val;
                break;
        case 2:
                _val = *(u16 *)val;
                break;
        case 4:
                _val = *(u32 *)val;
                break;
        case 8:
                _val = *(u64 *)val;
                break;
        default:
                return false;
        }

        return _val == p->datamatch ? true : false;

But now I see, w/o a correponding kernel change the patch shouldn't
be merged.


Andreas

Re: [PATCH 4/5] kvmtool: Save datamatch as little endian in {add,del}_event

[Will Deacon]

On Wed, Jun 17, 2015 at 08:17:49AM +0100, Andreas Herrmann wrote:
> On Tue, Jun 16, 2015 at 06:17:14PM +0100, Will Deacon wrote:
> > On Mon, Jun 15, 2015 at 12:49:45PM +0100, Andreas Herrmann wrote:
> > > W/o dedicated endianess it's impossible to find reliably a match
> > > e.g. in kernel/virt/kvm/eventfd.c ioeventfd_in_range.
> > 
> > Hmm, but shouldn't this be the endianness of the guest, rather than just
> > forcing things to little-endian?
> 
> With my patch and following adaption to
> ioeventfd_in_range (in virt/kvm/eventfd.c):

[...]

> But now I see, w/o a correponding kernel change the patch shouldn't
> be merged.

Digging a bit deeper, I think it's up to the architecture KVM backend
(in the kernel) to present the mmio buffer to core kvm in the host
endianness.

For example, on ARM, we honour the endianness of the vcpu in
vcpu_data_guest_to_host when we populate the buffer for kvm_io_bus_write
(which is what ends up in the ioeventfd code).

I couldn't find equivalent code for MIPs, but I may have been looking in
the wrong place.

Will

[PATCH 5/5] kvmtool: Fix length of ioevent for VIRTIO_PCI_QUEUE_NOTIFY

[Andreas Herrmann]

VIRTIO_PCI_QUEUE_NOTIFY is 16-bit and iowrite16 is used in
drivers/virtio/virtio_pci.c to notify the other side.

If the size doesn't match notification via mmio write will fail.

Signed-off-by: Andreas Herrmann <andreas.herrmann@caviumnetworks.com>
---
 virtio/pci.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virtio/pci.c b/virtio/pci.c
index 7556239..aa9bf09 100644
--- a/virtio/pci.c
+++ b/virtio/pci.c
@@ -57,7 +57,7 @@ static int virtio_pci__init_ioeventfd(struct kvm *kvm, struct virtio_device *vde
 
 	/* mmio */
 	ioevent.io_addr	= vpci->mmio_addr + VIRTIO_PCI_QUEUE_NOTIFY;
-	ioevent.io_len	= sizeof(u32);
+	ioevent.io_len	= sizeof(u16);
 	ioevent.fd	= fds[1] = eventfd(0, 0);
 	r = ioeventfd__add_event(&ioevent, flags);
 	if (r)
-- 
1.7.9.5

Re: [PATCH 0/5] kvmtool: Misc fixes

[Will Deacon]

On Mon, Jun 15, 2015 at 12:49:41PM +0100, Andreas Herrmann wrote:
> Following some patches to fix misc issues found when testing the
> standalone kvmtool version.
> 
> Please apply.

All applied, apart from the ioeventfd patch which I'm not sure about.

Will