[Qemu-devel] [PATCH 0/6] split out uses of kvm_irqchip_in_kernel()

[Qemu-devel] [PATCH 0/6] split out uses of kvm_irqchip_in_kernel()

[Peter Maydell]

This patch series removes all uses of kvm_irqchip_in_kernel()
from architecture-independent code, by creating a set of more
specific functions instead to test for the particular aspects
of behaviour that the calling code is actually interested in.

The uses in x86-specific code could in theory be further broken
down into kvm_ioapic(), kvm_pit(), etc, but I leave that for
one of the x86 maintainers if they think it's worthwhile.

Peter Maydell (6):
  kvm: Decouple 'interrupt injection is async' from 'kernel irqchip'
  kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq
  kvm: Move kvm_allows_irq0_override() to target-i386
  kvm: Don't assume irqchip-in-kernel implies irqfds
  kvm: Don't assume irqchip implies MSI routing via irqfds
  kvm: Add documentation comment for kvm_irqchip_in_kernel()

 cpus.c                    |    3 +-
 hw/kvm/i8259.c            |    2 +-
 hw/kvm/ioapic.c           |    2 +-
 hw/pc.c                   |    1 +
 hw/virtio-pci.c           |    4 +-
 kvm-all.c                 |   24 ++++++++++++----------
 kvm-stub.c                |    8 ++----
 kvm.h                     |   48 ++++++++++++++++++++++++++++++++++++++++++--
 target-i386/Makefile.objs |    1 +
 target-i386/kvm-stub.c    |   17 +++++++++++++++
 target-i386/kvm.c         |   12 +++++++++++
 target-i386/kvm_i386.h    |   16 +++++++++++++++
 12 files changed, 114 insertions(+), 24 deletions(-)
 create mode 100644 target-i386/kvm-stub.c
 create mode 100644 target-i386/kvm_i386.h

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/6] kvm: Decouple 'interrupt injection is async' from 'kernel irqchip'

[Peter Maydell]

On x86 interrupt injection is asynchronous (and therefore
VCPU idle management is done in the kernel) if and only
if there is an in-kernel irqchip. On other architectures this
isn't necessarily true (they may always do asynchronous
injection), so define a new kvm_async_interrupt_injection()
function instead of misusing kvm_irqchip_in_kernel().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpus.c     |    3 ++-
 kvm-all.c  |    7 ++++++-
 kvm-stub.c |    1 +
 kvm.h      |   11 +++++++++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 756e624..584b298 100644
--- a/cpus.c
+++ b/cpus.c
@@ -69,7 +69,8 @@ static bool cpu_thread_is_idle(CPUArchState *env)
     if (env->stopped || !runstate_is_running()) {
         return true;
     }
-    if (!env->halted || qemu_cpu_has_work(env) || kvm_irqchip_in_kernel()) {
+    if (!env->halted || qemu_cpu_has_work(env) ||
+        kvm_async_interrupt_injection()) {
         return false;
     }
     return true;
diff --git a/kvm-all.c b/kvm-all.c
index 2148b20..3354c6f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -100,6 +100,7 @@ struct KVMState
 
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
+bool kvm_async_interrupt_injection;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
@@ -857,7 +858,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
     struct kvm_irq_level event;
     int ret;
 
-    assert(kvm_irqchip_in_kernel());
+    assert(kvm_async_interrupt_injection());
 
     event.level = level;
     event.irq = irq;
@@ -1201,6 +1202,10 @@ static int kvm_irqchip_create(KVMState *s)
         s->irqchip_inject_ioctl = KVM_IRQ_LINE_STATUS;
     }
     kvm_kernel_irqchip = true;
+    /* If we have an in-kernel IRQ chip then we must have asynchronous
+     * interrupt injection (though the reverse is not necessarily true)
+     */
+    kvm_async_interrupt_injection = true;
 
     kvm_init_irq_routing(s);
 
diff --git a/kvm-stub.c b/kvm-stub.c
index d23b11c..b4023d1 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -19,6 +19,7 @@
 
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
+bool kvm_async_interrupt_injection;
 
 int kvm_init_vcpu(CPUArchState *env)
 {
diff --git a/kvm.h b/kvm.h
index 2617dd5..00abe36 100644
--- a/kvm.h
+++ b/kvm.h
@@ -24,13 +24,24 @@
 
 extern int kvm_allowed;
 extern bool kvm_kernel_irqchip;
+extern bool kvm_async_interrupt_injection;
 
 #if defined CONFIG_KVM || !defined NEED_CPU_H
 #define kvm_enabled()           (kvm_allowed)
 #define kvm_irqchip_in_kernel() (kvm_kernel_irqchip)
+/**
+ * kvm_async_interrupt_injection:
+ *
+ * Returns: true if we can inject interrupts into a KVM CPU
+ * asynchronously (ie by ioctl from any thread at any time)
+ * rather than having to do interrupt delivery synchronously
+ * (where the vcpu must be stopped at a suitable point first).
+ */
+#define kvm_async_interrupt_injection() (kvm_async_interrupt_injection)
 #else
 #define kvm_enabled()           (0)
 #define kvm_irqchip_in_kernel() (false)
+#define kvm_async_interrupt_injection() (false)
 #endif
 
 struct kvm_run;
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(),
since it can be used for asynchronous interrupt injection whether
there is a full irqchip model in the kernel or not.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/kvm/i8259.c  |    2 +-
 hw/kvm/ioapic.c |    2 +-
 kvm-all.c       |    6 +++---
 kvm.h           |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/kvm/i8259.c b/hw/kvm/i8259.c
index 94d1b9a..ab970db 100644
--- a/hw/kvm/i8259.c
+++ b/hw/kvm/i8259.c
@@ -94,7 +94,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level)
 {
     int delivered;
 
-    delivered = kvm_irqchip_set_irq(kvm_state, irq, level);
+    delivered = kvm_inject_async_irq(kvm_state, irq, level);
     apic_report_irq_delivered(delivered);
 }
 
diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c
index 3ae3175..d7add35 100644
--- a/hw/kvm/ioapic.c
+++ b/hw/kvm/ioapic.c
@@ -82,7 +82,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
     KVMIOAPICState *s = opaque;
     int delivered;
 
-    delivered = kvm_irqchip_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
+    delivered = kvm_inject_async_irq(kvm_state, s->kvm_gsi_base + irq, level);
     apic_report_irq_delivered(delivered);
 }
 
diff --git a/kvm-all.c b/kvm-all.c
index 3354c6f..9f14274 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -853,7 +853,7 @@ static void kvm_handle_interrupt(CPUArchState *env, int mask)
     }
 }
 
-int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
+int kvm_inject_async_irq(KVMState *s, int irq, int level)
 {
     struct kvm_irq_level event;
     int ret;
@@ -864,7 +864,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
     event.irq = irq;
     ret = kvm_vm_ioctl(s, s->irqchip_inject_ioctl, &event);
     if (ret < 0) {
-        perror("kvm_set_irqchip_line");
+        perror("kvm_inject_async_irq");
         abort();
     }
 
@@ -1089,7 +1089,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
 
     assert(route->kroute.type == KVM_IRQ_ROUTING_MSI);
 
-    return kvm_irqchip_set_irq(s, route->kroute.gsi, 1);
+    return kvm_inject_async_irq(s, route->kroute.gsi, 1);
 }
 
 int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
diff --git a/kvm.h b/kvm.h
index 00abe36..cfdc95e 100644
--- a/kvm.h
+++ b/kvm.h
@@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr);
 
 void kvm_arch_init_irq_routing(KVMState *s);
 
-int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
+int kvm_inject_async_irq(KVMState *s, int irq, int level);
 int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
 
 void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
-- 
1.7.5.4

[Qemu-devel] [PATCH 3/6] kvm: Move kvm_allows_irq0_override() to target-i386

[Peter Maydell]

kvm_allows_irq0_override() is a totally x86 specific concept:
move it to the target-specific source file where it belongs.
This means we need a new header file for the prototype:
kvm_i386.h, in line with the existing kvm_ppc.h.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pc.c                   |    1 +
 kvm-all.c                 |    5 -----
 kvm-stub.c                |    5 -----
 kvm.h                     |    2 --
 target-i386/Makefile.objs |    1 +
 target-i386/kvm-stub.c    |   17 +++++++++++++++++
 target-i386/kvm.c         |    6 ++++++
 target-i386/kvm_i386.h    |   16 ++++++++++++++++
 8 files changed, 41 insertions(+), 12 deletions(-)
 create mode 100644 target-i386/kvm-stub.c
 create mode 100644 target-i386/kvm_i386.h

diff --git a/hw/pc.c b/hw/pc.c
index 598267a..4953376 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -42,6 +42,7 @@
 #include "sysbus.h"
 #include "sysemu.h"
 #include "kvm.h"
+#include "kvm_i386.h"
 #include "xen.h"
 #include "blockdev.h"
 #include "hw/block-common.h"
diff --git a/kvm-all.c b/kvm-all.c
index 9f14274..8e21d81 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1672,11 +1672,6 @@ int kvm_has_gsi_routing(void)
 #endif
 }
 
-int kvm_allows_irq0_override(void)
-{
-    return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
-}
-
 void *kvm_vmalloc(ram_addr_t size)
 {
 #ifdef TARGET_S390X
diff --git a/kvm-stub.c b/kvm-stub.c
index b4023d1..af1cb5e 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -72,11 +72,6 @@ int kvm_has_many_ioeventfds(void)
     return 0;
 }
 
-int kvm_allows_irq0_override(void)
-{
-    return 1;
-}
-
 int kvm_has_pit_state2(void)
 {
     return 0;
diff --git a/kvm.h b/kvm.h
index cfdc95e..e6cbf12 100644
--- a/kvm.h
+++ b/kvm.h
@@ -73,8 +73,6 @@ int kvm_has_pit_state2(void);
 int kvm_has_many_ioeventfds(void);
 int kvm_has_gsi_routing(void);
 
-int kvm_allows_irq0_override(void);
-
 #ifdef NEED_CPU_H
 int kvm_init_vcpu(CPUArchState *env);
 
diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
index 683fd59..0715f58 100644
--- a/target-i386/Makefile.objs
+++ b/target-i386/Makefile.objs
@@ -3,6 +3,7 @@ obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
 obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
 obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o
 obj-$(CONFIG_KVM) += kvm.o hyperv.o
+obj-$(CONFIG_NO_KVM) += kvm-stub.o
 obj-$(CONFIG_LINUX_USER) += ioport-user.o
 obj-$(CONFIG_BSD_USER) += ioport-user.o
 
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
new file mode 100644
index 0000000..30037e4
--- /dev/null
+++ b/target-i386/kvm-stub.c
@@ -0,0 +1,17 @@
+/*
+ * QEMU KVM x86 specific function stubs
+ *
+ * Copyright Linaro Limited 2012
+ *
+ * Author: Peter Maydell <peter.maydell@linaro.org>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "kvm_i386.h"
+
+int kvm_allows_irq0_override(void)
+{
+    return 1;
+}
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index e53c2f6..503abeb 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -23,6 +23,7 @@
 #include "qemu-common.h"
 #include "sysemu.h"
 #include "kvm.h"
+#include "kvm_i386.h"
 #include "cpu.h"
 #include "gdbstub.h"
 #include "host-utils.h"
@@ -65,6 +66,11 @@ static bool has_msr_async_pf_en;
 static bool has_msr_misc_enable;
 static int lm_capable_kernel;
 
+int kvm_allows_irq0_override(void)
+{
+    return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
+}
+
 static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
 {
     struct kvm_cpuid2 *cpuid;
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
new file mode 100644
index 0000000..87031c0
--- /dev/null
+++ b/target-i386/kvm_i386.h
@@ -0,0 +1,16 @@
+/*
+ * QEMU KVM support -- x86 specific functions.
+ *
+ * Copyright (c) 2012 Linaro Limited
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_KVM_I386_H
+#define QEMU_KVM_I386_H
+
+int kvm_allows_irq0_override(void);
+
+#endif
-- 
1.7.5.4

[Qemu-devel] [PATCH 4/6] kvm: Don't assume irqchip-in-kernel implies irqfds

[Peter Maydell]

Instead of assuming that we can use irqfds if and only if
kvm_irqchip_in_kernel(), add a bool to the KVMState which
indicates this, and is set only on x86 and only if the
irqchip is in the kernel.

The kernel documentation implies that the only thing
you need to use KVM_IRQFD is that KVM_CAP_IRQFD is
advertised, but this seems to be untrue. In particular
the kernel does not (alas) return a sensible error if you
try to set up an irqfd when you haven't created an irqchip.
If it did we could remove all this nonsense and let the
kernel return the error code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 kvm-all.c         |    3 ++-
 kvm-stub.c        |    1 +
 kvm.h             |   10 ++++++++++
 target-i386/kvm.c |    4 ++++
 4 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 8e21d81..a88b8ad 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -101,6 +101,7 @@ struct KVMState
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupt_injection;
+bool kvm_irqfds_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
@@ -1126,7 +1127,7 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, int fd, int virq, bool assign)
         .flags = assign ? 0 : KVM_IRQFD_FLAG_DEASSIGN,
     };
 
-    if (!kvm_irqchip_in_kernel()) {
+    if (!kvm_irqfds_enabled()) {
         return -ENOSYS;
     }
 
diff --git a/kvm-stub.c b/kvm-stub.c
index af1cb5e..179e5de 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -20,6 +20,7 @@
 KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupt_injection;
+bool kvm_irqfds_allowed;
 
 int kvm_init_vcpu(CPUArchState *env)
 {
diff --git a/kvm.h b/kvm.h
index e6cbf12..2337eb0 100644
--- a/kvm.h
+++ b/kvm.h
@@ -25,6 +25,7 @@
 extern int kvm_allowed;
 extern bool kvm_kernel_irqchip;
 extern bool kvm_async_interrupt_injection;
+extern bool kvm_irqfds_allowed;
 
 #if defined CONFIG_KVM || !defined NEED_CPU_H
 #define kvm_enabled()           (kvm_allowed)
@@ -38,10 +39,19 @@ extern bool kvm_async_interrupt_injection;
  * (where the vcpu must be stopped at a suitable point first).
  */
 #define kvm_async_interrupt_injection() (kvm_async_interrupt_injection)
+/**
+ * kvm_irqfds_enabled:
+ *
+ * Returns: true if we can use irqfds to inject interrupts into
+ * a KVM CPU (ie the kernel supports irqfds and we are running
+ * with a configuration where it is meaningful to use them).
+ */
+#define kvm_irqfds_enabled() (kvm_irqfds_allowed)
 #else
 #define kvm_enabled()           (0)
 #define kvm_irqchip_in_kernel() (false)
 #define kvm_async_interrupt_injection() (false)
+#define kvm_irqfds_enabled() (false)
 #endif
 
 struct kvm_run;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 503abeb..8e19a4d 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2045,4 +2045,8 @@ void kvm_arch_init_irq_routing(KVMState *s)
          */
         no_hpet = 1;
     }
+    /* We know at this point that we're using the in-kernel
+     * irqchip, so we can use irqfds.
+     */
+    kvm_irqfds_allowed = true;
 }
-- 
1.7.5.4

[Qemu-devel] [PATCH 5/6] kvm: Don't assume irqchip implies MSI routing via irqfds

[Peter Maydell]

Decouple another x86-specific assumption about what irqchips imply.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/virtio-pci.c   |    4 ++--
 kvm-all.c         |    3 ++-
 kvm-stub.c        |    1 +
 kvm.h             |   12 ++++++++++++
 target-i386/kvm.c |    4 +++-
 5 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 4e03f0b..98e02ef 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -627,7 +627,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
     int r, n;
 
     /* Must unset vector notifier while guest notifier is still assigned */
-    if (kvm_irqchip_in_kernel() && !assign) {
+    if (kvm_msi_via_irqfd_enabled() && !assign) {
         msix_unset_vector_notifiers(&proxy->pci_dev);
         g_free(proxy->vector_irqfd);
         proxy->vector_irqfd = NULL;
@@ -645,7 +645,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
     }
 
     /* Must set vector notifier after guest notifier has been assigned */
-    if (kvm_irqchip_in_kernel() && assign) {
+    if (kvm_msi_via_irqfd_enabled() && assign) {
         proxy->vector_irqfd =
             g_malloc0(sizeof(*proxy->vector_irqfd) *
                       msix_nr_vectors_allocated(&proxy->pci_dev));
diff --git a/kvm-all.c b/kvm-all.c
index a88b8ad..c624137 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -102,6 +102,7 @@ KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupt_injection;
 bool kvm_irqfds_allowed;
+bool kvm_msi_via_irqfd_allowed;
 
 static const KVMCapabilityInfo kvm_required_capabilites[] = {
     KVM_CAP_INFO(USER_MEMORY),
@@ -1098,7 +1099,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
     struct kvm_irq_routing_entry kroute;
     int virq;
 
-    if (!kvm_irqchip_in_kernel()) {
+    if (!kvm_msi_via_irqfd_enabled()) {
         return -ENOSYS;
     }
 
diff --git a/kvm-stub.c b/kvm-stub.c
index 179e5de..6cdeb1c 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -21,6 +21,7 @@ KVMState *kvm_state;
 bool kvm_kernel_irqchip;
 bool kvm_async_interrupt_injection;
 bool kvm_irqfds_allowed;
+bool kvm_msi_via_irqfd_allowed;
 
 int kvm_init_vcpu(CPUArchState *env)
 {
diff --git a/kvm.h b/kvm.h
index 2337eb0..1449795 100644
--- a/kvm.h
+++ b/kvm.h
@@ -26,6 +26,7 @@ extern int kvm_allowed;
 extern bool kvm_kernel_irqchip;
 extern bool kvm_async_interrupt_injection;
 extern bool kvm_irqfds_allowed;
+extern bool kvm_msi_via_irqfd_allowed;
 
 #if defined CONFIG_KVM || !defined NEED_CPU_H
 #define kvm_enabled()           (kvm_allowed)
@@ -47,11 +48,22 @@ extern bool kvm_irqfds_allowed;
  * with a configuration where it is meaningful to use them).
  */
 #define kvm_irqfds_enabled() (kvm_irqfds_allowed)
+/**
+ * kvm_msi_via_irqfd_enabled:
+ *
+ * Returns: true if we can route a PCI MSI (Message Signaled Interrupt)
+ * to a KVM CPU via an irqfd. This requires that the kernel supports
+ * this and that we're running in a configuration that permits it.
+ * This should be checked before calling MSI related functions such as
+ * kvm_irqchip_add_msi_route.
+ */
+#define kvm_msi_via_irqfd_enabled() (kvm_msi_via_irqfd_allowed)
 #else
 #define kvm_enabled()           (0)
 #define kvm_irqchip_in_kernel() (false)
 #define kvm_async_interrupt_injection() (false)
 #define kvm_irqfds_enabled() (false)
+#define kvm_msi_via_irqfd_enabled() (false)
 #endif
 
 struct kvm_run;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 8e19a4d..03db818 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2046,7 +2046,9 @@ void kvm_arch_init_irq_routing(KVMState *s)
         no_hpet = 1;
     }
     /* We know at this point that we're using the in-kernel
-     * irqchip, so we can use irqfds.
+     * irqchip, so we can use irqfds, and on x86 we know
+     * we can use msi via irqfd.
      */
     kvm_irqfds_allowed = true;
+    kvm_msi_via_irqfd_allowed = true;
 }
-- 
1.7.5.4

[Qemu-devel] [PATCH 6/6] kvm: Add documentation comment for kvm_irqchip_in_kernel()

[Peter Maydell]

Now we've cleared out the architecture-independent uses of
kvm_irqchip_in_kernel(), we can add a doc comment describing
what it means.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 kvm.h |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/kvm.h b/kvm.h
index 1449795..ae9df2d 100644
--- a/kvm.h
+++ b/kvm.h
@@ -30,6 +30,17 @@ extern bool kvm_msi_via_irqfd_allowed;
 
 #if defined CONFIG_KVM || !defined NEED_CPU_H
 #define kvm_enabled()           (kvm_allowed)
+/**
+ * kvm_irqchip_in_kernel:
+ *
+ * Returns: true if the user asked us to create an in-kernel
+ * irqchip via the "kernel_irqchip=on" machine option.
+ * What this actually means is architecture and machine model
+ * specific: on PC, for instance, it means that the LAPIC,
+ * IOAPIC and PIT are all in kernel. This function should never
+ * be used from generic target-independent code: use one of the
+ * following functions or some other specific check instead.
+ */
 #define kvm_irqchip_in_kernel() (kvm_kernel_irqchip)
 /**
  * kvm_async_interrupt_injection:
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 6/6] kvm: Add documentation comment for kvm_irqchip_in_kernel()

[Andreas Färber]

Am 25.07.2012 15:24, schrieb Peter Maydell:
> Now we've cleared out the architecture-independent uses of
> kvm_irqchip_in_kernel(), we can add a doc comment describing
> what it means.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  kvm.h |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)

If you've cleared the arch-independent uses, can't it be moved out of
the generic kvm.h? Otherwise if just the commit message is confusing me:

> 
> diff --git a/kvm.h b/kvm.h
> index 1449795..ae9df2d 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -30,6 +30,17 @@ extern bool kvm_msi_via_irqfd_allowed;
>  
>  #if defined CONFIG_KVM || !defined NEED_CPU_H
>  #define kvm_enabled()           (kvm_allowed)

Could we add a white line here...

> +/**
> + * kvm_irqchip_in_kernel:
> + *
> + * Returns: true if the user asked us to create an in-kernel
> + * irqchip via the "kernel_irqchip=on" machine option.
> + * What this actually means is architecture and machine model
> + * specific: on PC, for instance, it means that the LAPIC,
> + * IOAPIC and PIT are all in kernel. This function should never
> + * be used from generic target-independent code: use one of the
> + * following functions or some other specific check instead.
> + */
>  #define kvm_irqchip_in_kernel() (kvm_kernel_irqchip)

...and here, to better group the macros and their documentation?

Andreas

>  /**
>   * kvm_async_interrupt_injection:
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/6] kvm: Decouple 'interrupt injection is async' from 'kernel irqchip'

[Jan Kiszka]

On 2012-07-25 15:24, Peter Maydell wrote:
> On x86 interrupt injection is asynchronous (and therefore
> VCPU idle management is done in the kernel) if and only
> if there is an in-kernel irqchip. On other architectures this
> isn't necessarily true (they may always do asynchronous
> injection), so define a new kvm_async_interrupt_injection()
> function instead of misusing kvm_irqchip_in_kernel().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  cpus.c     |    3 ++-
>  kvm-all.c  |    7 ++++++-
>  kvm-stub.c |    1 +
>  kvm.h      |   11 +++++++++++
>  4 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 756e624..584b298 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -69,7 +69,8 @@ static bool cpu_thread_is_idle(CPUArchState *env)
>      if (env->stopped || !runstate_is_running()) {
>          return true;
>      }
> -    if (!env->halted || qemu_cpu_has_work(env) || kvm_irqchip_in_kernel()) {
> +    if (!env->halted || qemu_cpu_has_work(env) ||
> +        kvm_async_interrupt_injection()) {
>          return false;
>      }
>      return true;
> diff --git a/kvm-all.c b/kvm-all.c
> index 2148b20..3354c6f 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -100,6 +100,7 @@ struct KVMState
>  
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
> +bool kvm_async_interrupt_injection;
>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -857,7 +858,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
>      struct kvm_irq_level event;
>      int ret;
>  
> -    assert(kvm_irqchip_in_kernel());
> +    assert(kvm_async_interrupt_injection());
>  
>      event.level = level;
>      event.irq = irq;
> @@ -1201,6 +1202,10 @@ static int kvm_irqchip_create(KVMState *s)
>          s->irqchip_inject_ioctl = KVM_IRQ_LINE_STATUS;
>      }
>      kvm_kernel_irqchip = true;
> +    /* If we have an in-kernel IRQ chip then we must have asynchronous
> +     * interrupt injection (though the reverse is not necessarily true)
> +     */
> +    kvm_async_interrupt_injection = true;
>  
>      kvm_init_irq_routing(s);
>  
> diff --git a/kvm-stub.c b/kvm-stub.c
> index d23b11c..b4023d1 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -19,6 +19,7 @@
>  
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
> +bool kvm_async_interrupt_injection;
>  
>  int kvm_init_vcpu(CPUArchState *env)
>  {
> diff --git a/kvm.h b/kvm.h
> index 2617dd5..00abe36 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -24,13 +24,24 @@
>  
>  extern int kvm_allowed;
>  extern bool kvm_kernel_irqchip;
> +extern bool kvm_async_interrupt_injection;
>  
>  #if defined CONFIG_KVM || !defined NEED_CPU_H
>  #define kvm_enabled()           (kvm_allowed)
>  #define kvm_irqchip_in_kernel() (kvm_kernel_irqchip)
> +/**
> + * kvm_async_interrupt_injection:
> + *
> + * Returns: true if we can inject interrupts into a KVM CPU
> + * asynchronously (ie by ioctl from any thread at any time)
> + * rather than having to do interrupt delivery synchronously
> + * (where the vcpu must be stopped at a suitable point first).
> + */
> +#define kvm_async_interrupt_injection() (kvm_async_interrupt_injection)
>  #else
>  #define kvm_enabled()           (0)
>  #define kvm_irqchip_in_kernel() (false)
> +#define kvm_async_interrupt_injection() (false)
>  #endif
>  
>  struct kvm_run;
> 

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 15:24, Peter Maydell wrote:
> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(),
> since it can be used for asynchronous interrupt injection whether
> there is a full irqchip model in the kernel or not.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/kvm/i8259.c  |    2 +-
>  hw/kvm/ioapic.c |    2 +-
>  kvm-all.c       |    6 +++---
>  kvm.h           |    2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/kvm/i8259.c b/hw/kvm/i8259.c
> index 94d1b9a..ab970db 100644
> --- a/hw/kvm/i8259.c
> +++ b/hw/kvm/i8259.c
> @@ -94,7 +94,7 @@ static void kvm_pic_set_irq(void *opaque, int irq, int level)
>  {
>      int delivered;
>  
> -    delivered = kvm_irqchip_set_irq(kvm_state, irq, level);
> +    delivered = kvm_inject_async_irq(kvm_state, irq, level);
>      apic_report_irq_delivered(delivered);
>  }
>  
> diff --git a/hw/kvm/ioapic.c b/hw/kvm/ioapic.c
> index 3ae3175..d7add35 100644
> --- a/hw/kvm/ioapic.c
> +++ b/hw/kvm/ioapic.c
> @@ -82,7 +82,7 @@ static void kvm_ioapic_set_irq(void *opaque, int irq, int level)
>      KVMIOAPICState *s = opaque;
>      int delivered;
>  
> -    delivered = kvm_irqchip_set_irq(kvm_state, s->kvm_gsi_base + irq, level);
> +    delivered = kvm_inject_async_irq(kvm_state, s->kvm_gsi_base + irq, level);
>      apic_report_irq_delivered(delivered);
>  }
>  
> diff --git a/kvm-all.c b/kvm-all.c
> index 3354c6f..9f14274 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -853,7 +853,7 @@ static void kvm_handle_interrupt(CPUArchState *env, int mask)
>      }
>  }
>  
> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
> +int kvm_inject_async_irq(KVMState *s, int irq, int level)
>  {
>      struct kvm_irq_level event;
>      int ret;
> @@ -864,7 +864,7 @@ int kvm_irqchip_set_irq(KVMState *s, int irq, int level)
>      event.irq = irq;
>      ret = kvm_vm_ioctl(s, s->irqchip_inject_ioctl, &event);
>      if (ret < 0) {
> -        perror("kvm_set_irqchip_line");
> +        perror("kvm_inject_async_irq");
>          abort();
>      }
>  
> @@ -1089,7 +1089,7 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
>  
>      assert(route->kroute.type == KVM_IRQ_ROUTING_MSI);
>  
> -    return kvm_irqchip_set_irq(s, route->kroute.gsi, 1);
> +    return kvm_inject_async_irq(s, route->kroute.gsi, 1);
>  }
>  
>  int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
> diff --git a/kvm.h b/kvm.h
> index 00abe36..cfdc95e 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr);
>  
>  void kvm_arch_init_irq_routing(KVMState *s);
>  
> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
> +int kvm_inject_async_irq(KVMState *s, int irq, int level);
>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  
>  void kvm_irqchip_add_irq_route(KVMState *s, int gsi, int irqchip, int pin);
> 

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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

Re: [Qemu-devel] [PATCH 3/6] kvm: Move kvm_allows_irq0_override() to target-i386

[Jan Kiszka]

On 2012-07-25 15:24, Peter Maydell wrote:
> kvm_allows_irq0_override() is a totally x86 specific concept:
> move it to the target-specific source file where it belongs.
> This means we need a new header file for the prototype:
> kvm_i386.h, in line with the existing kvm_ppc.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/pc.c                   |    1 +
>  kvm-all.c                 |    5 -----
>  kvm-stub.c                |    5 -----
>  kvm.h                     |    2 --
>  target-i386/Makefile.objs |    1 +
>  target-i386/kvm-stub.c    |   17 +++++++++++++++++
>  target-i386/kvm.c         |    6 ++++++
>  target-i386/kvm_i386.h    |   16 ++++++++++++++++
>  8 files changed, 41 insertions(+), 12 deletions(-)
>  create mode 100644 target-i386/kvm-stub.c
>  create mode 100644 target-i386/kvm_i386.h
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 598267a..4953376 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -42,6 +42,7 @@
>  #include "sysbus.h"
>  #include "sysemu.h"
>  #include "kvm.h"
> +#include "kvm_i386.h"
>  #include "xen.h"
>  #include "blockdev.h"
>  #include "hw/block-common.h"
> diff --git a/kvm-all.c b/kvm-all.c
> index 9f14274..8e21d81 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1672,11 +1672,6 @@ int kvm_has_gsi_routing(void)
>  #endif
>  }
>  
> -int kvm_allows_irq0_override(void)
> -{
> -    return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
> -}
> -
>  void *kvm_vmalloc(ram_addr_t size)
>  {
>  #ifdef TARGET_S390X
> diff --git a/kvm-stub.c b/kvm-stub.c
> index b4023d1..af1cb5e 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -72,11 +72,6 @@ int kvm_has_many_ioeventfds(void)
>      return 0;
>  }
>  
> -int kvm_allows_irq0_override(void)
> -{
> -    return 1;
> -}
> -
>  int kvm_has_pit_state2(void)
>  {
>      return 0;
> diff --git a/kvm.h b/kvm.h
> index cfdc95e..e6cbf12 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -73,8 +73,6 @@ int kvm_has_pit_state2(void);
>  int kvm_has_many_ioeventfds(void);
>  int kvm_has_gsi_routing(void);
>  
> -int kvm_allows_irq0_override(void);
> -
>  #ifdef NEED_CPU_H
>  int kvm_init_vcpu(CPUArchState *env);
>  
> diff --git a/target-i386/Makefile.objs b/target-i386/Makefile.objs
> index 683fd59..0715f58 100644
> --- a/target-i386/Makefile.objs
> +++ b/target-i386/Makefile.objs
> @@ -3,6 +3,7 @@ obj-y += excp_helper.o fpu_helper.o cc_helper.o int_helper.o svm_helper.o
>  obj-y += smm_helper.o misc_helper.o mem_helper.o seg_helper.o
>  obj-$(CONFIG_SOFTMMU) += machine.o arch_memory_mapping.o arch_dump.o
>  obj-$(CONFIG_KVM) += kvm.o hyperv.o
> +obj-$(CONFIG_NO_KVM) += kvm-stub.o
>  obj-$(CONFIG_LINUX_USER) += ioport-user.o
>  obj-$(CONFIG_BSD_USER) += ioport-user.o
>  
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> new file mode 100644
> index 0000000..30037e4
> --- /dev/null
> +++ b/target-i386/kvm-stub.c
> @@ -0,0 +1,17 @@
> +/*
> + * QEMU KVM x86 specific function stubs
> + *
> + * Copyright Linaro Limited 2012
> + *
> + * Author: Peter Maydell <peter.maydell@linaro.org>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +#include "kvm_i386.h"
> +
> +int kvm_allows_irq0_override(void)
> +{
> +    return 1;
> +}
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index e53c2f6..503abeb 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -23,6 +23,7 @@
>  #include "qemu-common.h"
>  #include "sysemu.h"
>  #include "kvm.h"
> +#include "kvm_i386.h"
>  #include "cpu.h"
>  #include "gdbstub.h"
>  #include "host-utils.h"
> @@ -65,6 +66,11 @@ static bool has_msr_async_pf_en;
>  static bool has_msr_misc_enable;
>  static int lm_capable_kernel;
>  
> +int kvm_allows_irq0_override(void)
> +{
> +    return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
> +}
> +
>  static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
>  {
>      struct kvm_cpuid2 *cpuid;
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> new file mode 100644
> index 0000000..87031c0
> --- /dev/null
> +++ b/target-i386/kvm_i386.h
> @@ -0,0 +1,16 @@
> +/*
> + * QEMU KVM support -- x86 specific functions.
> + *
> + * Copyright (c) 2012 Linaro Limited
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef QEMU_KVM_I386_H
> +#define QEMU_KVM_I386_H
> +
> +int kvm_allows_irq0_override(void);

Make it bool at this chance?

> +
> +#endif
> 

This looks better.

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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

Re: [Qemu-devel] [PATCH 4/6] kvm: Don't assume irqchip-in-kernel implies irqfds

[Jan Kiszka]

On 2012-07-25 15:24, Peter Maydell wrote:
> Instead of assuming that we can use irqfds if and only if
> kvm_irqchip_in_kernel(), add a bool to the KVMState which
> indicates this, and is set only on x86 and only if the
> irqchip is in the kernel.
> 
> The kernel documentation implies that the only thing
> you need to use KVM_IRQFD is that KVM_CAP_IRQFD is
> advertised, but this seems to be untrue. In particular
> the kernel does not (alas) return a sensible error if you
> try to set up an irqfd when you haven't created an irqchip.
> If it did we could remove all this nonsense and let the
> kernel return the error code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  kvm-all.c         |    3 ++-
>  kvm-stub.c        |    1 +
>  kvm.h             |   10 ++++++++++
>  target-i386/kvm.c |    4 ++++
>  4 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 8e21d81..a88b8ad 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -101,6 +101,7 @@ struct KVMState
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
> +bool kvm_irqfds_allowed;

Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.

>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -1126,7 +1127,7 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, int fd, int virq, bool assign)
>          .flags = assign ? 0 : KVM_IRQFD_FLAG_DEASSIGN,
>      };
>  
> -    if (!kvm_irqchip_in_kernel()) {
> +    if (!kvm_irqfds_enabled()) {
>          return -ENOSYS;
>      }
>  
> diff --git a/kvm-stub.c b/kvm-stub.c
> index af1cb5e..179e5de 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -20,6 +20,7 @@
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
> +bool kvm_irqfds_allowed;
>  
>  int kvm_init_vcpu(CPUArchState *env)
>  {
> diff --git a/kvm.h b/kvm.h
> index e6cbf12..2337eb0 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -25,6 +25,7 @@
>  extern int kvm_allowed;
>  extern bool kvm_kernel_irqchip;
>  extern bool kvm_async_interrupt_injection;
> +extern bool kvm_irqfds_allowed;
>  
>  #if defined CONFIG_KVM || !defined NEED_CPU_H
>  #define kvm_enabled()           (kvm_allowed)
> @@ -38,10 +39,19 @@ extern bool kvm_async_interrupt_injection;
>   * (where the vcpu must be stopped at a suitable point first).
>   */
>  #define kvm_async_interrupt_injection() (kvm_async_interrupt_injection)
> +/**
> + * kvm_irqfds_enabled:
> + *
> + * Returns: true if we can use irqfds to inject interrupts into
> + * a KVM CPU (ie the kernel supports irqfds and we are running
> + * with a configuration where it is meaningful to use them).
> + */
> +#define kvm_irqfds_enabled() (kvm_irqfds_allowed)
>  #else
>  #define kvm_enabled()           (0)
>  #define kvm_irqchip_in_kernel() (false)
>  #define kvm_async_interrupt_injection() (false)
> +#define kvm_irqfds_enabled() (false)
>  #endif
>  
>  struct kvm_run;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 503abeb..8e19a4d 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2045,4 +2045,8 @@ void kvm_arch_init_irq_routing(KVMState *s)
>           */
>          no_hpet = 1;
>      }
> +    /* We know at this point that we're using the in-kernel
> +     * irqchip, so we can use irqfds.
> +     */
> +    kvm_irqfds_allowed = true;
>  }
> 

Otherwise:

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Avi Kivity]

On 07/25/2012 04:24 PM, Peter Maydell wrote:
> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(),
> since it can be used for asynchronous interrupt injection whether
> there is a full irqchip model in the kernel or not.
> 

> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr);
>  
>  void kvm_arch_init_irq_routing(KVMState *s);
>  
> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
> +int kvm_inject_async_irq(KVMState *s, int irq, int level);
>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>  

"interrupt injection" refers to the act of setting up an interrupt to be
delivered to the guest at the next entry, so it is synchronous by
nature.  It was sloppy of me to use the term, but let's not introduce it
to the code as well.

The correct terminology would be:
  interrupt injection: synchronous, in-vcpu, after all masks have been
evaluated.  Straight into the vein.
  interrupt queueing: asynchronous, extra-vcpu, before any masks

Since interrupt queueing (well that name isn't perfect for level
triggered interrupts, since it may not queue anything...) is the norm, I
think it's better to keep the set_irq() naming and mark the synchronous
interrupt injection function as special.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 5/6] kvm: Don't assume irqchip implies MSI routing via irqfds

[Jan Kiszka]

On 2012-07-25 15:24, Peter Maydell wrote:
> Decouple another x86-specific assumption about what irqchips imply.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/virtio-pci.c   |    4 ++--
>  kvm-all.c         |    3 ++-
>  kvm-stub.c        |    1 +
>  kvm.h             |   12 ++++++++++++
>  target-i386/kvm.c |    4 +++-
>  5 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 4e03f0b..98e02ef 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -627,7 +627,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
>      int r, n;
>  
>      /* Must unset vector notifier while guest notifier is still assigned */
> -    if (kvm_irqchip_in_kernel() && !assign) {
> +    if (kvm_msi_via_irqfd_enabled() && !assign) {
>          msix_unset_vector_notifiers(&proxy->pci_dev);
>          g_free(proxy->vector_irqfd);
>          proxy->vector_irqfd = NULL;
> @@ -645,7 +645,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign)
>      }
>  
>      /* Must set vector notifier after guest notifier has been assigned */
> -    if (kvm_irqchip_in_kernel() && assign) {
> +    if (kvm_msi_via_irqfd_enabled() && assign) {
>          proxy->vector_irqfd =
>              g_malloc0(sizeof(*proxy->vector_irqfd) *
>                        msix_nr_vectors_allocated(&proxy->pci_dev));

These are ok.

> diff --git a/kvm-all.c b/kvm-all.c
> index a88b8ad..c624137 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -102,6 +102,7 @@ KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
>  bool kvm_irqfds_allowed;
> +bool kvm_msi_via_irqfd_allowed;
>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -1098,7 +1099,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, MSIMessage msg)
>      struct kvm_irq_routing_entry kroute;
>      int virq;
>  
> -    if (!kvm_irqchip_in_kernel()) {
> +    if (!kvm_msi_via_irqfd_enabled()) {

This is semantically wrong. Currently, this function is only used for
irqfd. But we will also use it to prepare MSI injections by KVM device
assignment. For other (yet non-existent) in-kernel sources of MSIs we
could use it as well. The proper check is some active version of
has_gsi_routing.

>          return -ENOSYS;
>      }
>  
> diff --git a/kvm-stub.c b/kvm-stub.c
> index 179e5de..6cdeb1c 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -21,6 +21,7 @@ KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
>  bool kvm_irqfds_allowed;
> +bool kvm_msi_via_irqfd_allowed;
>  
>  int kvm_init_vcpu(CPUArchState *env)
>  {
> diff --git a/kvm.h b/kvm.h
> index 2337eb0..1449795 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -26,6 +26,7 @@ extern int kvm_allowed;
>  extern bool kvm_kernel_irqchip;
>  extern bool kvm_async_interrupt_injection;
>  extern bool kvm_irqfds_allowed;
> +extern bool kvm_msi_via_irqfd_allowed;
>  
>  #if defined CONFIG_KVM || !defined NEED_CPU_H
>  #define kvm_enabled()           (kvm_allowed)
> @@ -47,11 +48,22 @@ extern bool kvm_irqfds_allowed;
>   * with a configuration where it is meaningful to use them).
>   */
>  #define kvm_irqfds_enabled() (kvm_irqfds_allowed)
> +/**
> + * kvm_msi_via_irqfd_enabled:
> + *
> + * Returns: true if we can route a PCI MSI (Message Signaled Interrupt)
> + * to a KVM CPU via an irqfd. This requires that the kernel supports
> + * this and that we're running in a configuration that permits it.
> + * This should be checked before calling MSI related functions such as
> + * kvm_irqchip_add_msi_route.

See above.

> + */
> +#define kvm_msi_via_irqfd_enabled() (kvm_msi_via_irqfd_allowed)
>  #else
>  #define kvm_enabled()           (0)
>  #define kvm_irqchip_in_kernel() (false)
>  #define kvm_async_interrupt_injection() (false)
>  #define kvm_irqfds_enabled() (false)
> +#define kvm_msi_via_irqfd_enabled() (false)
>  #endif
>  
>  struct kvm_run;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 8e19a4d..03db818 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2046,7 +2046,9 @@ void kvm_arch_init_irq_routing(KVMState *s)
>          no_hpet = 1;
>      }
>      /* We know at this point that we're using the in-kernel
> -     * irqchip, so we can use irqfds.
> +     * irqchip, so we can use irqfds, and on x86 we know
> +     * we can use msi via irqfd.
>       */
>      kvm_irqfds_allowed = true;
> +    kvm_msi_via_irqfd_allowed = true;
>  }
> 

Jan

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

Re: [Qemu-devel] [PATCH 4/6] kvm: Don't assume irqchip-in-kernel implies irqfds

[Peter Maydell]

On 25 July 2012 16:47, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-25 15:24, Peter Maydell wrote:
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -101,6 +101,7 @@ struct KVMState
>>  KVMState *kvm_state;
>>  bool kvm_kernel_irqchip;
>>  bool kvm_async_interrupt_injection;
>> +bool kvm_irqfds_allowed;
>
> Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.

I was trying to follow the existing pattern where the macro kvm_enabled()
tests the variable kvm_allowed (though as you noticed I got it wrong for
kvm_async_interrupt_injection: will fix that in v2.)

Having the two the same is valid C, it's just a style question whether
having a variable foo and a macro foo() is considered confusing I guess.

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 17:47, Avi Kivity wrote:
> On 07/25/2012 04:24 PM, Peter Maydell wrote:
>> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(),
>> since it can be used for asynchronous interrupt injection whether
>> there is a full irqchip model in the kernel or not.
>>
> 
>> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr);
>>  
>>  void kvm_arch_init_irq_routing(KVMState *s);
>>  
>> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
>> +int kvm_inject_async_irq(KVMState *s, int irq, int level);
>>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>>  
> 
> "interrupt injection" refers to the act of setting up an interrupt to be
> delivered to the guest at the next entry, so it is synchronous by
> nature.  It was sloppy of me to use the term, but let's not introduce it
> to the code as well.
> 
> The correct terminology would be:
>   interrupt injection: synchronous, in-vcpu, after all masks have been
> evaluated.  Straight into the vein.
>   interrupt queueing: asynchronous, extra-vcpu, before any masks
> 
> Since interrupt queueing (well that name isn't perfect for level
> triggered interrupts, since it may not queue anything...) is the norm, I
> think it's better to keep the set_irq() naming and mark the synchronous
> interrupt injection function as special.

We don't have a synchronous function anymore, it's part of the pre-run
code of x86 IIRC.

Jan

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

Re: [Qemu-devel] [PATCH 4/6] kvm: Don't assume irqchip-in-kernel implies irqfds

[Jan Kiszka]

On 2012-07-25 17:52, Peter Maydell wrote:
> On 25 July 2012 16:47, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-25 15:24, Peter Maydell wrote:
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -101,6 +101,7 @@ struct KVMState
>>>  KVMState *kvm_state;
>>>  bool kvm_kernel_irqchip;
>>>  bool kvm_async_interrupt_injection;
>>> +bool kvm_irqfds_allowed;
>>
>> Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.
> 
> I was trying to follow the existing pattern where the macro kvm_enabled()
> tests the variable kvm_allowed (though as you noticed I got it wrong for
> kvm_async_interrupt_injection: will fix that in v2.)
> 
> Having the two the same is valid C, it's just a style question whether
> having a variable foo and a macro foo() is considered confusing I guess.

I don't mind which way if they are consistent.

Jan

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Avi Kivity]

On 07/25/2012 06:53 PM, Jan Kiszka wrote:
> On 2012-07-25 17:47, Avi Kivity wrote:
>> On 07/25/2012 04:24 PM, Peter Maydell wrote:
>>> Rename the function kvm_irqchip_set_irq() to kvm_inject_async_irq(),
>>> since it can be used for asynchronous interrupt injection whether
>>> there is a full irqchip model in the kernel or not.
>>>
>> 
>>> @@ -144,7 +144,7 @@ int kvm_arch_on_sigbus(int code, void *addr);
>>>  
>>>  void kvm_arch_init_irq_routing(KVMState *s);
>>>  
>>> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
>>> +int kvm_inject_async_irq(KVMState *s, int irq, int level);
>>>  int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg);
>>>  
>> 
>> "interrupt injection" refers to the act of setting up an interrupt to be
>> delivered to the guest at the next entry, so it is synchronous by
>> nature.  It was sloppy of me to use the term, but let's not introduce it
>> to the code as well.
>> 
>> The correct terminology would be:
>>   interrupt injection: synchronous, in-vcpu, after all masks have been
>> evaluated.  Straight into the vein.
>>   interrupt queueing: asynchronous, extra-vcpu, before any masks
>> 
>> Since interrupt queueing (well that name isn't perfect for level
>> triggered interrupts, since it may not queue anything...) is the norm, I
>> think it's better to keep the set_irq() naming and mark the synchronous
>> interrupt injection function as special.
> 
> We don't have a synchronous function anymore, it's part of the pre-run
> code of x86 IIRC.

Right.  There's a DPRINTF() there that talks about injection, too.  So I
think this patch can be dropped.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 16:47, Avi Kivity <avi@redhat.com> wrote:
> On 07/25/2012 04:24 PM, Peter Maydell wrote:
>> -int kvm_irqchip_set_irq(KVMState *s, int irq, int level);
>> +int kvm_inject_async_irq(KVMState *s, int irq, int level);

> "interrupt injection" refers to the act of setting up an interrupt to be
> delivered to the guest at the next entry, so it is synchronous by
> nature.  It was sloppy of me to use the term, but let's not introduce it
> to the code as well.

Ah. I'd assumed it just meant "inject this into the kernel".
(ie the point at which the interrupt is no longer qemu's problem :-))

> The correct terminology would be:
>   interrupt injection: synchronous, in-vcpu, after all masks have been
> evaluated.  Straight into the vein.
>   interrupt queueing: asynchronous, extra-vcpu, before any masks
>
> Since interrupt queueing (well that name isn't perfect for level
> triggered interrupts, since it may not queue anything...) is the norm, I
> think it's better to keep the set_irq() naming and mark the synchronous
> interrupt injection function as special.

So just call this 'kvm_set_irq()' ?

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote:
> On 07/25/2012 06:53 PM, Jan Kiszka wrote:
>> We don't have a synchronous function anymore, it's part of the pre-run
>> code of x86 IIRC.
>
> Right.  There's a DPRINTF() there that talks about injection, too.  So I
> think this patch can be dropped.

The main purpose of the patch is to remove 'irqchip' from the
function name, because the function isn't restricted to use
with in-kernel irqchips.

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 17:56, Peter Maydell wrote:
> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote:
>> On 07/25/2012 06:53 PM, Jan Kiszka wrote:
>>> We don't have a synchronous function anymore, it's part of the pre-run
>>> code of x86 IIRC.
>>
>> Right.  There's a DPRINTF() there that talks about injection, too.  So I
>> think this patch can be dropped.
> 
> The main purpose of the patch is to remove 'irqchip' from the
> function name, because the function isn't restricted to use
> with in-kernel irqchips.

Hmm, what was question again? Ah: Do we have an arch that implements it
without providing a (logical) irqchip? At least at this time (including
ARM)?

Jan

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Avi Kivity]

On 07/25/2012 06:56 PM, Peter Maydell wrote:
> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote:
>> On 07/25/2012 06:53 PM, Jan Kiszka wrote:
>>> We don't have a synchronous function anymore, it's part of the pre-run
>>> code of x86 IIRC.
>>
>> Right.  There's a DPRINTF() there that talks about injection, too.  So I
>> think this patch can be dropped.
> 
> The main purpose of the patch is to remove 'irqchip' from the
> function name, because the function isn't restricted to use
> with in-kernel irqchips.

kvm_set_irq(), then.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Avi Kivity]

On 07/25/2012 06:55 PM, Peter Maydell wrote:
>> The correct terminology would be:
>>   interrupt injection: synchronous, in-vcpu, after all masks have been
>> evaluated.  Straight into the vein.
>>   interrupt queueing: asynchronous, extra-vcpu, before any masks
>>
>> Since interrupt queueing (well that name isn't perfect for level
>> triggered interrupts, since it may not queue anything...) is the norm, I
>> think it's better to keep the set_irq() naming and mark the synchronous
>> interrupt injection function as special.
> 
> So just call this 'kvm_set_irq()' ?

Yes.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-25 17:56, Peter Maydell wrote:
>> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote:
>>> On 07/25/2012 06:53 PM, Jan Kiszka wrote:
>>>> We don't have a synchronous function anymore, it's part of the pre-run
>>>> code of x86 IIRC.
>>>
>>> Right.  There's a DPRINTF() there that talks about injection, too.  So I
>>> think this patch can be dropped.
>>
>> The main purpose of the patch is to remove 'irqchip' from the
>> function name, because the function isn't restricted to use
>> with in-kernel irqchips.
>
> Hmm, what was question again? Ah: Do we have an arch that implements it
> without providing a (logical) irqchip? At least at this time (including
> ARM)?

Well, it depends what you mean by 'irqchip' (part of the point of
this series being that there isn't a coherent architecture
independent definition of that and so we shouldn't use the term
in architecture-independent code).
On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC
or not, because we always use async interrupt injection.

(That is, the same arguments for "why should this function be
guarded by kvm_async_interrupt_injection() rather than
kvm_irqchip_in_kernel()?" apply to "why should this function not
have 'irqchip' in the function name.)

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 18:09, Peter Maydell wrote:
> On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-25 17:56, Peter Maydell wrote:
>>> On 25 July 2012 16:55, Avi Kivity <avi@redhat.com> wrote:
>>>> On 07/25/2012 06:53 PM, Jan Kiszka wrote:
>>>>> We don't have a synchronous function anymore, it's part of the pre-run
>>>>> code of x86 IIRC.
>>>>
>>>> Right.  There's a DPRINTF() there that talks about injection, too.  So I
>>>> think this patch can be dropped.
>>>
>>> The main purpose of the patch is to remove 'irqchip' from the
>>> function name, because the function isn't restricted to use
>>> with in-kernel irqchips.
>>
>> Hmm, what was question again? Ah: Do we have an arch that implements it
>> without providing a (logical) irqchip? At least at this time (including
>> ARM)?
> 
> Well, it depends what you mean by 'irqchip' (part of the point of
> this series being that there isn't a coherent architecture
> independent definition of that and so we shouldn't use the term
> in architecture-independent code).
> On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC
> or not, because we always use async interrupt injection.
> 
> (That is, the same arguments for "why should this function be
> guarded by kvm_async_interrupt_injection() rather than
> kvm_irqchip_in_kernel()?" apply to "why should this function not
> have 'irqchip' in the function name.)

Wasn't Avi's point that you do have an irqchip in your KVM support?

Jan

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 17:11, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-25 18:09, Peter Maydell wrote:
>> On 25 July 2012 16:58, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> Hmm, what was question again? Ah: Do we have an arch that implements it
>>> without providing a (logical) irqchip? At least at this time (including
>>> ARM)?
>>
>> Well, it depends what you mean by 'irqchip' (part of the point of
>> this series being that there isn't a coherent architecture
>> independent definition of that and so we shouldn't use the term
>> in architecture-independent code).
>> On ARM we will use KVM_IRQ_LINE whether we have an in-kernel VGIC
>> or not, because we always use async interrupt injection.
>>
>> (That is, the same arguments for "why should this function be
>> guarded by kvm_async_interrupt_injection() rather than
>> kvm_irqchip_in_kernel()?" apply to "why should this function not
>> have 'irqchip' in the function name.)
>
> Wasn't Avi's point that you do have an irqchip in your KVM support?

Not in the sense of "you need to KVM_CREATE_IRQCHIP it",
or in the sense of "this might be a useful thing to expose
to the user as a togglable option", which are the main
interesting architecture-independent bits of kernel_irqchip=on.

IIRC Alex said that PPC has a similar setup -- interrupts are
always sent asynchronously whether you've created the kernel
irqchip or not.

In any case I think it's much clearer to actually name things
based on what they're really testing for rather than things
that might or might not be correlated with that, even in
the (hypothetical) case that all architectures had an irqchip
if and only if they did async interrupt delivery.

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> Not in the sense of "you need to KVM_CREATE_IRQCHIP it",

...incidentally I was thinking about maybe moving kvm_irqchip_create()
from being called by kvm_init() to being called by the device
init function for the relevant irqchip (particularly we'll need
to do that if we adopt Avi's suggestion of having a parameter
to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
But that's more invasive surgery so I didn't want to do it yet.

-- PMM

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 18:24, Peter Maydell wrote:
> On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Not in the sense of "you need to KVM_CREATE_IRQCHIP it",
> 
> ...incidentally I was thinking about maybe moving kvm_irqchip_create()
> from being called by kvm_init() to being called by the device
> init function for the relevant irqchip (particularly we'll need
> to do that if we adopt Avi's suggestion of having a parameter
> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
> But that's more invasive surgery so I didn't want to do it yet.

This won't fly as irchip affects the whole orchestra (vcpus & irqchip
stubs in user space), at least on x86, and has to be called in the
current order. That's also why kernel_irqchip is a machine options, not
an option of one of the many device models.

Jan

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Avi Kivity]

On 07/25/2012 07:28 PM, Jan Kiszka wrote:
> On 2012-07-25 18:24, Peter Maydell wrote:
>> On 25 July 2012 17:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Not in the sense of "you need to KVM_CREATE_IRQCHIP it",
>> 
>> ...incidentally I was thinking about maybe moving kvm_irqchip_create()
>> from being called by kvm_init() to being called by the device
>> init function for the relevant irqchip (particularly we'll need
>> to do that if we adopt Avi's suggestion of having a parameter
>> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
>> But that's more invasive surgery so I didn't want to do it yet.
> 
> This won't fly as irchip affects the whole orchestra (vcpus & irqchip
> stubs in user space), at least on x86, and has to be called in the
> current order. That's also why kernel_irqchip is a machine options, not
> an option of one of the many device models.

Yes, to elaborate, KVM_CREATE_IRQCHIP creates N+3 devices: N local APICs
(deferred until N vcpus are created), one IOAPIC, and two PICs.

We debated decoupling those devices, but since there are a lot of
intercommunication among those devices, it was deemed to difficult (plus
these were the early kvm days when we had different get it in/get it
right tradeoffs).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Peter Maydell]

On 25 July 2012 17:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-25 18:24, Peter Maydell wrote:
>> ...incidentally I was thinking about maybe moving kvm_irqchip_create()
>> from being called by kvm_init() to being called by the device
>> init function for the relevant irqchip (particularly we'll need
>> to do that if we adopt Avi's suggestion of having a parameter
>> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
>> But that's more invasive surgery so I didn't want to do it yet.
>
> This won't fly as irchip affects the whole orchestra (vcpus & irqchip
> stubs in user space), at least on x86, and has to be called in the
> current order. That's also why kernel_irqchip is a machine options, not
> an option of one of the many device models.

Yes, one of the things you'd need to do is move actual creation
of the vcpus (as opposed to the QEMU CPU QOM objects) to rather
later in the sequence than they are now.

Where you have multiple devices which all need to go the same way
you can put that in the machine model code and then have all the
devices take an option the machine model sets to say which way
they go.

(Oddities in the x86 specific bits of the KVM kernel code ought
to result in oddities in x86 specific bits of QEMU, not in
generic bits :-))

-- PMM

Re: [Qemu-devel] [PATCH 6/6] kvm: Add documentation comment for kvm_irqchip_in_kernel()

[Jan Kiszka]

On 2012-07-25 17:40, Andreas Färber wrote:
> Am 25.07.2012 15:24, schrieb Peter Maydell:
>> Now we've cleared out the architecture-independent uses of
>> kvm_irqchip_in_kernel(), we can add a doc comment describing
>> what it means.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  kvm.h |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> If you've cleared the arch-independent uses, can't it be moved out of
> the generic kvm.h? Otherwise if just the commit message is confusing me:

kvm_irqchip_in_kernel is still a generic service as it probes a generic
property - that just has arch-dependent detail semantics.

Jan

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

Re: [Qemu-devel] [PATCH 2/6] kvm: Rename kvm_irqchip_set_irq to kvm_inject_async_irq

[Jan Kiszka]

On 2012-07-25 18:41, Peter Maydell wrote:
> On 25 July 2012 17:28, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-25 18:24, Peter Maydell wrote:
>>> ...incidentally I was thinking about maybe moving kvm_irqchip_create()
>>> from being called by kvm_init() to being called by the device
>>> init function for the relevant irqchip (particularly we'll need
>>> to do that if we adopt Avi's suggestion of having a parameter
>>> to KVM_CREATE_IRQCHIP to specify a particular kind of irqchip).
>>> But that's more invasive surgery so I didn't want to do it yet.
>>
>> This won't fly as irchip affects the whole orchestra (vcpus & irqchip
>> stubs in user space), at least on x86, and has to be called in the
>> current order. That's also why kernel_irqchip is a machine options, not
>> an option of one of the many device models.
> 
> Yes, one of the things you'd need to do is move actual creation
> of the vcpus (as opposed to the QEMU CPU QOM objects) to rather
> later in the sequence than they are now.

KVM VCPU creation is bound to the QOM object creation phase if we want
hotplugging support.

> 
> Where you have multiple devices which all need to go the same way
> you can put that in the machine model code and then have all the
> devices take an option the machine model sets to say which way
> they go.
> 
> (Oddities in the x86 specific bits of the KVM kernel code ought
> to result in oddities in x86 specific bits of QEMU, not in
> generic bits :-))

I dreamed of this as well before porting in-kernel irqchip support
upstream. ;)

But the point of this service is not that arch-specific in fact: By the
time you have a set of kernel irqchips that cannot reasonably be
instantiated / enabled separately, a single service helps to avoid that
userspace tries to do this mistake at all. Not all archs may have this
requirement, but a generic service needs to address it if it wants to be
generic.

Jan

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