(Applies to kvm.git/master:84a3c081)
For details, please read the patch header.
Background: The original vbus code was tightly integrated with kvm.ko. Avi
suggested that we abstract the interfaces such that it could live outside
of kvm. Part of that discussion turned into what is now irqfd/ioeventfd
checked into kvm.git. The other part of the discussion (pointer-translation)
was suggested to be addressed by having the memory-slot info replicated
across interested modules.
I was looking into what is required for essentially hooking the
SET_MEMORY_REGION operations in QEMU yesterday by talking to Anthony and
Glauber on IRC. We came to the conclusion that we could possibly
do a minor cleanup on the various callsites of SET_MEMORY_REGION so that
a wrapper function was used. I could then add a hook at this wrapper
to essentially get notification events whenever memory-slots change, and
could forward this info to my module appropriate.
Anthony made the observation that replicating the slots info is
not the cleanest design, and I tend to agree with him. Its replicating
data and is going to be prone to synchronization problems, etc. He
also mentioned that other modules like virtio-net would want access
to this same information at some point, so perhaps a general solution was in
order.
Therefore, I stepped back from that initial approach of replicating slots
and instead provided a abstract mechanism to utilize the original structures.
This code has been tested with a prototype of the vbus-v4 code and appears
to function properly.
Comments/suggestions?
Regards,
-Greg
---
Gregory Haskins (1):
KVM: introduce "xinterface" API for external interaction with guests
arch/x86/Kbuild | 4 +
arch/x86/kvm/Makefile | 4 +
arch/x86/kvm/x86.c | 1
include/linux/kvm.h | 2 +
include/linux/kvm_host.h | 6 ++
include/linux/kvm_xinterface.h | 58 ++++++++++++++++
virt/kvm/kvm_main.c | 72 ++++++++++++++++++++
virt/kvm/xinterface.c | 147 ++++++++++++++++++++++++++++++++++++++++
8 files changed, 293 insertions(+), 1 deletions(-)
create mode 100644 include/linux/kvm_xinterface.h
create mode 100644 virt/kvm/xinterface.c
--
Signature
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. This
binding mechanism uses a userspace friendly token "u64 vmid" as a
handle. This vmid acts similar to a file-descriptor in the sense that
it can be extracted from a guest, passed to an end-point of interest,
and finally, converted back to a vtable pointer using a stable interface.
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.
Security considerations: This concept is not believed to expose KVM to
any kind of additional security risk. The vmid token itself can only be
acquired via an open handle to the vmfd (i.e. qemu-kvm), and the interface
is only available within the kernel. Therefore the xinterface
admission policy is delegated to the kernel/lkm admission policy, which
must be assumed secure or the system is already compromised independent of
this work.
Additionally, the xinterface design is hardened against malformed vmid
tokens, as well as race conditions against valid tokens (e.g. guest
exiting before the token is redeemed). It is additionally hardened
against races in the kvm.ko module itself by acquiring proper module
references. As a final measure, we link the xinterface code statically
into the kernel so that callers are guaranteed a stable interface to
kvm_xinterface_find() without implicitly pinning kvm.ko or racing against
it.
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 issue a KVM_GET_VMID operation to acquire
the u64-based vmid, and pass it to ioctl(foofd, FOO_SET_VMID, &vmid).
Upon receipt, the foo module can issue kvm_xinterface_find(vmid) 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().
Signed-off-by: Gregory Haskins <[email protected]>
---
arch/x86/Kbuild | 4 +
arch/x86/kvm/Makefile | 4 +
arch/x86/kvm/x86.c | 1
include/linux/kvm.h | 2 +
include/linux/kvm_host.h | 6 ++
include/linux/kvm_xinterface.h | 58 ++++++++++++++++
virt/kvm/kvm_main.c | 72 ++++++++++++++++++++
virt/kvm/xinterface.c | 147 ++++++++++++++++++++++++++++++++++++++++
8 files changed, 293 insertions(+), 1 deletions(-)
create mode 100644 include/linux/kvm_xinterface.h
create mode 100644 virt/kvm/xinterface.c
diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index ad8ec35..9f50cc3 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -1,5 +1,7 @@
-obj-$(CONFIG_KVM) += kvm/
+ifdef CONFIG_KVM
+obj-y += kvm/
+endif
# Xen paravirtualization support
obj-$(CONFIG_XEN) += xen/
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index afaaa76..80d951d 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -17,3 +17,7 @@ kvm-amd-y += svm.o
obj-$(CONFIG_KVM) += kvm.o
obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
obj-$(CONFIG_KVM_AMD) += kvm-amd.o
+
+ifdef CONFIG_KVM
+obj-y += $(addprefix ../../../virt/kvm/, xinterface.o)
+endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 48567fa..5725527 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1208,6 +1208,7 @@ int kvm_dev_ioctl_check_extension(long ext)
case KVM_CAP_IOEVENTFD:
case KVM_CAP_PIT2:
case KVM_CAP_PIT_STATE2:
+ case KVM_CAP_XINTERFACE:
r = 1;
break;
case KVM_CAP_COALESCED_MMIO:
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 230a91a..7790894 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -435,6 +435,7 @@ struct kvm_ioeventfd {
#define KVM_CAP_PIT_STATE2 35
#endif
#define KVM_CAP_IOEVENTFD 36
+#define KVM_CAP_XINTERFACE 37
#ifdef KVM_CAP_IRQ_ROUTING
@@ -544,6 +545,7 @@ struct kvm_irqfd {
#define KVM_CREATE_PIT2 _IOW(KVMIO, 0x77, struct kvm_pit_config)
#define KVM_SET_BOOT_CPU_ID _IO(KVMIO, 0x78)
#define KVM_IOEVENTFD _IOW(KVMIO, 0x79, struct kvm_ioeventfd)
+#define KVM_GET_VMID _IOR(KVMIO, 0x7a, __u64)
/*
* ioctls for vcpu fds
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f244f11..0ee95df 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -23,6 +23,7 @@
#include <linux/kvm_para.h>
#include <linux/kvm_types.h>
+#include <linux/kvm_xinterface.h>
#include <asm/kvm_host.h>
@@ -175,6 +176,7 @@ struct kvm {
unsigned long mmu_notifier_seq;
long mmu_notifier_count;
#endif
+ struct kvm_xinterface xinterface; /* interface for external modules */
};
/* The guest did something we don't support. */
@@ -199,6 +201,10 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
idx < atomic_read(&kvm->online_vcpus) && vcpup; \
vcpup = kvm_get_vcpu(kvm, ++idx))
+void kvm_xinterface_register(struct kvm_xinterface *intf,
+ const struct kvm_xinterface_ops *ops);
+void kvm_xinterface_unregister(struct kvm_xinterface *intf);
+
int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
diff --git a/include/linux/kvm_xinterface.h b/include/linux/kvm_xinterface.h
new file mode 100644
index 0000000..858acfd
--- /dev/null
+++ b/include/linux/kvm_xinterface.h
@@ -0,0 +1,58 @@
+#ifndef __KVM_XINTERFACE_H
+#define __KVM_XINTERFACE_H
+
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include <linux/kref.h>
+#include <linux/module.h>
+#include <linux/rbtree.h>
+
+struct kvm_xinterface;
+
+struct kvm_xinterface_ops {
+ struct module *owner;
+
+ unsigned long (*gpa_to_hva)(struct kvm_xinterface *, unsigned long gpa);
+ struct page* (*gpa_to_page)(struct kvm_xinterface *, unsigned long gpa);
+ void (*release)(struct kvm_xinterface *);
+};
+
+struct kvm_xinterface {
+ struct kref kref;
+ const struct kvm_xinterface_ops *ops;
+ struct rb_node node;
+};
+
+static inline void
+kvm_xinterface_get(struct kvm_xinterface *intf)
+{
+ kref_get(&intf->kref);
+}
+
+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->ops->owner;
+ rmb();
+
+ intf->ops->release(intf);
+ module_put(owner);
+}
+
+static inline void
+kvm_xinterface_put(struct kvm_xinterface *intf)
+{
+ kref_put(&intf->kref, _kvm_xinterface_release);
+}
+
+struct kvm_xinterface *kvm_xinterface_find(long vmid);
+
+#endif /* __KVM_XINTERFACE_H */
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7cd1c10..058cb6c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -935,6 +935,58 @@ static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
};
#endif /* CONFIG_MMU_NOTIFIER && KVM_ARCH_WANT_MMU_NOTIFIER */
+/*
+ * ------------
+ * XINTERFACE (External Interface)
+ * -------------
+ */
+
+static struct kvm *
+intf_to_kvm(struct kvm_xinterface *intf)
+{
+ return container_of(intf, struct kvm, xinterface);
+}
+
+static unsigned long
+xinterface_gpa_to_hva(struct kvm_xinterface *intf, unsigned long gpa)
+{
+ struct kvm *kvm = intf_to_kvm(intf);
+ unsigned long addr;
+
+ addr = gfn_to_hva(kvm, gpa >> PAGE_SHIFT);
+ if (kvm_is_error_hva(addr))
+ return 0;
+
+ return addr + offset_in_page(gpa);
+}
+
+static struct page *
+xinterface_gpa_to_page(struct kvm_xinterface *intf, unsigned long gpa)
+{
+ struct kvm *kvm = intf_to_kvm(intf);
+ struct page *page;
+
+ page = gfn_to_page(kvm, gpa >> PAGE_SHIFT);
+ if (page == bad_page)
+ return ERR_PTR(-EINVAL);
+
+ return page;
+}
+
+static void
+xinterface_release(struct kvm_xinterface *intf)
+{
+ struct kvm *kvm = intf_to_kvm(intf);
+
+ kvm_put_kvm(kvm);
+}
+
+struct kvm_xinterface_ops _kvm_xinterface_ops = {
+ .gpa_to_hva = xinterface_gpa_to_hva,
+ .gpa_to_page = xinterface_gpa_to_page,
+ .release = xinterface_release,
+};
+
static struct kvm *kvm_create_vm(void)
{
struct kvm *kvm = kvm_arch_create_vm();
@@ -991,6 +1043,8 @@ static struct kvm *kvm_create_vm(void)
#ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
kvm_coalesced_mmio_init(kvm);
#endif
+ kvm_get_kvm(kvm); /* the xinterface needs another ref */
+ kvm_xinterface_register(&kvm->xinterface, &_kvm_xinterface_ops);
out:
return kvm;
}
@@ -1073,6 +1127,7 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
struct kvm *kvm = filp->private_data;
kvm_irqfd_release(kvm);
+ kvm_xinterface_unregister(&kvm->xinterface);
kvm_put_kvm(kvm);
return 0;
@@ -2289,6 +2344,22 @@ static long kvm_vm_ioctl(struct file *filp,
mutex_unlock(&kvm->lock);
break;
#endif
+ case KVM_GET_VMID: {
+ u64 vmid = (u64)&kvm->xinterface.node;
+
+ /*
+ * our vmid is simply the address of our rb_node in the
+ * registry, which is guaranteed unique. This also simplifies
+ * the registry map-lookup since we dont need to do a deep
+ * decode on the pointer to figure out if we have a match
+ */
+
+ r = -EFAULT;
+ if (copy_to_user(argp, &vmid, (sizeof vmid)))
+ goto out;
+ r = 0;
+ break;
+ }
default:
r = kvm_arch_vm_ioctl(filp, ioctl, arg);
}
@@ -2761,6 +2832,7 @@ int kvm_init(void *opaque, unsigned int vcpu_size,
kvm_chardev_ops.owner = module;
kvm_vm_fops.owner = module;
kvm_vcpu_fops.owner = module;
+ _kvm_xinterface_ops.owner = module;
r = misc_register(&kvm_dev);
if (r) {
diff --git a/virt/kvm/xinterface.c b/virt/kvm/xinterface.c
new file mode 100644
index 0000000..fe9a214
--- /dev/null
+++ b/virt/kvm/xinterface.c
@@ -0,0 +1,147 @@
+/*
+ * KVM module interface - Allows external modules to interface with a guest
+ *
+ * This code is designed to be statically linked to the kernel, regardless
+ * of the configuration of kvm.ko. This allows the kvm_xinterface_find
+ * routine to be stably exported without dependencies on, or race conditions
+ * against acquiring the kvm.ko module itself.
+ *
+ * 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/module.h>
+#include <linux/mutex.h>
+#include <linux/kvm_host.h>
+#include <linux/kvm_xinterface.h>
+
+struct kvm_registry {
+ struct mutex lock;
+ struct rb_root root;
+};
+
+/* system wide registry of kvm based VMs */
+static struct kvm_registry kvm_registry = {
+ .lock = __MUTEX_INITIALIZER(kvm_registry.lock),
+ .root = RB_ROOT,
+};
+
+static struct kvm_xinterface *
+to_intf(struct rb_node *node)
+{
+ return node ? container_of(node, struct kvm_xinterface, node) : NULL;
+}
+
+struct kvm_xinterface *
+kvm_xinterface_find(long vmid)
+{
+ struct rb_node *node;
+ struct kvm_xinterface *intf;
+
+ mutex_lock(&kvm_registry.lock);
+
+ node = kvm_registry.root.rb_node;
+
+ while (node) {
+ long val;
+
+ val = vmid - (long)node;
+ if (val < 0)
+ node = node->rb_left;
+ else if (val > 0)
+ node = node->rb_right;
+ else
+ break;
+ }
+
+ intf = to_intf(node);
+ if (intf)
+ kvm_xinterface_get(intf);
+
+ mutex_unlock(&kvm_registry.lock);
+
+ return intf;
+}
+EXPORT_SYMBOL_GPL(kvm_xinterface_find);
+
+/*
+ * ------------------------------------------
+ * register/unregister
+ * ------------------------------------------
+ *
+ * These functions are private to the API and are only to be called
+ * by the KVM core
+ * ------------------------------------------
+ */
+
+/* caller must hold intf->ops->owner */
+void
+kvm_xinterface_register(struct kvm_xinterface *intf,
+ const struct kvm_xinterface_ops *ops)
+{
+ struct rb_root *root;
+ struct rb_node **new, *parent = NULL;
+ struct rb_node *node;
+
+ memset(intf, 0, sizeof(*intf));
+ kref_init(&intf->kref);
+ intf->ops = ops;
+
+ mutex_lock(&kvm_registry.lock);
+
+ root = &kvm_registry.root;
+ new = &(root->rb_node);
+ node = &intf->node;
+
+ /* Figure out where to put new node */
+ while (*new) {
+ long val;
+
+ parent = *new;
+
+ val = node - parent;
+ if (val < 0)
+ new = &((*new)->rb_left);
+ else if (val > 0)
+ new = &((*new)->rb_right);
+ else
+ panic("kvm_xinterface: duplicate entry: %ld\n", val);
+ }
+
+ /* Add new node and rebalance tree. */
+ rb_link_node(node, parent, new);
+ rb_insert_color(node, root);
+
+ /* released when the last xinterface reference is released */
+ __module_get(intf->ops->owner);
+
+ mutex_unlock(&kvm_registry.lock);
+}
+EXPORT_SYMBOL_GPL(kvm_xinterface_register);
+
+/* caller must hold intf->ops->owner */
+void
+kvm_xinterface_unregister(struct kvm_xinterface *intf)
+{
+ mutex_lock(&kvm_registry.lock);
+ rb_erase(&intf->node, &kvm_registry.root);
+ mutex_unlock(&kvm_registry.lock);
+
+ kvm_xinterface_put(intf);
+}
+EXPORT_SYMBOL_GPL(kvm_xinterface_unregister);
> diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
> index ad8ec35..9f50cc3 100644
> --- a/arch/x86/Kbuild
> +++ b/arch/x86/Kbuild
> @@ -1,5 +1,7 @@
>
> -obj-$(CONFIG_KVM) += kvm/
> +ifdef CONFIG_KVM
> +obj-y += kvm/
> +endif
What was wrong with the old version?
If this is because xinterface.o is builtin to the kernel then we should
always visit kvm/ and use:
obj-y += kvm/
unconditionally.
>
> # Xen paravirtualization support
> obj-$(CONFIG_XEN) += xen/
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index afaaa76..80d951d 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -17,3 +17,7 @@ kvm-amd-y += svm.o
> obj-$(CONFIG_KVM) += kvm.o
> obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
> obj-$(CONFIG_KVM_AMD) += kvm-amd.o
> +
> +ifdef CONFIG_KVM
> +obj-y += $(addprefix ../../../virt/kvm/, xinterface.o)
> +endif
And when we always visit kvm/ the above code snippet makes sense.
Before we never visited kvm/ unless CONFIG_KVM was defined.
Sam
Sam Ravnborg wrote:
>> diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
>> index ad8ec35..9f50cc3 100644
>> --- a/arch/x86/Kbuild
>> +++ b/arch/x86/Kbuild
>> @@ -1,5 +1,7 @@
>>
>> -obj-$(CONFIG_KVM) += kvm/
>> +ifdef CONFIG_KVM
>> +obj-y += kvm/
>> +endif
>>
>
> What was wrong with the old version?
> If this is because xinterface.o is builtin to the kernel
Bingo
> then we should
> always visit kvm/ and use:
>
> obj-y += kvm/
>
> unconditionally.
>
>
Ok, easy enough.
Thanks Sam,
-Greg
Gregory Haskins wrote:
> +/*
> + * ------------
> + * XINTERFACE (External Interface)
> + * -------------
> + */
> +
> +static struct kvm *
> +intf_to_kvm(struct kvm_xinterface *intf)
> +{
> + return container_of(intf, struct kvm, xinterface);
> +}
> +
> +static unsigned long
> +xinterface_gpa_to_hva(struct kvm_xinterface *intf, unsigned long gpa)
> +{
> + struct kvm *kvm = intf_to_kvm(intf);
> + unsigned long addr;
> +
> + addr = gfn_to_hva(kvm, gpa >> PAGE_SHIFT);
> + if (kvm_is_error_hva(addr))
> + return 0;
> +
> + return addr + offset_in_page(gpa);
> +}
> +
> +static struct page *
> +xinterface_gpa_to_page(struct kvm_xinterface *intf, unsigned long gpa)
> +{
> + struct kvm *kvm = intf_to_kvm(intf);
> + struct page *page;
> +
> + page = gfn_to_page(kvm, gpa >> PAGE_SHIFT);
> + if (page == bad_page)
> + return ERR_PTR(-EINVAL);
> +
> + return page;
> +}
> +
> +static void
> +xinterface_release(struct kvm_xinterface *intf)
> +{
> + struct kvm *kvm = intf_to_kvm(intf);
> +
> + kvm_put_kvm(kvm);
> +}
> +
> +struct kvm_xinterface_ops _kvm_xinterface_ops = {
> + .gpa_to_hva = xinterface_gpa_to_hva,
> + .gpa_to_page = xinterface_gpa_to_page,
> + .release = xinterface_release,
> +};
>
How do you deal with locking?
The mappings (gpa_to_page) are not fixed. They can and do change very
often. The interface doesn't seem to attempt to cope with this.
Regards,
Anthony Liguori
Anthony Liguori wrote:
> Gregory Haskins wrote:
>> +/*
>> + * ------------
>> + * XINTERFACE (External Interface)
>> + * -------------
>> + */
>> +
>> +static struct kvm *
>> +intf_to_kvm(struct kvm_xinterface *intf)
>> +{
>> + return container_of(intf, struct kvm, xinterface);
>> +}
>> +
>> +static unsigned long
>> +xinterface_gpa_to_hva(struct kvm_xinterface *intf, unsigned long gpa)
>> +{
>> + struct kvm *kvm = intf_to_kvm(intf);
>> + unsigned long addr;
>> +
>> + addr = gfn_to_hva(kvm, gpa >> PAGE_SHIFT);
>> + if (kvm_is_error_hva(addr))
>> + return 0;
>> +
>> + return addr + offset_in_page(gpa);
>> +}
>> +
>> +static struct page *
>> +xinterface_gpa_to_page(struct kvm_xinterface *intf, unsigned long gpa)
>> +{
>> + struct kvm *kvm = intf_to_kvm(intf);
>> + struct page *page;
>> +
>> + page = gfn_to_page(kvm, gpa >> PAGE_SHIFT);
>> + if (page == bad_page)
>> + return ERR_PTR(-EINVAL);
>> +
>> + return page;
>> +}
>> +
>> +static void
>> +xinterface_release(struct kvm_xinterface *intf)
>> +{
>> + struct kvm *kvm = intf_to_kvm(intf);
>> +
>> + kvm_put_kvm(kvm);
>> +}
>> +
>> +struct kvm_xinterface_ops _kvm_xinterface_ops = {
>> + .gpa_to_hva = xinterface_gpa_to_hva,
>> + .gpa_to_page = xinterface_gpa_to_page,
>> + .release = xinterface_release,
>> +};
>>
>
> How do you deal with locking?
>
> The mappings (gpa_to_page) are not fixed. They can and do change very
> often. The interface doesn't seem to attempt to cope with this.
Hmm, well I used to need gpa_to_page() in the older version of vbus that
did explicit hypercalls, but I don't actually use it today in v4. I
left it in because it seems like it might be useful in general (perhaps
for Michael). However, if this ends up being a real problem I suppose
we can just drop that vtable entry and let the guy that actually needs
it deal with the issues ;) I really only require gpa_to_hva() in the
short-term.
That said, I think the assumption that was made when I was using this
was that a proper ref for the page was acquired by the gfn_to_page() and
dropped by the caller. This was always used in the context of a
hypercall/vmexit so presumably the gpa should be considered stable
across that call. Is that not true?
Regards,
-Greg
Gregory Haskins wrote:
> That said, I think the assumption that was made when I was using this
> was that a proper ref for the page was acquired by the gfn_to_page() and
> dropped by the caller. This was always used in the context of a
> hypercall/vmexit so presumably the gpa should be considered stable
> across that call. Is that not true?
>
If you're in kvm.ko, then yes, that's a safe assumption to make because
the guest VCPU cannot run while you are running.
But you're opening this interface to any caller so the VCPU is likely to
be running while someone calls this function
> Regards,
> -Greg
>
>
>
--
Regards,
Anthony Liguori
On Thursday 16 July 2009, Gregory Haskins wrote:
> Background: The original vbus code was tightly integrated with kvm.ko. Avi
> suggested that we abstract the interfaces such that it could live outside
> of kvm.
The code is still highly kvm-specific, you would not be able to use
it with another hypervisor like lguest or vmware player, right?
> 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 issue a KVM_GET_VMID operation to acquire
> the u64-based vmid, and pass it to ioctl(foofd, FOO_SET_VMID, &vmid).
> Upon receipt, the foo module can issue kvm_xinterface_find(vmid) 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().
Your approach allows passing the vmid from a process that does
not own the kvm context. This looks like an intentional feature,
but I can't see what this gains us.
> As a final measure, we link the xinterface code statically
> into the kernel so that callers are guaranteed a stable interface to
> kvm_xinterface_find() without implicitly pinning kvm.ko or racing against
> it.
I also don't understand this. Are you worried about driver modules
breaking when an externally-compiled kvm.ko is loaded? The same could
be achieved by defining your data structures kvm_xinterface_ops and
kvm_xinterface in a kernel header that is not shipped by kvm-kmod but
always taken from the kernel headers.
It does not matter if the entry points are build into the kernel or
exported from a kvm.ko as long as you define a fixed ABI.
What is the problem with pinning kvm.ko from another module using
its features?
Can't you simply provide a function call to lookup the kvm context
pointer from the file descriptor to achieve the same functionality?
To take that thought further, maybe the dependency can be turned
around: If every user (pci-uio, virtio-net, ...) exposes a file
descriptor based interface to user space, you can have a kvm
ioctl to register the object behind that file descriptor with
an existing kvm context to associate it with a guest. That would
nicely solve the life time questions by pinning the external
object for the life time of the kvm context rather than the other
way round, and it would be completely separate from kvm in that
each such object could be used by other subsystems independent
of kvm.
Arnd <><
Arnd Bergmann wrote:
> On Thursday 16 July 2009, Gregory Haskins wrote:
>
>> Background: The original vbus code was tightly integrated with kvm.ko. Avi
>> suggested that we abstract the interfaces such that it could live outside
>> of kvm.
>>
>
> The code is still highly kvm-specific, you would not be able to use
> it with another hypervisor like lguest or vmware player, right?
>
In its current form, it is kvm specific primarily because it was not a
explicit design constraint of mine to support others. However, there is
no reason why we could not generalize the interface if that is a
desirable trait. Ultimately I would like to have my project support
other things like lguest, so this is not a bad idea. Otherwise, someone
will invariably be proposing an "lguest_xinterface" next ;)
>
>> 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 issue a KVM_GET_VMID operation to acquire
>> the u64-based vmid, and pass it to ioctl(foofd, FOO_SET_VMID, &vmid).
>> Upon receipt, the foo module can issue kvm_xinterface_find(vmid) 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().
>>
>
> Your approach allows passing the vmid from a process that does
> not own the kvm context. This looks like an intentional feature,
> but I can't see what this gains us.
This work is towards the implementation of lockless-shared-memory
subsystems, which includes ring constructs such as virtio-ring,
VJ-netchannels, and vbus-ioq. I find that these designs perform
optimally when you allow two distinct contexts (producer + consumer) to
process the ring concurrently, which implies a disparate context from
the guest in question. Note that the infrastructure we are discussing
does not impose a requirement for the contexts to be unique: it will
work equally well from the same or a different process.
For an example of this "producer/consumer" dynamic over shared memory in
action, please refer to my previous posting re: "vbus"
http://lkml.org/lkml/2009/4/21/408
I am working on v4 now, and this patch is part of the required support.
>
>
>
>> As a final measure, we link the xinterface code statically
>> into the kernel so that callers are guaranteed a stable interface to
>> kvm_xinterface_find() without implicitly pinning kvm.ko or racing against
>> it.
>>
>
> I also don't understand this. Are you worried about driver modules
> breaking when an externally-compiled kvm.ko is loaded? The same could
> be achieved by defining your data structures kvm_xinterface_ops and
> kvm_xinterface in a kernel header that is not shipped by kvm-kmod but
> always taken from the kernel headers.
> It does not matter if the entry points are build into the kernel or
> exported from a kvm.ko as long as you define a fixed ABI.
>
> What is the problem with pinning kvm.ko from another module using
> its features?
>
Well, there is always the chance that I am doing something dumb or
missing the point ;)
But my rationale was as follows: The problem is that kvm is a little
weird in the module ref department: If I were to do it the standard way
and link xinterface.o into kvm.o (and have any xinterface_find() users
do a tristate+"depends on KVM"), this would work as I believe you are
suggesting. That is to say: whenever I loaded "foo.ko", insmod would
automatically up the reference of kvm.ko. The issue is that is not
quite what I really want ;)
I want to hold the reference to the entire .text chain, which includes
kvm.ko + [kvm-intel.ko | kvm-amd.ko]. If you look carefully, the
ops->owner that is assigned is actually the arch.ko. In addition, I
wanted the kvm.ko lifetime to be associated with the lifetime of its
contexts actually in use, not the lifetime of its installed
dependencies. Therefore, I did it this way.
But to your point, I suppose the dependency lifetime thing is not a huge
deal. I could therefore modify the patch to simply link xinterface.o
into kvm.ko and still achieve the primary objective by retaining ops->owner.
Note that if we are going to generalize the interface to support other
guests as you may have been suggesting above, it should probably stay
statically linked (and perhaps live in ./lib or something)
> Can't you simply provide a function call to lookup the kvm context
> pointer from the file descriptor to achieve the same functionality?
>
You mean so have: struct kvm_xinterface *kvm_xinterface_find(int fd)
(instead of creating our own vmid namespace) ?
Or are you suggesting using fget() instead of kvm_xinterface_find()?
> To take that thought further, maybe the dependency can be turned
> around: If every user (pci-uio, virtio-net, ...) exposes a file
> descriptor based interface to user space, you can have a kvm
> ioctl to register the object behind that file descriptor with
> an existing kvm context to associate it with a guest.
FWIW: We do that already for the signaling path (see irqfd and ioeventfd
in kvm.git). Each side exposes interfaces that accept eventfds, and the
fds are passed around that way.
However, for the functions we are talking about now, I don't think it
really works well to go the other way. I could be misunderstanding what
you mean, though. What I mean is that it's KVM that is providing a
service to the other modules (in this case, translating memory
pointers), so what would an inverse interface look like for that? And
even if you came up with one, it seems to me that its just "6 of one,
half-dozen of the other" kind of thing.
> That would
> nicely solve the life time questions by pinning the external
> object for the life time of the kvm context
I suppose that is nice, but note that in practice both objects (the
kvm-guest and the "foo" module) are managed by the same entity (i.e.
QEMU) and therefore share the same approximate lifetime.
Kind Regards,
-Greg
Gregory Haskins wrote:
> Note that if we are going to generalize the interface to support other
> guests as you may have been suggesting above, it should probably stay
> statically linked (and perhaps live in ./lib or something)
>
More specifically, it can no longer live in kvm.ko. I guess it
technically doesn't have to be statically linked, though (e.g.
xinterface.ko is fine, too).
Regards,
-Greg
Gregory Haskins wrote:
> Gregory Haskins wrote:
>> Note that if we are going to generalize the interface to support other
>> guests as you may have been suggesting above, it should probably stay
>> statically linked (and perhaps live in ./lib or something)
>>
>
> More specifically, it can no longer live in kvm.ko. I guess it
> technically doesn't have to be statically linked, though (e.g.
> xinterface.ko is fine, too).
Speaking as someone who knows nothing about this, if this is going to be
a module and visible in places like lsmod, could you name it something else?
I see "xinterface.ko" and the first thing in my head is "something to do
with the X Window System."
How about kvm_xinterface or vguest_xinterface?
--
Zan Lynx
[email protected]
"Knowledge is Power. Power Corrupts. Study Hard. Be Evil."
On Thursday 16 July 2009, Gregory Haskins wrote:
> Arnd Bergmann wrote:
> > Your approach allows passing the vmid from a process that does
> > not own the kvm context. This looks like an intentional feature,
> > but I can't see what this gains us.
>
> This work is towards the implementation of lockless-shared-memory
> subsystems, which includes ring constructs such as virtio-ring,
> VJ-netchannels, and vbus-ioq. I find that these designs perform
> optimally when you allow two distinct contexts (producer + consumer) to
> process the ring concurrently, which implies a disparate context from
> the guest in question. Note that the infrastructure we are discussing
> does not impose a requirement for the contexts to be unique: it will
> work equally well from the same or a different process.
>
> For an example of this "producer/consumer" dynamic over shared memory in
> action, please refer to my previous posting re: "vbus"
>
> http://lkml.org/lkml/2009/4/21/408
>
> I am working on v4 now, and this patch is part of the required support.
Ok. I can see how your approach gives you more flexibility in this
regard, but it does not seem critical.
> But to your point, I suppose the dependency lifetime thing is not a huge
> deal. I could therefore modify the patch to simply link xinterface.o
> into kvm.ko and still achieve the primary objective by retaining ops->owner.
Right. And even if it's a separate module, holding an extra reference
on kvm.ko will not cause any harm.
> > Can't you simply provide a function call to lookup the kvm context
> > pointer from the file descriptor to achieve the same functionality?
> >
> You mean so have: struct kvm_xinterface *kvm_xinterface_find(int fd)
>
> (instead of creating our own vmid namespace) ?
>
> Or are you suggesting using fget() instead of kvm_xinterface_find()?
I guess they are roughly equivalent. Either you pass a fd to
kvm_xinterface_find, or pass the struct file pointer you get
from fget. The latter is probably more convenient because it
allows you to pass around the struct file in kernel contexts
that don't have that file descriptor open.
> > To take that thought further, maybe the dependency can be turned
> > around: If every user (pci-uio, virtio-net, ...) exposes a file
> > descriptor based interface to user space, you can have a kvm
> > ioctl to register the object behind that file descriptor with
> > an existing kvm context to associate it with a guest.
>
> FWIW: We do that already for the signaling path (see irqfd and ioeventfd
> in kvm.git). Each side exposes interfaces that accept eventfds, and the
> fds are passed around that way.
>
> However, for the functions we are talking about now, I don't think it
> really works well to go the other way. I could be misunderstanding what
> you mean, though. What I mean is that it's KVM that is providing a
> service to the other modules (in this case, translating memory
> pointers), so what would an inverse interface look like for that? And
> even if you came up with one, it seems to me that its just "6 of one,
> half-dozen of the other" kind of thing.
I mean something like
int kvm_ioctl_register_service(struct file *filp, unsigned long arg)
{
struct file *service = fget(arg);
struct kvm *kvm = filp->private_data;
if (!service->f_ops->new_xinterface_register)
return -EINVAL;
return service->f_ops->new_xinterface_register(service, (void*)kvm,
&kvm_xinterface_ops);
}
This would assume that we define a new file_operation specifically for this,
which would simplify the code, but there are other ways to achieve the same.
It would even mean that you don't need any static code as an interface layer.
Arnd <><
Zan Lynx wrote:
> Gregory Haskins wrote:
>> Gregory Haskins wrote:
>>> Note that if we are going to generalize the interface to support other
>>> guests as you may have been suggesting above, it should probably stay
>>> statically linked (and perhaps live in ./lib or something)
>>>
>>
>> More specifically, it can no longer live in kvm.ko. I guess it
>> technically doesn't have to be statically linked, though (e.g.
>> xinterface.ko is fine, too).
>
> Speaking as someone who knows nothing about this, if this is going to
> be a module and visible in places like lsmod, could you name it
> something else?
>
> I see "xinterface.ko" and the first thing in my head is "something to
> do with the X Window System."
>
> How about kvm_xinterface or vguest_xinterface?
>
Heh, I totally agree. That was just pseudo-naming, anyway. ;)
If it was going to be generic, I would do something like
"virt-xinterface.ko".
Kind Regards,
-Greg
Arnd Bergmann wrote:
> On Thursday 16 July 2009, Gregory Haskins wrote:
>
>> Arnd Bergmann wrote:
>>
>
>
>>> Your approach allows passing the vmid from a process that does
>>> not own the kvm context. This looks like an intentional feature,
>>> but I can't see what this gains us.
>>>
>> This work is towards the implementation of lockless-shared-memory
>> subsystems, which includes ring constructs such as virtio-ring,
>> VJ-netchannels, and vbus-ioq. I find that these designs perform
>> optimally when you allow two distinct contexts (producer + consumer) to
>> process the ring concurrently, which implies a disparate context from
>> the guest in question. Note that the infrastructure we are discussing
>> does not impose a requirement for the contexts to be unique: it will
>> work equally well from the same or a different process.
>>
>> For an example of this "producer/consumer" dynamic over shared memory in
>> action, please refer to my previous posting re: "vbus"
>>
>> http://lkml.org/lkml/2009/4/21/408
>>
>> I am working on v4 now, and this patch is part of the required support.
>>
>
> Ok. I can see how your approach gives you more flexibility in this
> regard, but it does not seem critical.
>
Yeah, and I think I missed your point the first time around. I thought
you were asking about how the resulting memory API was used, but now I
see you were referring to the scope of the vmid namespace (vs
file-descriptors).
On that topic, yes the vmid implementation here differs from
file-descriptors in the sense that the namespace is global to the
kernel, instead of per-process. And yes, I also agree with you that
this is not critical to the design, per se. For instance, the use cases
I have in mind would work fine with the per-task fd namespace as well
since I will be within the same QEMU instance.
So ultimately, I think that means the fd idea you presented would work
equally well.
>
>> But to your point, I suppose the dependency lifetime thing is not a huge
>> deal. I could therefore modify the patch to simply link xinterface.o
>> into kvm.ko and still achieve the primary objective by retaining ops->owner.
>>
>
> Right. And even if it's a separate module, holding an extra reference
> on kvm.ko will not cause any harm.
>
Yep, agreed.
>
>>> Can't you simply provide a function call to lookup the kvm context
>>> pointer from the file descriptor to achieve the same functionality?
>>>
>>>
>> You mean so have: struct kvm_xinterface *kvm_xinterface_find(int fd)
>>
>> (instead of creating our own vmid namespace) ?
>>
>> Or are you suggesting using fget() instead of kvm_xinterface_find()?
>>
>
> I guess they are roughly equivalent. Either you pass a fd to
> kvm_xinterface_find, or pass the struct file pointer you get
> from fget. The latter is probably more convenient because it
> allows you to pass around the struct file in kernel contexts
> that don't have that file descriptor open.
>
Interesting. I like it.
>
>>> To take that thought further, maybe the dependency can be turned
>>> around: If every user (pci-uio, virtio-net, ...) exposes a file
>>> descriptor based interface to user space, you can have a kvm
>>> ioctl to register the object behind that file descriptor with
>>> an existing kvm context to associate it with a guest.
>>>
>> FWIW: We do that already for the signaling path (see irqfd and ioeventfd
>> in kvm.git). Each side exposes interfaces that accept eventfds, and the
>> fds are passed around that way.
>>
>> However, for the functions we are talking about now, I don't think it
>> really works well to go the other way. I could be misunderstanding what
>> you mean, though. What I mean is that it's KVM that is providing a
>> service to the other modules (in this case, translating memory
>> pointers), so what would an inverse interface look like for that? And
>> even if you came up with one, it seems to me that its just "6 of one,
>> half-dozen of the other" kind of thing.
>>
>
> I mean something like
>
> int kvm_ioctl_register_service(struct file *filp, unsigned long arg)
> {
> struct file *service = fget(arg);
> struct kvm *kvm = filp->private_data;
>
> if (!service->f_ops->new_xinterface_register)
> return -EINVAL;
>
> return service->f_ops->new_xinterface_register(service, (void*)kvm,
> &kvm_xinterface_ops);
> }
>
> This would assume that we define a new file_operation specifically for this,
> which would simplify the code, but there are other ways to achieve the same.
> It would even mean that you don't need any static code as an interface layer.
>
I suspect I will have a much harder time getting that type of
modification accepted in mainline ;)
I think I will go with your suggestion for the fd/file interface, tho.
I can get rid of the rbtree logic and re-use the lookup via fget, which
is a nice win.
Thanks Arnd!
-Greg