[PATCH] KVM: Introduce dynamically registered hypercall capability

[PATCH] KVM: Introduce dynamically registered hypercall capability

[Phil White]

This introduces a list of entries which associate a function pointer of
kvm_hc_type to a hypercall number and allows the ability to register and
unregister entries.  In addition, it also allows the ability to retrieve a
function pointer of kvm_hc_type for a given hypercall number which is meant
to be called from the arch-specific section.

The main intent is to allow modules to register hypercalls which they own
rather than requiring the addition of a stub of some sort.  It will also
allow each arch to maintain separate lists of hypercalls rather than having
to respect changes in include/uapi/linux/kvm_para.h

Signed-off-by: Phil White <pwhite@mvista.com>
---
 arch/arm/kvm/Makefile               |   2 +-
 arch/arm64/kvm/Makefile             |   1 +
 arch/ia64/kvm/Makefile              |   2 +-
 arch/mips/include/asm/kvm_
para.h    |   6 ++
 arch/mips/kvm/Makefile              |   3 +-
 arch/powerpc/include/asm/kvm_para.h |   7 ++
 arch/powerpc/kvm/Makefile           |   2 +-
 arch/powerpc/kvm/powerpc.c          |   5 ++
 arch/s390/include/asm/kvm_para.h    |   7 +-
 arch/s390/kvm/Makefile              |   2 +-
 arch/x86/include/asm/kvm_para.h     |   6 ++
 arch/x86/kvm/Makefile               |   3 +-
 arch/x86/kvm/x86.c                  |   6 ++
 include/uapi/linux/kvm.h            |   1 +
 include/uapi/linux/kvm_para.h       |  19 +++++-
 virt/kvm/hypercall.c                | 125 ++++++++++++++++++++++++++++++++++++
 16 files changed, 187 insertions(+), 10 deletions(-)
 create mode 100644 virt/kvm/hypercall.c

diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
index f7057ed..0f9adf9 100644
--- a/arch/arm/kvm/Makefile
+++ b/arch/arm/kvm/Makefile
@@ -15,7 +15,7 @@ AFLAGS_init.o := -Wa,-march=armv7-a$(plus_virt)
 AFLAGS_interrupts.o := -Wa,-march=armv7-a$(plus_virt)

 KVM := ../../../virt/kvm
-kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/hypercall.o

 obj-y += kvm-arm.o init.o interrupts.o
 obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 32a0961..735ea53 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -12,6 +12,7 @@ ARM=../../../arch/arm/kvm
 obj-$(CONFIG_KVM_ARM_HOST) += kvm.o

 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/hypercall.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o

diff --git a/arch/ia64/kvm/Makefile b/arch/ia64/kvm/Makefile
index 18e45ec..abbde9a 100644
--- a/arch/ia64/kvm/Makefile
+++ b/arch/ia64/kvm/Makefile
@@ -49,7 +49,7 @@ ccflags-y := -Ivirt/kvm -Iarch/ia64/kvm/
 asflags-y := -Ivirt/kvm -Iarch/ia64/kvm/
 KVM := ../../../virt/kvm

-common-objs = $(KVM)/kvm_main.o $(KVM)/ioapic.o \
+common-objs = $(KVM)/kvm_main.o $(KVM)/ioapic.o $(KVM)/hypercall.o \
                $(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o

 ifeq ($(CONFIG_KVM_DEVICE_ASSIGNMENT),y)
diff --git a/arch/mips/include/asm/kvm_para.h b/arch/mips/include/asm/kvm_para.h
index 5a9aa91..85e44d02 100644
--- a/arch/mips/include/asm/kvm_para.h
+++ b/arch/mips/include/asm/kvm_para.h
@@ -2,6 +2,7 @@
 #define _ASM_MIPS_KVM_PARA_H

 #include <uapi/asm/kvm_para.h>
+#include <linux/kvm.h>

 #define KVM_HYPERCALL ".word 0x42000028"

@@ -105,5 +106,10 @@ static inline bool kvm_para_available(void)
 }
 #endif

+struct kvm_vcpu;
+
+typedef unsigned long (*kvm_hc_type)(struct kvm_vcpu *vcpu, unsigned long a0,
+               unsigned long a1, unsigned long a2, unsigned long a3);
+

 #endif /* _ASM_MIPS_KVM_PARA_H */
diff --git a/arch/mips/kvm/Makefile b/arch/mips/kvm/Makefile
index 401fe02..99afce8 100644
--- a/arch/mips/kvm/Makefile
+++ b/arch/mips/kvm/Makefile
@@ -1,7 +1,8 @@
 # Makefile for KVM support for MIPS
 #

-common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o)
+common-objs = $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o \
+               hypercall.o)

 EXTRA_CFLAGS += -Ivirt/kvm -Iarch/mips/kvm

diff --git a/arch/powerpc/include/asm/kvm_para.h
b/arch/powerpc/include/asm/kvm_para.h
index 336a91a..2818a15 100644
--- a/arch/powerpc/include/asm/kvm_para.h
+++ b/arch/powerpc/include/asm/kvm_para.h
@@ -20,6 +20,7 @@
 #define __POWERPC_KVM_PARA_H__

 #include <uapi/asm/kvm_para.h>
+#include <linux/kvm.h>

 #ifdef CONFIG_KVM_GUEST

@@ -66,4 +67,10 @@ static inline bool kvm_check_and_clear_guest_paused(void)
        return false;
 }

+struct kvm_vcpu;
+
+typedef unsigned long (*kvm_hc_type)(struct kvm_vcpu *vcpu,
+               unsigned long param1, unsigned long param2,
+               unsigned long param3, unsigned long param4);
+
 #endif /* __POWERPC_KVM_PARA_H__ */
diff --git a/arch/powerpc/kvm/Makefile b/arch/powerpc/kvm/Makefile
index 0570eef..9b5f239 100644
--- a/arch/powerpc/kvm/Makefile
+++ b/arch/powerpc/kvm/Makefile
@@ -8,7 +8,7 @@ ccflags-y := -Ivirt/kvm -Iarch/powerpc/kvm
 KVM := ../../../virt/kvm

 common-objs-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o \
-               $(KVM)/eventfd.o
+               $(KVM)/eventfd.o $(KVM)/hypercall.o

 CFLAGS_e500_mmu.o := -I.
 CFLAGS_e500_mmu_host.o := -I.
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index c1f8f53..d6f7620 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -229,6 +229,11 @@ int kvmppc_kvm_pv(struct kvm_vcpu *vcpu)
                break;
        default:
                r = EV_UNIMPLEMENTED;
+
+               kvm_hc_type *hc = kvm_hypercall_get(nr);
+               if (hc) {
+                       r = (*hc)(vcpu, param1, param2, param3, param4);
+               }
                break;
        }

diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h
index e0f8423..781fad0 100644
--- a/arch/s390/include/asm/kvm_para.h
+++ b/arch/s390/include/asm/kvm_para.h
@@ -27,7 +27,7 @@
 #define __S390_KVM_PARA_H

 #include <uapi/asm/kvm_para.h>
-
+#include <linux/kvm.h>


 static inline long kvm_hypercall0(unsigned long nr)
@@ -154,4 +154,9 @@ static inline bool kvm_check_and_clear_guest_paused(void)
        return false;
 }

+struct kvm_vcpu;
+
+typedef unsigned long (*kvm_hc_type)(struct kvm_vcpu *vcpu, unsigned long a0,
+               unsigned long a1, unsigned long a2, unsigned long a3);
+
 #endif /* __S390_KVM_PARA_H */
diff --git a/arch/s390/kvm/Makefile b/arch/s390/kvm/Makefile
index b3b5534..c9a2069 100644
--- a/arch/s390/kvm/Makefile
+++ b/arch/s390/kvm/Makefile
@@ -7,7 +7,7 @@
 # as published by the Free Software Foundation.

 KVM := ../../../virt/kvm
-common-objs = $(KVM)/kvm_main.o $(KVM)/eventfd.o  $(KVM)/async_pf.o
$(KVM)/irqchip.o
+common-objs = $(KVM)/kvm_main.o $(KVM)/eventfd.o  $(KVM)/async_pf.o
$(KVM)/irqchip.o $(KVM)/hypercall.o

 ccflags-y := -Ivirt/kvm -Iarch/s390/kvm

diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index e62cf89..b4f337a 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -3,6 +3,7 @@

 #include <asm/processor.h>
 #include <asm/alternative.h>
+#include <linux/kvm.h>
 #include <uapi/asm/kvm_para.h>

 extern void kvmclock_init(void);
@@ -134,4 +135,9 @@ static inline void kvm_disable_steal_time(void)
 }
 #endif

+struct kvm_vcpu;
+
+typedef unsigned long (*kvm_hc_type)(struct kvm_vcpu *vcpu, unsigned long a0,
+               unsigned long a1, unsigned long a2, unsigned long a3);
+
 #endif /* _ASM_X86_KVM_PARA_H */
diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 25d22b2..3294678 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -9,7 +9,8 @@ KVM := ../../../virt/kvm

 kvm-y                  += $(KVM)/kvm_main.o $(KVM)/ioapic.o \
                                $(KVM)/coalesced_mmio.o $(KVM)/irq_comm.o \
-                               $(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
+                               $(KVM)/eventfd.o $(KVM)/irqchip.o \
+                               $(KVM)/vfio.o $(KVM)/hypercall.o
 kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)    += $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)     += $(KVM)/async_pf.o

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0033df3..4ad8880 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5841,6 +5841,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
        unsigned long nr, a0, a1, a2, a3, ret;
        int op_64_bit, r = 1;
+       kvm_hc_type hc;

        if (kvm_hv_hypercall_enabled(vcpu->kvm))
                return kvm_hv_hypercall(vcpu);
@@ -5877,6 +5878,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
                break;
        default:
                ret = -KVM_ENOSYS;
+
+               hc = kvm_hypercall_get(nr);
+               if (hc) {
+                       ret = (*hc)(vcpu, a0, a1, a2, a3);
+               }
                break;
        }
 out:
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 6076882..023c125 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -11,6 +11,7 @@
 #include <linux/compiler.h>
 #include <linux/ioctl.h>
 #include <asm/kvm.h>
+#include <uapi/linux/kvm_para.h>

 #define KVM_API_VERSION 12

diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
index bf6cd7d..1e92db7 100644
--- a/include/uapi/linux/kvm_para.h
+++ b/include/uapi/linux/kvm_para.h
@@ -7,6 +7,7 @@
  * - kvm_hypercall0, kvm_hypercall1...
  * - kvm_arch_para_features
  * - kvm_para_available
+ * - kvm_hc_type
  */

 /* Return values for hypercalls */
@@ -15,6 +16,7 @@
 #define KVM_E2BIG              E2BIG
 #define KVM_EPERM              EPERM

+/* Legacy hypercalls which we need to respect */
 #define KVM_HC_VAPIC_POLL_IRQ          1
 #define KVM_HC_MMU_OP                  2
 #define KVM_HC_FEATURES                        3
@@ -24,9 +26,20 @@
 #define KVM_HC_MIPS_EXIT_VM            7
 #define KVM_HC_MIPS_CONSOLE_OUTPUT     8

-/*
- * hypercalls use architecture specific
- */
+/* The number after which we're comfortable assigning dynamic hypercalls */
+#define KVM_HC_LEGACY                  9
+
 #include <asm/kvm_para.h>
+#include <linux/list.h>
+
+struct kvm_hc {
+       kvm_hc_type hc;
+       uint32_t hc_index;
+       struct list_head hc_list;
+};
+
+extern kvm_hc_type kvm_hypercall_get (uint32_t hc_nr);
+extern int kvm_hypercall_register (struct kvm_hc *hc, uint32_t hc_nr);
+extern void kvm_hypercall_unregister (uint32_t hc_nr);

 #endif /* _UAPI__LINUX_KVM_PARA_H */
diff --git a/virt/kvm/hypercall.c b/virt/kvm/hypercall.c
new file mode 100644
index 0000000..c82578d
--- /dev/null
+++ b/virt/kvm/hypercall.c
@@ -0,0 +1,125 @@
+#include <linux/kvm.h>
+#include <linux/types.h>
+
+LIST_HEAD(hc_list);
+static uint32_t hc_max = KVM_HC_LEGACY;
+
+kvm_hc_type kvm_hypercall_get (uint32_t hc_nr)
+{
+       struct list_head *p;
+       struct kvm_hc *entry;
+       kvm_hc_type ret = NULL;
+
+       list_for_each(p, &hc_list) {
+               entry = list_entry(p, struct kvm_hc, hc_list);
+               if (entry->hc_index == hc_nr) {
+                       ret = entry->hc;
+                       break;
+               } else if (entry->hc_index > hc_nr) {
+                       break;
+               }
+       }
+
+       return ret;
+}
+EXPORT_SYMBOL(kvm_hypercall_get);
+
+void kvm_hypercall_unregister (uint32_t hc_nr)
+{
+       struct list_head *p;
+       struct kvm_hc *entry;
+
+       list_for_each(p, &hc_list) {
+               entry = list_entry(p, struct kvm_hc, hc_list);
+               if (entry->hc_index == hc_nr) {
+                       list_del_init(p);
+                       break;
+               } else if (entry->hc_index > hc_nr) {
+                       break;
+               }
+       }
+}
+EXPORT_SYMBOL(kvm_hypercall_unregister);
+
+int kvm_hypercall_register (struct kvm_hc *hc, uint32_t hc_nr)
+{
+       struct list_head *p;
+       struct kvm_hc *entry;
+       uint32_t hc_last_nr = 0;
+
+       /* If no hypercall number is requested, then we dynamically assign one
+        *    more than any hypercall we've seen thus far to avoid reusing
+        *    previously registered hypercalls.  Note that if hc_max is the
+        *    maximum possible value (UINT_MAX), then hc->hc_nr will be equal
+        *    to 0.
+        */
+       if (hc_nr == 0) {
+               hc_nr = hc_max + 1;
+       }
+
+       /* There's three possibilities entering this.
+        * The first is that the caller is requesting an exact hc_nr.  If this
+        *    is the case, then we will go through the list.  If we find an
+        *    entry equal to hc_nr, then we instantly return 1.  Otherwise, we
+        *    go until we've hit a hypercall with a larger number or the last
+        *    element in the list, then break out of the loop.
+        * The second is that the caller is requesting a dynamically assigned
+        *    hc_nr and hc_max != UINT_MAX.  hc_nr will have been set to a
+        *    non-zero number as a result.  We should never find an equal case
+        *    and if we do, then something I don't understand has happened.  So
+        *    we'll break out of the loop if we've hit a hypercall with a
+        *    larger number or the last element in the list.
+        * The third is that the caller is requesting a dynamically assigned
+        *    hc_nr and hc_max == UINT_MAX.  hc_nr will be equal to 0.  If
+        *    anyone had attempted to request 0 in the past, then their hc_nr
+        *    would be dynamically assigned, so the equal case should again
+        *    never happen.  We'll break out of the loop when we've hit a
+        *    hypercall that is at least two greater than the previous in which
+        *    case we'll set hc_nr to the current entry minus 1.  If we hit the
+        *    end of the list, then hc_nr will be equal to 0.
+        */
+       list_for_each(p, &hc_list) {
+               entry = list_entry(p, struct kvm_hc, hc_list);
+               if (entry->hc_index == hc_nr) {
+                       return -1;
+               } else if (entry->hc_index > hc_nr) {
+                       if (hc_nr == 0) {
+                               if ((hc_last_nr+1) != entry->hc_index) {
+                                       hc_nr = entry->hc_index-1;
+                                       break;
+                               }
+                       } else {
+                               break;
+                       }
+               }
+               hc_last_nr = entry->hc_index;
+       }
+
+       /* If hc_nr is zero, then hc_max was UINT_MAX and we couldn't find
+        *    any gaps in the list.  The only thing we need to worry about then
+        *    is if the last hypercall in the list was UINT_MAX.  If it was,
+        *    then we give up.  Otherwise, we set hc_nr to the last hypercall
+        *    in the list plus 1.
+        */
+       if (hc_nr == 0 && hc_last_nr != UINT_MAX) {
+               hc_nr = hc_last_nr+1;
+       } else {
+               return -1;
+       }
+
+       /* At this point, p is either at the first hypercall entry greater than
+        *    hc_nr or it's at the beginning of the list.  In either case, we
+        *    set hc->hc_index to hc_nr and add it right before p.
+        */
+       hc->hc_index = hc_nr;
+       list_add_tail(&hc->hc_list, p);
+
+       /* If this is the largest hypercall we've seen, then we'll want to know
+        *    that.  This helps us avoid reuse of hypercall indices.
+        */
+       if (hc_nr > hc_max)
+               hc_max = hc_nr;
+
+       return 0;
+}
+EXPORT_SYMBOL(kvm_hypercall_register);

--
2.0.4

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Jan Kiszka]

On 2014-11-27 14:30, Phil White wrote:
> This introduces a list of entries which associate a function pointer of
> kvm_hc_type to a hypercall number and allows the ability to register and
> unregister entries.  In addition, it also allows the ability to retrieve a
> function pointer of kvm_hc_type for a given hypercall number which is meant
> to be called from the arch-specific section.
> 
> The main intent is to allow modules to register hypercalls which they own
> rather than requiring the addition of a stub of some sort.  It will also
> allow each arch to maintain separate lists of hypercalls rather than having
> to respect changes in include/uapi/linux/kvm_para.h

Who is using this? The patch lacks a concrete reference to an in-tree
user or at least an open source out-of-tree use case. Will this follow?

And was EXPORT_SYMBOL only accidentally chosen over ..._GPL?

> 
> Signed-off-by: Phil White <pwhite@mvista.com>
> ---
>  arch/arm/kvm/Makefile               |   2 +-
>  arch/arm64/kvm/Makefile             |   1 +
>  arch/ia64/kvm/Makefile              |   2 +-
>  arch/mips/include/asm/kvm_
> para.h    |   6 ++

Posting was somehow mangled.

On first glance, the code also lacks any locking for managing the
hypercall lists.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Radim Krčmář]

2014-11-27 05:30-0800, Phil White:
> This introduces a list of entries which associate a function pointer of
> kvm_hc_type to a hypercall number and allows the ability to register and
> unregister entries.  In addition, it also allows the ability to retrieve a
> function pointer of kvm_hc_type for a given hypercall number which is meant
> to be called from the arch-specific section.
> 
> The main intent is to allow modules to register hypercalls which they own
> rather than requiring the addition of a stub of some sort.  It will also
> allow each arch to maintain separate lists of hypercalls rather than having
> to respect changes in include/uapi/linux/kvm_para.h
> 
> Signed-off-by: Phil White <pwhite@mvista.com>
> ---

Apart from other problems,
how are guests going to use these hypercalls?

(If hc_nr is dynamic, a guest doesn't know its number and even if it is
 static, someone could have registered it beforehand => this needs some
 kind of synchronization with host modules.  A hardcoded reservation?)

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Phil White]

Good questions.

One thing that prompted this code is the presence and proliferation of
architecture specific hypercalls in include/uapi/linux/kvm_para.h.
I'm not sure why the tree has continued on for as long as it has with
a list of reserved hypercall indices -- most of which are unused on
any given architecture.  Is there a reason that I'm unaware of?

This was written because I had a module which was meant to be loaded
for paravirtualized VMs and it needed a hook to receive a hypercall.
We originally reserved an index, but that struck me as sloppy for the
same reason I mentioned before -- not every system is going to need
that hypercall number reserved.

I didn't have a problem communicating the hypercall number to the
guest incidentally -- it had a bit of shared memory where the request
structure was placed.  That said, it could just as easily be used in a
static arrangement where each request is made to claim a particular
ID.

It does occur to me that in the absence of the setup which I had
available, one could simply treat hc_nr as a 4 character ID rather
than a particular digit.

-Phil

"The generation of random numbers is too important to be left to
chance." -Robert R. Coveyou, Oak Ridge National Laboratory, 1969


On Thu, Nov 27, 2014 at 7:31 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2014-11-27 05:30-0800, Phil White:
>> This introduces a list of entries which associate a function pointer of
>> kvm_hc_type to a hypercall number and allows the ability to register and
>> unregister entries.  In addition, it also allows the ability to retrieve a
>> function pointer of kvm_hc_type for a given hypercall number which is meant
>> to be called from the arch-specific section.
>>
>> The main intent is to allow modules to register hypercalls which they own
>> rather than requiring the addition of a stub of some sort.  It will also
>> allow each arch to maintain separate lists of hypercalls rather than having
>> to respect changes in include/uapi/linux/kvm_para.h
>>
>> Signed-off-by: Phil White <pwhite@mvista.com>
>> ---
>
> Apart from other problems,
> how are guests going to use these hypercalls?
>
> (If hc_nr is dynamic, a guest doesn't know its number and even if it is
>  static, someone could have registered it beforehand => this needs some
>  kind of synchronization with host modules.  A hardcoded reservation?)
-Phil

"The generation of random numbers is too important to be left to
chance." -Robert R. Coveyou, Oak Ridge National Laboratory, 1969


On Thu, Nov 27, 2014 at 7:31 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2014-11-27 05:30-0800, Phil White:
>> This introduces a list of entries which associate a function pointer of
>> kvm_hc_type to a hypercall number and allows the ability to register and
>> unregister entries.  In addition, it also allows the ability to retrieve a
>> function pointer of kvm_hc_type for a given hypercall number which is meant
>> to be called from the arch-specific section.
>>
>> The main intent is to allow modules to register hypercalls which they own
>> rather than requiring the addition of a stub of some sort.  It will also
>> allow each arch to maintain separate lists of hypercalls rather than having
>> to respect changes in include/uapi/linux/kvm_para.h
>>
>> Signed-off-by: Phil White <pwhite@mvista.com>
>> ---
>
> Apart from other problems,
> how are guests going to use these hypercalls?
>
> (If hc_nr is dynamic, a guest doesn't know its number and even if it is
>  static, someone could have registered it beforehand => this needs some
>  kind of synchronization with host modules.  A hardcoded reservation?)

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Radim Krčmář]

2014-11-28 17:29-0800, Phil White:
> Good questions.
> 
> One thing that prompted this code is the presence and proliferation of
> architecture specific hypercalls in include/uapi/linux/kvm_para.h.
> I'm not sure why the tree has continued on for as long as it has with
> a list of reserved hypercall indices -- most of which are unused on
> any given architecture.  Is there a reason that I'm unaware of?

Name-space won't be exhausted, so nothing forced them to separate and
centralization easily avoids conflicts with generic hypercalls.

> This was written because I had a module which was meant to be loaded
> for paravirtualized VMs and it needed a hook to receive a hypercall.
> We originally reserved an index, but that struck me as sloppy for the
> same reason I mentioned before -- not every system is going to need
> that hypercall number reserved.

Makes sense;  out-of-tree modules are in an especially bad position to
get their hypercalls into the kernel.  (Is a hypercall necessary?)

> I didn't have a problem communicating the hypercall number to the
> guest incidentally -- it had a bit of shared memory where the request
> structure was placed.  That said, it could just as easily be used in a
> static arrangement where each request is made to claim a particular
> ID.

My main trouble is that we would export a very specific/limited feature
for an unknown problem -- we can't even tell if it is appropriate, which
strongly favors rejecting it.

I'd say that a virtio device is the way to go if you want to stay in the
kernel, but outside of kvm modules.  In which ways is virtio lacking?

> It does occur to me that in the absence of the setup which I had
> available, one could simply treat hc_nr as a 4 character ID rather
> than a particular digit.

(This would probably solve the situation in practice, but the conflict
 is still there, so design hasn't improved.)

> "The generation of random numbers is too important to be left to
> chance." -Robert R. Coveyou, Oak Ridge National Laboratory, 1969

:)  (Exactly.)

> On Thu, Nov 27, 2014 at 7:31 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > 2014-11-27 05:30-0800, Phil White:
> >> This introduces a list of entries which associate a function pointer of
> >> kvm_hc_type to a hypercall number and allows the ability to register and
> >> unregister entries.  In addition, it also allows the ability to retrieve a
> >> function pointer of kvm_hc_type for a given hypercall number which is meant
> >> to be called from the arch-specific section.
> >>
> >> The main intent is to allow modules to register hypercalls which they own
> >> rather than requiring the addition of a stub of some sort.  It will also
> >> allow each arch to maintain separate lists of hypercalls rather than having
> >> to respect changes in include/uapi/linux/kvm_para.h
> >>
> >> Signed-off-by: Phil White <pwhite@mvista.com>
> >> ---
> >
> > Apart from other problems,
> > how are guests going to use these hypercalls?
> >
> > (If hc_nr is dynamic, a guest doesn't know its number and even if it is
> >  static, someone could have registered it beforehand => this needs some
> >  kind of synchronization with host modules.  A hardcoded reservation?)

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Phil White]

On Mon, Dec 1, 2014 at 5:47 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2014-11-28 17:29-0800, Phil White:
>> Good questions.
>>
>> One thing that prompted this code is the presence and proliferation of
>> architecture specific hypercalls in include/uapi/linux/kvm_para.h.
>> I'm not sure why the tree has continued on for as long as it has with
>> a list of reserved hypercall indices -- most of which are unused on
>> any given architecture.  Is there a reason that I'm unaware of?
>
> Name-space won't be exhausted, so nothing forced them to separate and
> centralization easily avoids conflicts with generic hypercalls.

Consider: All the arch specific defines were defined in asm/kvm_para.h.
Each asm/kvm_para.h defines KVM_HC_ARCH_MAX as a relative
index from which generic hypercalls ought to be applied (e.g.
KVM_HC_ARCH_MAX+1 rather than 11).

This would at least organize the hypercalls and avoid a situation in which a
number of hypercalls which are not applicable pollute the namespace.  I'll
grant that the grounds here may be largely aesthetic.

The other worry is that institutionalization of this method will lead to a
hesitance to associate a specific hypercall index with anything other than the
function which it has been assigned in past kernel revisions.

In addition, this leads to a maintenance problem for anyone seeking to add a
hypercall in the future in which their hypercalls will need to be
updated in order
to avoid collisions with the community as well as any other sources they may
be dealing with.

These are all minor headaches, but they can be avoided.  A registration
method like this -- albeit somewhat more refined -- could be used to eliminate
all of those headaches in my opinion.

> I'd say that a virtio device is the way to go if you want to stay in the
> kernel, but outside of kvm modules.  In which ways is virtio lacking?

Virtio has several limitations.  It implies a situation in which the system has
already booted.  Secondly, there's no easy way to access the kvm structure.
Thirdly, it cannot be used effectively to implement an optimization for
virtualization on a platform.  Fourthly, I believe it would require changes to
qemu command lines -- and any associated tools which might be used to
cobble together a qemu command line.

A simple way of putting it, using the existing in-kernel code: I don't see how
you could use virtio to map the powerpc magic page at bootup.

>> It does occur to me that in the absence of the setup which I had
>> available, one could simply treat hc_nr as a 4 character ID rather
>> than a particular digit.
>
> (This would probably solve the situation in practice, but the conflict
>  is still there, so design hasn't improved.)

I'm not sure which conflict you mean.  I presume you mean the possibility
that two separate modules may attempt to claim the same hypercall index?

Presuming you do -- and I may be arguing a straw man here -- I'm not sure
that's classifiable as a design flaw as no method occurs to me by which
one could add the capability of dynamically registering a hypercall and have
access to the capabilities I mentioned above.

However, I can be trivially proven wrong by the suggestion of a design by
which an out-of-tree module could dynamically register a hypercall, have
access to the kvm structure (at a minimum), and avoid the possibility that
some other user may race to claim that hypercall.

>> "The generation of random numbers is too important to be left to
>> chance." -Robert R. Coveyou, Oak Ridge National Laboratory, 1969
>
> :)  (Exactly.)
>
>> On Thu, Nov 27, 2014 at 7:31 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> > 2014-11-27 05:30-0800, Phil White:
>> >> This introduces a list of entries which associate a function pointer of
>> >> kvm_hc_type to a hypercall number and allows the ability to register and
>> >> unregister entries.  In addition, it also allows the ability to retrieve a
>> >> function pointer of kvm_hc_type for a given hypercall number which is meant
>> >> to be called from the arch-specific section.
>> >>
>> >> The main intent is to allow modules to register hypercalls which they own
>> >> rather than requiring the addition of a stub of some sort.  It will also
>> >> allow each arch to maintain separate lists of hypercalls rather than having
>> >> to respect changes in include/uapi/linux/kvm_para.h
>> >>
>> >> Signed-off-by: Phil White <pwhite@mvista.com>
>> >> ---
>> >
>> > Apart from other problems,
>> > how are guests going to use these hypercalls?
>> >
>> > (If hc_nr is dynamic, a guest doesn't know its number and even if it is
>> >  static, someone could have registered it beforehand => this needs some
>> >  kind of synchronization with host modules.  A hardcoded reservation?)

Re: [PATCH] KVM: Introduce dynamically registered hypercall capability

[Radim Krčmář]

(tl;dr version at the bottom)

2014-12-01 15:43-0800, Phil White:
> On Mon, Dec 1, 2014 at 5:47 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > 2014-11-28 17:29-0800, Phil White:
> >> Good questions.
> >>
> >> One thing that prompted this code is the presence and proliferation of
> >> architecture specific hypercalls in include/uapi/linux/kvm_para.h.
> >> I'm not sure why the tree has continued on for as long as it has with
> >> a list of reserved hypercall indices -- most of which are unused on
> >> any given architecture.  Is there a reason that I'm unaware of?
> >
> > Name-space won't be exhausted, so nothing forced them to separate and
> > centralization easily avoids conflicts with generic hypercalls.
> 
> Consider: All the arch specific defines were defined in asm/kvm_para.h.
> Each asm/kvm_para.h defines KVM_HC_ARCH_MAX as a relative
> index from which generic hypercalls ought to be applied (e.g.
> KVM_HC_ARCH_MAX+1 rather than 11).

Every hypercall is still hardcoded on host's side, so this is a harmful
complication.  (When compared with current state and pre-defined arch
space, like -1,-2,...)

The guest can deal with it quite easily, a special hypercall that
returns the first generic one, but there is no real reason to.

> This would at least organize the hypercalls and avoid a situation in which a
> number of hypercalls which are not applicable pollute the namespace.  I'll
> grant that the grounds here may be largely aesthetic.

(It is a commendable pursuit, working code definitely isn't enough.)

> The other worry is that institutionalization of this method will lead to a
> hesitance to associate a specific hypercall index with anything other than the
> function which it has been assigned in past kernel revisions.

Yes, we'd have keep "legacy" hypercalls.
Breaking old guests isn't what we want.

> In addition, this leads to a maintenance problem for anyone seeking to add a
> hypercall in the future in which their hypercalls will need to be
> updated in order
> to avoid collisions with the community as well as any other sources they may
> be dealing with.

(I think that this wouldn't increase the load on maintainers by much.)

> These are all minor headaches, but they can be avoided.  A registration
> method like this -- albeit somewhat more refined -- could be used to eliminate
> all of those headaches in my opinion.

What worries me is the hypercall negotiation ...
If we added truly dynamic hypercall numbers, then the guest would have
to agree with host on function/position of hypercalls.

This has a major drawback:  host and guest have no common definition for
hypercalls => they do not know what the other is talking about.
This can be solved by introducing a "hypercall protocol", which it is
just a more round-about way of having hardcoded ids ...

(You did that by having shared memory that exposed a structure that was
 decoded by your guest.)

> > I'd say that a virtio device is the way to go if you want to stay in the
> > kernel, but outside of kvm modules.  In which ways is virtio lacking?
> 
> Virtio has several limitations.  It implies a situation in which the system has
> already booted.  Secondly, there's no easy way to access the kvm structure.
> Thirdly, it cannot be used effectively to implement an optimization for
> virtualization on a platform.  Fourthly, I believe it would require changes to
> qemu command lines -- and any associated tools which might be used to
> cobble together a qemu command line.

True, I misunderstood the scope of your modification.  I think it would
be "easier" to merge the paravirtualization into KVM+Linux ...

Calling your code by live-patching the hypercall handler could be
mentioned as an easy solution, but it has its problems ...
(A continued use of forked kernel is definitely the easiest.)

> A simple way of putting it, using the existing in-kernel code: I don't see how
> you could use virtio to map the powerpc magic page at bootup.

Agreed, and this code dwells in KVM modules because of that.

(I wasn't talking about existing hypercalls, just foreign modules.)

> >> It does occur to me that in the absence of the setup which I had
> >> available, one could simply treat hc_nr as a 4 character ID rather
> >> than a particular digit.
> >
> > (This would probably solve the situation in practice, but the conflict
> >  is still there, so design hasn't improved.)
> 
> I'm not sure which conflict you mean.  I presume you mean the possibility
> that two separate modules may attempt to claim the same hypercall index?

Yes, integer -> char[4] just switched from sequential assignment to a
"random" one and shrinked the space.  (If people used random generator
for new hypercall numbers, it would have a similar effect.)

> Presuming you do -- and I may be arguing a straw man here -- I'm not sure
> that's classifiable as a design flaw as no method occurs to me by which
> one could add the capability of dynamically registering a hypercall and have
> access to the capabilities I mentioned above.

Linux has everything it cares about in one tree, so it probably isn't
dynamic in that sense -- solution is the hardcoded reservation.
You know how many hypercalls your code is going to need ...

> However, I can be trivially proven wrong by the suggestion of a design by
> which an out-of-tree module could dynamically register a hypercall, have
> access to the kvm structure (at a minimum), and avoid the possibility that
> some other user may race to claim that hypercall.

Sorry, neither I think this is possible:
a module can mimic the registration procedure of other module to the
point where it can't be distinguished in the guest.
(Modules can do *everything*, but we keep in-tree ones working.)


---
I'll try to gather the points above:
- Dynamic hypercall ids boil down to something hardcoded, so we
  actually gain by having a static system.
  (=> you should drop the dynamic part of the patch)

- Current mix of arch+generic is acceptable and function pointers in a
  list/tree don't improve it *at all*.

- I don't like that we would add code that has no in-tree use.
  What does Linux gain by having this?