Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 signature.asc Description: OpenPGP digital signature
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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
signature.asc
Description: OpenPGP digital signature
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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:
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 signature.asc Description: OpenPGP digital signature
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 signature.asc Description: OpenPGP digital signature
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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 zl...@acm.org Knowledge is Power. Power Corrupts. Study Hard. Be Evil. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM PATCH] KVM: introduce xinterface API for external interaction with guests
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
signature.asc
Description: OpenPGP digital signature
