[Qemu-devel] [PATCH v3 0/8] xics: reworks and in-kernel support

[Qemu-devel] [PATCH v3 0/8] xics: reworks and in-kernel support

[Alexey Kardashevskiy]

Yet another try with XICS-KVM.

v2->v3:
Addressed multiple comments from Andreas;
Added 2 patches for XICS from Ben - I included them into the series as they
are about XICS and they won't rebase automatically if moved before XICS rework
so it seemed to me that it would be better to carry them toghether. If it is
wrong, please let me know, I'll repost them separately.

v1->v2:
The main change is this adds "xics-common" parent for emulated XICS and XICS-KVM.
And many, many small changes, mostly to address Andreas comments.

Migration from XICS to XICS-KVM and vice versa still works.


Alexey Kardashevskiy (4):
  xics: add pre_save/post_load/cpu_setup dispatchers
  xics: move registration of global state to realize()
  xics: minor changes and cleanups
  xics: split to xics and xics-common

Benjamin Herrenschmidt (2):
  xics: Add H_IPOLL implementation
  xics: Implement H_XIRR_X

David Gibson (2):
  target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN
  xics-kvm: Support for in-kernel XICS interrupt controller

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xics.c                    | 358 +++++++++++++++++++++------
 hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c                    |  27 ++-
 include/hw/ppc/spapr.h            |   3 +-
 include/hw/ppc/xics.h             |  68 +++++-
 target-ppc/kvm.c                  |  14 ++
 target-ppc/kvm_ppc.h              |   7 +
 9 files changed, 894 insertions(+), 77 deletions(-)
 create mode 100644 hw/intc/xics_kvm.c

-- 
1.8.3.2

[Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexey Kardashevskiy]

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

Recent PowerKVM allows the kernel to intercept some RTAS calls from the
guest directly.  This is used to implement the more efficient in-kernel
XICS for example.  qemu is still responsible for assigning the RTAS token
numbers however, and needs to tell the kernel which RTAS function name is
assigned to a given token value.  This patch adds a convenience wrapper for
the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this purpose.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 target-ppc/kvm.c     | 14 ++++++++++++++
 target-ppc/kvm_ppc.h |  7 +++++++
 2 files changed, 21 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index eea8c09..ab46aaa 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1792,6 +1792,20 @@ static int kvm_ppc_register_host_cpu_type(void)
     return 0;
 }
 
+int kvmppc_define_rtas_token(uint32_t token, const char *function)
+{
+    struct kvm_rtas_token_args args = {
+        .token = token,
+    };
+
+    if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_RTAS)) {
+        return -ENOENT;
+    }
+
+    strncpy(args.name, function, sizeof(args.name));
+
+    return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN, &args);
+}
 
 int kvmppc_get_htab_fd(bool write)
 {
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 4ae7bf2..12564ef 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -38,6 +38,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
 #endif /* !CONFIG_USER_ONLY */
 int kvmppc_fixup_cpu(PowerPCCPU *cpu);
 bool kvmppc_has_cap_epr(void);
+int kvmppc_define_rtas_token(uint32_t token, const char *function);
 int kvmppc_get_htab_fd(bool write);
 int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
 int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
@@ -164,6 +165,12 @@ static inline bool kvmppc_has_cap_epr(void)
     return false;
 }
 
+static inline int kvmppc_define_rtas_token(uint32_t token,
+                                           const char *function)
+{
+    return -1;
+}
+
 static inline int kvmppc_get_htab_fd(bool write)
 {
     return -1;
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Alexey Kardashevskiy]

The upcoming support of in-kernel XICS will redefine migration callbacks
for both ICS and ICP so classes and callback pointers are added.

This adds a cpu_setup callback to the XICS device class (as XICS-KVM
will do it different) and xics_dispatch_cpu_setup(). This also moves
the place where xics_dispatch_cpu_setup() is called a bit further to
have VCPU initialized (required for XICS-KVM).

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* fixed local variables names
---
 hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
 hw/ppc/spapr.c        |  4 ++--
 include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 6b3c071..e3a957d 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -153,11 +153,35 @@ static void icp_irq(XICSState *icp, int server, int nr, uint8_t priority)
     }
 }
 
+static void icp_dispatch_pre_save(void *opaque)
+{
+    ICPState *ss = opaque;
+    ICPStateClass *info = ICP_GET_CLASS(ss);
+
+    if (info->pre_save) {
+        info->pre_save(ss);
+    }
+}
+
+static int icp_dispatch_post_load(void *opaque, int version_id)
+{
+    ICPState *ss = opaque;
+    ICPStateClass *info = ICP_GET_CLASS(ss);
+
+    if (info->post_load) {
+        return info->post_load(ss, 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_dispatch_pre_save,
+    .post_load = icp_dispatch_post_load,
     .fields      = (VMStateField []) {
         /* Sanity check */
         VMSTATE_UINT32(xirr, ICPState),
@@ -192,6 +216,7 @@ static TypeInfo icp_info = {
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICPState),
     .class_init = icp_class_init,
+    .class_size = sizeof(ICPStateClass),
 };
 
 /*
@@ -353,10 +378,9 @@ static void ics_reset(DeviceState *dev)
     }
 }
 
-static int ics_post_load(void *opaque, int version_id)
+static int ics_post_load(ICSState *ics, int version_id)
 {
     int i;
-    ICSState *ics = opaque;
 
     for (i = 0; i < ics->icp->nr_servers; i++) {
         icp_resend(ics->icp, i);
@@ -365,6 +389,28 @@ static int ics_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static void ics_dispatch_pre_save(void *opaque)
+{
+    ICSState *ics = opaque;
+    ICSStateClass *info = ICS_GET_CLASS(ics);
+
+    if (info->pre_save) {
+        info->pre_save(ics);
+    }
+}
+
+static int ics_dispatch_post_load(void *opaque, int version_id)
+{
+    ICSState *ics = opaque;
+    ICSStateClass *info = ICS_GET_CLASS(ics);
+
+    if (info->post_load) {
+        return info->post_load(ics, version_id);
+    }
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_ics_irq = {
     .name = "ics/irq",
     .version_id = 1,
@@ -384,7 +430,8 @@ static const VMStateDescription vmstate_ics = {
     .version_id = 1,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
-    .post_load = ics_post_load,
+    .pre_save = ics_dispatch_pre_save,
+    .post_load = ics_dispatch_post_load,
     .fields      = (VMStateField []) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
@@ -409,10 +456,12 @@ static int ics_realize(DeviceState *dev)
 static void ics_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ICSStateClass *isc = ICS_CLASS(klass);
 
     dc->init = ics_realize;
     dc->vmsd = &vmstate_ics;
     dc->reset = ics_reset;
+    isc->post_load = ics_post_load;
 }
 
 static TypeInfo ics_info = {
@@ -420,6 +469,7 @@ static TypeInfo ics_info = {
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICSState),
     .class_init = ics_class_init,
+    .class_size = sizeof(ICSStateClass),
 };
 
 /*
@@ -612,7 +662,7 @@ static void xics_reset(DeviceState *d)
     device_reset(DEVICE(icp->ics));
 }
 
-void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
@@ -674,10 +724,12 @@ static Property xics_properties[] = {
 static void xics_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    XICSStateClass *k = XICS_CLASS(oc);
 
     dc->realize = xics_realize;
     dc->props = xics_properties;
     dc->reset = xics_reset;
+    k->cpu_setup = xics_cpu_setup;
 
     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
@@ -694,6 +746,7 @@ static const TypeInfo xics_info = {
     .name          = TYPE_XICS,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(XICSState),
+    .class_size = sizeof(XICSStateClass),
     .class_init    = xics_class_init,
     .instance_init = xics_initfn,
 };
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 16bfab9..432f0d2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1155,8 +1155,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);
 
@@ -1170,6 +1168,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
             kvmppc_set_papr(cpu);
         }
 
+        xics_dispatch_cpu_setup(spapr->icp, cpu);
+
         qemu_register_reset(spapr_cpu_reset, cpu);
     }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 66364c5..3a25987 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -32,6 +32,11 @@
 #define TYPE_XICS "xics"
 #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
 
+#define XICS_CLASS(klass) \
+     OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
+#define XICS_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
+
 #define XICS_IPI        0x2
 #define XICS_BUID       0x1
 #define XICS_IRQ_BASE   (XICS_BUID << 12)
@@ -41,11 +46,20 @@
  * (the kernel implementation supports more but we don't exploit
  *  that yet)
  */
+typedef struct XICSStateClass XICSStateClass;
 typedef struct XICSState XICSState;
+typedef struct ICPStateClass ICPStateClass;
 typedef struct ICPState ICPState;
+typedef struct ICSStateClass ICSStateClass;
 typedef struct ICSState ICSState;
 typedef struct ICSIRQState ICSIRQState;
 
+struct XICSStateClass {
+    DeviceClass parent_class;
+
+    void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
+};
+
 struct XICSState {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -59,6 +73,18 @@ struct XICSState {
 #define TYPE_ICP "icp"
 #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
 
+#define ICP_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
+#define ICP_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ICPStateClass, (obj), TYPE_ICP)
+
+struct ICPStateClass {
+    DeviceClass parent_class;
+
+    void (*pre_save)(ICPState *s);
+    int (*post_load)(ICPState *s, int version_id);
+};
+
 struct ICPState {
     /*< private >*/
     DeviceState parent_obj;
@@ -72,6 +98,18 @@ struct ICPState {
 #define TYPE_ICS "ics"
 #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
 
+#define ICS_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
+#define ICS_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
+
+struct ICSStateClass {
+    DeviceClass parent_class;
+
+    void (*pre_save)(ICSState *s);
+    int (*post_load)(ICSState *s, int version_id);
+};
+
 struct ICSState {
     /*< private >*/
     DeviceState parent_obj;
@@ -98,6 +136,12 @@ struct ICSIRQState {
 qemu_irq xics_get_qirq(XICSState *icp, int irq);
 void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
 
-void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+    XICSStateClass *info = XICS_GET_CLASS(icp);
+
+    assert(info->cpu_setup);
+    info->cpu_setup(icp, cpu);
+}
 
 #endif /* __XICS_H__ */
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 3/8] xics: move registration of global state to realize()

[Alexey Kardashevskiy]

Registration of global state belongs into realize so move it there.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
 hw/intc/xics.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e3a957d..c80fa80 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -692,6 +692,17 @@ static void xics_realize(DeviceState *dev, Error **errp)
     ICSState *ics = icp->ics;
     int i;
 
+    /* Registration of global state belongs into realize */
+    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);
+
+    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);
+
     ics->nr_irqs = icp->nr_irqs;
     ics->offset = XICS_IRQ_BASE;
     ics->icp = icp;
@@ -730,16 +741,6 @@ static void xics_class_init(ObjectClass *oc, void *data)
     dc->props = xics_properties;
     dc->reset = xics_reset;
     k->cpu_setup = xics_cpu_setup;
-
-    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);
-
-    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);
 }
 
 static const TypeInfo xics_info = {
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 4/8] xics: minor changes and cleanups

[Alexey Kardashevskiy]

Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.

This does:
1. add assert in ics_realize()
2. change variable names from "k" to more informative ones
3. add "const" to every TypeInfo
4. replace fprintf(stderr, ..."\n") with error_report
5. replace old style qdev_init_nofail() with new style
object_property_set_bool().

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* ics_realize() fixed to be actual realize callback rather than initfn
* asserts replaced with Error**
---
 hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c80fa80..4d08c58 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -29,6 +29,7 @@
 #include "trace.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "qemu/error-report.h"
 
 /*
  * ICP: Presentation layer
@@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_icp_server;
 }
 
-static TypeInfo icp_info = {
+static const TypeInfo icp_info = {
     .name = TYPE_ICP,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICPState),
@@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = {
     },
 };
 
-static int ics_realize(DeviceState *dev)
+static void ics_realize(DeviceState *dev, Error **errp)
 {
     ICSState *ics = ICS(dev);
 
+    if (!ics->nr_irqs) {
+        error_setg(errp, "Number of interrupts needs to be greater 0");
+        return;
+    }
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
     ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
-
-    return 0;
 }
 
 static void ics_class_init(ObjectClass *klass, void *data)
@@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     ICSStateClass *isc = ICS_CLASS(klass);
 
-    dc->init = ics_realize;
+    dc->realize = ics_realize;
     dc->vmsd = &vmstate_ics;
     dc->reset = ics_reset;
     isc->post_load = ics_post_load;
+    isc->post_load = ics_post_load;
 }
 
-static TypeInfo ics_info = {
+static const TypeInfo ics_info = {
     .name = TYPE_ICS,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICSState),
@@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
         break;
 
     default:
-        fprintf(stderr, "XICS interrupt controller does not support this CPU "
-                "bus model\n");
+        error_report("XICS interrupt controller does not support this CPU "
+                "bus model");
         abort();
     }
 }
@@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp)
 {
     XICSState *icp = XICS(dev);
     ICSState *ics = icp->ics;
+    Error *error = NULL;
     int i;
 
+    if (!icp->nr_servers) {
+        error_setg(errp, "Number of servers needs to be greater 0");
+        return;
+    }
+
     /* Registration of global state belongs into realize */
     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
@@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
     ics->nr_irqs = icp->nr_irqs;
     ics->offset = XICS_IRQ_BASE;
     ics->icp = icp;
-    qdev_init_nofail(DEVICE(ics));
+    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
 
     icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
@@ -714,7 +728,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
         object_initialize(&icp->ss[i], TYPE_ICP);
         snprintf(buffer, sizeof(buffer), "icp[%d]", i);
         object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
-        qdev_init_nofail(DEVICE(&icp->ss[i]));
+        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
+        if (error) {
+            error_propagate(errp, error);
+            return;
+        }
     }
 }
 
@@ -735,12 +753,12 @@ static Property xics_properties[] = {
 static void xics_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
-    XICSStateClass *k = XICS_CLASS(oc);
+    XICSStateClass *xsc = XICS_CLASS(oc);
 
     dc->realize = xics_realize;
     dc->props = xics_properties;
     dc->reset = xics_reset;
-    k->cpu_setup = xics_cpu_setup;
+    xsc->cpu_setup = xics_cpu_setup;
 }
 
 static const TypeInfo xics_info = {
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 5/8] xics: split to xics and xics-common

[Alexey Kardashevskiy]

The upcoming XICS-KVM support will use bits of emulated XICS code.
So this introduces new level of hierarchy - "xics-common" class. Both
emulated XICS and XICS-KVM will inherit from it and override class
callbacks when required.

The new "xics-common" class implements:
1. replaces static "nr_irqs" and "nr_servers" properties with
the dynamic ones and adds callbacks to be executed when properties
are set.
2. xics_cpu_setup() callback renamed to xics_common_cpu_setup() as
it is a common part for both XICS'es
3. xics_reset() renamed to xics_common_reset() for the same reason.

The emulated XICS changes:
1. the part of xics_realize() which creates ICPs is moved to
the "nr_servers" property callback as realize() is too late to
create/initialize devices and instance_init() is too early to create
devices as the number of child devices comes via the "nr_servers"
property.
2. added ics_initfn() which does a little part of what xics_realize() did.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* added getters for dynamic properties
* fixed some indentations, added some comments
* moved ICS allocation from the nr_irqs property setter to XICS initfn
(where it was initially after Anthony's rework)
---
 hw/intc/xics.c        | 213 ++++++++++++++++++++++++++++++++++++++------------
 include/hw/ppc/xics.h |  11 ++-
 2 files changed, 175 insertions(+), 49 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 4d08c58..f439e8d 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -30,6 +30,143 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
 #include "qemu/error-report.h"
+#include "qapi/visitor.h"
+
+/*
+ * XICS Common class - parent for emulated XICS and KVM-XICS
+ */
+
+static void xics_common_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    ICPState *ss = &icp->ss[cs->cpu_index];
+
+    assert(cs->cpu_index < icp->nr_servers);
+
+    switch (PPC_INPUT(env)) {
+    case PPC_FLAGS_INPUT_POWER7:
+        ss->output = env->irq_inputs[POWER7_INPUT_INT];
+        break;
+
+    case PPC_FLAGS_INPUT_970:
+        ss->output = env->irq_inputs[PPC970_INPUT_INT];
+        break;
+
+    default:
+        error_report("XICS interrupt controller does not support this CPU "
+                     "bus model");
+        /* Called from machine_init only so no point in Error**, just abort */
+        abort();
+    }
+}
+
+static void xics_prop_get_nr_irqs(Object *obj, Visitor *v,
+                                  void *opaque, const char *name, Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    int64_t value = icp->nr_irqs;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void xics_prop_set_nr_irqs(Object *obj, Visitor *v,
+                                  void *opaque, const char *name, Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    if (icp->nr_irqs) {
+        error_setg(errp, "Number of interrupts is already set to %u",
+                   icp->nr_irqs);
+        return;
+    }
+
+    assert(info->set_nr_irqs);
+    assert(icp->ics);
+    info->set_nr_irqs(icp, value, errp);
+}
+
+static void xics_prop_get_nr_servers(Object *obj, Visitor *v,
+                                     void *opaque, const char *name,
+                                     Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    int64_t value = icp->nr_servers;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void xics_prop_set_nr_servers(Object *obj, Visitor *v,
+                                     void *opaque, const char *name,
+                                     Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    if (icp->nr_servers) {
+        error_setg(errp, "Number of servers is already set to %u",
+                   icp->nr_servers);
+        return;
+    }
+
+    assert(info->set_nr_servers);
+    info->set_nr_servers(icp, value, errp);
+}
+
+static void xics_common_initfn(Object *obj)
+{
+    object_property_add(obj, "nr_irqs", "int",
+                        xics_prop_get_nr_irqs, xics_prop_set_nr_irqs,
+                        NULL, NULL, NULL);
+    object_property_add(obj, "nr_servers", "int",
+                        xics_prop_get_nr_servers, xics_prop_set_nr_servers,
+                        NULL, NULL, NULL);
+}
+
+static void xics_common_reset(DeviceState *d)
+{
+    XICSState *icp = XICS_COMMON(d);
+    int i;
+
+    for (i = 0; i < icp->nr_servers; i++) {
+        device_reset(DEVICE(&icp->ss[i]));
+    }
+
+    device_reset(DEVICE(icp->ics));
+}
+
+static void xics_common_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
+
+    dc->reset = xics_common_reset;
+    xsc->cpu_setup = xics_common_cpu_setup;
+}
+
+static const TypeInfo xics_common_info = {
+    .name          = TYPE_XICS_COMMON,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XICSState),
+    .class_size    = sizeof(XICSStateClass),
+    .instance_init = xics_common_initfn,
+    .class_init    = xics_common_class_init,
+};
 
 /*
  * ICP: Presentation layer
@@ -443,6 +580,13 @@ static const VMStateDescription vmstate_ics = {
     },
 };
 
+static void ics_initfn(Object *obj)
+{
+    ICSState *ics = ICS(obj);
+
+    ics->offset = XICS_IRQ_BASE;
+}
+
 static void ics_realize(DeviceState *dev, Error **errp)
 {
     ICSState *ics = ICS(dev);
@@ -474,6 +618,7 @@ static const TypeInfo ics_info = {
     .instance_size = sizeof(ICSState),
     .class_init = ics_class_init,
     .class_size = sizeof(ICSStateClass),
+    .instance_init = ics_initfn,
 };
 
 /*
@@ -654,46 +799,31 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
  * XICS
  */
 
-static void xics_reset(DeviceState *d)
+static void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
+{
+    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
+}
+
+static void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers,
+                                Error **errp)
 {
-    XICSState *icp = XICS(d);
     int i;
 
+    icp->nr_servers = nr_servers;
+
+    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
-        device_reset(DEVICE(&icp->ss[i]));
-    }
-
-    device_reset(DEVICE(icp->ics));
-}
-
-static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    ICPState *ss = &icp->ss[cs->cpu_index];
-
-    assert(cs->cpu_index < icp->nr_servers);
-
-    switch (PPC_INPUT(env)) {
-    case PPC_FLAGS_INPUT_POWER7:
-        ss->output = env->irq_inputs[POWER7_INPUT_INT];
-        break;
-
-    case PPC_FLAGS_INPUT_970:
-        ss->output = env->irq_inputs[PPC970_INPUT_INT];
-        break;
-
-    default:
-        error_report("XICS interrupt controller does not support this CPU "
-                "bus model");
-        abort();
+        char buffer[32];
+        object_initialize(&icp->ss[i], TYPE_ICP);
+        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
+        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]),
+                                  errp);
     }
 }
 
 static void xics_realize(DeviceState *dev, Error **errp)
 {
     XICSState *icp = XICS(dev);
-    ICSState *ics = icp->ics;
     Error *error = NULL;
     int i;
 
@@ -713,21 +843,13 @@ static void xics_realize(DeviceState *dev, Error **errp)
     spapr_register_hypercall(H_XIRR, h_xirr);
     spapr_register_hypercall(H_EOI, h_eoi);
 
-    ics->nr_irqs = icp->nr_irqs;
-    ics->offset = XICS_IRQ_BASE;
-    ics->icp = icp;
     object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
     if (error) {
         error_propagate(errp, error);
         return;
     }
 
-    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
-        char buffer[32];
-        object_initialize(&icp->ss[i], TYPE_ICP);
-        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
-        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
         object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
         if (error) {
             error_propagate(errp, error);
@@ -742,28 +864,22 @@ static void xics_initfn(Object *obj)
 
     xics->ics = ICS(object_new(TYPE_ICS));
     object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
+    xics->ics->icp = xics;
 }
 
-static Property xics_properties[] = {
-    DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
-    DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void xics_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     XICSStateClass *xsc = XICS_CLASS(oc);
 
     dc->realize = xics_realize;
-    dc->props = xics_properties;
-    dc->reset = xics_reset;
-    xsc->cpu_setup = xics_cpu_setup;
+    xsc->set_nr_irqs = xics_set_nr_irqs;
+    xsc->set_nr_servers = xics_set_nr_servers;
 }
 
 static const TypeInfo xics_info = {
     .name          = TYPE_XICS,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_XICS_COMMON,
     .instance_size = sizeof(XICSState),
     .class_size = sizeof(XICSStateClass),
     .class_init    = xics_class_init,
@@ -772,6 +888,7 @@ static const TypeInfo xics_info = {
 
 static void xics_register_types(void)
 {
+    type_register_static(&xics_common_info);
     type_register_static(&xics_info);
     type_register_static(&ics_info);
     type_register_static(&icp_info);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 3a25987..540bb81 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -29,11 +29,18 @@
 
 #include "hw/sysbus.h"
 
+#define TYPE_XICS_COMMON "xics-common"
+#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMMON)
+
 #define TYPE_XICS "xics"
 #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
 
+#define XICS_COMMON_CLASS(klass) \
+     OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
 #define XICS_CLASS(klass) \
      OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
+#define XICS_COMMON_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS_COMMON)
 #define XICS_GET_CLASS(obj) \
      OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
 
@@ -58,6 +65,8 @@ struct XICSStateClass {
     DeviceClass parent_class;
 
     void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
+    void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
+    void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error **errp);
 };
 
 struct XICSState {
@@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
 
 static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
-    XICSStateClass *info = XICS_GET_CLASS(icp);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
 
     assert(info->cpu_setup);
     info->cpu_setup(icp, cpu);
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 6/8] xics-kvm: 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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* ics_kvm_realize() now is a realize callback rather than initfn callback
* asserts replaced with Error**
* KVM_ICS is created now in KVM_XICS's initfn rather than in the nr_irqs
property setter
* added KVM_XICS_GET_PARENT_CLASS() to get the common XICS class - needed
for xics_kvm_cpu_setup() to call parent's cpu_setup()
* fixed some indentations, removed some \n from error_report()
---
 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c                    |  23 +-
 include/hw/ppc/xics.h             |  13 +
 5 files changed, 528 insertions(+), 2 deletions(-)
 create mode 100644 hw/intc/xics_kvm.c

diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index 7831c2b..116f4ca 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -47,6 +47,7 @@ CONFIG_E500=y
 CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
 # For pSeries
 CONFIG_XICS=$(CONFIG_PSERIES)
+CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
 # For PReP
 CONFIG_I82378=y
 CONFIG_I8259=y
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 2851eed..47ac442 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
 obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
 obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
+obj-$(CONFIG_XICS_KVM) += xics_kvm.o
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
new file mode 100644
index 0000000..4ed4f33
--- /dev/null
+++ b/hw/intc/xics_kvm.c
@@ -0,0 +1,492 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
+ *
+ * Copyright (c) 2013 David Gibson, IBM Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+
+#include "hw/hw.h"
+#include "trace.h"
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/xics.h"
+#include "kvm_ppc.h"
+#include "qemu/config-file.h"
+#include "qemu/error-report.h"
+
+#include <sys/ioctl.h>
+
+typedef struct KVMXICSState {
+    XICSState parent_obj;
+
+    uint32_t set_xive_token;
+    uint32_t get_xive_token;
+    uint32_t int_off_token;
+    uint32_t int_on_token;
+    int kernel_xics_fd;
+} KVMXICSState;
+
+/*
+ * ICP-KVM
+ */
+static void icp_get_kvm_state(ICPState *ss)
+{
+    uint64_t state;
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_PPC_ICP_STATE,
+        .addr = (uintptr_t)&state,
+    };
+    int ret;
+
+    /* ICP for this CPU thread is not in use, exiting */
+    if (!ss->cs) {
+        return;
+    }
+
+    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
+    if (ret != 0) {
+        error_report("Unable to retrieve KVM interrupt controller state"
+                " for CPU %d: %s", 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;
+}
+
+static int icp_set_kvm_state(ICPState *ss, int version_id)
+{
+    uint64_t state;
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_PPC_ICP_STATE,
+        .addr = (uintptr_t)&state,
+    };
+    int ret;
+
+    /* ICP for this CPU thread is not in use, exiting */
+    if (!ss->cs) {
+        return 0;
+    }
+
+    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) {
+        error_report("Unable to restore KVM interrupt controller state (0x%"
+                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
+                strerror(errno));
+        return ret;
+    }
+
+    return 0;
+}
+
+static void icp_kvm_reset(DeviceState *dev)
+{
+    ICPState *icp = ICP(dev);
+
+    icp->xirr = 0;
+    icp->pending_priority = 0xff;
+    icp->mfrr = 0xff;
+
+    /* Make all outputs are deasserted */
+    qemu_set_irq(icp->output, 0);
+
+    icp_set_kvm_state(icp, 1);
+}
+
+static void icp_kvm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ICPStateClass *icpc = ICP_CLASS(klass);
+
+    dc->reset = icp_kvm_reset;
+    icpc->pre_save = icp_get_kvm_state;
+    icpc->post_load = icp_set_kvm_state;
+}
+
+static const TypeInfo icp_kvm_info = {
+    .name = TYPE_KVM_ICP,
+    .parent = TYPE_ICP,
+    .instance_size = sizeof(ICPState),
+    .class_init = icp_kvm_class_init,
+    .class_size = sizeof(ICPStateClass),
+};
+
+/*
+ * ICS-KVM
+ */
+static void ics_get_kvm_state(ICSState *ics)
+{
+    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
+    uint64_t state;
+    struct kvm_device_attr attr = {
+        .flags = 0,
+        .group = KVM_DEV_XICS_GRP_SOURCES,
+        .addr = (uint64_t)(uintptr_t)&state,
+    };
+    int i;
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        ICSIRQState *irq = &ics->irqs[i];
+        int ret;
+
+        attr.attr = i + ics->offset;
+
+        ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
+        if (ret != 0) {
+            error_report("Unable to retrieve KVM interrupt controller state"
+                    " for IRQ %d: %s", 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;
+        /*
+         * To be consistent with the software emulation in xics.c, we
+         * split out the masked state + priority that we get from the
+         * kernel into 'current priority' (0xff if masked) and
+         * 'saved priority' (if masked, this is the priority the
+         * interrupt had before it was masked).  Masking and unmasking
+         * are done with the ibm,int-off and ibm,int-on RTAS calls.
+         */
+        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 {
+                /*
+                 * A pending edge-triggered interrupt (or MSI)
+                 * must have been rejected previously when we
+                 * first detected it and tried to deliver it,
+                 * so mark it as pending and previously rejected
+                 * for consistency with how xics.c works.
+                 */
+                irq->status |= XICS_STATUS_MASKED_PENDING
+                    | XICS_STATUS_REJECTED;
+            }
+        }
+    }
+}
+
+static int ics_set_kvm_state(ICSState *ics, int version_id)
+{
+    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
+    uint64_t state;
+    struct kvm_device_attr attr = {
+        .flags = 0,
+        .group = KVM_DEV_XICS_GRP_SOURCES,
+        .addr = (uint64_t)(uintptr_t)&state,
+    };
+    int i;
+
+    for (i = 0; i < ics->nr_irqs; i++) {
+        ICSIRQState *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(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
+        if (ret != 0) {
+            error_report("Unable to restore KVM interrupt controller state"
+                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
+static void ics_kvm_set_irq(void *opaque, int srcno, int val)
+{
+    ICSState *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 void ics_kvm_reset(DeviceState *dev)
+{
+    ics_set_kvm_state(ICS(dev), 1);
+}
+
+static void ics_kvm_realize(DeviceState *dev, Error **errp)
+{
+    ICSState *ics = ICS(dev);
+
+    if (!ics->nr_irqs) {
+        error_setg(errp, "Number of interrupts needs to be greater 0");
+        return;
+    }
+    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
+    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
+    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
+}
+
+static void ics_kvm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ICSStateClass *icsc = ICS_CLASS(klass);
+
+    dc->realize = ics_kvm_realize;
+    dc->reset = ics_kvm_reset;
+    icsc->pre_save = ics_get_kvm_state;
+    icsc->post_load = ics_set_kvm_state;
+}
+
+static const TypeInfo ics_kvm_info = {
+    .name = TYPE_KVM_ICS,
+    .parent = TYPE_ICS,
+    .instance_size = sizeof(ICSState),
+    .class_init = ics_kvm_class_init,
+};
+
+/*
+ * XICS-KVM
+ */
+static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs;
+    ICPState *ss;
+    KVMXICSState *icpkvm = KVM_XICS(icp);
+    ObjectClass *poc = KVM_XICS_GET_PARENT_CLASS(OBJECT(icp));
+    XICSStateClass *xsc = XICS_COMMON_CLASS(poc);
+
+    cs = CPU(cpu);
+    ss = &icp->ss[cs->cpu_index];
+
+    assert(cs->cpu_index < icp->nr_servers);
+    if (icpkvm->kernel_xics_fd == -1) {
+        abort();
+    }
+
+    if (icpkvm->kernel_xics_fd != -1) {
+        int ret;
+        struct kvm_enable_cap xics_enable_cap = {
+            .cap = KVM_CAP_IRQ_XICS,
+            .flags = 0,
+            .args = {icpkvm->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) {
+            error_report("Unable to connect CPU%d to kernel XICS: %s",
+                    cs->cpu_index, strerror(errno));
+            exit(1);
+        }
+    }
+
+    /* Call emulated XICS implementation for consistency */
+    assert(xsc && xsc->cpu_setup);
+    xsc->cpu_setup(icp, cpu);
+}
+
+static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
+{
+    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
+}
+
+static void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers,
+                                    Error **errp)
+{
+    int i;
+
+    icp->nr_servers = nr_servers;
+
+    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
+    for (i = 0; i < icp->nr_servers; i++) {
+        char buffer[32];
+        object_initialize(&icp->ss[i], TYPE_KVM_ICP);
+        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
+        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]),
+                                  errp);
+    }
+}
+
+static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                       uint32_t token,
+                       uint32_t nargs, target_ulong args,
+                       uint32_t nret, target_ulong rets)
+{
+    error_report("pseries: %s must never be called for in-kernel XICS",
+            __func__);
+}
+
+static void xics_kvm_realize(DeviceState *dev, Error **errp)
+{
+    KVMXICSState *icpkvm = KVM_XICS(dev);
+    XICSState *icp = XICS_COMMON(dev);
+    int i, rc;
+    Error *error = NULL;
+    struct kvm_create_device xics_create_device = {
+        .type = KVM_DEV_TYPE_XICS,
+        .flags = 0,
+    };
+
+    if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
+        error_setg(errp,
+                   "KVM and IRQ_XICS capability must be present for in-kernel XICS");
+        goto fail;
+    }
+
+    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
+    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
+    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
+    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
+
+    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
+    if (rc < 0) {
+        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
+    if (rc < 0) {
+        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
+    if (rc < 0) {
+        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
+        goto fail;
+    }
+
+    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
+    if (rc < 0) {
+        error_setg(errp, "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) {
+        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
+        goto fail;
+    }
+
+    icpkvm->kernel_xics_fd = xics_create_device.fd;
+
+    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
+    if (error) {
+        error_propagate(errp, error);
+        goto fail;
+    }
+
+    assert(icp->nr_servers);
+    for (i = 0; i < icp->nr_servers; i++) {
+        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
+        if (error) {
+            error_propagate(errp, error);
+            goto fail;
+        }
+    }
+    return;
+
+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");
+}
+
+static void xics_kvm_initfn(Object *obj)
+{
+    XICSState *xics = XICS_COMMON(obj);
+
+    xics->ics = ICS(object_new(TYPE_KVM_ICS));
+    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
+    xics->ics->icp = xics;
+}
+
+static void xics_kvm_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
+
+    dc->realize = xics_kvm_realize;
+    xsc->cpu_setup = xics_kvm_cpu_setup;
+    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
+    xsc->set_nr_servers = xics_kvm_set_nr_servers;
+}
+
+static const TypeInfo xics_kvm_info = {
+    .name          = TYPE_KVM_XICS,
+    .parent        = TYPE_XICS_COMMON,
+    .instance_size = sizeof(KVMXICSState),
+    .class_init    = xics_kvm_class_init,
+    .instance_init = xics_kvm_initfn,
+};
+
+static void xics_kvm_register_types(void)
+{
+    type_register_static(&xics_kvm_info);
+    type_register_static(&ics_kvm_info);
+    type_register_static(&icp_kvm_info);
+}
+
+type_init(xics_kvm_register_types)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 432f0d2..8dc8565 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -141,14 +141,33 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
         return NULL;
     }
 
-    return XICS(dev);
+    return XICS_COMMON(dev);
 }
 
 static XICSState *xics_system_init(int nr_servers, int nr_irqs)
 {
     XICSState *icp = NULL;
 
-    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
+    if (kvm_enabled()) {
+        QemuOpts *machine_opts = qemu_get_machine_opts();
+        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
+                                                "kernel_irqchip", true);
+        bool irqchip_required = qemu_opt_get_bool(machine_opts,
+                                                  "kernel_irqchip", false);
+        if (irqchip_allowed) {
+            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
+        }
+
+        if (irqchip_required && !icp) {
+            perror("Failed to create in-kernel XICS\n");
+            abort();
+        }
+    }
+
+    if (!icp) {
+        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
+    }
+
     if (!icp) {
         perror("Failed to create XICS\n");
         abort();
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 540bb81..be3c0a1 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -35,6 +35,9 @@
 #define TYPE_XICS "xics"
 #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
 
+#define TYPE_KVM_XICS "xics-kvm"
+#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
+
 #define XICS_COMMON_CLASS(klass) \
      OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
 #define XICS_CLASS(klass) \
@@ -44,6 +47,9 @@
 #define XICS_GET_CLASS(obj) \
      OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
 
+#define KVM_XICS_GET_PARENT_CLASS(obj) \
+    object_class_get_parent(object_class_by_name(TYPE_KVM_XICS))
+
 #define XICS_IPI        0x2
 #define XICS_BUID       0x1
 #define XICS_IRQ_BASE   (XICS_BUID << 12)
@@ -82,6 +88,9 @@ struct XICSState {
 #define TYPE_ICP "icp"
 #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
 
+#define TYPE_KVM_ICP "icp-kvm"
+#define KVM_ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_KVM_ICP)
+
 #define ICP_CLASS(klass) \
      OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
 #define ICP_GET_CLASS(obj) \
@@ -98,6 +107,7 @@ struct ICPState {
     /*< private >*/
     DeviceState parent_obj;
     /*< public >*/
+    CPUState *cs;
     uint32_t xirr;
     uint8_t pending_priority;
     uint8_t mfrr;
@@ -107,6 +117,9 @@ struct ICPState {
 #define TYPE_ICS "ics"
 #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
 
+#define TYPE_KVM_ICS "icskvm"
+#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
+
 #define ICS_CLASS(klass) \
      OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
 #define ICS_GET_CLASS(obj) \
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Andreas Färber]

Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
> The upcoming support of in-kernel XICS will redefine migration callbacks
> for both ICS and ICP so classes and callback pointers are added.
> 
> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
> will do it different) and xics_dispatch_cpu_setup(). This also moves
> the place where xics_dispatch_cpu_setup() is called a bit further to
> have VCPU initialized (required for XICS-KVM).
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * fixed local variables names
> ---
>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/ppc/spapr.c        |  4 ++--
>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 6b3c071..e3a957d 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
[...]
> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>  static void xics_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *k = XICS_CLASS(oc);
>  
>      dc->realize = xics_realize;
>      dc->props = xics_properties;
>      dc->reset = xics_reset;
> +    k->cpu_setup = xics_cpu_setup;
>  
>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);

This hunk is fixed up in 4/8, please squash that bit here.

Otherwise looks good.

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 v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Alexey Kardashevskiy]

On 08/19/2013 11:54 PM, Andreas Färber wrote:
> Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
>> The upcoming support of in-kernel XICS will redefine migration callbacks
>> for both ICS and ICP so classes and callback pointers are added.
>>
>> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
>> will do it different) and xics_dispatch_cpu_setup(). This also moves
>> the place where xics_dispatch_cpu_setup() is called a bit further to
>> have VCPU initialized (required for XICS-KVM).
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v3:
>> * fixed local variables names
>> ---
>>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>>  hw/ppc/spapr.c        |  4 ++--
>>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>>  3 files changed, 104 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 6b3c071..e3a957d 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
> [...]
>> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>>  static void xics_class_init(ObjectClass *oc, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(oc);
>> +    XICSStateClass *k = XICS_CLASS(oc);
>>  
>>      dc->realize = xics_realize;
>>      dc->props = xics_properties;
>>      dc->reset = xics_reset;
>> +    k->cpu_setup = xics_cpu_setup;
>>  
>>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> 
> This hunk is fixed up in 4/8, please squash that bit here.

Thanks for the review, fixed this.


> Otherwise looks good.

What exactly looks good? This patch only? The whole series? If it is the
whole series, can I put Reviewed-By: Andreas into them before repost them
all? I am asking because Alex Graf won't even look at them before I get
them reviewed/acked/signed by some one less ignorant than me :)
Thanks!



-- 
Alexey

Re: [Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Andreas Färber]

Am 23.08.2013 05:39, schrieb Alexey Kardashevskiy:
> On 08/19/2013 11:54 PM, Andreas Färber wrote:
>> Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
>>> The upcoming support of in-kernel XICS will redefine migration callbacks
>>> for both ICS and ICP so classes and callback pointers are added.
>>>
>>> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
>>> will do it different) and xics_dispatch_cpu_setup(). This also moves
>>> the place where xics_dispatch_cpu_setup() is called a bit further to
>>> have VCPU initialized (required for XICS-KVM).
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v3:
>>> * fixed local variables names
>>> ---
>>>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>>>  hw/ppc/spapr.c        |  4 ++--
>>>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>>>  3 files changed, 104 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index 6b3c071..e3a957d 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>> [...]
>>> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>>>  static void xics_class_init(ObjectClass *oc, void *data)
>>>  {
>>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>> +    XICSStateClass *k = XICS_CLASS(oc);
>>>  
>>>      dc->realize = xics_realize;
>>>      dc->props = xics_properties;
>>>      dc->reset = xics_reset;
>>> +    k->cpu_setup = xics_cpu_setup;
>>>  
>>>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);
>>
>> This hunk is fixed up in 4/8, please squash that bit here.
> 
> Thanks for the review, fixed this.
> 
> 
>> Otherwise looks good.
> 
> What exactly looks good? This patch only?

Yes. And another one has by Rb already I believe. I hope Alex can start
cherry-picking some more patches when he gets back to NUE.

I know you had some open questions I still need to reply to. Among
others, yes, I concur with mst that ppc/spapr_pci.c could be moved to
pci-host/spapr.c, but remember to update MAINTAINERS for you guys to
still get CC'ed on it then.

I had asked Anthony to reply to the KVM XICS patch because he said that
accessing the parent's method seemed wrong to him and the parent
implement should be put in charge of calling rather than the derived.
I.e., I will be dropping my proposed OBJECT_GET_PARENT_CLASS() macro and
started redoing several series. Haven't looked into how exactly your
code (CPU setup I think?) may need to change.

Andreas

> The whole series? If it is the
> whole series, can I put Reviewed-By: Andreas into them before repost them
> all? I am asking because Alex Graf won't even look at them before I get
> them reviewed/acked/signed by some one less ignorant than me :)
> Thanks!

-- 
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 v3 0/8] xics: reworks and in-kernel support

[Andreas Färber]

Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
> Yet another try with XICS-KVM.
> 
> v2->v3:
> Addressed multiple comments from Andreas;
> Added 2 patches for XICS from Ben - I included them into the series as they
> are about XICS and they won't rebase automatically if moved before XICS rework
> so it seemed to me that it would be better to carry them toghether. If it is
> wrong, please let me know, I'll repost them separately.

Patches 7-8 never arrived on the list?

Andreas

> 
> v1->v2:
> The main change is this adds "xics-common" parent for emulated XICS and XICS-KVM.
> And many, many small changes, mostly to address Andreas comments.
> 
> Migration from XICS to XICS-KVM and vice versa still works.
> 
> 
> Alexey Kardashevskiy (4):
>   xics: add pre_save/post_load/cpu_setup dispatchers
>   xics: move registration of global state to realize()
>   xics: minor changes and cleanups
>   xics: split to xics and xics-common
> 
> Benjamin Herrenschmidt (2):
>   xics: Add H_IPOLL implementation
>   xics: Implement H_XIRR_X
> 
> David Gibson (2):
>   target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN
>   xics-kvm: Support for in-kernel XICS interrupt controller
> 
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/intc/Makefile.objs             |   1 +
>  hw/intc/xics.c                    | 358 +++++++++++++++++++++------
>  hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr.c                    |  27 ++-
>  include/hw/ppc/spapr.h            |   3 +-
>  include/hw/ppc/xics.h             |  68 +++++-
>  target-ppc/kvm.c                  |  14 ++
>  target-ppc/kvm_ppc.h              |   7 +
>  9 files changed, 894 insertions(+), 77 deletions(-)
>  create mode 100644 hw/intc/xics_kvm.c
> 


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

[Qemu-devel] [PATCH v3] xics: Add H_IPOLL implementation

[Alexey Kardashevskiy]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This adds support for the H_IPOLL hypercall which the guest
uses to poll for a pending interrupt. This hypercall is
mandatory for PAPR+ and there is no way for the guest to
detect whether it is supported or not so just add it.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index f439e8d..2e6111d 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -689,6 +689,18 @@ static target_ulong h_eoi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                            target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+    ICPState *ss = &spapr->icp->ss[cs->cpu_index];
+
+    args[0] = ss->xirr;
+    args[1] = ss->mfrr;
+
+    return H_SUCCESS;
+}
+
 static void rtas_set_xive(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                           uint32_t token,
                           uint32_t nargs, target_ulong args,
@@ -842,6 +854,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
     spapr_register_hypercall(H_IPI, h_ipi);
     spapr_register_hypercall(H_XIRR, h_xirr);
     spapr_register_hypercall(H_EOI, h_eoi);
+    spapr_register_hypercall(H_IPOLL, h_ipoll);
 
     object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
     if (error) {
-- 
1.8.4.rc4

[Qemu-devel] [PATCH v3] xics: Implement H_XIRR_X

[Alexey Kardashevskiy]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This implements H_XIRR_X hypercall in addition to H_XIRR as
it is mandatory for PAPR+ and there is no way for the guest to
detect whether it is supported or not so just add it.

As the Partition Adjunct Option is not supported at the moment,
the CPPR parameter of the hypercall is ignored.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c         | 14 ++++++++++++++
 include/hw/ppc/spapr.h |  3 ++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 2e6111d..b7f849a 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -27,6 +27,7 @@
 
 #include "hw/hw.h"
 #include "trace.h"
+#include "qemu/timer.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
 #include "qemu/error-report.h"
@@ -679,6 +680,18 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
+static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                             target_ulong opcode, target_ulong *args)
+{
+    CPUState *cs = CPU(cpu);
+    ICPState *ss = spapr->icp->ss + cs->cpu_index;
+    uint32_t xirr = icp_accept(ss);
+
+    args[0] = xirr;
+    args[1] = cpu_get_real_ticks();
+    return H_SUCCESS;
+}
+
 static target_ulong h_eoi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                           target_ulong opcode, target_ulong *args)
 {
@@ -853,6 +866,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
     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_XIRR_X, h_xirr_x);
     spapr_register_hypercall(H_EOI, h_eoi);
     spapr_register_hypercall(H_IPOLL, h_ipoll);
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 9fc1972..a84b8ff 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -267,7 +267,8 @@ typedef struct sPAPREnvironment {
 #define H_GET_EM_PARMS          0x2B8
 #define H_SET_MPP               0x2D0
 #define H_GET_MPP               0x2D4
-#define MAX_HCALL_OPCODE        H_GET_MPP
+#define H_XIRR_X                0x2FC
+#define MAX_HCALL_OPCODE        H_XIRR_X
 
 /* The hcalls above are standardized in PAPR and implemented by pHyp
  * as well.
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3 0/8] xics: reworks and in-kernel support

[Alexey Kardashevskiy]

On 08/23/2013 09:56 PM, Andreas Färber wrote:
> Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
>> Yet another try with XICS-KVM.
>>
>> v2->v3:
>> Addressed multiple comments from Andreas;
>> Added 2 patches for XICS from Ben - I included them into the series as they
>> are about XICS and they won't rebase automatically if moved before XICS rework
>> so it seemed to me that it would be better to carry them toghether. If it is
>> wrong, please let me know, I'll repost them separately.
> 
> Patches 7-8 never arrived on the list?

Weird. Posted them again as replies to the cover letter...


> 
> Andreas
> 
>>
>> v1->v2:
>> The main change is this adds "xics-common" parent for emulated XICS and XICS-KVM.
>> And many, many small changes, mostly to address Andreas comments.
>>
>> Migration from XICS to XICS-KVM and vice versa still works.
>>
>>
>> Alexey Kardashevskiy (4):
>>   xics: add pre_save/post_load/cpu_setup dispatchers
>>   xics: move registration of global state to realize()
>>   xics: minor changes and cleanups
>>   xics: split to xics and xics-common
>>
>> Benjamin Herrenschmidt (2):
>>   xics: Add H_IPOLL implementation
>>   xics: Implement H_XIRR_X
>>
>> David Gibson (2):
>>   target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN
>>   xics-kvm: Support for in-kernel XICS interrupt controller
>>
>>  default-configs/ppc64-softmmu.mak |   1 +
>>  hw/intc/Makefile.objs             |   1 +
>>  hw/intc/xics.c                    | 358 +++++++++++++++++++++------
>>  hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
>>  hw/ppc/spapr.c                    |  27 ++-
>>  include/hw/ppc/spapr.h            |   3 +-
>>  include/hw/ppc/xics.h             |  68 +++++-
>>  target-ppc/kvm.c                  |  14 ++
>>  target-ppc/kvm_ppc.h              |   7 +
>>  9 files changed, 894 insertions(+), 77 deletions(-)
>>  create mode 100644 hw/intc/xics_kvm.c
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent PowerKVM allows the kernel to intercept some RTAS calls from the
> guest directly.  This is used to implement the more efficient in-kernel
> XICS for example.  qemu is still responsible for assigning the RTAS token
> numbers however, and needs to tell the kernel which RTAS function name is
> assigned to a given token value.  This patch adds a convenience wrapper for
> the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this purpose.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> target-ppc/kvm.c     | 14 ++++++++++++++
> target-ppc/kvm_ppc.h |  7 +++++++
> 2 files changed, 21 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index eea8c09..ab46aaa 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1792,6 +1792,20 @@ static int kvm_ppc_register_host_cpu_type(void)
>     return 0;
> }
> 
> +int kvmppc_define_rtas_token(uint32_t token, const char *function)

The naming here is slightly confusing. What the ioctl does is define an rtas token that gets handled in-kernel. The name of the function should reflect this. How about something like kvmppc_define_rtas_in_kernel()?


Alex

> +{
> +    struct kvm_rtas_token_args args = {
> +        .token = token,
> +    };
> +
> +    if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_RTAS)) {
> +        return -ENOENT;
> +    }
> +
> +    strncpy(args.name, function, sizeof(args.name));
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN, &args);
> +}
> 
> int kvmppc_get_htab_fd(bool write)
> {
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4ae7bf2..12564ef 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -38,6 +38,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> #endif /* !CONFIG_USER_ONLY */
> int kvmppc_fixup_cpu(PowerPCCPU *cpu);
> bool kvmppc_has_cap_epr(void);
> +int kvmppc_define_rtas_token(uint32_t token, const char *function);
> int kvmppc_get_htab_fd(bool write);
> int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
> int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> @@ -164,6 +165,12 @@ static inline bool kvmppc_has_cap_epr(void)
>     return false;
> }
> 
> +static inline int kvmppc_define_rtas_token(uint32_t token,
> +                                           const char *function)
> +{
> +    return -1;
> +}
> +
> static inline int kvmppc_get_htab_fd(bool write)
> {
>     return -1;
> -- 
> 1.8.3.2
> 

Re: [Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> The upcoming support of in-kernel XICS will redefine migration callbacks
> for both ICS and ICP so classes and callback pointers are added.
> 
> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
> will do it different) and xics_dispatch_cpu_setup(). This also moves
> the place where xics_dispatch_cpu_setup() is called a bit further to
> have VCPU initialized (required for XICS-KVM).
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * fixed local variables names
> ---
> hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
> hw/ppc/spapr.c        |  4 ++--
> include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 6b3c071..e3a957d 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -153,11 +153,35 @@ static void icp_irq(XICSState *icp, int server, int nr, uint8_t priority)
>     }
> }
> 
> +static void icp_dispatch_pre_save(void *opaque)
> +{
> +    ICPState *ss = opaque;
> +    ICPStateClass *info = ICP_GET_CLASS(ss);
> +
> +    if (info->pre_save) {
> +        info->pre_save(ss);
> +    }
> +}
> +
> +static int icp_dispatch_post_load(void *opaque, int version_id)
> +{
> +    ICPState *ss = opaque;
> +    ICPStateClass *info = ICP_GET_CLASS(ss);
> +
> +    if (info->post_load) {
> +        return info->post_load(ss, 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_dispatch_pre_save,
> +    .post_load = icp_dispatch_post_load,
>     .fields      = (VMStateField []) {
>         /* Sanity check */
>         VMSTATE_UINT32(xirr, ICPState),
> @@ -192,6 +216,7 @@ static TypeInfo icp_info = {
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICPState),
>     .class_init = icp_class_init,
> +    .class_size = sizeof(ICPStateClass),
> };
> 
> /*
> @@ -353,10 +378,9 @@ static void ics_reset(DeviceState *dev)
>     }
> }
> 
> -static int ics_post_load(void *opaque, int version_id)
> +static int ics_post_load(ICSState *ics, int version_id)
> {
>     int i;
> -    ICSState *ics = opaque;
> 
>     for (i = 0; i < ics->icp->nr_servers; i++) {
>         icp_resend(ics->icp, i);
> @@ -365,6 +389,28 @@ static int ics_post_load(void *opaque, int version_id)
>     return 0;
> }
> 
> +static void ics_dispatch_pre_save(void *opaque)
> +{
> +    ICSState *ics = opaque;
> +    ICSStateClass *info = ICS_GET_CLASS(ics);
> +
> +    if (info->pre_save) {
> +        info->pre_save(ics);
> +    }
> +}
> +
> +static int ics_dispatch_post_load(void *opaque, int version_id)
> +{
> +    ICSState *ics = opaque;
> +    ICSStateClass *info = ICS_GET_CLASS(ics);
> +
> +    if (info->post_load) {
> +        return info->post_load(ics, version_id);
> +    }
> +
> +    return 0;
> +}
> +
> static const VMStateDescription vmstate_ics_irq = {
>     .name = "ics/irq",
>     .version_id = 1,
> @@ -384,7 +430,8 @@ static const VMStateDescription vmstate_ics = {
>     .version_id = 1,
>     .minimum_version_id = 1,
>     .minimum_version_id_old = 1,
> -    .post_load = ics_post_load,
> +    .pre_save = ics_dispatch_pre_save,
> +    .post_load = ics_dispatch_post_load,
>     .fields      = (VMStateField []) {
>         /* Sanity check */
>         VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
> @@ -409,10 +456,12 @@ static int ics_realize(DeviceState *dev)
> static void ics_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *isc = ICS_CLASS(klass);
> 
>     dc->init = ics_realize;
>     dc->vmsd = &vmstate_ics;
>     dc->reset = ics_reset;
> +    isc->post_load = ics_post_load;
> }
> 
> static TypeInfo ics_info = {
> @@ -420,6 +469,7 @@ static TypeInfo ics_info = {
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICSState),
>     .class_init = ics_class_init,
> +    .class_size = sizeof(ICSStateClass),
> };
> 
> /*
> @@ -612,7 +662,7 @@ static void xics_reset(DeviceState *d)
>     device_reset(DEVICE(icp->ics));
> }
> 
> -void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> {
>     CPUState *cs = CPU(cpu);
>     CPUPPCState *env = &cpu->env;
> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
> static void xics_class_init(ObjectClass *oc, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *k = XICS_CLASS(oc);
> 
>     dc->realize = xics_realize;
>     dc->props = xics_properties;
>     dc->reset = xics_reset;
> +    k->cpu_setup = xics_cpu_setup;
> 
>     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> @@ -694,6 +746,7 @@ static const TypeInfo xics_info = {
>     .name          = TYPE_XICS,
>     .parent        = TYPE_SYS_BUS_DEVICE,
>     .instance_size = sizeof(XICSState),
> +    .class_size = sizeof(XICSStateClass),
>     .class_init    = xics_class_init,
>     .instance_init = xics_initfn,
> };
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 16bfab9..432f0d2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1155,8 +1155,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);
> 
> @@ -1170,6 +1168,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>             kvmppc_set_papr(cpu);
>         }
> 
> +        xics_dispatch_cpu_setup(spapr->icp, cpu);

Please move the code movement to a later patch where you actually introduce kvm xics. This patch should only do the rename.


Alex

Re: [Qemu-devel] [PATCH v3 3/8] xics: move registration of global state to realize()

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> Registration of global state belongs into realize so move it there.

Thanks, applied to ppc-next.


Alex

Re: [Qemu-devel] [PATCH v3 4/8] xics: minor changes and cleanups

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
> 
> This does:
> 1. add assert in ics_realize()
> 2. change variable names from "k" to more informative ones
> 3. add "const" to every TypeInfo
> 4. replace fprintf(stderr, ..."\n") with error_report
> 5. replace old style qdev_init_nofail() with new style
> object_property_set_bool().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * ics_realize() fixed to be actual realize callback rather than initfn
> * asserts replaced with Error**
> ---
> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c80fa80..4d08c58 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -29,6 +29,7 @@
> #include "trace.h"
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/xics.h"
> +#include "qemu/error-report.h"
> 
> /*
>  * ICP: Presentation layer
> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>     dc->vmsd = &vmstate_icp_server;
> }
> 
> -static TypeInfo icp_info = {
> +static const TypeInfo icp_info = {
>     .name = TYPE_ICP,
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICPState),
> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = {
>     },
> };
> 
> -static int ics_realize(DeviceState *dev)
> +static void ics_realize(DeviceState *dev, Error **errp)
> {
>     ICSState *ics = ICS(dev);
> 
> +    if (!ics->nr_irqs) {
> +        error_setg(errp, "Number of interrupts needs to be greater 0");
> +        return;
> +    }
>     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>     ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
> -
> -    return 0;
> }
> 
> static void ics_class_init(ObjectClass *klass, void *data)
> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data)
>     DeviceClass *dc = DEVICE_CLASS(klass);
>     ICSStateClass *isc = ICS_CLASS(klass);
> 
> -    dc->init = ics_realize;
> +    dc->realize = ics_realize;
>     dc->vmsd = &vmstate_ics;
>     dc->reset = ics_reset;
>     isc->post_load = ics_post_load;
> +    isc->post_load = ics_post_load;

Same line twice?

> }
> 
> -static TypeInfo ics_info = {
> +static const TypeInfo ics_info = {
>     .name = TYPE_ICS,
>     .parent = TYPE_DEVICE,
>     .instance_size = sizeof(ICSState),
> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>         break;
> 
>     default:
> -        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> -                "bus model\n");
> +        error_report("XICS interrupt controller does not support this CPU "
> +                "bus model");
>         abort();
>     }
> }
> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp)
> {
>     XICSState *icp = XICS(dev);
>     ICSState *ics = icp->ics;
> +    Error *error = NULL;
>     int i;
> 
> +    if (!icp->nr_servers) {
> +        error_setg(errp, "Number of servers needs to be greater 0");
> +        return;
> +    }
> +
>     /* Registration of global state belongs into realize */
>     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
>     ics->nr_irqs = icp->nr_irqs;
>     ics->offset = XICS_IRQ_BASE;
>     ics->icp = icp;
> -    qdev_init_nofail(DEVICE(ics));
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);

Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas?


Alex

Re: [Qemu-devel] [PATCH v3 5/8] xics: split to xics and xics-common

[Alexander Graf]

On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:

> The upcoming XICS-KVM support will use bits of emulated XICS code.
> So this introduces new level of hierarchy - "xics-common" class. Both
> emulated XICS and XICS-KVM will inherit from it and override class
> callbacks when required.
> 
> The new "xics-common" class implements:
> 1. replaces static "nr_irqs" and "nr_servers" properties with
> the dynamic ones and adds callbacks to be executed when properties
> are set.
> 2. xics_cpu_setup() callback renamed to xics_common_cpu_setup() as
> it is a common part for both XICS'es
> 3. xics_reset() renamed to xics_common_reset() for the same reason.
> 
> The emulated XICS changes:
> 1. the part of xics_realize() which creates ICPs is moved to
> the "nr_servers" property callback as realize() is too late to
> create/initialize devices and instance_init() is too early to create
> devices as the number of child devices comes via the "nr_servers"
> property.
> 2. added ics_initfn() which does a little part of what xics_realize() did.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH v3 4/8] xics: minor changes and cleanups

[Andreas Färber]

Am 26.08.2013 15:36, schrieb Alexander Graf:
> 
> On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:
> 
>> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
>>
>> This does:
>> 1. add assert in ics_realize()
>> 2. change variable names from "k" to more informative ones
>> 3. add "const" to every TypeInfo
>> 4. replace fprintf(stderr, ..."\n") with error_report
>> 5. replace old style qdev_init_nofail() with new style
>> object_property_set_bool().
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v3:
>> * ics_realize() fixed to be actual realize callback rather than initfn
>> * asserts replaced with Error**
>> ---
>> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------
>> 1 file changed, 30 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index c80fa80..4d08c58 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -29,6 +29,7 @@
>> #include "trace.h"
>> #include "hw/ppc/spapr.h"
>> #include "hw/ppc/xics.h"
>> +#include "qemu/error-report.h"
>>
>> /*
>>  * ICP: Presentation layer
>> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>>     dc->vmsd = &vmstate_icp_server;
>> }
>>
>> -static TypeInfo icp_info = {
>> +static const TypeInfo icp_info = {
>>     .name = TYPE_ICP,
>>     .parent = TYPE_DEVICE,
>>     .instance_size = sizeof(ICPState),
>> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = {
>>     },
>> };
>>
>> -static int ics_realize(DeviceState *dev)
>> +static void ics_realize(DeviceState *dev, Error **errp)
>> {
>>     ICSState *ics = ICS(dev);
>>
>> +    if (!ics->nr_irqs) {
>> +        error_setg(errp, "Number of interrupts needs to be greater 0");
>> +        return;
>> +    }
>>     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>>     ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>>     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>> -
>> -    return 0;
>> }
>>
>> static void ics_class_init(ObjectClass *klass, void *data)
>> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data)
>>     DeviceClass *dc = DEVICE_CLASS(klass);
>>     ICSStateClass *isc = ICS_CLASS(klass);
>>
>> -    dc->init = ics_realize;
>> +    dc->realize = ics_realize;
>>     dc->vmsd = &vmstate_ics;
>>     dc->reset = ics_reset;
>>     isc->post_load = ics_post_load;
>> +    isc->post_load = ics_post_load;
> 
> Same line twice?
> 
>> }
>>
>> -static TypeInfo ics_info = {
>> +static const TypeInfo ics_info = {
>>     .name = TYPE_ICS,
>>     .parent = TYPE_DEVICE,
>>     .instance_size = sizeof(ICSState),
>> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>         break;
>>
>>     default:
>> -        fprintf(stderr, "XICS interrupt controller does not support this CPU "
>> -                "bus model\n");
>> +        error_report("XICS interrupt controller does not support this CPU "
>> +                "bus model");
>>         abort();
>>     }
>> }
>> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp)
>> {
>>     XICSState *icp = XICS(dev);
>>     ICSState *ics = icp->ics;
>> +    Error *error = NULL;
>>     int i;
>>
>> +    if (!icp->nr_servers) {
>> +        error_setg(errp, "Number of servers needs to be greater 0");
>> +        return;
>> +    }
>> +
>>     /* Registration of global state belongs into realize */
>>     spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>     spapr_rtas_register("ibm,get-xive", rtas_get_xive);
>> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
>>     ics->nr_irqs = icp->nr_irqs;
>>     ics->offset = XICS_IRQ_BASE;
>>     ics->icp = icp;
>> -    qdev_init_nofail(DEVICE(ics));
>> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> 
> Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas?

Yes, it's correct, I requested it. This assures that errors are
forwarded up the chain to whomever sets realized = true. That's a
general pattern that some devices have been ignoring. Obviously for KVM

Another request apart from checking the duplicate line I have, is to
please split up this big patch. Everything except const should be in its
own patch with short description for better review and independent
ack'ing, so that we can move forward here. An explicit list of changes
within one patch should be a yellow warning sign. ;)
(I would prefer the patches to go through ppc-next with my Rb, but if
you prefer nonfunctional QOM changes to go through qom-next I'm sure we
could make that work as well.)

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 v3 4/8] xics: minor changes and cleanups

[Alexander Graf]

On 26.08.2013, at 16:20, Andreas Färber wrote:

> Am 26.08.2013 15:36, schrieb Alexander Graf:
>> 
>> On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:
>> 
>>> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
>>> 
>>> This does:
>>> 1. add assert in ics_realize()
>>> 2. change variable names from "k" to more informative ones
>>> 3. add "const" to every TypeInfo
>>> 4. replace fprintf(stderr, ..."\n") with error_report
>>> 5. replace old style qdev_init_nofail() with new style
>>> object_property_set_bool().
>>> 
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v3:
>>> * ics_realize() fixed to be actual realize callback rather than initfn
>>> * asserts replaced with Error**
>>> ---
>>> hw/intc/xics.c | 42 ++++++++++++++++++++++++++++++------------
>>> 1 file changed, 30 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index c80fa80..4d08c58 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -29,6 +29,7 @@
>>> #include "trace.h"
>>> #include "hw/ppc/spapr.h"
>>> #include "hw/ppc/xics.h"
>>> +#include "qemu/error-report.h"
>>> 
>>> /*
>>> * ICP: Presentation layer
>>> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>>>    dc->vmsd = &vmstate_icp_server;
>>> }
>>> 
>>> -static TypeInfo icp_info = {
>>> +static const TypeInfo icp_info = {
>>>    .name = TYPE_ICP,
>>>    .parent = TYPE_DEVICE,
>>>    .instance_size = sizeof(ICPState),
>>> @@ -442,15 +443,17 @@ static const VMStateDescription vmstate_ics = {
>>>    },
>>> };
>>> 
>>> -static int ics_realize(DeviceState *dev)
>>> +static void ics_realize(DeviceState *dev, Error **errp)
>>> {
>>>    ICSState *ics = ICS(dev);
>>> 
>>> +    if (!ics->nr_irqs) {
>>> +        error_setg(errp, "Number of interrupts needs to be greater 0");
>>> +        return;
>>> +    }
>>>    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>>>    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>>>    ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>>> -
>>> -    return 0;
>>> }
>>> 
>>> static void ics_class_init(ObjectClass *klass, void *data)
>>> @@ -458,13 +461,14 @@ static void ics_class_init(ObjectClass *klass, void *data)
>>>    DeviceClass *dc = DEVICE_CLASS(klass);
>>>    ICSStateClass *isc = ICS_CLASS(klass);
>>> 
>>> -    dc->init = ics_realize;
>>> +    dc->realize = ics_realize;
>>>    dc->vmsd = &vmstate_ics;
>>>    dc->reset = ics_reset;
>>>    isc->post_load = ics_post_load;
>>> +    isc->post_load = ics_post_load;
>> 
>> Same line twice?
>> 
>>> }
>>> 
>>> -static TypeInfo ics_info = {
>>> +static const TypeInfo ics_info = {
>>>    .name = TYPE_ICS,
>>>    .parent = TYPE_DEVICE,
>>>    .instance_size = sizeof(ICSState),
>>> @@ -680,8 +684,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>        break;
>>> 
>>>    default:
>>> -        fprintf(stderr, "XICS interrupt controller does not support this CPU "
>>> -                "bus model\n");
>>> +        error_report("XICS interrupt controller does not support this CPU "
>>> +                "bus model");
>>>        abort();
>>>    }
>>> }
>>> @@ -690,8 +694,14 @@ static void xics_realize(DeviceState *dev, Error **errp)
>>> {
>>>    XICSState *icp = XICS(dev);
>>>    ICSState *ics = icp->ics;
>>> +    Error *error = NULL;
>>>    int i;
>>> 
>>> +    if (!icp->nr_servers) {
>>> +        error_setg(errp, "Number of servers needs to be greater 0");
>>> +        return;
>>> +    }
>>> +
>>>    /* Registration of global state belongs into realize */
>>>    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>>    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
>>> @@ -706,7 +716,11 @@ static void xics_realize(DeviceState *dev, Error **errp)
>>>    ics->nr_irqs = icp->nr_irqs;
>>>    ics->offset = XICS_IRQ_BASE;
>>>    ics->icp = icp;
>>> -    qdev_init_nofail(DEVICE(ics));
>>> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
>> 
>> Are you sure this is correct? So you are trying to force set the ics to realize? Is this the right way to do this? Andreas?
> 
> Yes, it's correct, I requested it. This assures that errors are
> forwarded up the chain to whomever sets realized = true. That's a
> general pattern that some devices have been ignoring. Obviously for KVM
> 
> Another request apart from checking the duplicate line I have, is to
> please split up this big patch. Everything except const should be in its
> own patch with short description for better review and independent
> ack'ing, so that we can move forward here. An explicit list of changes
> within one patch should be a yellow warning sign. ;)
> (I would prefer the patches to go through ppc-next with my Rb, but if
> you prefer nonfunctional QOM changes to go through qom-next I'm sure we
> could make that work as well.)

I'm more than happy to take them through ppc-next :).


Alex

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 15:11 +0200, Alexander Graf wrote:
> On 19.08.2013, at 07:55, Alexey Kardashevskiy wrote:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Recent PowerKVM allows the kernel to intercept some RTAS calls from
> the
> > guest directly.  This is used to implement the more efficient
> in-kernel
> > XICS for example.  qemu is still responsible for assigning the RTAS
> token
> > numbers however, and needs to tell the kernel which RTAS function
> name is
> > assigned to a given token value.  This patch adds a convenience
> wrapper for
> > the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this
> purpose.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > target-ppc/kvm.c     | 14 ++++++++++++++
> > target-ppc/kvm_ppc.h |  7 +++++++
> > 2 files changed, 21 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index eea8c09..ab46aaa 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1792,6 +1792,20 @@ static int
> kvm_ppc_register_host_cpu_type(void)
> >     return 0;
> > }
> > 
> > +int kvmppc_define_rtas_token(uint32_t token, const char *function)
> 
> The naming here is slightly confusing. What the ioctl does is define
> an rtas token that gets handled in-kernel. The name of the function
> should reflect this. How about something like
> kvmppc_define_rtas_in_kernel()?

Am I dreaming ? Those patches were written about a year ago and you guys
are still nitpicking on names ? They should have been merged a LONG time
ago ...

I'm seriously wondering how people get anything done with qemu KVM when
it takes about a year of bike shed painting to get the most basic
functionality merged.

Alex, you know, a maintainer's job is not to bounce a patch over and
over again until it looks EXACTLY what you would have written in the
first place. If that was the case you may as well write it yourself.

In this specific case, it's even more annoying because that comment
could have been done ages ago, and because the name you propose is even
more horrible than what was there... If you are really keen about that
pick up at least something that doesn't suck such as

	kvmppc_define_rtas_kernel_token()

What you propose is even more confusing as to what the function is for.

At the end of the day, I despair though. Really.

Ben.

> 
> Alex
> 
> > +{
> > +    struct kvm_rtas_token_args args = {
> > +        .token = token,
> > +    };
> > +
> > +    if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_RTAS)) {
> > +        return -ENOENT;
> > +    }
> > +
> > +    strncpy(args.name, function, sizeof(args.name));
> > +
> > +    return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN,
> &args);
> > +}
> > 
> > int kvmppc_get_htab_fd(bool write)
> > {
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> > index 4ae7bf2..12564ef 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -38,6 +38,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size,
> unsigned int hash_shift);
> > #endif /* !CONFIG_USER_ONLY */
> > int kvmppc_fixup_cpu(PowerPCCPU *cpu);
> > bool kvmppc_has_cap_epr(void);
> > +int kvmppc_define_rtas_token(uint32_t token, const char *function);
> > int kvmppc_get_htab_fd(bool write);
> > int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t
> max_ns);
> > int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
> > @@ -164,6 +165,12 @@ static inline bool kvmppc_has_cap_epr(void)
> >     return false;
> > }
> > 
> > +static inline int kvmppc_define_rtas_token(uint32_t token,
> > +                                           const char *function)
> > +{
> > +    return -1;
> > +}
> > +
> > static inline int kvmppc_get_htab_fd(bool write)
> > {
> >     return -1;
> > -- 
> > 1.8.3.2
> > 

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Andreas Färber]

Am 27.08.2013 02:43, schrieb Benjamin Herrenschmidt:
> Am I dreaming ? Those patches were written about a year ago and you guys
> are still nitpicking on names ? They should have been merged a LONG time
> ago ...
[snip]

Ben, this patch was first posted to qemu-devel June 4th, this year.

If patches don't get commented on and/or applied within a reasonable
time, contributors are requested to ping them. But don't blame Alex if
you forget to post the patches you've written upstream. ;)

Also, QEMU is definitely not the only project that has higher acceptance
criteria than patch-works-for-the-patch-author. :)

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 v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Benjamin Herrenschmidt]

On Tue, 2013-08-27 at 03:48 +0200, Andreas Färber wrote:
> Also, QEMU is definitely not the only project that has higher
> acceptance
> criteria than patch-works-for-the-patch-author. :)

There's a difference between high acceptance criteria and systematic
bike shed painting including in some case request to turn reasonably
meaningful identifiers into something that nobody would get :-)

Ben.

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexander Graf]

On 27.08.2013, at 06:10, Benjamin Herrenschmidt wrote:

> On Tue, 2013-08-27 at 03:48 +0200, Andreas Färber wrote:
>> Also, QEMU is definitely not the only project that has higher
>> acceptance
>> criteria than patch-works-for-the-patch-author. :)
> 
> There's a difference between high acceptance criteria and systematic
> bike shed painting including in some case request to turn reasonably
> meaningful identifiers into something that nobody would get :-)

The name doesn't tell at all what the function is doing. This is even true for the kernel ioctl, but that one is in now, so it's there to stay. Heck, I even had to look up the API documentation to know whether this sets an RTAS to be handled in-kernel or in-user-space.

Just come up with a good name and all is well. Or are you enjoying to complain on every patch review I do now?


Alex

Re: [Qemu-devel] [PATCH v3 6/8] xics-kvm: Support for in-kernel XICS interrupt controller

[Alexander Graf]

On 19.08.2013, at 07:55, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Alexander Graf <agraf@suse.de>

Very nice!


Alex

Re: [Qemu-devel] [PATCH v3] xics: Add H_IPOLL implementation

[Alexander Graf]

On 23.08.2013, at 14:02, Alexey Kardashevskiy wrote:

> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This adds support for the H_IPOLL hypercall which the guest
> uses to poll for a pending interrupt. This hypercall is
> mandatory for PAPR+ and there is no way for the guest to
> detect whether it is supported or not so just add it.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Acked-by: Alexander Graf <agraf@suse.de>

Alex

Re: [Qemu-devel] [PATCH v3] xics: Implement H_XIRR_X

[Alexander Graf]

On 23.08.2013, at 14:03, Alexey Kardashevskiy wrote:

> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This implements H_XIRR_X hypercall in addition to H_XIRR as
> it is mandatory for PAPR+ and there is no way for the guest to
> detect whether it is supported or not so just add it.
> 
> As the Partition Adjunct Option is not supported at the moment,
> the CPPR parameter of the hypercall is ignored.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/intc/xics.c         | 14 ++++++++++++++
> include/hw/ppc/spapr.h |  3 ++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 2e6111d..b7f849a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -27,6 +27,7 @@
> 
> #include "hw/hw.h"
> #include "trace.h"
> +#include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> #include "hw/ppc/xics.h"
> #include "qemu/error-report.h"
> @@ -679,6 +680,18 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>     return H_SUCCESS;
> }
> 
> +static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                             target_ulong opcode, target_ulong *args)
> +{
> +    CPUState *cs = CPU(cpu);
> +    ICPState *ss = spapr->icp->ss + cs->cpu_index;

Let's be consistent in coding style and use &...ss[cs->cpu_index];.

The rest looks good :).


Alex

> +    uint32_t xirr = icp_accept(ss);
> +
> +    args[0] = xirr;
> +    args[1] = cpu_get_real_ticks();
> +    return H_SUCCESS;
> +}
> +
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                           target_ulong opcode, target_ulong *args)
> {
> @@ -853,6 +866,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
>     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_XIRR_X, h_xirr_x);
>     spapr_register_hypercall(H_EOI, h_eoi);
>     spapr_register_hypercall(H_IPOLL, h_ipoll);
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9fc1972..a84b8ff 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -267,7 +267,8 @@ typedef struct sPAPREnvironment {
> #define H_GET_EM_PARMS          0x2B8
> #define H_SET_MPP               0x2D0
> #define H_GET_MPP               0x2D4
> -#define MAX_HCALL_OPCODE        H_GET_MPP
> +#define H_XIRR_X                0x2FC
> +#define MAX_HCALL_OPCODE        H_XIRR_X
> 
> /* The hcalls above are standardized in PAPR and implemented by pHyp
>  * as well.
> -- 
> 1.8.4.rc4
> 

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexey Kardashevskiy]

On 08/27/2013 06:36 PM, Alexander Graf wrote:
> 
> On 27.08.2013, at 06:10, Benjamin Herrenschmidt wrote:
> 
>> On Tue, 2013-08-27 at 03:48 +0200, Andreas Färber wrote:
>>> Also, QEMU is definitely not the only project that has higher
>>> acceptance
>>> criteria than patch-works-for-the-patch-author. :)
>>
>> There's a difference between high acceptance criteria and systematic
>> bike shed painting including in some case request to turn reasonably
>> meaningful identifiers into something that nobody would get :-)
> 
> The name doesn't tell at all what the function is doing. This is even true for the kernel ioctl, but that one is in now, so it's there to stay. Heck, I even had to look up the API documentation to know whether this sets an RTAS to be handled in-kernel or in-user-space.
> 
> Just come up with a good name and all is well. Or are you enjoying to complain on every patch review I do now?


So - kvmppc_define_rtas_in_kernel() or kvmppc_define_rtas_kernel_token()?

I would actually though that the very first "k" is for "Kernel" already but...


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Alexey Kardashevskiy]

On 08/23/2013 09:38 PM, Andreas Färber wrote:
> Am 23.08.2013 05:39, schrieb Alexey Kardashevskiy:
>> On 08/19/2013 11:54 PM, Andreas Färber wrote:
>>> Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
>>>> The upcoming support of in-kernel XICS will redefine migration callbacks
>>>> for both ICS and ICP so classes and callback pointers are added.
>>>>
>>>> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
>>>> will do it different) and xics_dispatch_cpu_setup(). This also moves
>>>> the place where xics_dispatch_cpu_setup() is called a bit further to
>>>> have VCPU initialized (required for XICS-KVM).
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v3:
>>>> * fixed local variables names
>>>> ---
>>>>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>  hw/ppc/spapr.c        |  4 ++--
>>>>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 104 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>>> index 6b3c071..e3a957d 100644
>>>> --- a/hw/intc/xics.c
>>>> +++ b/hw/intc/xics.c
>>> [...]
>>>> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>>>>  static void xics_class_init(ObjectClass *oc, void *data)
>>>>  {
>>>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>>> +    XICSStateClass *k = XICS_CLASS(oc);
>>>>  
>>>>      dc->realize = xics_realize;
>>>>      dc->props = xics_properties;
>>>>      dc->reset = xics_reset;
>>>> +    k->cpu_setup = xics_cpu_setup;
>>>>  
>>>>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>>>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);
>>>
>>> This hunk is fixed up in 4/8, please squash that bit here.
>>
>> Thanks for the review, fixed this.
>>
>>
>>> Otherwise looks good.
>>
>> What exactly looks good? This patch only?
> 
> Yes. And another one has by Rb already I believe. I hope Alex can start
> cherry-picking some more patches when he gets back to NUE.

> I know you had some open questions I still need to reply to. Among
> others, yes, I concur with mst that ppc/spapr_pci.c could be moved to
> pci-host/spapr.c, but remember to update MAINTAINERS for you guys to
> still get CC'ed on it then.
> 
> I had asked Anthony to reply to the KVM XICS patch
> because he said that

Do we really expect Anthony to reply? He seems to disappear from this
conversation and Alex already Rb'ed that patch.

> accessing the parent's method seemed wrong to him and the parent
> implement should be put in charge of calling rather than the derived.
> I.e., I will be dropping my proposed OBJECT_GET_PARENT_CLASS() macro and
> started redoing several series. Haven't looked into how exactly your
> code (CPU setup I think?) may need to change.


xics_dispatch_cpu_setup() should call XICS common's cpu_setup which must
call then KVM-XICS's cpu_setup? I do not follow you here, sorry.



> 
> Andreas
> 
>> The whole series? If it is the
>> whole series, can I put Reviewed-By: Andreas into them before repost them
>> all? I am asking because Alex Graf won't even look at them before I get
>> them reviewed/acked/signed by some one less ignorant than me :)
>> Thanks!
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 2/8] xics: add pre_save/post_load/cpu_setup dispatchers

[Andreas Färber]

Am 29.08.2013 10:25, schrieb Alexey Kardashevskiy:
> On 08/23/2013 09:38 PM, Andreas Färber wrote:
>> Am 23.08.2013 05:39, schrieb Alexey Kardashevskiy:
>>> On 08/19/2013 11:54 PM, Andreas Färber wrote:
>>>> Am 19.08.2013 07:55, schrieb Alexey Kardashevskiy:
>>>>> The upcoming support of in-kernel XICS will redefine migration callbacks
>>>>> for both ICS and ICP so classes and callback pointers are added.
>>>>>
>>>>> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
>>>>> will do it different) and xics_dispatch_cpu_setup(). This also moves
>>>>> the place where xics_dispatch_cpu_setup() is called a bit further to
>>>>> have VCPU initialized (required for XICS-KVM).
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>> Changes:
>>>>> v3:
>>>>> * fixed local variables names
>>>>> ---
>>>>>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>  hw/ppc/spapr.c        |  4 ++--
>>>>>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>>>>>  3 files changed, 104 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>>>> index 6b3c071..e3a957d 100644
>>>>> --- a/hw/intc/xics.c
>>>>> +++ b/hw/intc/xics.c
>>>> [...]
>>>>> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>>>>>  static void xics_class_init(ObjectClass *oc, void *data)
>>>>>  {
>>>>>      DeviceClass *dc = DEVICE_CLASS(oc);
>>>>> +    XICSStateClass *k = XICS_CLASS(oc);
>>>>>  
>>>>>      dc->realize = xics_realize;
>>>>>      dc->props = xics_properties;
>>>>>      dc->reset = xics_reset;
>>>>> +    k->cpu_setup = xics_cpu_setup;
>>>>>  
>>>>>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>>>>>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);
>>>>
>>>> This hunk is fixed up in 4/8, please squash that bit here.
>>>
>>> Thanks for the review, fixed this.
>>>
>>>
>>>> Otherwise looks good.
>>>
>>> What exactly looks good? This patch only?
>>
>> Yes. And another one has by Rb already I believe. I hope Alex can start
>> cherry-picking some more patches when he gets back to NUE.
> 
>> I know you had some open questions I still need to reply to. Among
>> others, yes, I concur with mst that ppc/spapr_pci.c could be moved to
>> pci-host/spapr.c, but remember to update MAINTAINERS for you guys to
>> still get CC'ed on it then.
>>
>> I had asked Anthony to reply to the KVM XICS patch
>> because he said that
> 
> Do we really expect Anthony to reply? He seems to disappear from this
> conversation and Alex already Rb'ed that patch.
> 
>> accessing the parent's method seemed wrong to him and the parent
>> implement should be put in charge of calling rather than the derived.
>> I.e., I will be dropping my proposed OBJECT_GET_PARENT_CLASS() macro and
>> started redoing several series. Haven't looked into how exactly your
>> code (CPU setup I think?) may need to change.
> 
> 
> xics_dispatch_cpu_setup() should call XICS common's cpu_setup which must
> call then KVM-XICS's cpu_setup? I do not follow you here, sorry.

I assume Anthony meant to have xics_dispatch_cpu_setup() or whatever to
contain the common logic and call cpu_setup hook *only* for the
emulation/KVM specific logic. But that's what he was/is supposed to
explain himself. :) He is CC'ed and if he doesn't reply, ping him.

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 v3 6/8] xics-kvm: Support for in-kernel XICS interrupt controller

[Andreas Färber]

Am 19.08.2013 07:55, 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.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * ics_kvm_realize() now is a realize callback rather than initfn callback
> * asserts replaced with Error**
> * KVM_ICS is created now in KVM_XICS's initfn rather than in the nr_irqs
> property setter
> * added KVM_XICS_GET_PARENT_CLASS() to get the common XICS class - needed
> for xics_kvm_cpu_setup() to call parent's cpu_setup()
> * fixed some indentations, removed some \n from error_report()
> ---
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/intc/Makefile.objs             |   1 +
>  hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr.c                    |  23 +-
>  include/hw/ppc/xics.h             |  13 +
>  5 files changed, 528 insertions(+), 2 deletions(-)
>  create mode 100644 hw/intc/xics_kvm.c
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 7831c2b..116f4ca 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -47,6 +47,7 @@ CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> +CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 2851eed..47ac442 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
>  obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
>  obj-$(CONFIG_SH4) += sh_intc.o
>  obj-$(CONFIG_XICS) += xics.o
> +obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..4ed4f33
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
> @@ -0,0 +1,492 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
> + *
> + * Copyright (c) 2013 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "trace.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +
> +#include <sys/ioctl.h>
> +
> +typedef struct KVMXICSState {
> +    XICSState parent_obj;
> +
> +    uint32_t set_xive_token;
> +    uint32_t get_xive_token;
> +    uint32_t int_off_token;
> +    uint32_t int_on_token;
> +    int kernel_xics_fd;
> +} KVMXICSState;
> +
> +/*
> + * ICP-KVM
> + */
> +static void icp_get_kvm_state(ICPState *ss)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return;
> +    }
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        error_report("Unable to retrieve KVM interrupt controller state"
> +                " for CPU %d: %s", 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;
> +}
> +
> +static int icp_set_kvm_state(ICPState *ss, int version_id)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return 0;
> +    }
> +
> +    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) {
> +        error_report("Unable to restore KVM interrupt controller state (0x%"
> +                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static void icp_kvm_reset(DeviceState *dev)
> +{
> +    ICPState *icp = ICP(dev);
> +
> +    icp->xirr = 0;
> +    icp->pending_priority = 0xff;
> +    icp->mfrr = 0xff;
> +
> +    /* Make all outputs are deasserted */
> +    qemu_set_irq(icp->output, 0);
> +
> +    icp_set_kvm_state(icp, 1);
> +}
> +
> +static void icp_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICPStateClass *icpc = ICP_CLASS(klass);
> +
> +    dc->reset = icp_kvm_reset;
> +    icpc->pre_save = icp_get_kvm_state;
> +    icpc->post_load = icp_set_kvm_state;
> +}
> +
> +static const TypeInfo icp_kvm_info = {
> +    .name = TYPE_KVM_ICP,
> +    .parent = TYPE_ICP,
> +    .instance_size = sizeof(ICPState),
> +    .class_init = icp_kvm_class_init,
> +    .class_size = sizeof(ICPStateClass),
> +};
> +
> +/*
> + * ICS-KVM
> + */
> +static void ics_get_kvm_state(ICSState *ics)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to retrieve KVM interrupt controller state"
> +                    " for IRQ %d: %s", 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;
> +        /*
> +         * To be consistent with the software emulation in xics.c, we
> +         * split out the masked state + priority that we get from the
> +         * kernel into 'current priority' (0xff if masked) and
> +         * 'saved priority' (if masked, this is the priority the
> +         * interrupt had before it was masked).  Masking and unmasking
> +         * are done with the ibm,int-off and ibm,int-on RTAS calls.
> +         */
> +        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 {
> +                /*
> +                 * A pending edge-triggered interrupt (or MSI)
> +                 * must have been rejected previously when we
> +                 * first detected it and tried to deliver it,
> +                 * so mark it as pending and previously rejected
> +                 * for consistency with how xics.c works.
> +                 */
> +                irq->status |= XICS_STATUS_MASKED_PENDING
> +                    | XICS_STATUS_REJECTED;
> +            }
> +        }
> +    }
> +}
> +
> +static int ics_set_kvm_state(ICSState *ics, int version_id)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *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(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to restore KVM interrupt controller state"
> +                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +{
> +    ICSState *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 void ics_kvm_reset(DeviceState *dev)
> +{
> +    ics_set_kvm_state(ICS(dev), 1);
> +}
> +
> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    ICSState *ics = ICS(dev);
> +
> +    if (!ics->nr_irqs) {
> +        error_setg(errp, "Number of interrupts needs to be greater 0");
> +        return;
> +    }
> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> +    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *icsc = ICS_CLASS(klass);
> +
> +    dc->realize = ics_kvm_realize;
> +    dc->reset = ics_kvm_reset;
> +    icsc->pre_save = ics_get_kvm_state;
> +    icsc->post_load = ics_set_kvm_state;
> +}
> +
> +static const TypeInfo ics_kvm_info = {
> +    .name = TYPE_KVM_ICS,
> +    .parent = TYPE_ICS,
> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs;
> +    ICPState *ss;
> +    KVMXICSState *icpkvm = KVM_XICS(icp);
> +    ObjectClass *poc = KVM_XICS_GET_PARENT_CLASS(OBJECT(icp));
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(poc);
> +
> +    cs = CPU(cpu);
> +    ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +    if (icpkvm->kernel_xics_fd == -1) {
> +        abort();
> +    }
> +
> +    if (icpkvm->kernel_xics_fd != -1) {
> +        int ret;
> +        struct kvm_enable_cap xics_enable_cap = {
> +            .cap = KVM_CAP_IRQ_XICS,
> +            .flags = 0,
> +            .args = {icpkvm->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) {
> +            error_report("Unable to connect CPU%d to kernel XICS: %s",
> +                    cs->cpu_index, strerror(errno));
> +            exit(1);
> +        }
> +    }
> +
> +    /* Call emulated XICS implementation for consistency */
> +    assert(xsc && xsc->cpu_setup);
> +    xsc->cpu_setup(icp, cpu);
> +}
> +
> +static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
> +{
> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
> +}
> +
> +static void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers,
> +                                    Error **errp)
> +{
> +    int i;
> +
> +    icp->nr_servers = nr_servers;
> +
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        char buffer[32];
> +        object_initialize(&icp->ss[i], TYPE_KVM_ICP);
> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]),
> +                                  errp);
> +    }
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                       uint32_t token,
> +                       uint32_t nargs, target_ulong args,
> +                       uint32_t nret, target_ulong rets)
> +{
> +    error_report("pseries: %s must never be called for in-kernel XICS",
> +            __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(dev);
> +    XICSState *icp = XICS_COMMON(dev);
> +    int i, rc;
> +    Error *error = NULL;
> +    struct kvm_create_device xics_create_device = {
> +        .type = KVM_DEV_TYPE_XICS,
> +        .flags = 0,
> +    };
> +
> +    if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> +        error_setg(errp,
> +                   "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> +    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> +    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> +    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> +    if (rc < 0) {
> +        error_setg(errp, "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) {
> +        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        goto fail;
> +    }
> +
> +    assert(icp->nr_servers);
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            goto fail;
> +        }
> +    }
> +    return;
> +
> +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");
> +}
> +
> +static void xics_kvm_initfn(Object *obj)
> +{
> +    XICSState *xics = XICS_COMMON(obj);
> +
> +    xics->ics = ICS(object_new(TYPE_KVM_ICS));
> +    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> +    xics->ics->icp = xics;
> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
> +
> +    dc->realize = xics_kvm_realize;
> +    xsc->cpu_setup = xics_kvm_cpu_setup;
> +    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
> +    xsc->set_nr_servers = xics_kvm_set_nr_servers;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> +    .name          = TYPE_KVM_XICS,
> +    .parent        = TYPE_XICS_COMMON,
> +    .instance_size = sizeof(KVMXICSState),
> +    .class_init    = xics_kvm_class_init,
> +    .instance_init = xics_kvm_initfn,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> +    type_register_static(&xics_kvm_info);
> +    type_register_static(&ics_kvm_info);
> +    type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..8dc8565 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -141,14 +141,33 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>          return NULL;
>      }
>  
> -    return XICS(dev);
> +    return XICS_COMMON(dev);
>  }
>  
>  static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>  {
>      XICSState *icp = NULL;
>  
> -    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    if (kvm_enabled()) {
> +        QemuOpts *machine_opts = qemu_get_machine_opts();
> +        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> +                                                "kernel_irqchip", true);
> +        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> +                                                  "kernel_irqchip", false);
> +        if (irqchip_allowed) {
> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
> +        }
> +
> +        if (irqchip_required && !icp) {
> +            perror("Failed to create in-kernel XICS\n");
> +            abort();
> +        }
> +    }
> +
> +    if (!icp) {
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    }
> +
>      if (!icp) {
>          perror("Failed to create XICS\n");
>          abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 540bb81..be3c0a1 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -35,6 +35,9 @@
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define TYPE_KVM_XICS "xics-kvm"
> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
> +
>  #define XICS_COMMON_CLASS(klass) \
>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>  #define XICS_CLASS(klass) \
> @@ -44,6 +47,9 @@
>  #define XICS_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>  
> +#define KVM_XICS_GET_PARENT_CLASS(obj) \
> +    object_class_get_parent(object_class_by_name(TYPE_KVM_XICS))

Anthony stated on IRC he didn't want this, but so far hasn't replied
despite me asking him to explain what exactly he wants instead here. :/

Also Alexey, please fix the indentation of the line after error_report()
to be aligned after "(". Thought I pointed that out on the previous
version...

Andreas

> +
>  #define XICS_IPI        0x2
>  #define XICS_BUID       0x1
>  #define XICS_IRQ_BASE   (XICS_BUID << 12)
> @@ -82,6 +88,9 @@ struct XICSState {
>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> +#define TYPE_KVM_ICP "icp-kvm"
> +#define KVM_ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_KVM_ICP)
> +
>  #define ICP_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
>  #define ICP_GET_CLASS(obj) \
> @@ -98,6 +107,7 @@ struct ICPState {
>      /*< private >*/
>      DeviceState parent_obj;
>      /*< public >*/
> +    CPUState *cs;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -107,6 +117,9 @@ struct ICPState {
>  #define TYPE_ICS "ics"
>  #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>  
> +#define TYPE_KVM_ICS "icskvm"
> +#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
> +
>  #define ICS_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>  #define ICS_GET_CLASS(obj) \
> 


-- 
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 v3 6/8] xics-kvm: Support for in-kernel XICS interrupt controller

[Alexey Kardashevskiy]

On 08/29/2013 06:37 PM, Andreas Färber wrote:
> Am 19.08.2013 07:55, 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.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v3:
>> * ics_kvm_realize() now is a realize callback rather than initfn callback
>> * asserts replaced with Error**
>> * KVM_ICS is created now in KVM_XICS's initfn rather than in the nr_irqs
>> property setter
>> * added KVM_XICS_GET_PARENT_CLASS() to get the common XICS class - needed
>> for xics_kvm_cpu_setup() to call parent's cpu_setup()
>> * fixed some indentations, removed some \n from error_report()
>> ---
>>  default-configs/ppc64-softmmu.mak |   1 +
>>  hw/intc/Makefile.objs             |   1 +
>>  hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
>>  hw/ppc/spapr.c                    |  23 +-
>>  include/hw/ppc/xics.h             |  13 +
>>  5 files changed, 528 insertions(+), 2 deletions(-)
>>  create mode 100644 hw/intc/xics_kvm.c
>>
>> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
>> index 7831c2b..116f4ca 100644
>> --- a/default-configs/ppc64-softmmu.mak
>> +++ b/default-configs/ppc64-softmmu.mak
>> @@ -47,6 +47,7 @@ CONFIG_E500=y
>>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>>  # For pSeries
>>  CONFIG_XICS=$(CONFIG_PSERIES)
>> +CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>>  # For PReP
>>  CONFIG_I82378=y
>>  CONFIG_I8259=y
>> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
>> index 2851eed..47ac442 100644
>> --- a/hw/intc/Makefile.objs
>> +++ b/hw/intc/Makefile.objs
>> @@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
>>  obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
>>  obj-$(CONFIG_SH4) += sh_intc.o
>>  obj-$(CONFIG_XICS) += xics.o
>> +obj-$(CONFIG_XICS_KVM) += xics_kvm.o
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> new file mode 100644
>> index 0000000..4ed4f33
>> --- /dev/null
>> +++ b/hw/intc/xics_kvm.c
>> @@ -0,0 +1,492 @@
>> +/*
>> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
>> + *
>> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
>> + *
>> + * Copyright (c) 2013 David Gibson, IBM Corporation.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "hw/hw.h"
>> +#include "trace.h"
>> +#include "hw/ppc/spapr.h"
>> +#include "hw/ppc/xics.h"
>> +#include "kvm_ppc.h"
>> +#include "qemu/config-file.h"
>> +#include "qemu/error-report.h"
>> +
>> +#include <sys/ioctl.h>
>> +
>> +typedef struct KVMXICSState {
>> +    XICSState parent_obj;
>> +
>> +    uint32_t set_xive_token;
>> +    uint32_t get_xive_token;
>> +    uint32_t int_off_token;
>> +    uint32_t int_on_token;
>> +    int kernel_xics_fd;
>> +} KVMXICSState;
>> +
>> +/*
>> + * ICP-KVM
>> + */
>> +static void icp_get_kvm_state(ICPState *ss)
>> +{
>> +    uint64_t state;
>> +    struct kvm_one_reg reg = {
>> +        .id = KVM_REG_PPC_ICP_STATE,
>> +        .addr = (uintptr_t)&state,
>> +    };
>> +    int ret;
>> +
>> +    /* ICP for this CPU thread is not in use, exiting */
>> +    if (!ss->cs) {
>> +        return;
>> +    }
>> +
>> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
>> +    if (ret != 0) {
>> +        error_report("Unable to retrieve KVM interrupt controller state"
>> +                " for CPU %d: %s", 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;
>> +}
>> +
>> +static int icp_set_kvm_state(ICPState *ss, int version_id)
>> +{
>> +    uint64_t state;
>> +    struct kvm_one_reg reg = {
>> +        .id = KVM_REG_PPC_ICP_STATE,
>> +        .addr = (uintptr_t)&state,
>> +    };
>> +    int ret;
>> +
>> +    /* ICP for this CPU thread is not in use, exiting */
>> +    if (!ss->cs) {
>> +        return 0;
>> +    }
>> +
>> +    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) {
>> +        error_report("Unable to restore KVM interrupt controller state (0x%"
>> +                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
>> +                strerror(errno));
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void icp_kvm_reset(DeviceState *dev)
>> +{
>> +    ICPState *icp = ICP(dev);
>> +
>> +    icp->xirr = 0;
>> +    icp->pending_priority = 0xff;
>> +    icp->mfrr = 0xff;
>> +
>> +    /* Make all outputs are deasserted */
>> +    qemu_set_irq(icp->output, 0);
>> +
>> +    icp_set_kvm_state(icp, 1);
>> +}
>> +
>> +static void icp_kvm_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ICPStateClass *icpc = ICP_CLASS(klass);
>> +
>> +    dc->reset = icp_kvm_reset;
>> +    icpc->pre_save = icp_get_kvm_state;
>> +    icpc->post_load = icp_set_kvm_state;
>> +}
>> +
>> +static const TypeInfo icp_kvm_info = {
>> +    .name = TYPE_KVM_ICP,
>> +    .parent = TYPE_ICP,
>> +    .instance_size = sizeof(ICPState),
>> +    .class_init = icp_kvm_class_init,
>> +    .class_size = sizeof(ICPStateClass),
>> +};
>> +
>> +/*
>> + * ICS-KVM
>> + */
>> +static void ics_get_kvm_state(ICSState *ics)
>> +{
>> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
>> +    uint64_t state;
>> +    struct kvm_device_attr attr = {
>> +        .flags = 0,
>> +        .group = KVM_DEV_XICS_GRP_SOURCES,
>> +        .addr = (uint64_t)(uintptr_t)&state,
>> +    };
>> +    int i;
>> +
>> +    for (i = 0; i < ics->nr_irqs; i++) {
>> +        ICSIRQState *irq = &ics->irqs[i];
>> +        int ret;
>> +
>> +        attr.attr = i + ics->offset;
>> +
>> +        ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
>> +        if (ret != 0) {
>> +            error_report("Unable to retrieve KVM interrupt controller state"
>> +                    " for IRQ %d: %s", 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;
>> +        /*
>> +         * To be consistent with the software emulation in xics.c, we
>> +         * split out the masked state + priority that we get from the
>> +         * kernel into 'current priority' (0xff if masked) and
>> +         * 'saved priority' (if masked, this is the priority the
>> +         * interrupt had before it was masked).  Masking and unmasking
>> +         * are done with the ibm,int-off and ibm,int-on RTAS calls.
>> +         */
>> +        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 {
>> +                /*
>> +                 * A pending edge-triggered interrupt (or MSI)
>> +                 * must have been rejected previously when we
>> +                 * first detected it and tried to deliver it,
>> +                 * so mark it as pending and previously rejected
>> +                 * for consistency with how xics.c works.
>> +                 */
>> +                irq->status |= XICS_STATUS_MASKED_PENDING
>> +                    | XICS_STATUS_REJECTED;
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +static int ics_set_kvm_state(ICSState *ics, int version_id)
>> +{
>> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
>> +    uint64_t state;
>> +    struct kvm_device_attr attr = {
>> +        .flags = 0,
>> +        .group = KVM_DEV_XICS_GRP_SOURCES,
>> +        .addr = (uint64_t)(uintptr_t)&state,
>> +    };
>> +    int i;
>> +
>> +    for (i = 0; i < ics->nr_irqs; i++) {
>> +        ICSIRQState *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(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
>> +        if (ret != 0) {
>> +            error_report("Unable to restore KVM interrupt controller state"
>> +                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void ics_kvm_set_irq(void *opaque, int srcno, int val)
>> +{
>> +    ICSState *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 void ics_kvm_reset(DeviceState *dev)
>> +{
>> +    ics_set_kvm_state(ICS(dev), 1);
>> +}
>> +
>> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
>> +{
>> +    ICSState *ics = ICS(dev);
>> +
>> +    if (!ics->nr_irqs) {
>> +        error_setg(errp, "Number of interrupts needs to be greater 0");
>> +        return;
>> +    }
>> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> +    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>> +    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>> +}
>> +
>> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    ICSStateClass *icsc = ICS_CLASS(klass);
>> +
>> +    dc->realize = ics_kvm_realize;
>> +    dc->reset = ics_kvm_reset;
>> +    icsc->pre_save = ics_get_kvm_state;
>> +    icsc->post_load = ics_set_kvm_state;
>> +}
>> +
>> +static const TypeInfo ics_kvm_info = {
>> +    .name = TYPE_KVM_ICS,
>> +    .parent = TYPE_ICS,
>> +    .instance_size = sizeof(ICSState),
>> +    .class_init = ics_kvm_class_init,
>> +};
>> +
>> +/*
>> + * XICS-KVM
>> + */
>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>> +{
>> +    CPUState *cs;
>> +    ICPState *ss;
>> +    KVMXICSState *icpkvm = KVM_XICS(icp);
>> +    ObjectClass *poc = KVM_XICS_GET_PARENT_CLASS(OBJECT(icp));
>> +    XICSStateClass *xsc = XICS_COMMON_CLASS(poc);
>> +
>> +    cs = CPU(cpu);
>> +    ss = &icp->ss[cs->cpu_index];
>> +
>> +    assert(cs->cpu_index < icp->nr_servers);
>> +    if (icpkvm->kernel_xics_fd == -1) {
>> +        abort();
>> +    }
>> +
>> +    if (icpkvm->kernel_xics_fd != -1) {
>> +        int ret;
>> +        struct kvm_enable_cap xics_enable_cap = {
>> +            .cap = KVM_CAP_IRQ_XICS,
>> +            .flags = 0,
>> +            .args = {icpkvm->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) {
>> +            error_report("Unable to connect CPU%d to kernel XICS: %s",
>> +                    cs->cpu_index, strerror(errno));
>> +            exit(1);
>> +        }
>> +    }
>> +
>> +    /* Call emulated XICS implementation for consistency */
>> +    assert(xsc && xsc->cpu_setup);
>> +    xsc->cpu_setup(icp, cpu);
>> +}
>> +
>> +static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
>> +{
>> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
>> +}
>> +
>> +static void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers,
>> +                                    Error **errp)
>> +{
>> +    int i;
>> +
>> +    icp->nr_servers = nr_servers;
>> +
>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>> +    for (i = 0; i < icp->nr_servers; i++) {
>> +        char buffer[32];
>> +        object_initialize(&icp->ss[i], TYPE_KVM_ICP);
>> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]),
>> +                                  errp);
>> +    }
>> +}
>> +
>> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +                       uint32_t token,
>> +                       uint32_t nargs, target_ulong args,
>> +                       uint32_t nret, target_ulong rets)
>> +{
>> +    error_report("pseries: %s must never be called for in-kernel XICS",
>> +            __func__);
>> +}
>> +
>> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
>> +{
>> +    KVMXICSState *icpkvm = KVM_XICS(dev);
>> +    XICSState *icp = XICS_COMMON(dev);
>> +    int i, rc;
>> +    Error *error = NULL;
>> +    struct kvm_create_device xics_create_device = {
>> +        .type = KVM_DEV_TYPE_XICS,
>> +        .flags = 0,
>> +    };
>> +
>> +    if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
>> +        error_setg(errp,
>> +                   "KVM and IRQ_XICS capability must be present for in-kernel XICS");
>> +        goto fail;
>> +    }
>> +
>> +    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
>> +    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
>> +    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
>> +    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
>> +
>> +    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
>> +    if (rc < 0) {
>> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
>> +        goto fail;
>> +    }
>> +
>> +    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
>> +    if (rc < 0) {
>> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
>> +        goto fail;
>> +    }
>> +
>> +    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
>> +    if (rc < 0) {
>> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
>> +        goto fail;
>> +    }
>> +
>> +    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
>> +    if (rc < 0) {
>> +        error_setg(errp, "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) {
>> +        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>> +        goto fail;
>> +    }
>> +
>> +    icpkvm->kernel_xics_fd = xics_create_device.fd;
>> +
>> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
>> +    if (error) {
>> +        error_propagate(errp, error);
>> +        goto fail;
>> +    }
>> +
>> +    assert(icp->nr_servers);
>> +    for (i = 0; i < icp->nr_servers; i++) {
>> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
>> +        if (error) {
>> +            error_propagate(errp, error);
>> +            goto fail;
>> +        }
>> +    }
>> +    return;
>> +
>> +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");
>> +}
>> +
>> +static void xics_kvm_initfn(Object *obj)
>> +{
>> +    XICSState *xics = XICS_COMMON(obj);
>> +
>> +    xics->ics = ICS(object_new(TYPE_KVM_ICS));
>> +    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
>> +    xics->ics->icp = xics;
>> +}
>> +
>> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
>> +
>> +    dc->realize = xics_kvm_realize;
>> +    xsc->cpu_setup = xics_kvm_cpu_setup;
>> +    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
>> +    xsc->set_nr_servers = xics_kvm_set_nr_servers;
>> +}
>> +
>> +static const TypeInfo xics_kvm_info = {
>> +    .name          = TYPE_KVM_XICS,
>> +    .parent        = TYPE_XICS_COMMON,
>> +    .instance_size = sizeof(KVMXICSState),
>> +    .class_init    = xics_kvm_class_init,
>> +    .instance_init = xics_kvm_initfn,
>> +};
>> +
>> +static void xics_kvm_register_types(void)
>> +{
>> +    type_register_static(&xics_kvm_info);
>> +    type_register_static(&ics_kvm_info);
>> +    type_register_static(&icp_kvm_info);
>> +}
>> +
>> +type_init(xics_kvm_register_types)
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 432f0d2..8dc8565 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -141,14 +141,33 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>>          return NULL;
>>      }
>>  
>> -    return XICS(dev);
>> +    return XICS_COMMON(dev);
>>  }
>>  
>>  static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>>  {
>>      XICSState *icp = NULL;
>>  
>> -    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
>> +    if (kvm_enabled()) {
>> +        QemuOpts *machine_opts = qemu_get_machine_opts();
>> +        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
>> +                                                "kernel_irqchip", true);
>> +        bool irqchip_required = qemu_opt_get_bool(machine_opts,
>> +                                                  "kernel_irqchip", false);
>> +        if (irqchip_allowed) {
>> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
>> +        }
>> +
>> +        if (irqchip_required && !icp) {
>> +            perror("Failed to create in-kernel XICS\n");
>> +            abort();
>> +        }
>> +    }
>> +
>> +    if (!icp) {
>> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
>> +    }
>> +
>>      if (!icp) {
>>          perror("Failed to create XICS\n");
>>          abort();
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 540bb81..be3c0a1 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -35,6 +35,9 @@
>>  #define TYPE_XICS "xics"
>>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>>  
>> +#define TYPE_KVM_XICS "xics-kvm"
>> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
>> +
>>  #define XICS_COMMON_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>>  #define XICS_CLASS(klass) \
>> @@ -44,6 +47,9 @@
>>  #define XICS_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>>  
>> +#define KVM_XICS_GET_PARENT_CLASS(obj) \
>> +    object_class_get_parent(object_class_by_name(TYPE_KVM_XICS))
> 
> Anthony stated on IRC he didn't want this, but so far hasn't replied
> despite me asking him to explain what exactly he wants instead here. :/


Anthony, could you please comment on the issue? This is the real stopper of
the series I guess. Thank you.


> Also Alexey, please fix the indentation of the line after error_report()
> to be aligned after "(". Thought I pointed that out on the previous
> version...
> 
> Andreas
> 
>> +
>>  #define XICS_IPI        0x2
>>  #define XICS_BUID       0x1
>>  #define XICS_IRQ_BASE   (XICS_BUID << 12)
>> @@ -82,6 +88,9 @@ struct XICSState {
>>  #define TYPE_ICP "icp"
>>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>>  
>> +#define TYPE_KVM_ICP "icp-kvm"
>> +#define KVM_ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_KVM_ICP)
>> +
>>  #define ICP_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
>>  #define ICP_GET_CLASS(obj) \
>> @@ -98,6 +107,7 @@ struct ICPState {
>>      /*< private >*/
>>      DeviceState parent_obj;
>>      /*< public >*/
>> +    CPUState *cs;
>>      uint32_t xirr;
>>      uint8_t pending_priority;
>>      uint8_t mfrr;
>> @@ -107,6 +117,9 @@ struct ICPState {
>>  #define TYPE_ICS "ics"
>>  #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>>  
>> +#define TYPE_KVM_ICS "icskvm"
>> +#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
>> +
>>  #define ICS_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>>  #define ICS_GET_CLASS(obj) \
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 6/8] xics-kvm: Support for in-kernel XICS interrupt controller

[Anthony Liguori]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> 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.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * ics_kvm_realize() now is a realize callback rather than initfn callback
> * asserts replaced with Error**
> * KVM_ICS is created now in KVM_XICS's initfn rather than in the nr_irqs
> property setter
> * added KVM_XICS_GET_PARENT_CLASS() to get the common XICS class - needed
> for xics_kvm_cpu_setup() to call parent's cpu_setup()
> * fixed some indentations, removed some \n from error_report()
> ---
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/intc/Makefile.objs             |   1 +
>  hw/intc/xics_kvm.c                | 492 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr.c                    |  23 +-
>  include/hw/ppc/xics.h             |  13 +
>  5 files changed, 528 insertions(+), 2 deletions(-)
>  create mode 100644 hw/intc/xics_kvm.c
>
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 7831c2b..116f4ca 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -47,6 +47,7 @@ CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> +CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 2851eed..47ac442 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
>  obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
>  obj-$(CONFIG_SH4) += sh_intc.o
>  obj-$(CONFIG_XICS) += xics.o
> +obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..4ed4f33
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
> @@ -0,0 +1,492 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
> + *
> + * Copyright (c) 2013 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "trace.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +
> +#include <sys/ioctl.h>
> +
> +typedef struct KVMXICSState {
> +    XICSState parent_obj;
> +
> +    uint32_t set_xive_token;
> +    uint32_t get_xive_token;
> +    uint32_t int_off_token;
> +    uint32_t int_on_token;
> +    int kernel_xics_fd;
> +} KVMXICSState;
> +
> +/*
> + * ICP-KVM
> + */
> +static void icp_get_kvm_state(ICPState *ss)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return;
> +    }
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        error_report("Unable to retrieve KVM interrupt controller state"
> +                " for CPU %d: %s", 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;
> +}
> +
> +static int icp_set_kvm_state(ICPState *ss, int version_id)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return 0;
> +    }
> +
> +    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) {
> +        error_report("Unable to restore KVM interrupt controller state (0x%"
> +                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static void icp_kvm_reset(DeviceState *dev)
> +{
> +    ICPState *icp = ICP(dev);
> +
> +    icp->xirr = 0;
> +    icp->pending_priority = 0xff;
> +    icp->mfrr = 0xff;
> +
> +    /* Make all outputs are deasserted */
> +    qemu_set_irq(icp->output, 0);
> +
> +    icp_set_kvm_state(icp, 1);
> +}
> +
> +static void icp_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICPStateClass *icpc = ICP_CLASS(klass);
> +
> +    dc->reset = icp_kvm_reset;
> +    icpc->pre_save = icp_get_kvm_state;
> +    icpc->post_load = icp_set_kvm_state;
> +}
> +
> +static const TypeInfo icp_kvm_info = {
> +    .name = TYPE_KVM_ICP,
> +    .parent = TYPE_ICP,
> +    .instance_size = sizeof(ICPState),
> +    .class_init = icp_kvm_class_init,
> +    .class_size = sizeof(ICPStateClass),
> +};
> +
> +/*
> + * ICS-KVM
> + */
> +static void ics_get_kvm_state(ICSState *ics)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to retrieve KVM interrupt controller state"
> +                    " for IRQ %d: %s", 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;
> +        /*
> +         * To be consistent with the software emulation in xics.c, we
> +         * split out the masked state + priority that we get from the
> +         * kernel into 'current priority' (0xff if masked) and
> +         * 'saved priority' (if masked, this is the priority the
> +         * interrupt had before it was masked).  Masking and unmasking
> +         * are done with the ibm,int-off and ibm,int-on RTAS calls.
> +         */
> +        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 {
> +                /*
> +                 * A pending edge-triggered interrupt (or MSI)
> +                 * must have been rejected previously when we
> +                 * first detected it and tried to deliver it,
> +                 * so mark it as pending and previously rejected
> +                 * for consistency with how xics.c works.
> +                 */
> +                irq->status |= XICS_STATUS_MASKED_PENDING
> +                    | XICS_STATUS_REJECTED;
> +            }
> +        }
> +    }
> +}
> +
> +static int ics_set_kvm_state(ICSState *ics, int version_id)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *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(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to restore KVM interrupt controller state"
> +                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +{
> +    ICSState *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 void ics_kvm_reset(DeviceState *dev)
> +{
> +    ics_set_kvm_state(ICS(dev), 1);
> +}
> +
> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    ICSState *ics = ICS(dev);
> +
> +    if (!ics->nr_irqs) {
> +        error_setg(errp, "Number of interrupts needs to be greater 0");
> +        return;
> +    }
> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> +    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *icsc = ICS_CLASS(klass);
> +
> +    dc->realize = ics_kvm_realize;
> +    dc->reset = ics_kvm_reset;
> +    icsc->pre_save = ics_get_kvm_state;
> +    icsc->post_load = ics_set_kvm_state;
> +}
> +
> +static const TypeInfo ics_kvm_info = {
> +    .name = TYPE_KVM_ICS,
> +    .parent = TYPE_ICS,
> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs;
> +    ICPState *ss;
> +    KVMXICSState *icpkvm = KVM_XICS(icp);
> +    ObjectClass *poc = KVM_XICS_GET_PARENT_CLASS(OBJECT(icp));
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(poc);

This is not right.

If a method is truly polymorphic, than you shouldn't have to chain to
what you're overloading.

If you want to create a specific point for a subclass to implement a
behavior, you should introduce another method with an empty
implementation that the child can overload.

Regards,

Anthony Liguori

> +
> +    cs = CPU(cpu);
> +    ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +    if (icpkvm->kernel_xics_fd == -1) {
> +        abort();
> +    }
> +
> +    if (icpkvm->kernel_xics_fd != -1) {
> +        int ret;
> +        struct kvm_enable_cap xics_enable_cap = {
> +            .cap = KVM_CAP_IRQ_XICS,
> +            .flags = 0,
> +            .args = {icpkvm->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) {
> +            error_report("Unable to connect CPU%d to kernel XICS: %s",
> +                    cs->cpu_index, strerror(errno));
> +            exit(1);
> +        }
> +    }
> +
> +    /* Call emulated XICS implementation for consistency */
> +    assert(xsc && xsc->cpu_setup);
> +    xsc->cpu_setup(icp, cpu);
> +}
> +
> +static void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs, Error **errp)
> +{
> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
> +}
> +
> +static void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers,
> +                                    Error **errp)
> +{
> +    int i;
> +
> +    icp->nr_servers = nr_servers;
> +
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        char buffer[32];
> +        object_initialize(&icp->ss[i], TYPE_KVM_ICP);
> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]),
> +                                  errp);
> +    }
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                       uint32_t token,
> +                       uint32_t nargs, target_ulong args,
> +                       uint32_t nret, target_ulong rets)
> +{
> +    error_report("pseries: %s must never be called for in-kernel XICS",
> +            __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(dev);
> +    XICSState *icp = XICS_COMMON(dev);
> +    int i, rc;
> +    Error *error = NULL;
> +    struct kvm_create_device xics_create_device = {
> +        .type = KVM_DEV_TYPE_XICS,
> +        .flags = 0,
> +    };
> +
> +    if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> +        error_setg(errp,
> +                   "KVM and IRQ_XICS capability must be present for in-kernel XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> +    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> +    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> +    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> +    if (rc < 0) {
> +        error_setg(errp, "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) {
> +        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        goto fail;
> +    }
> +
> +    assert(icp->nr_servers);
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            goto fail;
> +        }
> +    }
> +    return;
> +
> +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");
> +}
> +
> +static void xics_kvm_initfn(Object *obj)
> +{
> +    XICSState *xics = XICS_COMMON(obj);
> +
> +    xics->ics = ICS(object_new(TYPE_KVM_ICS));
> +    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> +    xics->ics->icp = xics;
> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
> +
> +    dc->realize = xics_kvm_realize;
> +    xsc->cpu_setup = xics_kvm_cpu_setup;
> +    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
> +    xsc->set_nr_servers = xics_kvm_set_nr_servers;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> +    .name          = TYPE_KVM_XICS,
> +    .parent        = TYPE_XICS_COMMON,
> +    .instance_size = sizeof(KVMXICSState),
> +    .class_init    = xics_kvm_class_init,
> +    .instance_init = xics_kvm_initfn,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> +    type_register_static(&xics_kvm_info);
> +    type_register_static(&ics_kvm_info);
> +    type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..8dc8565 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -141,14 +141,33 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>          return NULL;
>      }
>  
> -    return XICS(dev);
> +    return XICS_COMMON(dev);
>  }
>  
>  static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>  {
>      XICSState *icp = NULL;
>  
> -    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    if (kvm_enabled()) {
> +        QemuOpts *machine_opts = qemu_get_machine_opts();
> +        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> +                                                "kernel_irqchip", true);
> +        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> +                                                  "kernel_irqchip", false);
> +        if (irqchip_allowed) {
> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
> +        }
> +
> +        if (irqchip_required && !icp) {
> +            perror("Failed to create in-kernel XICS\n");
> +            abort();
> +        }
> +    }
> +
> +    if (!icp) {
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    }
> +
>      if (!icp) {
>          perror("Failed to create XICS\n");
>          abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 540bb81..be3c0a1 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -35,6 +35,9 @@
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define TYPE_KVM_XICS "xics-kvm"
> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
> +
>  #define XICS_COMMON_CLASS(klass) \
>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>  #define XICS_CLASS(klass) \
> @@ -44,6 +47,9 @@
>  #define XICS_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>  
> +#define KVM_XICS_GET_PARENT_CLASS(obj) \
> +    object_class_get_parent(object_class_by_name(TYPE_KVM_XICS))
> +
>  #define XICS_IPI        0x2
>  #define XICS_BUID       0x1
>  #define XICS_IRQ_BASE   (XICS_BUID << 12)
> @@ -82,6 +88,9 @@ struct XICSState {
>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> +#define TYPE_KVM_ICP "icp-kvm"
> +#define KVM_ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_KVM_ICP)
> +
>  #define ICP_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
>  #define ICP_GET_CLASS(obj) \
> @@ -98,6 +107,7 @@ struct ICPState {
>      /*< private >*/
>      DeviceState parent_obj;
>      /*< public >*/
> +    CPUState *cs;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -107,6 +117,9 @@ struct ICPState {
>  #define TYPE_ICS "ics"
>  #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>  
> +#define TYPE_KVM_ICS "icskvm"
> +#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
> +
>  #define ICS_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>  #define ICS_GET_CLASS(obj) \
> -- 
> 1.8.3.2

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexander Graf]

On 29.08.2013, at 10:18, Alexey Kardashevskiy wrote:

> On 08/27/2013 06:36 PM, Alexander Graf wrote:
>> 
>> On 27.08.2013, at 06:10, Benjamin Herrenschmidt wrote:
>> 
>>> On Tue, 2013-08-27 at 03:48 +0200, Andreas Färber wrote:
>>>> Also, QEMU is definitely not the only project that has higher
>>>> acceptance
>>>> criteria than patch-works-for-the-patch-author. :)
>>> 
>>> There's a difference between high acceptance criteria and systematic
>>> bike shed painting including in some case request to turn reasonably
>>> meaningful identifiers into something that nobody would get :-)
>> 
>> The name doesn't tell at all what the function is doing. This is even true for the kernel ioctl, but that one is in now, so it's there to stay. Heck, I even had to look up the API documentation to know whether this sets an RTAS to be handled in-kernel or in-user-space.
>> 
>> Just come up with a good name and all is well. Or are you enjoying to complain on every patch review I do now?
> 
> 
> So - kvmppc_define_rtas_in_kernel() or kvmppc_define_rtas_kernel_token()?

The former probably.

> I would actually though that the very first "k" is for "Kernel" already but...

I a symmetric API you could also remove rtas handling from the kernel, so you could tell the kernel "handle this in kernel space" and "don't handle this in kernel space". Our naming needs to be precise enough that you can guess what is going on from it.


Alex

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Laszlo Ersek]

Hello Benjamin,

On 08/27/13 02:43, Benjamin Herrenschmidt wrote:

> Am I dreaming ? Those patches were written about a year ago and you guys
> are still nitpicking on names ? They should have been merged a LONG time
> ago ...
> 
> I'm seriously wondering how people get anything done with qemu KVM when
> it takes about a year of bike shed painting to get the most basic
> functionality merged.

(apologies for answering a rhetorical (non-)question)

"people" don't get anything done here. A handful of maintainers are
responsible for 80% of the changes (who can easily get in patches,
frequently without review, based on personal connections & trust between
each other), and the remaining 20% is done by hundreds of lowly droids,
including myself, who are put through the bikeshedding that you so aptly
describe (reposts up to v10 are commonplace).

http://thread.gmane.org/gmane.comp.emulators.qemu/228260
http://pastebin.com/Q85q2MUj

This two-caste process is celebrated as the pinnacle of quality assurance.

http://wiki.qemu.org/Contribute/SubmitAPatch

> Alex, you know, a maintainer's job is not to bounce a patch over and
> over again until it looks EXACTLY what you would have written in the
> first place. If that was the case you may as well write it yourself.

Now why are these thoughts so familiar... Oh wait:

http://thread.gmane.org/gmane.comp.emulators.qemu/230604/focus=230654

Many powerful people on the list are firmly and unwaveringly enamored of
their own voices.

> In this specific case, it's even more annoying because that comment
> could have been done ages ago, and because the name you propose is even
> more horrible than what was there... If you are really keen about that
> pick up at least something that doesn't suck such as
> 
> 	kvmppc_define_rtas_kernel_token()
> 
> What you propose is even more confusing as to what the function is for.
> 
> At the end of the day, I despair though. Really.

qemu-devel is possibly more in-bred and more elitist than lkml.

If you don't have the networking capital, $DEITY help you with your
patches. (Well illustrated by several recent series of a submitter with
spotless track record, but maybe not with the necessary networking
capital, being ignored for months.)

Note that with this email I'm not trying to elevate myself to your level
-- I'm just a nameless droid as I said, but I can't help notice the
overlap between your points and mine.

qemu maintainers: when a Linux arch maintainer calls your process
"systematic bikeshed painting", maybe you should try to get over
yourselves once in a while.

Laszlo, butthurt

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Benjamin Herrenschmidt]

On Fri, 2013-08-30 at 16:34 +0200, Alexander Graf wrote:

> > So - kvmppc_define_rtas_in_kernel() or kvmppc_define_rtas_kernel_token()?
> 
> The former probably.

I told him the latter :-) Honestly, your name is gross :-) Mine
describes exactly what the calls does.

> > I would actually though that the very first "k" is for "Kernel" already but...
> 
> I a symmetric API you could also remove rtas handling from the kernel, so you could tell the kernel "handle this in kernel space" and "don't handle this in kernel space". Our naming needs to be precise enough that you can guess what is going on from it.

But that's not quite what the API is doing and we don't want a symetric
API it's pointless.

Yet another "my CS teacher told me a good API is symetric" (oh and
that's a nice word "symetric", it sounds *good*, using it makes it
look like we are plenty smart !) but in the end totally pointless
for the problem at hand.

Ben.

Re: [Qemu-devel] [PATCH v3 1/8] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN

[Alexander Graf]

On 31.08.2013, at 00:19, Benjamin Herrenschmidt wrote:

> On Fri, 2013-08-30 at 16:34 +0200, Alexander Graf wrote:
> 
>>> So - kvmppc_define_rtas_in_kernel() or kvmppc_define_rtas_kernel_token()?
>> 
>> The former probably.
> 
> I told him the latter :-) Honestly, your name is gross :-) Mine
> describes exactly what the calls does.

I really don't care which one, as long as it describes what it does. Both names fit that description, just that the former is smaller :). Really, pick any, as long as a random reader has the chance to know what's going on.

> 
>>> I would actually though that the very first "k" is for "Kernel" already but...
>> 
>> I a symmetric API you could also remove rtas handling from the kernel, so you could tell the kernel "handle this in kernel space" and "don't handle this in kernel space". Our naming needs to be precise enough that you can guess what is going on from it.
> 
> But that's not quite what the API is doing and we don't want a symetric
> API it's pointless.
> 
> Yet another "my CS teacher told me a good API is symetric" (oh and
> that's a nice word "symetric", it sounds *good*, using it makes it
> look like we are plenty smart !) but in the end totally pointless
> for the problem at hand.

Imagine you don't know the KVM API in this case, but simply read the kvm code. You don't know whether it's symmetric or not. It's not, and I don't think it's a big issue that it isn't. But the reader doesn't know at this point.

So now you need to figure out what's going on. Either the function name tells you (no time loss), you know where to find the KVM API documentation and look it up there (small time loss) or you check the actual code (even more time loss).

Overall, I'm fairly sure we wasted more time arguing on this one by now than anyone would ever waste on looking up anything on this function.


Alex