KVM: xinterface

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides a mechanism to find and bind to this interface at run-time.

Why: There are various subsystems that would like to interact with a KVM guest which are ideally suited to exist outside the domain of the kvm.ko core logic. For instance, external pci-passthrough, virtual-bus, and virtio-net modules are currently under development. In order for these modules to successfully interact with the guest, they need, at the very least, various interfaces for signaling IO events, pointer translation, and possibly memory mapping.

The signaling case is covered by the recent introduction of the irqfd/ioeventfd mechanisms. This patch provides a mechanism to cover the other cases. Note that today we only expose pointer-translation related functions, but more could be added at a future date as needs arise.

Example usage: QEMU instantiates a guest, and an external module "foo" that desires the ability to interface with the guest (say via open("/dev/foo")). QEMU may then pass the kvmfd to foo via an ioctl, such as: ioctl(foofd, FOO_SET_VMID, &kvmfd). Upon receipt, the foo module can issue kvm_xinterface_bind(kvmfd) to acquire the proper context. Internally, the struct kvm* and associated struct module* will remain pinned at least until the foo module calls kvm_xinterface_put().

virt/kvm/xinterface.c new file +78 -0

@@ -0,0 +1,409 @@
+/*
+ * KVM module interface - Allows external modules to interface with a guest
+ *
+ * Copyright 2009 Novell.  All Rights Reserved.
+ *
+ * Author:
+ *      Gregory Haskins <[email protected]>
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
+#include <linux/highmem.h>
+#include <linux/module.h>
+#include <linux/mmu_context.h>
+#include <linux/kvm_host.h>
+#include <linux/kvm_xinterface.h>
+
+struct _xinterface {
+	struct kvm             *kvm;
+	struct task_struct     *task;
+	struct mm_struct       *mm;
+	struct kvm_xinterface   intf;
+	struct kvm_memory_slot *slotcache[NR_CPUS];
+};
+
+struct _xvmap {
+	struct kvm_memory_slot    *memslot;
+	unsigned long              npages;
+	struct kvm_xvmap           vmap;
+};
+
+static struct _xinterface *
+to_intf(struct kvm_xinterface *intf)
+{
+	return container_of(intf, struct _xinterface, intf);
+}
+
+#define _gfn_to_hva(gfn, memslot) \
+	(memslot->userspace_addr + (gfn - memslot->base_gfn) * PAGE_SIZE)
+
+/*
+ * gpa_to_hva() - translate a guest-physical to host-virtual using
+ * a per-cpu cache of the memslot.
+ *
+ * The gfn_to_memslot() call is relatively expensive, and the gpa access
+ * patterns exhibit a high degree of locality.  Therefore, lets cache
+ * the last slot used on a per-cpu basis to optimize the lookup
+ *
+ * assumes slots_lock held for read
+ */
+static unsigned long
+gpa_to_hva(struct _xinterface *_intf, unsigned long gpa)
+{
+	int                     cpu     = get_cpu();
+	unsigned long           gfn     = gpa >> PAGE_SHIFT;
+	struct kvm_memory_slot *memslot = _intf->slotcache[cpu];
+	unsigned long           addr    = 0;
+
+	if (!memslot
+	    || gfn < memslot->base_gfn
+	    || gfn >= memslot->base_gfn + memslot->npages) {
+
+		memslot = gfn_to_memslot(_intf->kvm, gfn);
+		if (!memslot)
+			goto out;

virt/kvm/xinterface.c new file +4 -0

+
+	page_list = (struct page **) __get_free_page(GFP_KERNEL);
+	if (!page_list)
+		return NULL;

Quoted 114 lines · Gregory Haskins

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides a mechanism to find and bind to this interface at run-time.

Why: There are various subsystems that would like to interact with a KVM guest which are ideally suited to exist outside the domain of the kvm.ko core logic. For instance, external pci-passthrough, virtual-bus, and virtio-net modules are currently under development. In order for these modules to successfully interact with the guest, they need, at the very least, various interfaces for signaling IO events, pointer translation, and possibly memory mapping.

The signaling case is covered by the recent introduction of the irqfd/ioeventfd mechanisms. This patch provides a mechanism to cover the other cases. Note that today we only expose pointer-translation related functions, but more could be added at a future date as needs arise.

Example usage: QEMU instantiates a guest, and an external module "foo" that desires the ability to interface with the guest (say via open("/dev/foo")). QEMU may then pass the kvmfd to foo via an ioctl, such as: ioctl(foofd, FOO_SET_VMID, &kvmfd). Upon receipt, the foo module can issue kvm_xinterface_bind(kvmfd) to acquire the proper context. Internally, the struct kvm* and associated struct module* will remain pinned at least until the foo module calls kvm_xinterface_put().

virt/kvm/xinterface.c new file +83 -0

@@ -0,0 +1,409 @@
+/*
+ * KVM module interface - Allows external modules to interface with a guest
+ *
+ * Copyright 2009 Novell.  All Rights Reserved.
+ *
+ * Author:
+ *      Gregory Haskins <[email protected]>
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
+#include <linux/highmem.h>
+#include <linux/module.h>
+#include <linux/mmu_context.h>
+#include <linux/kvm_host.h>
+#include <linux/kvm_xinterface.h>
+
+struct _xinterface {
+	struct kvm             *kvm;
+	struct task_struct     *task;
+	struct mm_struct       *mm;
+	struct kvm_xinterface   intf;
+	struct kvm_memory_slot *slotcache[NR_CPUS];
+};
+
+struct _xvmap {
+	struct kvm_memory_slot    *memslot;
+	unsigned long              npages;
+	struct kvm_xvmap           vmap;
+};
+
+static struct _xinterface *
+to_intf(struct kvm_xinterface *intf)
+{
+	return container_of(intf, struct _xinterface, intf);
+}
+
+#define _gfn_to_hva(gfn, memslot) \
+	(memslot->userspace_addr + (gfn - memslot->base_gfn) * PAGE_SIZE)
+
+/*
+ * gpa_to_hva() - translate a guest-physical to host-virtual using
+ * a per-cpu cache of the memslot.
+ *
+ * The gfn_to_memslot() call is relatively expensive, and the gpa access
+ * patterns exhibit a high degree of locality.  Therefore, lets cache
+ * the last slot used on a per-cpu basis to optimize the lookup
+ *
+ * assumes slots_lock held for read
+ */
+static unsigned long
+gpa_to_hva(struct _xinterface *_intf, unsigned long gpa)
+{
+	int                     cpu     = get_cpu();
+	unsigned long           gfn     = gpa >> PAGE_SHIFT;
+	struct kvm_memory_slot *memslot = _intf->slotcache[cpu];
+	unsigned long           addr    = 0;
+
+	if (!memslot
+	    || gfn < memslot->base_gfn
+	    || gfn >= memslot->base_gfn + memslot->npages) {
+
+		memslot = gfn_to_memslot(_intf->kvm, gfn);
+		if (!memslot)
+			goto out;
+
+		_intf->slotcache[cpu] = memslot;
+	}
+
+	addr = _gfn_to_hva(gfn, memslot) + offset_in_page(gpa);

virt/kvm/xinterface.c line 88

Quoted 5 lines

virt/kvm/xinterface.c new file +5 -0

+
+out:
+	put_cpu();
+
+	return addr;

Please optimize gfn_to_memslot() instead, so everybody benefits. It shows very often on profiles.

Quoted 5 lines

virt/kvm/xinterface.c new file +4 -0

+
+	page_list = (struct page **) __get_free_page(GFP_KERNEL);
+	if (!page_list)
+		return NULL;

virt/kvm/xinterface.c line 118

Quoted 10 lines

virt/kvm/xinterface.c new file +10 -0

+
+	down_write(&mm->mmap_sem);
+
+	ret = get_user_pages(p, mm, addr, npages, 1, 0, page_list, NULL);
+	if (ret < 0)
+		goto out;
+
+	ptr = vmap(page_list, npages, VM_MAP, PAGE_KERNEL);
+	if (ptr)
+		mm->locked_vm += npages;

Why don't you use gfn_to_page (here and elsewhere in the patch).

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides a mechanism to find and bind to this interface at run-time.

Why: There are various subsystems that would like to interact with a KVM guest which are ideally suited to exist outside the domain of the kvm.ko core logic. For instance, external pci-passthrough, virtual-bus, and virtio-net modules are currently under development. In order for these modules to successfully interact with the guest, they need, at the very least, various interfaces for signaling IO events, pointer translation, and possibly memory mapping.

The signaling case is covered by the recent introduction of the irqfd/ioeventfd mechanisms. This patch provides a mechanism to cover the other cases. Note that today we only expose pointer-translation related functions, but more could be added at a future date as needs arise.

Example usage: QEMU instantiates a guest, and an external module "foo" that desires the ability to interface with the guest (say via open("/dev/foo")). QEMU may then pass the kvmfd to foo via an ioctl, such as: ioctl(foofd, FOO_SET_VMID,&kvmfd). Upon receipt, the foo module can issue kvm_xinterface_bind(kvmfd) to acquire the proper context. Internally, the struct kvm* and associated struct module* will remain pinned at least until the foo module calls kvm_xinterface_put().

virt/kvm/xinterface.c new file +32 -0

+
+	intf->ops->release(intf);
+	module_put(owner);
+}
+
 
+
+/*
+ * gpa_to_hva() - translate a guest-physical to host-virtual using
+ * a per-cpu cache of the memslot.
+ *
+ * The gfn_to_memslot() call is relatively expensive, and the gpa access
+ * patterns exhibit a high degree of locality.  Therefore, lets cache
+ * the last slot used on a per-cpu basis to optimize the lookup
+ *
+ * assumes slots_lock held for read
+ */
+static unsigned long
+gpa_to_hva(struct _xinterface *_intf, unsigned long gpa)
+{
+	int                     cpu     = get_cpu();
+	unsigned long           gfn     = gpa>>  PAGE_SHIFT;
+	struct kvm_memory_slot *memslot = _intf->slotcache[cpu];
+	unsigned long           addr    = 0;
+
+	if (!memslot
+	    || gfn<  memslot->base_gfn
+	    || gfn>= memslot->base_gfn + memslot->npages) {
+
+		memslot = gfn_to_memslot(_intf->kvm, gfn);
+		if (!memslot)
+			goto out;
+

virt/kvm/xinterface.c new file +11 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+		   const void *src, unsigned long n)
+{
+	struct _xinterface *_intf = to_intf(intf);
+	unsigned long dst;
+	bool kthread = !current->mm;
+
+	down_read(&_intf->kvm->slots_lock);
+
+	dst = gpa_to_hva(_intf, gpa);

Quoted 6 lines · Gregory Haskins

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides a mechanism to find and bind to this interface at run-time.

If this is needed, it should be done as a virt_address_space to which kvm and other modules bind, instead of as something that kvm exports and other modules import. The virt_address_space can be identified by an fd and passed around to kvm and other modules.

Quoted 14 lines

Why: There are various subsystems that would like to interact with a KVM guest which are ideally suited to exist outside the domain of the kvm.ko core logic. For instance, external pci-passthrough, virtual-bus, and virtio-net modules are currently under development. In order for these modules to successfully interact with the guest, they need, at the very least, various interfaces for signaling IO events, pointer translation, and possibly memory mapping.

The signaling case is covered by the recent introduction of the irqfd/ioeventfd mechanisms. This patch provides a mechanism to cover the other cases. Note that today we only expose pointer-translation related functions, but more could be added at a future date as needs arise.

Quoted 10 lines

Example usage: QEMU instantiates a guest, and an external module "foo" that desires the ability to interface with the guest (say via open("/dev/foo")). QEMU may then pass the kvmfd to foo via an ioctl, such as: ioctl(foofd, FOO_SET_VMID,&kvmfd). Upon receipt, the foo module can issue kvm_xinterface_bind(kvmfd) to acquire the proper context. Internally, the struct kvm* and associated struct module* will remain pinned at least until the foo module calls kvm_xinterface_put().

So, under my suggestion above, you'd call sys_create_virt_address_space(), populate it, and pass the result to kvm and to foo. This allows the use of virt_address_space without kvm and doesn't require foo to interact with kvm.

include/linux/kvm_xinterface.h line 24

Quoted 11 lines

include/linux/kvm_xinterface.h new file +9 -0

+struct kvm_xinterface_ops {
+    unsigned long (*copy_to)(struct kvm_xinterface *intf,
+                 unsigned long gpa, const void *src,
+                 unsigned long len);
+    unsigned long (*copy_from)(struct kvm_xinterface *intf, void *dst,
+                   unsigned long gpa, unsigned long len);
+    struct kvm_xvmap* (*vmap)(struct kvm_xinterface *intf,
+                  unsigned long gpa,
+                  unsigned long len);

How would vmap() work with live migration?

include/linux/kvm_xinterface.h line 49

Quoted 13 lines

include/linux/kvm_xinterface.h new file +11 -0

+
+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 41 lines

virt/kvm/xinterface.c new file +39 -0

+
+    intf->ops->release(intf);
+    module_put(owner);
+}
+
 
+
+/*
+ * gpa_to_hva() - translate a guest-physical to host-virtual using
+ * a per-cpu cache of the memslot.
+ *
+ * The gfn_to_memslot() call is relatively expensive, and the gpa access
+ * patterns exhibit a high degree of locality.  Therefore, lets cache
+ * the last slot used on a per-cpu basis to optimize the lookup
+ *
+ * assumes slots_lock held for read
+ */
+static unsigned long
+gpa_to_hva(struct _xinterface *_intf, unsigned long gpa)
+{
+    int                     cpu     = get_cpu();
+    unsigned long           gfn     = gpa>>  PAGE_SHIFT;
+    struct kvm_memory_slot *memslot = _intf->slotcache[cpu];
+    unsigned long           addr    = 0;
+
+    if (!memslot
+        || gfn<  memslot->base_gfn
+        || gfn>= memslot->base_gfn + memslot->npages) {
+
+        memslot = gfn_to_memslot(_intf->kvm, gfn);
+        if (!memslot)
+            goto out;
+
+        _intf->slotcache[cpu] = memslot;
+    }
+
+    addr = _gfn_to_hva(gfn, memslot) + offset_in_page(gpa);
+
+out:
+    put_cpu();

virt/kvm/xinterface.c line 89

Quoted 4 lines

virt/kvm/xinterface.c new file +3 -0

+
+    return addr;
+}

A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better results.

Quoted 17 lines

virt/kvm/xinterface.c new file +16 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+           const void *src, unsigned long n)
+{
+    struct _xinterface *_intf = to_intf(intf);
+    unsigned long dst;
+    bool kthread = !current->mm;
+
+    down_read(&_intf->kvm->slots_lock);
+
+    dst = gpa_to_hva(_intf, gpa);
+    if (!dst)
+        goto out;
+
+    if (kthread)
+        use_mm(_intf->mm);

virt/kvm/xinterface.c line 221

Quoted 6 lines

virt/kvm/xinterface.c new file +5 -0

+
+    if (kthread || _intf->mm == current->mm)
+        n = copy_to_user((void *)dst, src, n);
+    else
+        n = _slow_copy_to_user(_intf, dst, src, n);

Can't you switch the mm temporarily instead of this?

Quoted 9 lines · Avi Kivity

Quoted 7 lines · Gregory Haskins

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides a mechanism to find and bind to this interface at run-time.

Quoted 5 lines

If this is needed, it should be done as a virt_address_space to which kvm and other modules bind, instead of as something that kvm exports and other modules import. The virt_address_space can be identified by an fd and passed around to kvm and other modules.

IIUC, what you are proposing is something similar to generalizing the vbus::memctx object. I had considered doing something like that in the early design phase of vbus, but then decided it would be a hard-sell to the mm crowd, and difficult to generalize.

What do you propose as the interface to program the object?

Quoted 5 lines

So, under my suggestion above, you'd call sys_create_virt_address_space(), populate it, and pass the result to kvm and to foo. This allows the use of virt_address_space without kvm and doesn't require foo to interact with kvm.

The problem I see here is that the only way I can think to implement this generally is something that looks very kvm-esque (slots-to-pages kind of translation). Is there a way you can think of that does not involve a kvm.ko originated vtable that is also not kvm centric?

Quoted 11 lines

Quoted 11 lines

include/linux/kvm_xinterface.h new file +9 -0

+struct kvm_xinterface_ops {
+    unsigned long (*copy_to)(struct kvm_xinterface *intf,
+                 unsigned long gpa, const void *src,
+                 unsigned long len);
+    unsigned long (*copy_from)(struct kvm_xinterface *intf, void *dst,
+                   unsigned long gpa, unsigned long len);
+    struct kvm_xvmap* (*vmap)(struct kvm_xinterface *intf,
+                  unsigned long gpa,
+                  unsigned long len);

Quoted 2 lines

How would vmap() work with live migration?

vmap represents shmem regions, and is a per-guest-instance resource. So my plan there is that the new and old guest instance would each have the vmap region instated at the same GPA location (assumption: gpas are stable across migration), and any state relevant data local to the shmem (like ring head/tail position) is conveyed in the serialized stream for the device model.

Quoted 6 lines

Quoted 6 lines

include/linux/kvm_xinterface.h new file +6 -0

+
+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;

Quoted 9 lines

include/linux/kvm_xinterface.h line 49

Quoted 7 lines

include/linux/kvm_xinterface.h new file +5 -0

+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

Quoted 4 lines

A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better results.

per-vcpu will not work well here, unfortunately, since this is an external interface mechanism. The callers will generally be from a kthread or some other non-vcpu related context. Even if we could figure out a vcpu to use as a basis, we would require some kind of heavier-weight synchronization which would not be as desirable.

Therefore, I opted to go per-cpu and use the presumably lighterweight get_cpu/put_cpu() instead.

Quoted 16 lines

Quoted 16 lines

virt/kvm/xinterface.c new file +16 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+           const void *src, unsigned long n)
+{
+    struct _xinterface *_intf = to_intf(intf);
+    unsigned long dst;
+    bool kthread = !current->mm;
+
+    down_read(&_intf->kvm->slots_lock);
+
+    dst = gpa_to_hva(_intf, gpa);
+    if (!dst)
+        goto out;
+
+    if (kthread)
+        use_mm(_intf->mm);

Quoted 9 lines

virt/kvm/xinterface.c line 221

Quoted 7 lines

virt/kvm/xinterface.c new file +5 -0

+
+    if (kthread || _intf->mm == current->mm)
+        n = copy_to_user((void *)dst, src, n);
+    else
+        n = _slow_copy_to_user(_intf, dst, src, n);

Can't you switch the mm temporarily instead of this?

Thats actually what I do for the fast-path (use_mm() does a switch_to() internally).

The slow-path is only there for completeness for when switching is not possible (such as if called with an mm already active i.e. process-context).

In practice, however, this doesnt happen. Virtually
100% of the calls in vbus hit the fast-path here, and I suspect most xinterface clients would find the same conditions as well.

Quoted 22 lines · Gregory Haskins

Quoted 10 lines · Avi Kivity

Quoted 8 lines · Gregory Haskins

What: xinterface is a mechanism that allows kernel modules external to the kvm.ko proper to interface with a running guest. It accomplishes this by creating an abstracted interface which does not expose any private details of the guest or its related KVM structures, and provides
a mechanism to find and bind to this interface at run-time.

Quoted 5 lines

If this is needed, it should be done as a virt_address_space to which kvm and other modules bind, instead of as something that kvm exports and other modules import. The virt_address_space can be identified by an fd and passed around to kvm and other modules.

IIUC, what you are proposing is something similar to generalizing the vbus::memctx object. I had considered doing something like that in the early design phase of vbus, but then decided it would be a hard-sell to the mm crowd, and difficult to generalize.

Quoted 2 lines

What do you propose as the interface to program the object?

Something like the current kvm interfaces, de-warted. It will be a hard sell indeed, for good reasons.

Quoted 6 lines

Quoted 6 lines

So, under my suggestion above, you'd call sys_create_virt_address_space(), populate it, and pass the result to kvm and to foo. This allows the use of virt_address_space without kvm and doesn't require foo to interact with kvm.

Quoted 5 lines

The problem I see here is that the only way I can think to implement this generally is something that looks very kvm-esque (slots-to-pages kind of translation). Is there a way you can think of that does not involve a kvm.ko originated vtable that is also not kvm centric?

slots would be one implementation, if you can think of others then you'd add them.

If you can't, I think it indicates that the whole thing isn't necessary and we're better off with slots and virtual memory.

The only thing missing is dma, which you don't deal with anyway.

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

include/linux/kvm_xinterface.h new file +9 -0

+struct kvm_xinterface_ops {
+    unsigned long (*copy_to)(struct kvm_xinterface *intf,
+                 unsigned long gpa, const void *src,
+                 unsigned long len);
+    unsigned long (*copy_from)(struct kvm_xinterface *intf, void *dst,
+                   unsigned long gpa, unsigned long len);
+    struct kvm_xvmap* (*vmap)(struct kvm_xinterface *intf,
+                  unsigned long gpa,
+                  unsigned long len);

Quoted 9 lines

Quoted 2 lines

How would vmap() work with live migration?

vmap represents shmem regions, and is a per-guest-instance resource. So my plan there is that the new and old guest instance would each have the vmap region instated at the same GPA location (assumption: gpas are stable across migration), and any state relevant data local to the shmem (like ring head/tail position) is conveyed in the serialized stream for the device model.

You'd have to copy the entire range since you don't know what the guest might put there. I guess it's acceptable for small areas.

Quoted 16 lines

Quoted 16 lines

Quoted 4 lines

include/linux/kvm_xinterface.h new file +1 -0

+

include/linux/kvm_xinterface.h line 49

Quoted 10 lines

include/linux/kvm_xinterface.h new file +10 -0

+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 5 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

Quoted 14 lines

Quoted 8 lines

A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better results.

per-vcpu will not work well here, unfortunately, since this is an external interface mechanism. The callers will generally be from a kthread or some other non-vcpu related context. Even if we could figure out a vcpu to use as a basis, we would require some kind of heavier-weight synchronization which would not be as desirable.

Quoted 3 lines

Therefore, I opted to go per-cpu and use the presumably lighterweight get_cpu/put_cpu() instead.

This just assumes a low context switch rate.

How about a gfn_to_pfn_cached(..., struct gfn_to_pfn_cache *cache)? Each user can place it in a natural place.

Quoted 26 lines

Quoted 26 lines

Quoted 14 lines

virt/kvm/xinterface.c new file +11 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+           const void *src, unsigned long n)
+{
+    struct _xinterface *_intf = to_intf(intf);
+    unsigned long dst;
+    bool kthread = !current->mm;
+
+    down_read(&_intf->kvm->slots_lock);
+
+    dst = gpa_to_hva(_intf, gpa);

virt/kvm/xinterface.c line 221

Quoted 10 lines

virt/kvm/xinterface.c new file +10 -0

+    if (!dst)
+        goto out;
+
+    if (kthread)
+        use_mm(_intf->mm);
+
+    if (kthread || _intf->mm == current->mm)
+        n = copy_to_user((void *)dst, src, n);
+    else
+        n = _slow_copy_to_user(_intf, dst, src, n);

Can't you switch the mm temporarily instead of this?

Quoted 6 lines

Thats actually what I do for the fast-path (use_mm() does a switch_to() internally).

The slow-path is only there for completeness for when switching is not possible (such as if called with an mm already active i.e. process-context).

Still, why can't you switch temporarily?

Quoted 5 lines

In practice, however, this doesnt happen. Virtually
100% of the calls in vbus hit the fast-path here, and I suspect most xinterface clients would find the same conditions as well.

So you have 100% untested code here.

Quoted 20 lines · Avi Kivity

Quoted 19 lines · Gregory Haskins

Quoted 17 lines · Avi Kivity

include/linux/kvm_xinterface.h line 49

Quoted 13 lines · Gregory Haskins

include/linux/kvm_xinterface.h new file +11 -0

+
+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 7 lines

Quoted 5 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

include/linux/kvm_xinterface.h new file +1 -0

+

include/linux/kvm_xinterface.h line 49

Quoted 10 lines

include/linux/kvm_xinterface.h new file +10 -0

+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 9 lines

Quoted 6 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

Quoted 7 lines

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we worry about the case where one cpu reorders the read to be after the ->release(), and another cpu might grab the memory that was kfree()'d within the ->release() and scribble something else on it before the read completes.

I know rmb() typically needs to be paired with wmb() to be correct, so you are probably right to say that the rmb() itself is not appropriate.
This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required. As David mentions, perhaps "smp_mb()" is more appropriate for this application. I also speculate barrier() may be all that we need.

Quoted 39 lines · Gregory Haskins

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

(quoted diff) +1 -0

+

Review

Quoted 10 lines

(quoted diff) +10 -0

+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 9 lines

Quoted 6 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

Quoted 7 lines

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we worry about the case where one cpu reorders the read to be after the ->release(), and another cpu might grab the memory that was kfree()'d within the ->release() and scribble something else on it before the read completes.

Quoted 7 lines

I know rmb() typically needs to be paired with wmb() to be correct, so you are probably right to say that the rmb() itself is not appropriate.
This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required. As David mentions, perhaps "smp_mb()" is more appropriate for this application. I also speculate barrier() may be all that we need.

barrier() is the operation for a compiler barrier. But it's unneeded here - unless the compiler can prove that ->release(intf) will not modify intf->owner it is not allowed to move the access afterwards. An indirect function call is generally a barrier() since the compiler can't assume memory has not been modified.

Quoted 47 lines · Avi Kivity

Quoted 46 lines · Gregory Haskins

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

(quoted diff) +1 -0

+

Review

Quoted 10 lines

(quoted diff) +10 -0

+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 9 lines

Quoted 6 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

Quoted 7 lines

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we worry about the case where one cpu reorders the read to be after the ->release(), and another cpu might grab the memory that was kfree()'d within the ->release() and scribble something else on it before the read completes.

I know rmb() typically needs to be paired with wmb() to be correct, so you are probably right to say that the rmb() itself is not appropriate.
This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required. As David mentions, perhaps "smp_mb()" is more appropriate for this application. I also speculate barrier() may be all that we need.

Quoted 6 lines

barrier() is the operation for a compiler barrier. But it's unneeded here - unless the compiler can prove that ->release(intf) will not modify intf->owner it is not allowed to move the access afterwards. An indirect function call is generally a barrier() since the compiler can't assume memory has not been modified.

You're logic

seems reasonable to me. I will defer to David, since he brought up the issue with the similar logic originally.

Kind Regards,
-Greg

Quoted 54 lines · Gregory Haskins

Quoted 53 lines · Avi Kivity

Quoted 39 lines · Gregory Haskins

Quoted 26 lines

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 4 lines

(quoted diff) +1 -0

+

Review

Quoted 10 lines

(quoted diff) +10 -0

+static inline void
+_kvm_xinterface_release(struct kref *kref)
+{
+    struct kvm_xinterface *intf;
+    struct module *owner;
+
+    intf = container_of(kref, struct kvm_xinterface, kref);
+
+    owner = intf->owner;
+    rmb();

Why rmb?

Quoted 9 lines

Quoted 6 lines

the intf->ops->release() line may invalidate the intf pointer, so we want to ensure that the read completes before the release() is called.

TBH: I'm not 100% its needed, but I was being conservative.

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

Quoted 7 lines

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

BTW: In case it is not clear, the rationale as I understand it is we worry about the case where one cpu reorders the read to be after the ->release(), and another cpu might grab the memory that was kfree()'d within the ->release() and scribble something else on it before the read completes.

Quoted 7 lines

I know rmb() typically needs to be paired with wmb() to be correct, so you are probably right to say that the rmb() itself is not appropriate.
This problem in general makes my head hurt, which is why I said I am
not 100% sure of what is required. As David mentions, perhaps "smp_mb()" is more appropriate for this application. I also speculate barrier() may be all that we need.

barrier() is the operation for a compiler barrier. But it's unneeded here - unless the compiler can prove that ->release(intf) will not modify intf->owner it is not allowed to move the access afterwards. An indirect function call is generally a barrier() since the compiler can't assume memory has not been modified.

Quoted 1 line

You're logic

gak. or "your logic" even.

Quoted 6 lines

seems reasonable to me. I will defer to David, since he brought up the issue with the similar logic originally.

Kind Regards,
-Greg

Thinking about this some more over lunch, I think we (Avi and I) might both be wrong (and David is right). Avi is right that we don't need rmb() or barrier() for the reasons already stated, but I think David is right that we need an smp_mb() to ensure the cpu doesn't do the reordering. Otherwise a different cpu could invalidate the memory if it reuses the freed memory in the meantime, iiuc. IOW: its not a compiler issue but a cpu issue.

Or am I still confused?

Quoted 11 lines · Gregory Haskins

Thinking about this some more over lunch, I think we (Avi and I) might both be wrong (and David is right). Avi is right that we don't need rmb() or barrier() for the reasons already stated, but I think David is right that we need an smp_mb() to ensure the cpu doesn't do the reordering. Otherwise a different cpu could invalidate the memory if it reuses the freed memory in the meantime, iiuc. IOW: its not a compiler issue but a cpu issue.

Or am I still confused?

The sequence of operations is:

    v = p->v;
    f();
    // rmb() ?
    g(v);

You are worried that the compiler

or cpu will fetch p->v after f() has executed?

The compiler may not, since it can't tell whether f() might change p->v.

If f() can cause another agent to write to p (by freeing
it to a global list, for example), then it is its responsibility to issue the smp_rmb(), otherwise no calculation that took place before f() and accessed p is safe.

Quoted 7 lines

If f() can cause another agent to write to p (by freeing
it to a global list, for example), then it is its responsibility to issue the smp_rmb(), otherwise no calculation that took place before f() and accessed p is safe.

IOW: David is right. You need a cpu-barrier one way or the other. We can either allow ->release() to imply one (and probably document it that way, like we did for slow-work), or we can be explicit.

I chose to be
explicit since it is kind of self-documenting, and there is no need to be worried about performance since the release is slow-path.

Quoted 4 lines

slots would be one implementation, if you can think of others then you'd add them.

I'm more interested in *how* you'd add them more than "if" we would add them. What I am getting at are the logistics of such a beast.

For instance, would I have /dev/slots-vas with ioctls for adding slots, and /dev/foo-vas for adding foos? And each one would instantiate a different vas_struct object with its own vas_struct->ops? Or were you thinking of something different.

Quoted 3 lines

If you can't, I think it indicates that the whole thing isn't necessary and we're better off with slots and virtual memory.

I'm not sure if we are talking about the same thing yet, but if we are, there are uses of a generalized interface outside of slots/virtual memory (Ira's physical box being a good example).

In any case, I think the best approach is what I already proposed. KVM's arrangement of memory is going to tend to be KVM specific, and what better place to implement the interface than close to the kvm.ko core.

Quoted 3 lines

The only thing missing is dma, which you don't deal with anyway.

Afaict I do support dma in the generalized vbus::memctx, though I do not use it on anything related to KVM or xinterface. Can you elaborate on the problem here? Does the SG interface in 4/4 help get us closer to what you envision?

Quoted 3 lines

You'd have to copy the entire range since you don't know what the guest might put there. I guess it's acceptable for small areas.

? The vmap is presumably part of an ABI between guest and host, so the host should always know what structure is present within the region, and what is relevant from within that structure to migrate once that state is "frozen".

These regions (for vbus, anyway) equate to things like virtqueue metadata, and presumably the same problem exists for virtio-net in userspace as it does here, since that is another form of a "vmap". So whatever solution works for virtio-net migrating its virtqueues in userspace should be close to what will work here. The primary difference is the location of the serializer.

Quoted 3 lines

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

Quoted 10 lines

Quoted 10 lines

Quoted 4 lines

A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better results.

per-vcpu will not work well here, unfortunately, since this is an external interface mechanism. The callers will generally be from a kthread or some other non-vcpu related context. Even if we could figure out a vcpu to use as a basis, we would require some kind of heavier-weight synchronization which would not be as desirable.

Quoted 6 lines

Quoted 4 lines

Therefore, I opted to go per-cpu and use the presumably lighterweight get_cpu/put_cpu() instead.

This just assumes a low context switch rate.

It primarily assumes a low _migration_ rate, since you do not typically have two contexts on the same cpu pounding on the memslots.

And even if
you did, there's a good chance for locality between the threads, since the IO activity is likely related. For the odd times where locality fails to yield a hit, the time-slice or migration rate should be sufficiently infrequent enough to still yield high 90s hit rates for when it matters. For low-volume, the lookup is in the noise so I am not as concerned.

IOW: Where the lookup hurts the most is trying to walk an SG list, since each pointer is usually within the same slot. For this case, at least, this cache helps immensely, at least according to profiles.

Quoted 3 lines

How about a gfn_to_pfn_cached(..., struct gfn_to_pfn_cache *cache)? Each user can place it in a natural place.

Sounds good. I will incorporate this into the split patch.

Quoted 30 lines

Quoted 30 lines

Quoted 17 lines

Quoted 17 lines

virt/kvm/xinterface.c new file +17 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+           const void *src, unsigned long n)
+{
+    struct _xinterface *_intf = to_intf(intf);
+    unsigned long dst;
+    bool kthread = !current->mm;
+
+    down_read(&_intf->kvm->slots_lock);
+
+    dst = gpa_to_hva(_intf, gpa);
+    if (!dst)
+        goto out;
+
+    if (kthread)
+        use_mm(_intf->mm);
+

Quoted 10 lines

virt/kvm/xinterface.c line 221

Quoted 7 lines

virt/kvm/xinterface.c new file +4 -0

+    if (kthread || _intf->mm == current->mm)
+        n = copy_to_user((void *)dst, src, n);
+    else
+        n = _slow_copy_to_user(_intf, dst, src, n);

Can't you switch the mm temporarily instead of this?

Thats actually what I do for the fast-path (use_mm() does a switch_to() internally).

Quoted 6 lines

Quoted 4 lines

The slow-path is only there for completeness for when switching is not possible (such as if called with an mm already active i.e. process-context).

Still, why can't you switch temporarily?

I am not an mm expert, but iiuc you cannot call switch_to() from anything other than kthread context. Thats what the doc says, anyway.

Quoted 5 lines

Quoted 5 lines

In practice, however, this doesnt happen. Virtually
100% of the calls in vbus hit the fast-path here, and I suspect most xinterface clients would find the same conditions as well.

Quoted 3 lines

So you have 100% untested code here.

Actually, no. Before Michael enlightened me recently regarding switch_to/use_mm, the '_slow_xx' functions were my _only_ path. So they have indeed multiple months (and multiple GB) of testing, as it turns out. I only recently optimized them away to "backup" duty.

Quoted 7 lines · Gregory Haskins

Quoted 4 lines

slots would be one implementation, if you can think of others then you'd add them.

I'm more interested in *how* you'd add them more than "if" we would add them. What I am getting at are the logistics of such a beast.

Add alternative ioctls, or have one ioctl with a 'type' field.

Quoted 5 lines

For instance, would I have /dev/slots-vas with ioctls for adding slots, and /dev/foo-vas for adding foos? And each one would instantiate a different vas_struct object with its own vas_struct->ops? Or were you thinking of something different.

I think a single /dev/foo is sufficient, unless some of those address spaces are backed by real devices.

Quoted 3 lines

Quoted 3 lines

If you can't, I think it indicates that the whole thing isn't necessary and we're better off with slots and virtual memory.

Quoted 4 lines

I'm not sure if we are talking about the same thing yet, but if we are, there are uses of a generalized interface outside of slots/virtual memory (Ira's physical box being a good example).

I'm not sure Ira's case is not best supported by virtual memory.

Quoted 7 lines

In any case, I think the best approach is what I already proposed. KVM's arrangement of memory is going to tend to be KVM specific, and what better place to implement the interface than close to the kvm.ko core.

Quoted 8 lines

Quoted 3 lines

The only thing missing is dma, which you don't deal with anyway.

Afaict I do support dma in the generalized vbus::memctx, though I do not use it on anything related to KVM or xinterface. Can you elaborate on the problem here? Does the SG interface in 4/4 help get us closer to what you envision?

There's no dma method in there. Mapping to physical addresses is equivalent to get_user_pages(), so it doesn't add anything over virtual memory slots.

Quoted 9 lines

Quoted 4 lines

You'd have to copy the entire range since you don't know what the guest might put there. I guess it's acceptable for small areas.

? The vmap is presumably part of an ABI between guest and host, so the host should always know what structure is present within the region, and what is relevant from within that structure to migrate once that state is "frozen".

I was thinking of the case where the page is shared with some other (guest-private) data. But dirtying that data would be tracked by kvm, so it isn't a problem.

Quoted 8 lines

These regions (for vbus, anyway) equate to things like virtqueue metadata, and presumably the same problem exists for virtio-net in userspace as it does here, since that is another form of a "vmap". So whatever solution works for virtio-net migrating its virtqueues in userspace should be close to what will work here. The primary difference is the location of the serializer.

Actually, virtio doesn't serialize this data, instead it marks the page dirty and lets normal RAM migration move it.

Quoted 9 lines

Quoted 5 lines

rmb()s are only needed if an external agent can issue writes, otherwise you'd need one after every statement.

I was following lessons learned here:

http://lkml.org/lkml/2009/7/7/175

Quoted 3 lines

Perhaps mb() or barrier() are more appropriate than rmb()? I'm CC'ing David Howells in case he has more insight.

I'm not sure I understand the reference. Callers and callees don't need memory barriers since they're guaranteed program order. You only need memory barriers when you have an external agent (a device, or another cpu). What external agent can touch things during ->release()?

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

Quoted 5 lines

A simple per-vcpu cache (in struct kvm_vcpu) is likely to give better results.

per-vcpu will not work well here, unfortunately, since this is an external interface mechanism. The callers will generally be from a kthread or some other non-vcpu related context. Even if we could figure
out a vcpu to use as a basis, we would require some kind of heavier-weight synchronization which would not be as desirable.

Therefore, I opted to go per-cpu and use the presumably lighterweight get_cpu/put_cpu() instead.

Quoted 4 lines

Quoted 2 lines

This just assumes a low context switch rate.

It primarily assumes a low _migration_ rate, since you do not typically have two contexts on the same cpu pounding on the memslots.

Why not? If you're overcommitted, you will, especially if you have multiple guests doing request/reply interaction (perhaps with each other).

Quoted 9 lines

And even if
you did, there's a good chance for locality between the threads, since the IO activity is likely related. For the odd times where locality fails to yield a hit, the time-slice or migration rate should be sufficiently infrequent enough to still yield high 90s hit rates for when it matters. For low-volume, the lookup is in the noise so I am not as concerned.

Quoted 4 lines

IOW: Where the lookup hurts the most is trying to walk an SG list, since each pointer is usually within the same slot. For this case, at least, this cache helps immensely, at least according to profiles.

I'm wary of adding per-cpu data where it's easier to have a per-caller or per-vcpu cache.

Quoted 6 lines

Quoted 4 lines

How about a gfn_to_pfn_cached(..., struct gfn_to_pfn_cache *cache)? Each user can place it in a natural place.

Sounds good. I will incorporate this into the split patch.

Note that for slots, typically most accesses hit just one slot (the largest). For guests below 4G, there's only one large slot, for guests above 4G there are two.

Quoted 35 lines

Quoted 35 lines

Quoted 35 lines

Quoted 18 lines

Quoted 18 lines

virt/kvm/xinterface.c new file +17 -0

+static unsigned long
+xinterface_copy_to(struct kvm_xinterface *intf, unsigned long gpa,
+           const void *src, unsigned long n)
+{
+    struct _xinterface *_intf = to_intf(intf);
+    unsigned long dst;
+    bool kthread = !current->mm;
+
+    down_read(&_intf->kvm->slots_lock);
+
+    dst = gpa_to_hva(_intf, gpa);
+    if (!dst)
+        goto out;
+
+    if (kthread)
+        use_mm(_intf->mm);
+

Quoted 10 lines

virt/kvm/xinterface.c line 221

Quoted 7 lines

virt/kvm/xinterface.c new file +4 -0

+    if (kthread || _intf->mm == current->mm)
+        n = copy_to_user((void *)dst, src, n);
+    else
+        n = _slow_copy_to_user(_intf, dst, src, n);

Can't you switch the mm temporarily instead of this?

Thats actually what I do for the fast-path (use_mm() does a switch_to() internally).

The slow-path is only there for completeness for when switching is not possible (such as if called with an mm already active i.e. process-context).

Quoted 5 lines

Quoted 2 lines

Still, why can't you switch temporarily?

I am not an mm expert, but iiuc you cannot call switch_to() from anything other than kthread context. Thats what the doc says, anyway.

Can you provide a pointer? I don't see why this limitation exists.

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

In practice, however, this doesnt happen. Virtually
100% of the calls in vbus hit the fast-path here, and I suspect most xinterface clients would find the same conditions as well.

So you have 100% untested code here.

Quoted 5 lines

Actually, no. Before Michael enlightened me recently regarding switch_to/use_mm, the '_slow_xx' functions were my _only_ path. So they have indeed multiple months (and multiple GB) of testing, as it turns out. I only recently optimized them away to "backup" duty.

I meant, unexercised. This will bitrot quickly.

Quoted 28 lines · Avi Kivity

Quoted 7 lines · Gregory Haskins

Quoted 4 lines

slots would be one implementation, if you can think of others then you'd add them.

I'm more interested in *how* you'd add them more than "if" we would add them. What I am getting at are the logistics of such a beast.

Add alternative ioctls, or have one ioctl with a 'type' field.

Quoted 5 lines

For instance, would I have /dev/slots-vas with ioctls for adding slots, and /dev/foo-vas for adding foos? And each one would instantiate a different vas_struct object with its own vas_struct->ops? Or were you thinking of something different.

I think a single /dev/foo is sufficient, unless some of those address spaces are backed by real devices.

Quoted 7 lines

Quoted 3 lines

If you can't, I think it indicates that the whole thing isn't necessary and we're better off with slots and virtual memory.

I'm not sure if we are talking about the same thing yet, but if we are, there are uses of a generalized interface outside of slots/virtual memory (Ira's physical box being a good example).

Quoted 1 line

I'm not sure Ira's case is not best supported by virtual memory.

Perhaps, but there are surely some cases where the memory is not pageable, but is accessible indirectly through some DMA controller. So if we require it to be pagable we will limit the utility of the interface, though admittedly it will probably cover most cases.

The limitation I have is that memory made available from the host system (PCI card) as PCI BAR1 must not be migrated around in memory. I can only change the address decoding to hit a specific physical address. AFAIK, this means it cannot be userspace memory (since the underlying physical page could change, or it could be in swap), and must be allocated with something like __get_free_pages() or dma_alloc_coherent().

The limitation I have is that memory made available from the host system (PCI card) as PCI BAR1 must not be migrated around in memory. I can only change the address decoding to hit a specific physical address. AFAIK, this means it cannot be userspace memory (since the underlying physical page could change, or it could be in swap), and must be allocated with something like __get_free_pages() or dma_alloc_coherent().

We want to add a more efficient way to get PIO signals out of the guest, so we add an "xioevent" interface. This allows a client to register for notifications when a specific MMIO/PIO address is touched by the guest. This is an alternative interface to ioeventfd, which is performance limited by io-bus scaling and eventfd wait-queue based notification mechanism. This also has the advantage of retaining the full PIO data payload and passing it to the recipient.

This allows a scatter-gather approach to IO, which will be useful for building high performance interfaces, like zero-copy and low-latency copy (avoiding multiple calls to copy_to/from).

The interface is based on the existing scatterlist infrastructure. The caller is expected to pass in a scatterlist with its "dma" field populated with valid GPAs. The xinterface will then populate each entry by translating the GPA to a page*.

The caller signifies completion by simply performing a put_page() on each page returned in the list.

Signed-off-by Gregory Haskins<[email protected]>

---

  include/linux/kvm_xinterface.h |    4 ++
  virt/kvm/xinterface.c          |   72 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 76 insertions(+), 0 deletions(-)

include/linux/kvm_xinterface.h +4 -0

index 684b6f8..eefb575 100644
@@ -9,6 +9,7 @@
  #include<linux/kref.h>
  #include<linux/module.h>
  #include<linux/file.h>
+#include<linux/scatterlist.h>
 
  struct kvm_xinterface;
  struct kvm_xvmap;

@@ -36,6 +37,9 @@ struct kvm_xinterface_ops {
  					u64 addr,
  					unsigned long len,
  					unsigned long flags);
+	unsigned long (*sgmap)(struct kvm_xinterface *intf,
+			       struct scatterlist *sgl, int nents,
+			       unsigned long flags);
  	void (*release)(struct kvm_xinterface *);
  };
 

virt/kvm/xinterface.c +43 -0

index c356835..16729f6 100644
@@ -467,6 +467,77 @@ fail:
 
  }
 
+static unsigned long
+xinterface_sgmap(struct kvm_xinterface *intf,
+		 struct scatterlist *sgl, int nents,
+		 unsigned long flags)
+{
+	struct _xinterface     *_intf   = to_intf(intf);
+	struct task_struct     *p       = _intf->task;
+	struct mm_struct       *mm      = _intf->mm;
+	struct kvm             *kvm     = _intf->kvm;
+	struct kvm_memory_slot *memslot = NULL;
+	bool                    kthread = !current->mm;
+	int                     ret;
+	struct scatterlist     *sg;
+	int                     i;
+
+	down_read(&kvm->slots_lock);
+
+	if (kthread)
+		use_mm(_intf->mm);
+
+	for_each_sg(sgl, sg, nents, i) {
+		unsigned long           gpa    = sg_dma_address(sg);
+		unsigned long           len    = sg_dma_len(sg);
+		unsigned long           gfn    = gpa>>  PAGE_SHIFT;
+		off_t                   offset = offset_in_page(gpa);
+		unsigned long           hva;
+		struct page            *pg;
+
+		/* ensure that we do not have more than one page per entry */
+		if ((PAGE_ALIGN(len + offset)>>  PAGE_SHIFT) != 1) {
+			ret = -EINVAL;
+			break;
+		}
+
+		/* check for a memslot-cache miss */
+		if (!memslot
+		    || gfn<  memslot->base_gfn
+		    || gfn>= memslot->base_gfn + memslot->npages) {
+			memslot = gfn_to_memslot(kvm, gfn);
+			if (!memslot) {
+				ret = -EFAULT;
+				break;
+			}

virt/kvm/xinterface.c +19 -0

+
+		if (ret != 1) {
+			if (ret>= 0)
+				ret = -EFAULT;
+			break;
+		}
+
+		sg_set_page(sg, pg, len, offset);
+		ret = 0;
+	}
+
+	if (kthread)
+		unuse_mm(_intf->mm);
+
+	up_read(&kvm->slots_lock);
+
+	return ret;
+}
+
  static void
  xinterface_release(struct kvm_xinterface *intf)

  {

virt/kvm/xinterface.c +1 -1

@@ -483,6 +554,7 @@ struct kvm_xinterface_ops _xinterface_ops = {
  	.copy_from   = xinterface_copy_from,
  	.vmap        = xinterface_vmap,
  	.ioevent     = xinterface_ioevent,
+	.sgmap       = xinterface_sgmap,
  	.release     = xinterface_release,
  };
 
 
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 98 lines · Gregory Haskins

This allows a scatter-gather approach to IO, which will be useful for building high performance interfaces, like zero-copy and low-latency copy (avoiding multiple calls to copy_to/from).

The interface is based on the existing scatterlist infrastructure. The caller is expected to pass in a scatterlist with its "dma" field populated with valid GPAs. The xinterface will then populate each entry by translating the GPA to a page*.

The caller signifies completion by simply performing a put_page() on each page returned in the list.

Signed-off-by Gregory Haskins<[email protected]>

---

  include/linux/kvm_xinterface.h |    4 ++
  virt/kvm/xinterface.c          |   72
++++++++++++++++++++++++++++++++++++++++
  2 files changed, 76 insertions(+), 0 deletions(-)

include/linux/kvm_xinterface.h +4 -0

index 684b6f8..eefb575 100644
@@ -9,6 +9,7 @@
  #include<linux/kref.h>
  #include<linux/module.h>
  #include<linux/file.h>
+#include<linux/scatterlist.h>
 
  struct kvm_xinterface;
  struct kvm_xvmap;

@@ -36,6 +37,9 @@ struct kvm_xinterface_ops {
                      u64 addr,
                      unsigned long len,
                      unsigned long flags);
+    unsigned long (*sgmap)(struct kvm_xinterface *intf,
+                   struct scatterlist *sgl, int nents,
+                   unsigned long flags);
      void (*release)(struct kvm_xinterface *);
  };
 

virt/kvm/xinterface.c +47 -0

index c356835..16729f6 100644
@@ -467,6 +467,77 @@ fail:
 
  }
 
+static unsigned long
+xinterface_sgmap(struct kvm_xinterface *intf,
+         struct scatterlist *sgl, int nents,
+         unsigned long flags)
+{
+    struct _xinterface     *_intf   = to_intf(intf);
+    struct task_struct     *p       = _intf->task;
+    struct mm_struct       *mm      = _intf->mm;
+    struct kvm             *kvm     = _intf->kvm;
+    struct kvm_memory_slot *memslot = NULL;
+    bool                    kthread = !current->mm;
+    int                     ret;
+    struct scatterlist     *sg;
+    int                     i;
+
+    down_read(&kvm->slots_lock);
+
+    if (kthread)
+        use_mm(_intf->mm);
+
+    for_each_sg(sgl, sg, nents, i) {
+        unsigned long           gpa    = sg_dma_address(sg);
+        unsigned long           len    = sg_dma_len(sg);
+        unsigned long           gfn    = gpa>>  PAGE_SHIFT;
+        off_t                   offset = offset_in_page(gpa);
+        unsigned long           hva;
+        struct page            *pg;
+
+        /* ensure that we do not have more than one page per entry */
+        if ((PAGE_ALIGN(len + offset)>>  PAGE_SHIFT) != 1) {
+            ret = -EINVAL;
+            break;
+        }
+
+        /* check for a memslot-cache miss */
+        if (!memslot
+            || gfn<  memslot->base_gfn
+            || gfn>= memslot->base_gfn + memslot->npages) {
+            memslot = gfn_to_memslot(kvm, gfn);
+            if (!memslot) {
+                ret = -EFAULT;
+                break;
+            }
+        }
+
+        hva = (memslot->userspace_addr +
+               (gfn - memslot->base_gfn) * PAGE_SIZE);

virt/kvm/xinterface.c fail:

Quoted 6 lines

virt/kvm/xinterface.c +5 -0

+
+        if (kthread || current->mm == mm)
+            ret = get_user_pages_fast(hva, 1, 1,&pg);
+        else
+            ret = get_user_pages(p, mm, hva, 1, 1, 0,&pg, NULL);
 

One of these needs the mm semaphore.

Quoted 37 lines

virt/kvm/xinterface.c +19 -0

+
+        if (ret != 1) {
+            if (ret>= 0)
+                ret = -EFAULT;
+            break;
+        }
+
+        sg_set_page(sg, pg, len, offset);
+        ret = 0;
+    }
+
+    if (kthread)
+        unuse_mm(_intf->mm);
+
+    up_read(&kvm->slots_lock);
+
+    return ret;
+}
+
  static void
  xinterface_release(struct kvm_xinterface *intf)

  {

virt/kvm/xinterface.c +1 -0

@@ -483,6 +554,7 @@ struct kvm_xinterface_ops _xinterface_ops = {
      .copy_from   = xinterface_copy_from,
      .vmap        = xinterface_vmap,
      .ioevent     = xinterface_ioevent,
+    .sgmap       = xinterface_sgmap,
      .release     = xinterface_release,
  };
 

Signature · 4 lines

-- 
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html