[Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Alexey Kardashevskiy]

From: David Gibson <david@gibson.dropbear.id.au>

Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
controller system within KVM.  This patch allows qemu to initialize and
configure the in-kernel XICS, and keep its state in sync with qemu's XICS
state as necessary.

This should give considerable performance improvements.  e.g. on a simple
IPI ping-pong test between hardware threads, using qemu XICS gives us
around 5,000 irqs/second, whereas the in-kernel XICS gives us around
70,000 irqs/s on the same hardware configuration.

[Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

This depends on the "pseries: savevm support for XICS interrupt controller"
patch posted earlier.

---
 hw/ppc/spapr.c        |    4 +-
 hw/ppc/xics.c         |  333 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/ppc/xics.h |    8 +-
 3 files changed, 336 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 71da11b..04e0eae 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1136,8 +1136,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
         }
         env = &cpu->env;
 
-        xics_cpu_setup(spapr->icp, cpu);
-
         /* Set time-base frequency to 512 MHz */
         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
 
@@ -1151,6 +1149,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
             kvmppc_set_papr(cpu);
         }
 
+        xics_cpu_setup(spapr->icp, cpu);
+
         qemu_register_reset(spapr_cpu_reset, cpu);
     }
 
diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
index 02e44a0..b83f19f 100644
--- a/hw/ppc/xics.c
+++ b/hw/ppc/xics.c
@@ -29,12 +29,19 @@
 #include "trace.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "kvm_ppc.h"
+#include "sysemu/kvm.h"
+#include "config.h"
+#include "qemu/config-file.h"
+
+#include <sys/ioctl.h>
 
 /*
  * ICP: Presentation layer
  */
 
 struct icp_server_state {
+    CPUState *cs;
     uint32_t xirr;
     uint8_t pending_priority;
     uint8_t mfrr;
@@ -53,6 +60,9 @@ struct icp_state {
     uint32_t nr_servers;
     struct icp_server_state *ss;
     struct ics_state *ics;
+    uint32_t set_xive_token, get_xive_token,
+        int_off_token, int_on_token;
+    int kernel_xics_fd;
 };
 
 static void ics_reject(struct ics_state *ics, int nr);
@@ -168,6 +178,66 @@ static void icp_irq(struct icp_state *icp, int server, int nr, uint8_t priority)
     }
 }
 
+static void icp_get_kvm_state(struct icp_server_state *ss)
+{
+#ifdef CONFIG_KVM
+    uint64_t state;
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_PPC_ICP_STATE,
+        .addr = (uintptr_t)&state,
+    };
+    int ret;
+
+    if (!ss->cs) {
+        return; /* kernel irqchip not in use */
+    }
+
+    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
+                " for CPU %d: %s\n", ss->cs->cpu_index, strerror(errno));
+        exit(1);
+    }
+
+    ss->xirr = state >> KVM_REG_PPC_ICP_XISR_SHIFT;
+    ss->mfrr = (state >> KVM_REG_PPC_ICP_MFRR_SHIFT)
+        & KVM_REG_PPC_ICP_MFRR_MASK;
+    ss->pending_priority = (state >> KVM_REG_PPC_ICP_PPRI_SHIFT)
+        & KVM_REG_PPC_ICP_PPRI_MASK;
+#endif /* CONFIG_KVM */
+}
+
+static int icp_set_kvm_state(struct icp_server_state *ss)
+{
+#ifdef CONFIG_KVM
+    uint64_t state;
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_PPC_ICP_STATE,
+        .addr = (uintptr_t)&state,
+    };
+    int ret;
+
+    if (!ss->cs) {
+        return 0; /* kernel irqchip not in use */
+    }
+
+    state = ((uint64_t)ss->xirr << KVM_REG_PPC_ICP_XISR_SHIFT)
+        | ((uint64_t)ss->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
+        | ((uint64_t)ss->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
+
+    ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, &reg);
+    if (ret != 0) {
+        fprintf(stderr, "Unable to restore KVM interrupt controller state (0x%"
+                PRIx64 ") for CPU %d: %s\n", state, ss->cs->cpu_index,
+                strerror(errno));
+        exit(1);
+        return ret;
+    }
+#endif /* CONFIG_KVM */
+
+    return 0;
+}
+
 /*
  * ICS: Source layer
  */
@@ -336,6 +406,107 @@ static void ics_eoi(struct ics_state *ics, int nr)
     }
 }
 
+static void ics_get_kvm_state(struct ics_state *ics)
+{
+#ifdef CONFIG_KVM
+    uint64_t state;
+    struct kvm_device_attr attr = {
+        .flags = 0,
+        .group = KVM_DEV_XICS_GRP_SOURCES,
+        .addr = (uint64_t)(uintptr_t)&state,
+    };
+    int i;
+
+    if (ics->icp->kernel_xics_fd == -1) {
+        return; /* kernel irqchip not in use */
+    }
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        struct ics_irq_state *irq = &ics->irqs[i];
+        int ret;
+
+        attr.attr = i + ics->offset;
+
+        ret = ioctl(ics->icp->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
+        if (ret != 0) {
+            fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
+                    " for IRQ %d: %s\n", i + ics->offset, strerror(errno));
+            exit(1);
+        }
+
+        irq->server = state & KVM_XICS_DESTINATION_MASK;
+        irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
+            & KVM_XICS_PRIORITY_MASK;
+        if (state & KVM_XICS_MASKED) {
+            irq->priority = 0xff;
+        } else {
+            irq->priority = irq->saved_priority;
+        }
+
+        if (state & KVM_XICS_PENDING) {
+            if (state & KVM_XICS_LEVEL_SENSITIVE) {
+                irq->status |= XICS_STATUS_ASSERTED;
+            } else {
+                irq->status |= XICS_STATUS_MASKED_PENDING
+                    | XICS_STATUS_REJECTED;
+            }
+        }
+    }
+#endif /* CONFIG_KVM */
+}
+
+static int ics_set_kvm_state(struct ics_state *ics)
+{
+#ifdef CONFIG_KVM
+    uint64_t state;
+    struct kvm_device_attr attr = {
+        .flags = 0,
+        .group = KVM_DEV_XICS_GRP_SOURCES,
+        .addr = (uint64_t)(uintptr_t)&state,
+    };
+    int i;
+
+    if (ics->icp->kernel_xics_fd == -1) {
+        return 0; /* kernel irqchip not in use */
+    }
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        struct ics_irq_state *irq = &ics->irqs[i];
+        int ret;
+
+        attr.attr = i + ics->offset;
+
+        state = irq->server;
+        state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
+            << KVM_XICS_PRIORITY_SHIFT;
+        if (irq->priority != irq->saved_priority) {
+            assert(irq->priority == 0xff);
+            state |= KVM_XICS_MASKED;
+        }
+
+        if (ics->islsi[i]) {
+            state |= KVM_XICS_LEVEL_SENSITIVE;
+            if (irq->status & XICS_STATUS_ASSERTED) {
+                state |= KVM_XICS_PENDING;
+            }
+        } else {
+            if (irq->status & XICS_STATUS_MASKED_PENDING) {
+                state |= KVM_XICS_PENDING;
+            }
+        }
+
+        ret = ioctl(ics->icp->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
+        if (ret != 0) {
+            fprintf(stderr, "Unable to restore KVM interrupt controller state"
+                    " for IRQs %d: %s\n", i + ics->offset, strerror(errno));
+            return ret;
+        }
+    }
+#endif /* CONFIG_KVM */
+
+    return 0;
+}
+
 /*
  * Exported functions
  */
@@ -514,6 +685,10 @@ static void xics_reset(void *opaque)
         icp->ss[i].mfrr = 0xff;
         /* Make all outputs are deasserted */
         qemu_set_irq(icp->ss[i].output, 0);
+
+        if (icp->ss[i].cs) {
+            icp_set_kvm_state(&icp->ss[i]);
+        }
     }
 
     memset(ics->irqs, 0, sizeof(struct ics_irq_state) * ics->nr_irqs);
@@ -521,6 +696,26 @@ static void xics_reset(void *opaque)
         ics->irqs[i].priority = 0xff;
         ics->irqs[i].saved_priority = 0xff;
     }
+    ics_set_kvm_state(ics);
+}
+
+static void icp_pre_save(void *opaque)
+{
+    struct icp_server_state *ss = opaque;
+    icp_get_kvm_state(ss);
+}
+
+static int icp_post_load(void *opaque, int version_id)
+{
+    struct icp_server_state *ss = opaque;
+
+    return icp_set_kvm_state(ss);
+}
+
+static void ics_pre_save(void *opaque)
+{
+    struct ics_state *ics = opaque;
+    ics_get_kvm_state(ics);
 }
 
 static int ics_post_load(void *opaque, int version_id)
@@ -528,6 +723,10 @@ static int ics_post_load(void *opaque, int version_id)
     int i;
     struct ics_state *ics = opaque;
 
+    if (ics->icp->kernel_xics_fd != -1) {
+        return ics_set_kvm_state(ics);
+    }
+
     for (i = 0; i < ics->icp->nr_servers; i++) {
         icp_resend(ics->icp, i);
     }
@@ -535,11 +734,14 @@ static int ics_post_load(void *opaque, int version_id)
     return 0;
 }
 
+
 static const VMStateDescription vmstate_icp_server = {
     .name = "icp/server",
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .pre_save = icp_pre_save,
+    .post_load = icp_post_load,
     .fields      = (VMStateField []) {
         /* Sanity check */
         VMSTATE_UINT32(xirr, struct icp_server_state),
@@ -568,6 +770,7 @@ static const VMStateDescription vmstate_ics = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
+    .pre_save = ics_pre_save,
     .post_load = ics_post_load,
     .fields      = (VMStateField []) {
         /* Sanity check */
@@ -586,6 +789,28 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
 
     assert(cs->cpu_index < icp->nr_servers);
 
+    if (icp->kernel_xics_fd != -1) {
+#ifdef CONFIG_KVM
+        int ret;
+        struct kvm_enable_cap xics_enable_cap = {
+            .cap = KVM_CAP_IRQ_XICS,
+            .flags = 0,
+            .args = {icp->kernel_xics_fd, cs->cpu_index, 0, 0},
+        };
+
+        ss->cs = cs;
+
+        ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
+        if (ret < 0) {
+            fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
+                    cs->cpu_index, strerror(errno));
+            exit(1);
+        }
+#else
+        abort();
+#endif
+    }
+
     switch (PPC_INPUT(env)) {
     case PPC_FLAGS_INPUT_POWER7:
         ss->output = env->irq_inputs[POWER7_INPUT_INT];
@@ -604,6 +829,97 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
     vmstate_register(NULL, cs->cpu_index, &vmstate_icp_server, ss);
 }
 
+#ifdef CONFIG_KVM
+static void ics_set_irq_kvm(void *opaque, int srcno, int val)
+{
+    struct ics_state *ics = opaque;
+    struct kvm_irq_level args;
+    int rc;
+
+    args.irq = srcno + ics->offset;
+    if (!ics->islsi[srcno]) {
+        if (!val) {
+            return;
+        }
+        args.level = KVM_INTERRUPT_SET;
+    } else {
+        args.level = val ? KVM_INTERRUPT_SET_LEVEL : KVM_INTERRUPT_UNSET;
+    }
+    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
+    if (rc < 0) {
+        perror("kvm_irq_line");
+    }
+}
+
+static int xics_kernel_init(struct icp_state *icp)
+{
+    QemuOptsList *list = qemu_find_opts("machine");
+    struct ics_state *ics = icp->ics;
+    int rc;
+    struct kvm_create_device xics_create_device = {
+        .type = KVM_DEV_TYPE_XICS,
+        .flags = 0,
+    };
+
+    if (!kvm_enabled()) {
+        return -ENODEV;
+    }
+
+    if (QTAILQ_EMPTY(&list->head) ||
+        !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+                           "kernel_irqchip", true) ||
+        !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
+        return -ENODEV;
+    }
+
+    rc = kvmppc_define_rtas_token(icp->set_xive_token, "ibm,set-xive");
+    if (rc < 0) {
+        perror("kvmppc_define_rtas_token: ibm,set-xive");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icp->get_xive_token, "ibm,get-xive");
+    if (rc < 0) {
+        perror("kvmppc_define_rtas_token: ibm,get-xive");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icp->int_on_token, "ibm,int-on");
+    if (rc < 0) {
+        perror("kvmppc_define_rtas_token: ibm,int-on");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icp->int_off_token, "ibm,int-off");
+    if (rc < 0) {
+        perror("kvmppc_define_rtas_token: ibm,int-off");
+        goto fail;
+    }
+
+    /* Create the kernel ICP */
+    rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
+    if (rc < 0) {
+        fprintf(stderr, "Error on KVM_CREATE_DEVICE for XICS: %s",
+                strerror(errno));
+        goto fail;
+    }
+
+    icp->kernel_xics_fd = xics_create_device.fd;
+    ics->qirqs = qemu_allocate_irqs(ics_set_irq_kvm, ics, ics->nr_irqs);
+
+    return 0;
+
+fail:
+    kvmppc_define_rtas_token(0, "ibm,set-xive");
+    kvmppc_define_rtas_token(0, "ibm,get-xive");
+    kvmppc_define_rtas_token(0, "ibm,int-on");
+    kvmppc_define_rtas_token(0, "ibm,int-off");
+    return rc;
+}
+#else
+static int xics_kernel_init(struct icp_state *icp) { return -1; }
+#endif
+
 struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
 {
     struct icp_state *icp;
@@ -611,6 +927,7 @@ struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
 
     icp = g_malloc0(sizeof(*icp));
     icp->nr_servers = nr_servers;
+    icp->kernel_xics_fd = -1;
     icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
 
     ics = g_malloc0(sizeof(*ics));
@@ -622,17 +939,21 @@ struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
     icp->ics = ics;
     ics->icp = icp;
 
-    ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, nr_irqs);
-
+    /* We don't actually use these functions with in-kernel XICS, but
+     * registering them always doesn't hurt */
     spapr_register_hypercall(H_CPPR, h_cppr);
     spapr_register_hypercall(H_IPI, h_ipi);
     spapr_register_hypercall(H_XIRR, h_xirr);
     spapr_register_hypercall(H_EOI, h_eoi);
 
-    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
-    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
-    spapr_rtas_register("ibm,int-off", rtas_int_off);
-    spapr_rtas_register("ibm,int-on", rtas_int_on);
+    icp->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_set_xive);
+    icp->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_get_xive);
+    icp->int_off_token = spapr_rtas_register("ibm,int-off", rtas_int_off);
+    icp->int_on_token = spapr_rtas_register("ibm,int-on", rtas_int_on);
+
+    if (xics_kernel_init(icp) != 0) {
+        ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, nr_irqs);
+    }
 
     qemu_register_reset(xics_reset, icp);
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6bce042..d52215d 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -28,12 +28,18 @@
 #define __XICS_H__
 
 #define XICS_IPI        0x2
-#define XICS_IRQ_BASE   0x10
+#define XICS_BUID       0x1
+#define XICS_IRQ_BASE   (XICS_BUID << 12)
 
 struct icp_state;
 
 qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
 void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
+/*
+ * We currently only support one BUID which is our interrupt base
+ * (the kernel implementation supports more but we don't exploit
+ *  that yet)
+ */
 
 struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
 void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

On Wed, Jun 05, 2013 at 05:39:58PM +1000, Alexey Kardashevskiy wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

I think this one also needs the headers update for qemu first, yes?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Alexey Kardashevskiy]

On 06/06/2013 05:41 PM, David Gibson wrote:
> On Wed, Jun 05, 2013 at 05:39:58PM +1000, Alexey Kardashevskiy wrote:
>> From: David Gibson <david@gibson.dropbear.id.au>
>>
>> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
>> controller system within KVM.  This patch allows qemu to initialize and
>> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
>> state as necessary.
>>
>> This should give considerable performance improvements.  e.g. on a simple
>> IPI ping-pong test between hardware threads, using qemu XICS gives us
>> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
>> 70,000 irqs/s on the same hardware configuration.
>>
>> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> I think this one also needs the headers update for qemu first, yes?

Yes, my bad, I sent headers patch + one patch which Scott Wood already posted.



-- 
Alexey

Re: [Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Andreas Färber]

Am 05.06.2013 09:39, schrieb Alexey Kardashevskiy:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

If a Mike Qiu changed this patch, don't we require his Signed-off-by?

CPUState usage looks fine, can't judge the kernel interface, two
nitpicks below.

[...]
> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> index 02e44a0..b83f19f 100644
> --- a/hw/ppc/xics.c
> +++ b/hw/ppc/xics.c
> @@ -29,12 +29,19 @@
>  #include "trace.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "sysemu/kvm.h"
> +#include "config.h"
> +#include "qemu/config-file.h"
> +
> +#include <sys/ioctl.h>
>  
>  /*
>   * ICP: Presentation layer
>   */
>  
>  struct icp_server_state {
> +    CPUState *cs;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -53,6 +60,9 @@ struct icp_state {
>      uint32_t nr_servers;
>      struct icp_server_state *ss;
>      struct ics_state *ics;
> +    uint32_t set_xive_token, get_xive_token,
> +        int_off_token, int_on_token;

FWIW normally we place struct fields below each other...

> +    int kernel_xics_fd;
>  };
>  
>  static void ics_reject(struct ics_state *ics, int nr);
[...]
> @@ -535,11 +734,14 @@ static int ics_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +

Intentional white space change?

>  static const VMStateDescription vmstate_icp_server = {
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> +    .pre_save = icp_pre_save,
> +    .post_load = icp_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32(xirr, struct icp_server_state),
[snip]

Regards,
Andreas

-- 
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] pseries: Support for in-kernel XICS interrupt controller

[Alexey Kardashevskiy]

On 06/08/2013 08:20 PM, Andreas Färber wrote:
> Am 05.06.2013 09:39, schrieb Alexey Kardashevskiy:
>> From: David Gibson <david@gibson.dropbear.id.au>
>>
>> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
>> controller system within KVM.  This patch allows qemu to initialize and
>> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
>> state as necessary.
>>
>> This should give considerable performance improvements.  e.g. on a simple
>> IPI ping-pong test between hardware threads, using qemu XICS gives us
>> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
>> 70,000 irqs/s on the same hardware configuration.
>>
>> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> If a Mike Qiu changed this patch, don't we require his Signed-off-by?


He did not change this patch, he found a mistype in our local source tree
which I decided to merge with this patch. I did not want him not to be
mentioned at all so I added this line. What is the general rule who needs
to s-o-b?



> CPUState usage looks fine, can't judge the kernel interface, two
> nitpicks below.
> 
> [...]
>> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
>> index 02e44a0..b83f19f 100644
>> --- a/hw/ppc/xics.c
>> +++ b/hw/ppc/xics.c
>> @@ -29,12 +29,19 @@
>>  #include "trace.h"
>>  #include "hw/ppc/spapr.h"
>>  #include "hw/ppc/xics.h"
>> +#include "kvm_ppc.h"
>> +#include "sysemu/kvm.h"
>> +#include "config.h"
>> +#include "qemu/config-file.h"
>> +
>> +#include <sys/ioctl.h>
>>  
>>  /*
>>   * ICP: Presentation layer
>>   */
>>  
>>  struct icp_server_state {
>> +    CPUState *cs;
>>      uint32_t xirr;
>>      uint8_t pending_priority;
>>      uint8_t mfrr;
>> @@ -53,6 +60,9 @@ struct icp_state {
>>      uint32_t nr_servers;
>>      struct icp_server_state *ss;
>>      struct ics_state *ics;
>> +    uint32_t set_xive_token, get_xive_token,
>> +        int_off_token, int_on_token;
> 
> FWIW normally we place struct fields below each other...


Is it mandatory? I personally do not see _any_ benefit in aligning struct
members with spaces.


-- 
Alexey

Re: [Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Andreas Färber]

Am 08.06.2013 16:11, schrieb Alexey Kardashevskiy:
> On 06/08/2013 08:20 PM, Andreas Färber wrote:
>> Am 05.06.2013 09:39, schrieb Alexey Kardashevskiy:
>>> From: David Gibson <david@gibson.dropbear.id.au>
>>>
>>> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
>>> controller system within KVM.  This patch allows qemu to initialize and
>>> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
>>> state as necessary.
>>>
>>> This should give considerable performance improvements.  e.g. on a simple
>>> IPI ping-pong test between hardware threads, using qemu XICS gives us
>>> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
>>> 70,000 irqs/s on the same hardware configuration.
>>>
>>> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> If a Mike Qiu changed this patch, don't we require his Signed-off-by?
> 
> 
> He did not change this patch, he found a mistype in our local source tree
> which I decided to merge with this patch. I did not want him not to be
> mentioned at all so I added this line.

Then that notation is misleading: [author: ...] usually indicates that
author applied the noted changes to the patch, and just like tags - if
at all - this should get recorded in chronological order, i.e.

S-o-b David ...
[aik: fixed mistype ... spotted by Mike ...]
S-o-b you ...

making clearer who signed off which version.

> What is the general rule who needs
> to s-o-b?

For a formal description see Linux' SubmittingPatches docs.

In practice, whenever you git-am or git-cherry-pick a patch it should
have at least one Signed-off-by from the person you got it from.
Whenever you submit a patch it should carry your Sob as last one,
thereby recording the sequence of through whose hands a patch went.

>>> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
>>> index 02e44a0..b83f19f 100644
>>> --- a/hw/ppc/xics.c
>>> +++ b/hw/ppc/xics.c
>>> @@ -29,12 +29,19 @@
>>>  #include "trace.h"
>>>  #include "hw/ppc/spapr.h"
>>>  #include "hw/ppc/xics.h"
>>> +#include "kvm_ppc.h"
>>> +#include "sysemu/kvm.h"
>>> +#include "config.h"
>>> +#include "qemu/config-file.h"
>>> +
>>> +#include <sys/ioctl.h>
>>>  
>>>  /*
>>>   * ICP: Presentation layer
>>>   */
>>>  
>>>  struct icp_server_state {
>>> +    CPUState *cs;
>>>      uint32_t xirr;
>>>      uint8_t pending_priority;
>>>      uint8_t mfrr;
>>> @@ -53,6 +60,9 @@ struct icp_state {
>>>      uint32_t nr_servers;
>>>      struct icp_server_state *ss;
>>>      struct ics_state *ics;
>>> +    uint32_t set_xive_token, get_xive_token,
>>> +        int_off_token, int_on_token;
>>
>> FWIW normally we place struct fields below each other...
> 
> 
> Is it mandatory? I personally do not see _any_ benefit in aligning struct
> members with spaces.

Dunno about whether that is somewhere in HACKING or CODING_STYLE, and I
don't really mind either way.

But let me clarify that I wasn't talking about space-alignment, I was
talking about duplicating the type as you can see for pending_priority
and mfrr field above being on their own line each.

Andreas

-- 
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] pseries: Support for in-kernel XICS interrupt controller

[Alexander Graf]

On 05.06.2013, at 09:39, Alexey Kardashevskiy wrote:

> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> This depends on the "pseries: savevm support for XICS interrupt controller"
> patch posted earlier.
> 
> ---
> hw/ppc/spapr.c        |    4 +-
> hw/ppc/xics.c         |  333 ++++++++++++++++++++++++++++++++++++++++++++++++-
> include/hw/ppc/xics.h |    8 +-
> 3 files changed, 336 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 71da11b..04e0eae 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1136,8 +1136,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>         }
>         env = &cpu->env;
> 
> -        xics_cpu_setup(spapr->icp, cpu);
> -
>         /* Set time-base frequency to 512 MHz */
>         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> 
> @@ -1151,6 +1149,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>             kvmppc_set_papr(cpu);
>         }
> 
> +        xics_cpu_setup(spapr->icp, cpu);
> +
>         qemu_register_reset(spapr_cpu_reset, cpu);
>     }
> 
> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> index 02e44a0..b83f19f 100644
> --- a/hw/ppc/xics.c
> +++ b/hw/ppc/xics.c
> @@ -29,12 +29,19 @@
> #include "trace.h"
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "sysemu/kvm.h"
> +#include "config.h"
> +#include "qemu/config-file.h"
> +
> +#include <sys/ioctl.h>

Huh? This breaks compilation on non-Linux.

> 
> /*
>  * ICP: Presentation layer
>  */
> 
> struct icp_server_state {
> +    CPUState *cs;

Why did you get around this earlier without the CPUState pointer?

>     uint32_t xirr;
>     uint8_t pending_priority;
>     uint8_t mfrr;
> @@ -53,6 +60,9 @@ struct icp_state {
>     uint32_t nr_servers;
>     struct icp_server_state *ss;
>     struct ics_state *ics;
> +    uint32_t set_xive_token, get_xive_token,
> +        int_off_token, int_on_token;

Separate declaration lines please.

> +    int kernel_xics_fd;
> };
> 
> static void ics_reject(struct ics_state *ics, int nr);
> @@ -168,6 +178,66 @@ static void icp_irq(struct icp_state *icp, int server, int nr, uint8_t priority)
>     }
> }
> 
> +static void icp_get_kvm_state(struct icp_server_state *ss)
> +{
> +#ifdef CONFIG_KVM
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    if (!ss->cs) {
> +        return; /* kernel irqchip not in use */
> +    }
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
> +                " for CPU %d: %s\n", ss->cs->cpu_index, strerror(errno));
> +        exit(1);
> +    }
> +
> +    ss->xirr = state >> KVM_REG_PPC_ICP_XISR_SHIFT;
> +    ss->mfrr = (state >> KVM_REG_PPC_ICP_MFRR_SHIFT)
> +        & KVM_REG_PPC_ICP_MFRR_MASK;
> +    ss->pending_priority = (state >> KVM_REG_PPC_ICP_PPRI_SHIFT)
> +        & KVM_REG_PPC_ICP_PPRI_MASK;
> +#endif /* CONFIG_KVM */

This needs to get encapsulated into a kvm helper function that gets a dummy definition for non-KVM. We've been through this multiple times now in other areas.

> +}
> +
> +static int icp_set_kvm_state(struct icp_server_state *ss)
> +{
> +#ifdef CONFIG_KVM
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    if (!ss->cs) {
> +        return 0; /* kernel irqchip not in use */
> +    }
> +
> +    state = ((uint64_t)ss->xirr << KVM_REG_PPC_ICP_XISR_SHIFT)
> +        | ((uint64_t)ss->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
> +        | ((uint64_t)ss->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        fprintf(stderr, "Unable to restore KVM interrupt controller state (0x%"
> +                PRIx64 ") for CPU %d: %s\n", state, ss->cs->cpu_index,
> +                strerror(errno));
> +        exit(1);
> +        return ret;
> +    }
> +#endif /* CONFIG_KVM */
> +
> +    return 0;
> +}
> +
> /*
>  * ICS: Source layer
>  */
> @@ -336,6 +406,107 @@ static void ics_eoi(struct ics_state *ics, int nr)
>     }
> }
> 
> +static void ics_get_kvm_state(struct ics_state *ics)
> +{
> +#ifdef CONFIG_KVM
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    if (ics->icp->kernel_xics_fd == -1) {
> +        return; /* kernel irqchip not in use */
> +    }
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        struct ics_irq_state *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        ret = ioctl(ics->icp->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
> +                    " for IRQ %d: %s\n", i + ics->offset, strerror(errno));
> +            exit(1);
> +        }
> +
> +        irq->server = state & KVM_XICS_DESTINATION_MASK;
> +        irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
> +            & KVM_XICS_PRIORITY_MASK;
> +        if (state & KVM_XICS_MASKED) {

This needs explanation in a comment

> +            irq->priority = 0xff;
> +        } else {
> +            irq->priority = irq->saved_priority;
> +        }
> +
> +        if (state & KVM_XICS_PENDING) {
> +            if (state & KVM_XICS_LEVEL_SENSITIVE) {
> +                irq->status |= XICS_STATUS_ASSERTED;
> +            } else {

This needs explanation in a comment. Why is an EDGE pending interrupt rejected?

> +                irq->status |= XICS_STATUS_MASKED_PENDING
> +                    | XICS_STATUS_REJECTED;
> +            }
> +        }
> +    }
> +#endif /* CONFIG_KVM */
> +}
> +
> +static int ics_set_kvm_state(struct ics_state *ics)
> +{
> +#ifdef CONFIG_KVM
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    if (ics->icp->kernel_xics_fd == -1) {
> +        return 0; /* kernel irqchip not in use */
> +    }
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        struct ics_irq_state *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        state = irq->server;
> +        state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
> +            << KVM_XICS_PRIORITY_SHIFT;
> +        if (irq->priority != irq->saved_priority) {
> +            assert(irq->priority == 0xff);
> +            state |= KVM_XICS_MASKED;
> +        }
> +
> +        if (ics->islsi[i]) {
> +            state |= KVM_XICS_LEVEL_SENSITIVE;
> +            if (irq->status & XICS_STATUS_ASSERTED) {
> +                state |= KVM_XICS_PENDING;
> +            }
> +        } else {
> +            if (irq->status & XICS_STATUS_MASKED_PENDING) {
> +                state |= KVM_XICS_PENDING;
> +            }
> +        }
> +
> +        ret = ioctl(ics->icp->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            fprintf(stderr, "Unable to restore KVM interrupt controller state"
> +                    " for IRQs %d: %s\n", i + ics->offset, strerror(errno));
> +            return ret;
> +        }
> +    }
> +#endif /* CONFIG_KVM */
> +
> +    return 0;
> +}
> +
> /*
>  * Exported functions
>  */
> @@ -514,6 +685,10 @@ static void xics_reset(void *opaque)
>         icp->ss[i].mfrr = 0xff;
>         /* Make all outputs are deasserted */
>         qemu_set_irq(icp->ss[i].output, 0);
> +
> +        if (icp->ss[i].cs) {
> +            icp_set_kvm_state(&icp->ss[i]);
> +        }
>     }
> 
>     memset(ics->irqs, 0, sizeof(struct ics_irq_state) * ics->nr_irqs);
> @@ -521,6 +696,26 @@ static void xics_reset(void *opaque)
>         ics->irqs[i].priority = 0xff;
>         ics->irqs[i].saved_priority = 0xff;
>     }
> +    ics_set_kvm_state(ics);
> +}
> +
> +static void icp_pre_save(void *opaque)
> +{
> +    struct icp_server_state *ss = opaque;
> +    icp_get_kvm_state(ss);
> +}
> +
> +static int icp_post_load(void *opaque, int version_id)
> +{
> +    struct icp_server_state *ss = opaque;
> +
> +    return icp_set_kvm_state(ss);
> +}
> +
> +static void ics_pre_save(void *opaque)
> +{
> +    struct ics_state *ics = opaque;
> +    ics_get_kvm_state(ics);
> }
> 
> static int ics_post_load(void *opaque, int version_id)
> @@ -528,6 +723,10 @@ static int ics_post_load(void *opaque, int version_id)
>     int i;
>     struct ics_state *ics = opaque;
> 
> +    if (ics->icp->kernel_xics_fd != -1) {
> +        return ics_set_kvm_state(ics);
> +    }
> +
>     for (i = 0; i < ics->icp->nr_servers; i++) {
>         icp_resend(ics->icp, i);
>     }
> @@ -535,11 +734,14 @@ static int ics_post_load(void *opaque, int version_id)
>     return 0;
> }
> 
> +
> static const VMStateDescription vmstate_icp_server = {
>     .name = "icp/server",
>     .version_id = 1,
>     .minimum_version_id = 1,
>     .minimum_version_id_old = 1,
> +    .pre_save = icp_pre_save,
> +    .post_load = icp_post_load,
>     .fields      = (VMStateField []) {
>         /* Sanity check */
>         VMSTATE_UINT32(xirr, struct icp_server_state),
> @@ -568,6 +770,7 @@ static const VMStateDescription vmstate_ics = {
>     .version_id = 1,
>     .minimum_version_id = 1,
>     .minimum_version_id_old = 1,
> +    .pre_save = ics_pre_save,
>     .post_load = ics_post_load,
>     .fields      = (VMStateField []) {
>         /* Sanity check */
> @@ -586,6 +789,28 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
> 
>     assert(cs->cpu_index < icp->nr_servers);
> 
> +    if (icp->kernel_xics_fd != -1) {
> +#ifdef CONFIG_KVM
> +        int ret;
> +        struct kvm_enable_cap xics_enable_cap = {
> +            .cap = KVM_CAP_IRQ_XICS,
> +            .flags = 0,
> +            .args = {icp->kernel_xics_fd, cs->cpu_index, 0, 0},
> +        };
> +
> +        ss->cs = cs;
> +
> +        ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
> +        if (ret < 0) {
> +            fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
> +                    cs->cpu_index, strerror(errno));
> +            exit(1);
> +        }
> +#else
> +        abort();
> +#endif
> +    }
> +
>     switch (PPC_INPUT(env)) {
>     case PPC_FLAGS_INPUT_POWER7:
>         ss->output = env->irq_inputs[POWER7_INPUT_INT];
> @@ -604,6 +829,97 @@ void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
>     vmstate_register(NULL, cs->cpu_index, &vmstate_icp_server, ss);
> }
> 
> +#ifdef CONFIG_KVM
> +static void ics_set_irq_kvm(void *opaque, int srcno, int val)
> +{
> +    struct ics_state *ics = opaque;
> +    struct kvm_irq_level args;
> +    int rc;
> +
> +    args.irq = srcno + ics->offset;
> +    if (!ics->islsi[srcno]) {
> +        if (!val) {
> +            return;
> +        }
> +        args.level = KVM_INTERRUPT_SET;
> +    } else {
> +        args.level = val ? KVM_INTERRUPT_SET_LEVEL : KVM_INTERRUPT_UNSET;
> +    }
> +    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> +    if (rc < 0) {
> +        perror("kvm_irq_line");
> +    }
> +}
> +
> +static int xics_kernel_init(struct icp_state *icp)
> +{
> +    QemuOptsList *list = qemu_find_opts("machine");
> +    struct ics_state *ics = icp->ics;
> +    int rc;
> +    struct kvm_create_device xics_create_device = {
> +        .type = KVM_DEV_TYPE_XICS,
> +        .flags = 0,
> +    };
> +
> +    if (!kvm_enabled()) {
> +        return -ENODEV;
> +    }
> +
> +    if (QTAILQ_EMPTY(&list->head) ||
> +        !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> +                           "kernel_irqchip", true) ||
> +        !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> +        return -ENODEV;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icp->set_xive_token, "ibm,set-xive");
> +    if (rc < 0) {
> +        perror("kvmppc_define_rtas_token: ibm,set-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icp->get_xive_token, "ibm,get-xive");
> +    if (rc < 0) {
> +        perror("kvmppc_define_rtas_token: ibm,get-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icp->int_on_token, "ibm,int-on");
> +    if (rc < 0) {
> +        perror("kvmppc_define_rtas_token: ibm,int-on");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icp->int_off_token, "ibm,int-off");
> +    if (rc < 0) {
> +        perror("kvmppc_define_rtas_token: ibm,int-off");
> +        goto fail;
> +    }
> +
> +    /* Create the kernel ICP */
> +    rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
> +    if (rc < 0) {
> +        fprintf(stderr, "Error on KVM_CREATE_DEVICE for XICS: %s",
> +                strerror(errno));
> +        goto fail;
> +    }
> +
> +    icp->kernel_xics_fd = xics_create_device.fd;
> +    ics->qirqs = qemu_allocate_irqs(ics_set_irq_kvm, ics, ics->nr_irqs);
> +
> +    return 0;
> +
> +fail:
> +    kvmppc_define_rtas_token(0, "ibm,set-xive");
> +    kvmppc_define_rtas_token(0, "ibm,get-xive");
> +    kvmppc_define_rtas_token(0, "ibm,int-on");
> +    kvmppc_define_rtas_token(0, "ibm,int-off");
> +    return rc;
> +}
> +#else
> +static int xics_kernel_init(struct icp_state *icp) { return -1; }
> +#endif
> +
> struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> {
>     struct icp_state *icp;
> @@ -611,6 +927,7 @@ struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> 
>     icp = g_malloc0(sizeof(*icp));
>     icp->nr_servers = nr_servers;
> +    icp->kernel_xics_fd = -1;
>     icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> 
>     ics = g_malloc0(sizeof(*ics));
> @@ -622,17 +939,21 @@ struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
>     icp->ics = ics;
>     ics->icp = icp;
> 
> -    ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, nr_irqs);
> -
> +    /* We don't actually use these functions with in-kernel XICS, but
> +     * registering them always doesn't hurt */
>     spapr_register_hypercall(H_CPPR, h_cppr);
>     spapr_register_hypercall(H_IPI, h_ipi);
>     spapr_register_hypercall(H_XIRR, h_xirr);
>     spapr_register_hypercall(H_EOI, h_eoi);
> 
> -    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> -    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> -    spapr_rtas_register("ibm,int-off", rtas_int_off);
> -    spapr_rtas_register("ibm,int-on", rtas_int_on);
> +    icp->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> +    icp->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> +    icp->int_off_token = spapr_rtas_register("ibm,int-off", rtas_int_off);
> +    icp->int_on_token = spapr_rtas_register("ibm,int-on", rtas_int_on);
> +
> +    if (xics_kernel_init(icp) != 0) {
> +        ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, nr_irqs);
> +    }

Looking at all this, it seems as if logic similar to hw/intc/openpic_kvm.c makes more sense. Just spawn as separate device for an in-kernel XICS controller and share the few definitions you can share in a common header file.


Alex

> 
>     qemu_register_reset(xics_reset, icp);
> 
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6bce042..d52215d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -28,12 +28,18 @@
> #define __XICS_H__
> 
> #define XICS_IPI        0x2
> -#define XICS_IRQ_BASE   0x10
> +#define XICS_BUID       0x1
> +#define XICS_IRQ_BASE   (XICS_BUID << 12)
> 
> struct icp_state;
> 
> qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
> void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
> +/*
> + * We currently only support one BUID which is our interrupt base
> + * (the kernel implementation supports more but we don't exploit
> + *  that yet)
> + */
> 
> struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
> void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
> -- 
> 1.7.10.4
>