[Qemu-devel] [PATCH 00/10] xics: Get rid of KVM specific classes

[Qemu-devel] [PATCH 00/10] xics: Get rid of KVM specific classes

[Greg Kurz]

As recently pointed [1], using different object types for the KVM and
non-KVM scenarios was a bad idea: "In general different device types
should represent guest-visibly different objects, not just implementation
differences."

This series converts the base ICP class and the "simple" ICS class to
support KVM. This is done through the use of kvm_irqchip_in_kernel(),
which allows the compiler to optimize the KVM branches out when CONFIG_KVM
isn't set.

The removal of the KVM specific object types greatly simplifies the code.

Please comment.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg03580.html

--
Greg

---

Greg Kurz (10):
      xics: Explicitely call KVM ICP methods from the common code
      xics: Handle KVM ICP reset from the common code
      xics: Handle KVM ICP realize from the common code
      spapr/irq: Use the base ICP class for KVM
      xics: Drop the KVM ICP class
      xics: Explicitely call KVM ICS methods from the common code
      xics: Handle KVM ICS reset from the "simple" ICS code
      xics: Handle KVM interrupt presentation from "simple" ICS code
      spapr/irq: Use the "simple" ICS class for KVM
      xics: Drop the KVM ICS class


 hw/intc/xics.c         |   76 +++++++++++++++++-------------
 hw/intc/xics_kvm.c     |  120 +++---------------------------------------------
 hw/ppc/spapr_irq.c     |   28 ++++-------
 include/hw/ppc/spapr.h |    1 
 include/hw/ppc/xics.h  |   26 ++++------
 5 files changed, 71 insertions(+), 180 deletions(-)

[Qemu-devel] [PATCH 01/10] xics: Explicitely call KVM ICP methods from the common code

[Greg Kurz]

The pre_save(), post_load() and synchronize_state() methods of the
ICPStateClass type are really KVM only things. Make that obvious
by dropping the indirections and directly calling the KVM functions
instead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |   24 +++++++++++-------------
 hw/intc/xics_kvm.c    |   12 ++++--------
 include/hw/ppc/xics.h |    9 +++++----
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 16e8ffa2aaf7..988b53abd17d 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -37,18 +37,18 @@
 #include "qapi/visitor.h"
 #include "monitor/monitor.h"
 #include "hw/intc/intc.h"
+#include "sysemu/kvm.h"
 
 void icp_pic_print_info(ICPState *icp, Monitor *mon)
 {
-    ICPStateClass *icpc = ICP_GET_CLASS(icp);
     int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
 
     if (!icp->output) {
         return;
     }
 
-    if (icpc->synchronize_state) {
-        icpc->synchronize_state(icp);
+    if (kvm_irqchip_in_kernel()) {
+        icp_synchronize_state(icp);
     }
 
     monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n",
@@ -252,25 +252,23 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
     }
 }
 
-static int icp_dispatch_pre_save(void *opaque)
+static int icp_pre_save(void *opaque)
 {
     ICPState *icp = opaque;
-    ICPStateClass *info = ICP_GET_CLASS(icp);
 
-    if (info->pre_save) {
-        info->pre_save(icp);
+    if (kvm_irqchip_in_kernel()) {
+        icp_get_kvm_state(icp);
     }
 
     return 0;
 }
 
-static int icp_dispatch_post_load(void *opaque, int version_id)
+static int icp_post_load(void *opaque, int version_id)
 {
     ICPState *icp = opaque;
-    ICPStateClass *info = ICP_GET_CLASS(icp);
 
-    if (info->post_load) {
-        return info->post_load(icp, version_id);
+    if (kvm_irqchip_in_kernel()) {
+        return icp_set_kvm_state(icp);
     }
 
     return 0;
@@ -280,8 +278,8 @@ static const VMStateDescription vmstate_icp_server = {
     .name = "icp/server",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_save = icp_dispatch_pre_save,
-    .post_load = icp_dispatch_post_load,
+    .pre_save = icp_pre_save,
+    .post_load = icp_post_load,
     .fields = (VMStateField[]) {
         /* Sanity check */
         VMSTATE_UINT32(xirr, ICPState),
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index dff13300504c..7efa99b8b434 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -54,7 +54,7 @@ static QLIST_HEAD(, KVMEnabledICP)
 /*
  * ICP-KVM
  */
-static void icp_get_kvm_state(ICPState *icp)
+void icp_get_kvm_state(ICPState *icp)
 {
     uint64_t state;
     int ret;
@@ -83,14 +83,14 @@ static void do_icp_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
     icp_get_kvm_state(arg.host_ptr);
 }
 
-static void icp_synchronize_state(ICPState *icp)
+void icp_synchronize_state(ICPState *icp)
 {
     if (icp->cs) {
         run_on_cpu(icp->cs, do_icp_synchronize_state, RUN_ON_CPU_HOST_PTR(icp));
     }
 }
 
-static int icp_set_kvm_state(ICPState *icp, int version_id)
+int icp_set_kvm_state(ICPState *icp)
 {
     uint64_t state;
     int ret;
@@ -121,7 +121,7 @@ static void icp_kvm_reset(DeviceState *dev)
 
     icpc->parent_reset(dev);
 
-    icp_set_kvm_state(ICP(dev), 1);
+    icp_set_kvm_state(ICP(dev));
 }
 
 static void icp_kvm_realize(DeviceState *dev, Error **errp)
@@ -178,10 +178,6 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
                                     &icpc->parent_realize);
     device_class_set_parent_reset(dc, icp_kvm_reset,
                                   &icpc->parent_reset);
-
-    icpc->pre_save = icp_get_kvm_state;
-    icpc->post_load = icp_set_kvm_state;
-    icpc->synchronize_state = icp_synchronize_state;
 }
 
 static const TypeInfo icp_kvm_info = {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index fad786e8b22d..3236ccec924c 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -66,10 +66,6 @@ struct ICPStateClass {
 
     DeviceRealize parent_realize;
     DeviceReset parent_reset;
-
-    void (*pre_save)(ICPState *icp);
-    int (*post_load)(ICPState *icp, int version_id);
-    void (*synchronize_state)(ICPState *icp);
 };
 
 struct ICPState {
@@ -203,4 +199,9 @@ void icp_resend(ICPState *ss);
 Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
                    Error **errp);
 
+/* KVM */
+void icp_get_kvm_state(ICPState *icp);
+int icp_set_kvm_state(ICPState *icp);
+void icp_synchronize_state(ICPState *icp);
+
 #endif /* XICS_H */

[Qemu-devel] [PATCH 02/10] xics: Handle KVM ICP reset from the common code

[Greg Kurz]

The KVM ICP reset handler simply writes the ICP state to KVM. This
doesn't need the overkill parent_reset logic we have today. Call
icp_set_kvm_state() from the base ICP reset function instead.

Since there are no other users for ICPStateClass::parent_reset, and
it isn't currently expected to change, drop it as well.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |   12 ++++--------
 hw/intc/xics_kvm.c    |   11 -----------
 include/hw/ppc/xics.h |    1 -
 3 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 988b53abd17d..822d367e6388 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -289,7 +289,7 @@ static const VMStateDescription vmstate_icp_server = {
     },
 };
 
-static void icp_reset(DeviceState *dev)
+static void icp_reset_handler(void *dev)
 {
     ICPState *icp = ICP(dev);
 
@@ -299,13 +299,10 @@ static void icp_reset(DeviceState *dev)
 
     /* Make all outputs are deasserted */
     qemu_set_irq(icp->output, 0);
-}
 
-static void icp_reset_handler(void *dev)
-{
-    DeviceClass *dc = DEVICE_GET_CLASS(dev);
-
-    dc->reset(dev);
+    if (kvm_irqchip_in_kernel()) {
+        icp_set_kvm_state(ICP(dev));
+    }
 }
 
 static void icp_realize(DeviceState *dev, Error **errp)
@@ -370,7 +367,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
 
     dc->realize = icp_realize;
     dc->unrealize = icp_unrealize;
-    dc->reset = icp_reset;
 }
 
 static const TypeInfo icp_info = {
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 7efa99b8b434..80321e9b75ab 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -115,15 +115,6 @@ int icp_set_kvm_state(ICPState *icp)
     return 0;
 }
 
-static void icp_kvm_reset(DeviceState *dev)
-{
-    ICPStateClass *icpc = ICP_GET_CLASS(dev);
-
-    icpc->parent_reset(dev);
-
-    icp_set_kvm_state(ICP(dev));
-}
-
 static void icp_kvm_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
@@ -176,8 +167,6 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
 
     device_class_set_parent_realize(dc, icp_kvm_realize,
                                     &icpc->parent_realize);
-    device_class_set_parent_reset(dc, icp_kvm_reset,
-                                  &icpc->parent_reset);
 }
 
 static const TypeInfo icp_kvm_info = {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 3236ccec924c..e33282a576d0 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -65,7 +65,6 @@ struct ICPStateClass {
     DeviceClass parent_class;
 
     DeviceRealize parent_realize;
-    DeviceReset parent_reset;
 };
 
 struct ICPState {

[Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Greg Kurz]

The realization of KVM ICP currently follows the parent_realize logic,
which is a bit overkill here. Also we want to get rid of the KVM ICP
class. Explicitely call icp_kvm_realize() from the base ICP realize
function.

Note that ICPStateClass::parent_realize is retained because powernv
needs it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |    8 ++++++++
 hw/intc/xics_kvm.c    |   10 +---------
 include/hw/ppc/xics.h |    1 +
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 822d367e6388..acd63ab5e0b9 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (kvm_irqchip_in_kernel()) {
+        icp_kvm_realize(dev, &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
+    }
+
     qemu_register_reset(icp_reset_handler, dev);
     vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 80321e9b75ab..4eebced516b6 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
     return 0;
 }
 
-static void icp_kvm_realize(DeviceState *dev, Error **errp)
+void icp_kvm_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
-    ICPStateClass *icpc = ICP_GET_CLASS(icp);
-    Error *local_err = NULL;
     CPUState *cs;
     KVMEnabledICP *enabled_icp;
     unsigned long vcpu_id;
@@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
         abort();
     }
 
-    icpc->parent_realize(dev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
     cs = icp->cs;
     vcpu_id = kvm_arch_vcpu_id(cs);
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index e33282a576d0..ab61dc24010a 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
 void icp_get_kvm_state(ICPState *icp);
 int icp_set_kvm_state(ICPState *icp);
 void icp_synchronize_state(ICPState *icp);
+void icp_kvm_realize(DeviceState *dev, Error **errp);
 
 #endif /* XICS_H */

[Qemu-devel] [PATCH 04/10] spapr/irq: Use the base ICP class for KVM

[Greg Kurz]

The base ICP class knows how to interact with KVM. Adapt sPAPR to use it
instead of the ICP KVM class.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_irq.c     |    4 +---
 include/hw/ppc/spapr.h |    1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 48d6b2daed6e..e6893df61e76 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -102,7 +102,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
     if (kvm_enabled()) {
         if (machine_kernel_irqchip_allowed(machine) &&
             !xics_kvm_init(spapr, &local_err)) {
-            spapr->icp_type = TYPE_KVM_ICP;
             spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
                                           &local_err);
         }
@@ -117,7 +116,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
 
     if (!spapr->ics) {
         xics_spapr_init(spapr);
-        spapr->icp_type = TYPE_ICP;
         spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
                                       &local_err);
     }
@@ -199,7 +197,7 @@ static void spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
     Object *obj;
     sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
 
-    obj = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
+    obj = icp_create(OBJECT(cpu), TYPE_ICP, XICS_FABRIC(spapr),
                      &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index cbd276ed2b6a..631fc5103b7b 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -178,7 +178,6 @@ struct sPAPRMachineState {
     /*< public >*/
     char *kvm_type;
 
-    const char *icp_type;
     int32_t irq_map_nr;
     unsigned long *irq_map;
     sPAPRXive  *xive;

[Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Greg Kurz]

The KVM ICP class isn't used anymore. Drop it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_kvm.c    |   18 ------------------
 include/hw/ppc/xics.h |    3 ---
 2 files changed, 21 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 4eebced516b6..fae4ac431f2f 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -152,23 +152,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
     QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
 }
 
-static void icp_kvm_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    ICPStateClass *icpc = ICP_CLASS(klass);
-
-    device_class_set_parent_realize(dc, icp_kvm_realize,
-                                    &icpc->parent_realize);
-}
-
-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
  */
@@ -425,7 +408,6 @@ fail:
 static void xics_kvm_register_types(void)
 {
     type_register_static(&ics_kvm_info);
-    type_register_static(&icp_kvm_info);
 }
 
 type_init(xics_kvm_register_types)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index ab61dc24010a..fae54e6f28ae 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -50,9 +50,6 @@ typedef struct XICSFabric XICSFabric;
 #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 TYPE_PNV_ICP "pnv-icp"
 #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
 

[Qemu-devel] [PATCH 06/10] xics: Explicitely call KVM ICS methods from the common code

[Greg Kurz]

The pre_save(), post_load() and synchronize_state() methods of the
ICSStateClass type are really KVM only things. Make that obvious
by dropping the indirections and directly calling the KVM functions
instead.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |   23 ++++++++++-------------
 hw/intc/xics_kvm.c    |   12 ++++--------
 include/hw/ppc/xics.h |    7 ++++---
 3 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index acd63ab5e0b9..ae5d5ea135b8 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -58,7 +58,6 @@ void icp_pic_print_info(ICPState *icp, Monitor *mon)
 
 void ics_pic_print_info(ICSState *ics, Monitor *mon)
 {
-    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
     uint32_t i;
 
     monitor_printf(mon, "ICS %4x..%4x %p\n",
@@ -68,8 +67,8 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
         return;
     }
 
-    if (icsc->synchronize_state) {
-        icsc->synchronize_state(ics);
+    if (kvm_irqchip_in_kernel()) {
+        ics_synchronize_state(ics);
     }
 
     for (i = 0; i < ics->nr_irqs; i++) {
@@ -647,25 +646,23 @@ static void ics_base_instance_init(Object *obj)
     ics->offset = XICS_IRQ_BASE;
 }
 
-static int ics_base_dispatch_pre_save(void *opaque)
+static int ics_base_pre_save(void *opaque)
 {
     ICSState *ics = opaque;
-    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
 
-    if (info->pre_save) {
-        info->pre_save(ics);
+    if (kvm_irqchip_in_kernel()) {
+        ics_get_kvm_state(ics);
     }
 
     return 0;
 }
 
-static int ics_base_dispatch_post_load(void *opaque, int version_id)
+static int ics_base_post_load(void *opaque, int version_id)
 {
     ICSState *ics = opaque;
-    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
 
-    if (info->post_load) {
-        return info->post_load(ics, version_id);
+    if (kvm_irqchip_in_kernel()) {
+        return ics_set_kvm_state(ics);
     }
 
     return 0;
@@ -689,8 +686,8 @@ static const VMStateDescription vmstate_ics_base = {
     .name = "ics",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_save = ics_base_dispatch_pre_save,
-    .post_load = ics_base_dispatch_post_load,
+    .pre_save = ics_base_pre_save,
+    .post_load = ics_base_post_load,
     .fields = (VMStateField[]) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index fae4ac431f2f..642351e5790f 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -155,7 +155,7 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
 /*
  * ICS-KVM
  */
-static void ics_get_kvm_state(ICSState *ics)
+void ics_get_kvm_state(ICSState *ics)
 {
     uint64_t state;
     int i;
@@ -208,12 +208,12 @@ static void ics_get_kvm_state(ICSState *ics)
     }
 }
 
-static void ics_synchronize_state(ICSState *ics)
+void ics_synchronize_state(ICSState *ics)
 {
     ics_get_kvm_state(ics);
 }
 
-static int ics_set_kvm_state(ICSState *ics, int version_id)
+int ics_set_kvm_state(ICSState *ics)
 {
     uint64_t state;
     int i;
@@ -286,7 +286,7 @@ static void ics_kvm_reset(DeviceState *dev)
 
     icsc->parent_reset(dev);
 
-    ics_set_kvm_state(ICS_KVM(dev), 1);
+    ics_set_kvm_state(ICS_KVM(dev));
 }
 
 static void ics_kvm_reset_handler(void *dev)
@@ -318,10 +318,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
                                     &icsc->parent_realize);
     device_class_set_parent_reset(dc, ics_kvm_reset,
                                   &icsc->parent_reset);
-
-    icsc->pre_save = ics_get_kvm_state;
-    icsc->post_load = ics_set_kvm_state;
-    icsc->synchronize_state = ics_synchronize_state;
 }
 
 static const TypeInfo ics_kvm_info = {
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index fae54e6f28ae..06e87128f88e 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -109,12 +109,9 @@ struct ICSStateClass {
     DeviceRealize parent_realize;
     DeviceReset parent_reset;
 
-    void (*pre_save)(ICSState *s);
-    int (*post_load)(ICSState *s, int version_id);
     void (*reject)(ICSState *s, uint32_t irq);
     void (*resend)(ICSState *s);
     void (*eoi)(ICSState *s, uint32_t irq);
-    void (*synchronize_state)(ICSState *s);
 };
 
 struct ICSState {
@@ -201,4 +198,8 @@ int icp_set_kvm_state(ICPState *icp);
 void icp_synchronize_state(ICPState *icp);
 void icp_kvm_realize(DeviceState *dev, Error **errp);
 
+void ics_get_kvm_state(ICSState *ics);
+int ics_set_kvm_state(ICSState *ics);
+void ics_synchronize_state(ICSState *ics);
+
 #endif /* XICS_H */

[Qemu-devel] [PATCH 07/10] xics: Handle KVM ICS reset from the "simple" ICS code

[Greg Kurz]

The KVM ICS reset handler simply writes the ICS state to KVM. This
doesn't need the overkill parent_reset logic we have today. Also
we want to use the same ICS type for the KVM and non-KVM case with
pseries.

Call icp_set_kvm_state() from the "simple" ICS reset function.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c     |    4 ++++
 hw/intc/xics_kvm.c |   18 ------------------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index ae5d5ea135b8..49401745c410 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -553,6 +553,10 @@ static void ics_simple_reset(DeviceState *dev)
     ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
 
     icsc->parent_reset(dev);
+
+    if (kvm_irqchip_in_kernel()) {
+        ics_set_kvm_state(ICS_BASE(dev));
+    }
 }
 
 static void ics_simple_reset_handler(void *dev)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 642351e5790f..e7b8d4c29ce6 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -280,20 +280,6 @@ void ics_kvm_set_irq(void *opaque, int srcno, int val)
     }
 }
 
-static void ics_kvm_reset(DeviceState *dev)
-{
-    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
-
-    icsc->parent_reset(dev);
-
-    ics_set_kvm_state(ICS_KVM(dev));
-}
-
-static void ics_kvm_reset_handler(void *dev)
-{
-    ics_kvm_reset(dev);
-}
-
 static void ics_kvm_realize(DeviceState *dev, Error **errp)
 {
     ICSState *ics = ICS_KVM(dev);
@@ -305,8 +291,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
         error_propagate(errp, local_err);
         return;
     }
-
-    qemu_register_reset(ics_kvm_reset_handler, ics);
 }
 
 static void ics_kvm_class_init(ObjectClass *klass, void *data)
@@ -316,8 +300,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
 
     device_class_set_parent_realize(dc, ics_kvm_realize,
                                     &icsc->parent_realize);
-    device_class_set_parent_reset(dc, ics_kvm_reset,
-                                  &icsc->parent_reset);
 }
 
 static const TypeInfo ics_kvm_info = {

[Qemu-devel] [PATCH 08/10] xics: Handle KVM interrupt presentation from "simple" ICS code

[Greg Kurz]

We want to use the "simple" ICS type in both KVM and non-KVM setups.
Teach the "simple" ICS how to present interrupts to KVM and adapt
sPAPR accordingly.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics.c        |    5 +++++
 hw/intc/xics_kvm.c    |    3 +--
 hw/ppc/spapr_irq.c    |    7 +------
 include/hw/ppc/xics.h |    2 +-
 4 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 49401745c410..3009fa747277 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -466,6 +466,11 @@ void ics_simple_set_irq(void *opaque, int srcno, int val)
 {
     ICSState *ics = (ICSState *)opaque;
 
+    if (kvm_irqchip_in_kernel()) {
+        ics_kvm_set_irq(ics, srcno, val);
+        return;
+    }
+
     if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
         ics_simple_set_irq_lsi(ics, srcno, val);
     } else {
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index e7b8d4c29ce6..f34eacda03e7 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -259,9 +259,8 @@ int ics_set_kvm_state(ICSState *ics)
     return 0;
 }
 
-void ics_kvm_set_irq(void *opaque, int srcno, int val)
+void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
 {
-    ICSState *ics = opaque;
     struct kvm_irq_level args;
     int rc;
 
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index e6893df61e76..9f43b7b3bf16 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -222,13 +222,8 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
 static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
 {
     sPAPRMachineState *spapr = opaque;
-    MachineState *machine = MACHINE(opaque);
 
-    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
-        ics_kvm_set_irq(spapr->ics, srcno, val);
-    } else {
-        ics_simple_set_irq(spapr->ics, srcno, val);
-    }
+    ics_simple_set_irq(spapr->ics, srcno, val);
 }
 
 static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 06e87128f88e..61bd0fb9784f 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -180,7 +180,6 @@ void icp_eoi(ICPState *icp, uint32_t xirr);
 void ics_simple_write_xive(ICSState *ics, int nr, int server,
                            uint8_t priority, uint8_t saved_priority);
 void ics_simple_set_irq(void *opaque, int srcno, int val);
-void ics_kvm_set_irq(void *opaque, int srcno, int val);
 
 void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
 void icp_pic_print_info(ICPState *icp, Monitor *mon);
@@ -201,5 +200,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp);
 void ics_get_kvm_state(ICSState *ics);
 int ics_set_kvm_state(ICSState *ics);
 void ics_synchronize_state(ICSState *ics);
+void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
 
 #endif /* XICS_H */

[Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[Greg Kurz]

The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
it instead of the ICS KVM class.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_irq.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 9f43b7b3bf16..4aa8165307c7 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
  */
 
 static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
-                                  const char *type_ics,
                                   int nr_irqs, Error **errp)
 {
     Error *local_err = NULL;
     Object *obj;
 
-    obj = object_new(type_ics);
+    obj = object_new(TYPE_ICS_SIMPLE);
     object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
     object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
                                    &error_abort);
@@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
 {
     MachineState *machine = MACHINE(spapr);
     Error *local_err = NULL;
+    bool xics_kvm = false;
 
     if (kvm_enabled()) {
         if (machine_kernel_irqchip_allowed(machine) &&
             !xics_kvm_init(spapr, &local_err)) {
-            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
-                                          &local_err);
+            xics_kvm = true;
         }
-        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
+        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
             error_prepend(&local_err,
                           "kernel_irqchip requested but unavailable: ");
             goto error;
@@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
         local_err = NULL;
     }
 
-    if (!spapr->ics) {
+    if (!xics_kvm) {
         xics_spapr_init(spapr);
-        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
-                                      &local_err);
     }
 
+    spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err);
+
 error:
     error_propagate(errp, local_err);
 }

[Qemu-devel] [PATCH 10/10] xics: Drop the KVM ICS class

[Greg Kurz]

The KVM ICS class isn't used anymore. Drop it.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/xics_kvm.c    |   40 ----------------------------------------
 hw/ppc/spapr_irq.c    |    2 +-
 include/hw/ppc/xics.h |    3 ---
 3 files changed, 1 insertion(+), 44 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index f34eacda03e7..a00d0a7962e1 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -279,39 +279,6 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
     }
 }
 
-static void ics_kvm_realize(DeviceState *dev, Error **errp)
-{
-    ICSState *ics = ICS_KVM(dev);
-    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
-    Error *local_err = NULL;
-
-    icsc->parent_realize(dev, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-}
-
-static void ics_kvm_class_init(ObjectClass *klass, void *data)
-{
-    ICSStateClass *icsc = ICS_BASE_CLASS(klass);
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    device_class_set_parent_realize(dc, ics_kvm_realize,
-                                    &icsc->parent_realize);
-}
-
-static const TypeInfo ics_kvm_info = {
-    .name = TYPE_ICS_KVM,
-    .parent = TYPE_ICS_BASE,
-    .instance_size = sizeof(ICSState),
-    .class_init = ics_kvm_class_init,
-};
-
-/*
- * XICS-KVM
- */
-
 static void rtas_dummy(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                        uint32_t token,
                        uint32_t nargs, target_ulong args,
@@ -381,10 +348,3 @@ fail:
     kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
     return -1;
 }
-
-static void xics_kvm_register_types(void)
-{
-    type_register_static(&ics_kvm_info);
-}
-
-type_init(xics_kvm_register_types)
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 4aa8165307c7..4297eed600f9 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -208,7 +208,7 @@ static void spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
 
 static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
 {
-    if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
+    if (!kvm_irqchip_in_kernel()) {
         CPUState *cs;
         CPU_FOREACH(cs) {
             PowerPCCPU *cpu = POWERPC_CPU(cs);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 61bd0fb9784f..d36bbe11ee2e 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -95,9 +95,6 @@ struct PnvICPState {
 #define TYPE_ICS_SIMPLE "ics"
 #define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
 
-#define TYPE_ICS_KVM "icskvm"
-#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
-
 #define ICS_BASE_CLASS(klass) \
      OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE)
 #define ICS_BASE_GET_CLASS(obj) \

Re: [Qemu-devel] [PATCH 01/10] xics: Explicitely call KVM ICP methods from the common code

[Cédric Le Goater]

On 2/15/19 12:39 PM, Greg Kurz wrote:
> The pre_save(), post_load() and synchronize_state() methods of the
> ICPStateClass type are really KVM only things. Make that obvious
> by dropping the indirections and directly calling the KVM functions
> instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/xics.c        |   24 +++++++++++-------------
>  hw/intc/xics_kvm.c    |   12 ++++--------
>  include/hw/ppc/xics.h |    9 +++++----
>  3 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 16e8ffa2aaf7..988b53abd17d 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -37,18 +37,18 @@
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
> +#include "sysemu/kvm.h"
>  
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>  
>      if (!icp->output) {
>          return;
>      }
>  
> -    if (icpc->synchronize_state) {
> -        icpc->synchronize_state(icp);
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_synchronize_state(icp);
>      }
>  
>      monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n",
> @@ -252,25 +252,23 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>      }
>  }
>  
> -static int icp_dispatch_pre_save(void *opaque)
> +static int icp_pre_save(void *opaque)
>  {
>      ICPState *icp = opaque;
> -    ICPStateClass *info = ICP_GET_CLASS(icp);
>  
> -    if (info->pre_save) {
> -        info->pre_save(icp);
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_get_kvm_state(icp);
>      }
>  
>      return 0;
>  }
>  
> -static int icp_dispatch_post_load(void *opaque, int version_id)
> +static int icp_post_load(void *opaque, int version_id)
>  {
>      ICPState *icp = opaque;
> -    ICPStateClass *info = ICP_GET_CLASS(icp);
>  
> -    if (info->post_load) {
> -        return info->post_load(icp, version_id);
> +    if (kvm_irqchip_in_kernel()) {
> +        return icp_set_kvm_state(icp);
>      }
>  
>      return 0;
> @@ -280,8 +278,8 @@ static const VMStateDescription vmstate_icp_server = {
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = icp_dispatch_pre_save,
> -    .post_load = icp_dispatch_post_load,
> +    .pre_save = icp_pre_save,
> +    .post_load = icp_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32(xirr, ICPState),
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index dff13300504c..7efa99b8b434 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -54,7 +54,7 @@ static QLIST_HEAD(, KVMEnabledICP)
>  /*
>   * ICP-KVM
>   */
> -static void icp_get_kvm_state(ICPState *icp)
> +void icp_get_kvm_state(ICPState *icp)
>  {
>      uint64_t state;
>      int ret;
> @@ -83,14 +83,14 @@ static void do_icp_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
>      icp_get_kvm_state(arg.host_ptr);
>  }
>  
> -static void icp_synchronize_state(ICPState *icp)
> +void icp_synchronize_state(ICPState *icp)
>  {
>      if (icp->cs) {
>          run_on_cpu(icp->cs, do_icp_synchronize_state, RUN_ON_CPU_HOST_PTR(icp));
>      }
>  }
>  
> -static int icp_set_kvm_state(ICPState *icp, int version_id)
> +int icp_set_kvm_state(ICPState *icp)
>  {
>      uint64_t state;
>      int ret;
> @@ -121,7 +121,7 @@ static void icp_kvm_reset(DeviceState *dev)
>  
>      icpc->parent_reset(dev);
>  
> -    icp_set_kvm_state(ICP(dev), 1);
> +    icp_set_kvm_state(ICP(dev));
>  }
>  
>  static void icp_kvm_realize(DeviceState *dev, Error **errp)
> @@ -178,10 +178,6 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
>                                      &icpc->parent_realize);
>      device_class_set_parent_reset(dc, icp_kvm_reset,
>                                    &icpc->parent_reset);
> -
> -    icpc->pre_save = icp_get_kvm_state;
> -    icpc->post_load = icp_set_kvm_state;
> -    icpc->synchronize_state = icp_synchronize_state;
>  }
>  
>  static const TypeInfo icp_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index fad786e8b22d..3236ccec924c 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -66,10 +66,6 @@ struct ICPStateClass {
>  
>      DeviceRealize parent_realize;
>      DeviceReset parent_reset;
> -
> -    void (*pre_save)(ICPState *icp);
> -    int (*post_load)(ICPState *icp, int version_id);
> -    void (*synchronize_state)(ICPState *icp);
>  };
>  
>  struct ICPState {
> @@ -203,4 +199,9 @@ void icp_resend(ICPState *ss);
>  Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>                     Error **errp);
>  
> +/* KVM */
> +void icp_get_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp);
> +void icp_synchronize_state(ICPState *icp);
> +
>  #endif /* XICS_H */
> 

Re: [Qemu-devel] [PATCH 02/10] xics: Handle KVM ICP reset from the common code

[Cédric Le Goater]

On 2/15/19 12:39 PM, Greg Kurz wrote:
> The KVM ICP reset handler simply writes the ICP state to KVM. This
> doesn't need the overkill parent_reset logic we have today. Call
> icp_set_kvm_state() from the base ICP reset function instead.
> 
> Since there are no other users for ICPStateClass::parent_reset, and
> it isn't currently expected to change, drop it as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/xics.c        |   12 ++++--------
>  hw/intc/xics_kvm.c    |   11 -----------
>  include/hw/ppc/xics.h |    1 -
>  3 files changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 988b53abd17d..822d367e6388 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -289,7 +289,7 @@ static const VMStateDescription vmstate_icp_server = {
>      },
>  };
>  
> -static void icp_reset(DeviceState *dev)
> +static void icp_reset_handler(void *dev)								
>  {
>      ICPState *icp = ICP(dev);
>  
> @@ -299,13 +299,10 @@ static void icp_reset(DeviceState *dev)
>  
>      /* Make all outputs are deasserted */
>      qemu_set_irq(icp->output, 0);
> -}
>  
> -static void icp_reset_handler(void *dev)
> -{
> -    DeviceClass *dc = DEVICE_GET_CLASS(dev);
> -
> -    dc->reset(dev);
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_set_kvm_state(ICP(dev));
> +    }
>  }
>  
>  static void icp_realize(DeviceState *dev, Error **errp)
> @@ -370,7 +367,6 @@ static void icp_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = icp_realize;
>      dc->unrealize = icp_unrealize;
> -    dc->reset = icp_reset;
>  }
>  
>  static const TypeInfo icp_info = {
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 7efa99b8b434..80321e9b75ab 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -115,15 +115,6 @@ int icp_set_kvm_state(ICPState *icp)
>      return 0;
>  }
>  
> -static void icp_kvm_reset(DeviceState *dev)
> -{
> -    ICPStateClass *icpc = ICP_GET_CLASS(dev);
> -
> -    icpc->parent_reset(dev);
> -
> -    icp_set_kvm_state(ICP(dev));
> -}
> -
>  static void icp_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> @@ -176,8 +167,6 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, icp_kvm_realize,
>                                      &icpc->parent_realize);
> -    device_class_set_parent_reset(dc, icp_kvm_reset,
> -                                  &icpc->parent_reset);
>  }
>  
>  static const TypeInfo icp_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 3236ccec924c..e33282a576d0 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -65,7 +65,6 @@ struct ICPStateClass {
>      DeviceClass parent_class;
>  
>      DeviceRealize parent_realize;
> -    DeviceReset parent_reset;
>  };
>  
>  struct ICPState {
> 

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The realization of KVM ICP currently follows the parent_realize logic,
> which is a bit overkill here. Also we want to get rid of the KVM ICP
> class. Explicitely call icp_kvm_realize() from the base ICP realize
> function.
> 
> Note that ICPStateClass::parent_realize is retained because powernv
> needs it.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>>
> ---
>  hw/intc/xics.c        |    8 ++++++++
>  hw/intc/xics_kvm.c    |   10 +---------
>  include/hw/ppc/xics.h |    1 +
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 822d367e6388..acd63ab5e0b9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_kvm_realize(dev, &err);

While we are at changing things, I would prefix all the KVM 
backends routine with kvmppc_*. so that icp_kvm_realize() 
becomes kvmppc_icp_realize()

Apart from that,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +    }
> +
>      qemu_register_reset(icp_reset_handler, dev);
>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 80321e9b75ab..4eebced516b6 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
>      return 0;
>  }
>  
> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> +void icp_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      ICPState *icp = ICP(dev);
> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> -    Error *local_err = NULL;
>      CPUState *cs;
>      KVMEnabledICP *enabled_icp;
>      unsigned long vcpu_id;
> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
>          abort();
>      }
>  
> -    icpc->parent_realize(dev, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
>      cs = icp->cs;
>      vcpu_id = kvm_arch_vcpu_id(cs);
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index e33282a576d0..ab61dc24010a 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>  void icp_get_kvm_state(ICPState *icp);
>  int icp_set_kvm_state(ICPState *icp);
>  void icp_synchronize_state(ICPState *icp);
> +void icp_kvm_realize(DeviceState *dev, Error **errp);
>  
>  #endif /* XICS_H */
> 

Re: [Qemu-devel] [PATCH 04/10] spapr/irq: Use the base ICP class for KVM

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The base ICP class knows how to interact with KVM. Adapt sPAPR to use it
> instead of the ICP KVM class.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

ah. Good one ! 



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/ppc/spapr_irq.c     |    4 +---
>  include/hw/ppc/spapr.h |    1 -
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 48d6b2daed6e..e6893df61e76 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -102,7 +102,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>      if (kvm_enabled()) {
>          if (machine_kernel_irqchip_allowed(machine) &&
>              !xics_kvm_init(spapr, &local_err)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
>              spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
>                                            &local_err);
>          }
> @@ -117,7 +116,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>  
>      if (!spapr->ics) {
>          xics_spapr_init(spapr);
> -        spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
>                                        &local_err);
>      }
> @@ -199,7 +197,7 @@ static void spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
>      Object *obj;
>      sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>  
> -    obj = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> +    obj = icp_create(OBJECT(cpu), TYPE_ICP, XICS_FABRIC(spapr),
>                       &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index cbd276ed2b6a..631fc5103b7b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -178,7 +178,6 @@ struct sPAPRMachineState {
>      /*< public >*/
>      char *kvm_type;
>  
> -    const char *icp_type;
>      int32_t irq_map_nr;
>      unsigned long *irq_map;
>      sPAPRXive  *xive;
> 

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The KVM ICP class isn't used anymore. Drop it.

Isn't migration complaining ?  If not,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.



> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics_kvm.c    |   18 ------------------
>  include/hw/ppc/xics.h |    3 ---
>  2 files changed, 21 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 4eebced516b6..fae4ac431f2f 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -152,23 +152,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
>      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
>  }
>  
> -static void icp_kvm_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    ICPStateClass *icpc = ICP_CLASS(klass);
> -
> -    device_class_set_parent_realize(dc, icp_kvm_realize,
> -                                    &icpc->parent_realize);
> -}
> -
> -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
>   */
> @@ -425,7 +408,6 @@ fail:
>  static void xics_kvm_register_types(void)
>  {
>      type_register_static(&ics_kvm_info);
> -    type_register_static(&icp_kvm_info);
>  }
>  
>  type_init(xics_kvm_register_types)
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index ab61dc24010a..fae54e6f28ae 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -50,9 +50,6 @@ typedef struct XICSFabric XICSFabric;
>  #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 TYPE_PNV_ICP "pnv-icp"
>  #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
>  
> 

Re: [Qemu-devel] [PATCH 06/10] xics: Explicitely call KVM ICS methods from the common code

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The pre_save(), post_load() and synchronize_state() methods of the
> ICSStateClass type are really KVM only things. Make that obvious
> by dropping the indirections and directly calling the KVM functions
> instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

A part from the kvmppc_ prefix,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/intc/xics.c        |   23 ++++++++++-------------
>  hw/intc/xics_kvm.c    |   12 ++++--------
>  include/hw/ppc/xics.h |    7 ++++---
>  3 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index acd63ab5e0b9..ae5d5ea135b8 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -58,7 +58,6 @@ void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  
>  void ics_pic_print_info(ICSState *ics, Monitor *mon)
>  {
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
>      uint32_t i;
>  
>      monitor_printf(mon, "ICS %4x..%4x %p\n",
> @@ -68,8 +67,8 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
>          return;
>      }
>  
> -    if (icsc->synchronize_state) {
> -        icsc->synchronize_state(ics);
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_synchronize_state(ics);
>      }
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
> @@ -647,25 +646,23 @@ static void ics_base_instance_init(Object *obj)
>      ics->offset = XICS_IRQ_BASE;
>  }
>  
> -static int ics_base_dispatch_pre_save(void *opaque)
> +static int ics_base_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
> -    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>  
> -    if (info->pre_save) {
> -        info->pre_save(ics);
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_get_kvm_state(ics);
>      }
>  
>      return 0;
>  }
>  
> -static int ics_base_dispatch_post_load(void *opaque, int version_id)
> +static int ics_base_post_load(void *opaque, int version_id)
>  {
>      ICSState *ics = opaque;
> -    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>  
> -    if (info->post_load) {
> -        return info->post_load(ics, version_id);
> +    if (kvm_irqchip_in_kernel()) {
> +        return ics_set_kvm_state(ics);
>      }
>  
>      return 0;
> @@ -689,8 +686,8 @@ static const VMStateDescription vmstate_ics_base = {
>      .name = "ics",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = ics_base_dispatch_pre_save,
> -    .post_load = ics_base_dispatch_post_load,
> +    .pre_save = ics_base_pre_save,
> +    .post_load = ics_base_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index fae4ac431f2f..642351e5790f 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -155,7 +155,7 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
>  /*
>   * ICS-KVM
>   */
> -static void ics_get_kvm_state(ICSState *ics)
> +void ics_get_kvm_state(ICSState *ics)
>  {
>      uint64_t state;
>      int i;
> @@ -208,12 +208,12 @@ static void ics_get_kvm_state(ICSState *ics)
>      }
>  }
>  
> -static void ics_synchronize_state(ICSState *ics)
> +void ics_synchronize_state(ICSState *ics)
>  {
>      ics_get_kvm_state(ics);
>  }
>  
> -static int ics_set_kvm_state(ICSState *ics, int version_id)
> +int ics_set_kvm_state(ICSState *ics)
>  {
>      uint64_t state;
>      int i;
> @@ -286,7 +286,7 @@ static void ics_kvm_reset(DeviceState *dev)
>  
>      icsc->parent_reset(dev);
>  
> -    ics_set_kvm_state(ICS_KVM(dev), 1);
> +    ics_set_kvm_state(ICS_KVM(dev));
>  }
>  
>  static void ics_kvm_reset_handler(void *dev)
> @@ -318,10 +318,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>                                      &icsc->parent_realize);
>      device_class_set_parent_reset(dc, ics_kvm_reset,
>                                    &icsc->parent_reset);
> -
> -    icsc->pre_save = ics_get_kvm_state;
> -    icsc->post_load = ics_set_kvm_state;
> -    icsc->synchronize_state = ics_synchronize_state;
>  }
>  
>  static const TypeInfo ics_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index fae54e6f28ae..06e87128f88e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -109,12 +109,9 @@ struct ICSStateClass {
>      DeviceRealize parent_realize;
>      DeviceReset parent_reset;
>  
> -    void (*pre_save)(ICSState *s);
> -    int (*post_load)(ICSState *s, int version_id);
>      void (*reject)(ICSState *s, uint32_t irq);
>      void (*resend)(ICSState *s);
>      void (*eoi)(ICSState *s, uint32_t irq);
> -    void (*synchronize_state)(ICSState *s);
>  };
>  
>  struct ICSState {
> @@ -201,4 +198,8 @@ int icp_set_kvm_state(ICPState *icp);
>  void icp_synchronize_state(ICPState *icp);
>  void icp_kvm_realize(DeviceState *dev, Error **errp);
>  
> +void ics_get_kvm_state(ICSState *ics);
> +int ics_set_kvm_state(ICSState *ics);
> +void ics_synchronize_state(ICSState *ics);
> +
>  #endif /* XICS_H */
> 

Re: [Qemu-devel] [PATCH 07/10] xics: Handle KVM ICS reset from the "simple" ICS code

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The KVM ICS reset handler simply writes the ICS state to KVM. This
> doesn't need the overkill parent_reset logic we have today. Also
> we want to use the same ICS type for the KVM and non-KVM case with
> pseries.
> 
> Call icp_set_kvm_state() from the "simple" ICS reset function.

A part from the kvmppc_ prefix,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c     |    4 ++++
>  hw/intc/xics_kvm.c |   18 ------------------
>  2 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ae5d5ea135b8..49401745c410 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -553,6 +553,10 @@ static void ics_simple_reset(DeviceState *dev)
>      ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>  
>      icsc->parent_reset(dev);
> +
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_set_kvm_state(ICS_BASE(dev));
> +    }
>  }
>  
>  static void ics_simple_reset_handler(void *dev)
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 642351e5790f..e7b8d4c29ce6 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -280,20 +280,6 @@ void ics_kvm_set_irq(void *opaque, int srcno, int val)
>      }
>  }
>  
> -static void ics_kvm_reset(DeviceState *dev)
> -{
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> -
> -    icsc->parent_reset(dev);
> -
> -    ics_set_kvm_state(ICS_KVM(dev));
> -}
> -
> -static void ics_kvm_reset_handler(void *dev)
> -{
> -    ics_kvm_reset(dev);
> -}
> -
>  static void ics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_KVM(dev);
> @@ -305,8 +291,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    qemu_register_reset(ics_kvm_reset_handler, ics);
>  }
>  
>  static void ics_kvm_class_init(ObjectClass *klass, void *data)
> @@ -316,8 +300,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, ics_kvm_realize,
>                                      &icsc->parent_realize);
> -    device_class_set_parent_reset(dc, ics_kvm_reset,
> -                                  &icsc->parent_reset);
>  }
>  
>  static const TypeInfo ics_kvm_info = {
> 

Re: [Qemu-devel] [PATCH 08/10] xics: Handle KVM interrupt presentation from "simple" ICS code

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> We want to use the "simple" ICS type in both KVM and non-KVM setups.
> Teach the "simple" ICS how to present interrupts to KVM and adapt
> sPAPR accordingly.

I don't see the benefits of this change.

C.


> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics.c        |    5 +++++
>  hw/intc/xics_kvm.c    |    3 +--
>  hw/ppc/spapr_irq.c    |    7 +------
>  include/hw/ppc/xics.h |    2 +-
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 49401745c410..3009fa747277 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -466,6 +466,11 @@ void ics_simple_set_irq(void *opaque, int srcno, int val)
>  {
>      ICSState *ics = (ICSState *)opaque;
>  
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_kvm_set_irq(ics, srcno, val);
> +        return;
> +    }
> +
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
>          ics_simple_set_irq_lsi(ics, srcno, val);
>      } else {
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index e7b8d4c29ce6..f34eacda03e7 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -259,9 +259,8 @@ int ics_set_kvm_state(ICSState *ics)
>      return 0;
>  }
>  
> -void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>  {
> -    ICSState *ics = opaque;
>      struct kvm_irq_level args;
>      int rc;
>  
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index e6893df61e76..9f43b7b3bf16 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -222,13 +222,8 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
>  static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
>  {
>      sPAPRMachineState *spapr = opaque;
> -    MachineState *machine = MACHINE(opaque);
>  
> -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        ics_kvm_set_irq(spapr->ics, srcno, val);
> -    } else {
> -        ics_simple_set_irq(spapr->ics, srcno, val);
> -    }
> +    ics_simple_set_irq(spapr->ics, srcno, val);
>  }
>  
>  static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 06e87128f88e..61bd0fb9784f 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -180,7 +180,6 @@ void icp_eoi(ICPState *icp, uint32_t xirr);
>  void ics_simple_write_xive(ICSState *ics, int nr, int server,
>                             uint8_t priority, uint8_t saved_priority);
>  void ics_simple_set_irq(void *opaque, int srcno, int val);
> -void ics_kvm_set_irq(void *opaque, int srcno, int val);
>  
>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
>  void icp_pic_print_info(ICPState *icp, Monitor *mon);
> @@ -201,5 +200,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp);
>  void ics_get_kvm_state(ICSState *ics);
>  int ics_set_kvm_state(ICSState *ics);
>  void ics_synchronize_state(ICSState *ics);
> +void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>  
>  #endif /* XICS_H */
> 

Re: [Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
> it instead of the ICS KVM class.

You are changing the type name. What about migration ? 

Can't we move the xics_kvm_init() and xics_spapr_init() call under 
spapr_ics_create() ? It would simplify a lot the routine I think
if these were done before creating the ICSState. 

C.

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_irq.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 9f43b7b3bf16..4aa8165307c7 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>   */
>  
>  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> -                                  const char *type_ics,
>                                    int nr_irqs, Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
>  
> -    obj = object_new(type_ics);
> +    obj = object_new(TYPE_ICS_SIMPLE);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                     &error_abort);
> @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>  {
>      MachineState *machine = MACHINE(spapr);
>      Error *local_err = NULL;
> +    bool xics_kvm = false;
>  
>      if (kvm_enabled()) {
>          if (machine_kernel_irqchip_allowed(machine) &&
>              !xics_kvm_init(spapr, &local_err)) {
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> -                                          &local_err);
> +            xics_kvm = true;
>          }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> +        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
>              error_prepend(&local_err,
>                            "kernel_irqchip requested but unavailable: ");
>              goto error;
> @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>          local_err = NULL;
>      }
>  
> -    if (!spapr->ics) {
> +    if (!xics_kvm) {
>          xics_spapr_init(spapr);
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> -                                      &local_err);
>      }
>  
> +    spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err);
> +
>  error:
>      error_propagate(errp, local_err);
>  }
> 

Re: [Qemu-devel] [PATCH 10/10] xics: Drop the KVM ICS class

[Cédric Le Goater]

On 2/15/19 12:40 PM, Greg Kurz wrote:
> The KVM ICS class isn't used anymore. Drop it.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/xics_kvm.c    |   40 ----------------------------------------
>  hw/ppc/spapr_irq.c    |    2 +-
>  include/hw/ppc/xics.h |    3 ---
>  3 files changed, 1 insertion(+), 44 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index f34eacda03e7..a00d0a7962e1 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -279,39 +279,6 @@ void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>      }
>  }
>  
> -static void ics_kvm_realize(DeviceState *dev, Error **errp)
> -{
> -    ICSState *ics = ICS_KVM(dev);
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
> -    Error *local_err = NULL;
> -
> -    icsc->parent_realize(dev, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -}
> -
> -static void ics_kvm_class_init(ObjectClass *klass, void *data)
> -{
> -    ICSStateClass *icsc = ICS_BASE_CLASS(klass);
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -
> -    device_class_set_parent_realize(dc, ics_kvm_realize,
> -                                    &icsc->parent_realize);
> -}
> -
> -static const TypeInfo ics_kvm_info = {
> -    .name = TYPE_ICS_KVM,
> -    .parent = TYPE_ICS_BASE,
> -    .instance_size = sizeof(ICSState),
> -    .class_init = ics_kvm_class_init,
> -};
> -
> -/*
> - * XICS-KVM
> - */
> -
>  static void rtas_dummy(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                         uint32_t token,
>                         uint32_t nargs, target_ulong args,
> @@ -381,10 +348,3 @@ fail:
>      kvmppc_define_rtas_kernel_token(0, "ibm,int-off");
>      return -1;
>  }
> -
> -static void xics_kvm_register_types(void)
> -{
> -    type_register_static(&ics_kvm_info);
> -}
> -
> -type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 4aa8165307c7..4297eed600f9 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -208,7 +208,7 @@ static void spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
>  
>  static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
>  {
> -    if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> +    if (!kvm_irqchip_in_kernel()) {
>          CPUState *cs;
>          CPU_FOREACH(cs) {
>              PowerPCCPU *cpu = POWERPC_CPU(cs);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 61bd0fb9784f..d36bbe11ee2e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -95,9 +95,6 @@ struct PnvICPState {
>  #define TYPE_ICS_SIMPLE "ics"
>  #define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>  
> -#define TYPE_ICS_KVM "icskvm"
> -#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
> -
>  #define ICS_BASE_CLASS(klass) \
>       OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE)
>  #define ICS_BASE_GET_CLASS(obj) \
> 

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Greg Kurz]

On Fri, 15 Feb 2019 13:54:02 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 12:40 PM, Greg Kurz wrote:
> > The realization of KVM ICP currently follows the parent_realize logic,
> > which is a bit overkill here. Also we want to get rid of the KVM ICP
> > class. Explicitely call icp_kvm_realize() from the base ICP realize
> > function.
> > 
> > Note that ICPStateClass::parent_realize is retained because powernv
> > needs it.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>>
> > ---
> >  hw/intc/xics.c        |    8 ++++++++
> >  hw/intc/xics_kvm.c    |   10 +---------
> >  include/hw/ppc/xics.h |    1 +
> >  3 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 822d367e6388..acd63ab5e0b9 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >  
> > +    if (kvm_irqchip_in_kernel()) {
> > +        icp_kvm_realize(dev, &err);  
> 
> While we are at changing things, I would prefix all the KVM 
> backends routine with kvmppc_*. so that icp_kvm_realize() 
> becomes kvmppc_icp_realize()
> 

Well... kvmppc_* routines have historically been sitting under
target/ppc so I'm not sure we want to use the same prefix
elsewhere...

> Apart from that,
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
> > +        if (err) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> > +    }
> > +
> >      qemu_register_reset(icp_reset_handler, dev);
> >      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> >  }
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 80321e9b75ab..4eebced516b6 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
> >      return 0;
> >  }
> >  
> > -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> > +void icp_kvm_realize(DeviceState *dev, Error **errp)
> >  {
> >      ICPState *icp = ICP(dev);
> > -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > -    Error *local_err = NULL;
> >      CPUState *cs;
> >      KVMEnabledICP *enabled_icp;
> >      unsigned long vcpu_id;
> > @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
> >          abort();
> >      }
> >  
> > -    icpc->parent_realize(dev, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > -
> >      cs = icp->cs;
> >      vcpu_id = kvm_arch_vcpu_id(cs);
> >  
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index e33282a576d0..ab61dc24010a 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> >  void icp_get_kvm_state(ICPState *icp);
> >  int icp_set_kvm_state(ICPState *icp);
> >  void icp_synchronize_state(ICPState *icp);
> > +void icp_kvm_realize(DeviceState *dev, Error **errp);
> >  
> >  #endif /* XICS_H */
> >   
> 

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Cédric Le Goater]

On 2/15/19 2:03 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 13:54:02 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 2/15/19 12:40 PM, Greg Kurz wrote:
>>> The realization of KVM ICP currently follows the parent_realize logic,
>>> which is a bit overkill here. Also we want to get rid of the KVM ICP
>>> class. Explicitely call icp_kvm_realize() from the base ICP realize
>>> function.
>>>
>>> Note that ICPStateClass::parent_realize is retained because powernv
>>> needs it.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>>
>>> ---
>>>  hw/intc/xics.c        |    8 ++++++++
>>>  hw/intc/xics_kvm.c    |   10 +---------
>>>  include/hw/ppc/xics.h |    1 +
>>>  3 files changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index 822d367e6388..acd63ab5e0b9 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>          return;
>>>      }
>>>  
>>> +    if (kvm_irqchip_in_kernel()) {
>>> +        icp_kvm_realize(dev, &err);  
>>
>> While we are at changing things, I would prefix all the KVM 
>> backends routine with kvmppc_*. so that icp_kvm_realize() 
>> becomes kvmppc_icp_realize()
>>
> 
> Well... kvmppc_* routines have historically been sitting under
> target/ppc so I'm not sure we want to use the same prefix
> elsewhere...

Well, they could also be moved there but I think what is important 
is that the kvmppc_* routine should be used under the kvm_enabled() 
flag. 

Those under target/ppc have and extra dummy stub provided for the 
!kvm_enabled() case. 

C.



> 
>> Apart from that,
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Thanks,
>>
>> C.
>>
>>
>>> +        if (err) {
>>> +            error_propagate(errp, err);
>>> +            return;
>>> +        }
>>> +    }
>>> +
>>>      qemu_register_reset(icp_reset_handler, dev);
>>>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>>>  }
>>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>>> index 80321e9b75ab..4eebced516b6 100644
>>> --- a/hw/intc/xics_kvm.c
>>> +++ b/hw/intc/xics_kvm.c
>>> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
>>>      return 0;
>>>  }
>>>  
>>> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
>>> +void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      ICPState *icp = ICP(dev);
>>> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>>> -    Error *local_err = NULL;
>>>      CPUState *cs;
>>>      KVMEnabledICP *enabled_icp;
>>>      unsigned long vcpu_id;
>>> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>          abort();
>>>      }
>>>  
>>> -    icpc->parent_realize(dev, &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -        return;
>>> -    }
>>> -
>>>      cs = icp->cs;
>>>      vcpu_id = kvm_arch_vcpu_id(cs);
>>>  
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index e33282a576d0..ab61dc24010a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>>>  void icp_get_kvm_state(ICPState *icp);
>>>  int icp_set_kvm_state(ICPState *icp);
>>>  void icp_synchronize_state(ICPState *icp);
>>> +void icp_kvm_realize(DeviceState *dev, Error **errp);
>>>  
>>>  #endif /* XICS_H */
>>>   
>>
> 

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Greg Kurz]

On Fri, 15 Feb 2019 13:55:53 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 12:40 PM, Greg Kurz wrote:
> > The KVM ICP class isn't used anymore. Drop it.  
> 
> Isn't migration complaining ?  If not,
> 

Hm.. no, but why would migration complain ?

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> 
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics_kvm.c    |   18 ------------------
> >  include/hw/ppc/xics.h |    3 ---
> >  2 files changed, 21 deletions(-)
> > 
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index 4eebced516b6..fae4ac431f2f 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -152,23 +152,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
> >      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
> >  }
> >  
> > -static void icp_kvm_class_init(ObjectClass *klass, void *data)
> > -{
> > -    DeviceClass *dc = DEVICE_CLASS(klass);
> > -    ICPStateClass *icpc = ICP_CLASS(klass);
> > -
> > -    device_class_set_parent_realize(dc, icp_kvm_realize,
> > -                                    &icpc->parent_realize);
> > -}
> > -
> > -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
> >   */
> > @@ -425,7 +408,6 @@ fail:
> >  static void xics_kvm_register_types(void)
> >  {
> >      type_register_static(&ics_kvm_info);
> > -    type_register_static(&icp_kvm_info);
> >  }
> >  
> >  type_init(xics_kvm_register_types)
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index ab61dc24010a..fae54e6f28ae 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -50,9 +50,6 @@ typedef struct XICSFabric XICSFabric;
> >  #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 TYPE_PNV_ICP "pnv-icp"
> >  #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
> >  
> >   
> 

Re: [Qemu-devel] [PATCH 08/10] xics: Handle KVM interrupt presentation from "simple" ICS code

[Greg Kurz]

On Fri, 15 Feb 2019 13:59:36 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 12:40 PM, Greg Kurz wrote:
> > We want to use the "simple" ICS type in both KVM and non-KVM setups.
> > Teach the "simple" ICS how to present interrupts to KVM and adapt
> > sPAPR accordingly.  
> 
> I don't see the benefits of this change.
> 

Consistency with what the rest of the series actually does: hiding the
KVM details under XICS. But I agree it doesn't buy anything more so
I don't mind if it is dropped.

> C.
> 
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/xics.c        |    5 +++++
> >  hw/intc/xics_kvm.c    |    3 +--
> >  hw/ppc/spapr_irq.c    |    7 +------
> >  include/hw/ppc/xics.h |    2 +-
> >  4 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index 49401745c410..3009fa747277 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -466,6 +466,11 @@ void ics_simple_set_irq(void *opaque, int srcno, int val)
> >  {
> >      ICSState *ics = (ICSState *)opaque;
> >  
> > +    if (kvm_irqchip_in_kernel()) {
> > +        ics_kvm_set_irq(ics, srcno, val);
> > +        return;
> > +    }
> > +
> >      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
> >          ics_simple_set_irq_lsi(ics, srcno, val);
> >      } else {
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index e7b8d4c29ce6..f34eacda03e7 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -259,9 +259,8 @@ int ics_set_kvm_state(ICSState *ics)
> >      return 0;
> >  }
> >  
> > -void ics_kvm_set_irq(void *opaque, int srcno, int val)
> > +void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
> >  {
> > -    ICSState *ics = opaque;
> >      struct kvm_irq_level args;
> >      int rc;
> >  
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index e6893df61e76..9f43b7b3bf16 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -222,13 +222,8 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
> >  static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
> >  {
> >      sPAPRMachineState *spapr = opaque;
> > -    MachineState *machine = MACHINE(opaque);
> >  
> > -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> > -        ics_kvm_set_irq(spapr->ics, srcno, val);
> > -    } else {
> > -        ics_simple_set_irq(spapr->ics, srcno, val);
> > -    }
> > +    ics_simple_set_irq(spapr->ics, srcno, val);
> >  }
> >  
> >  static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 06e87128f88e..61bd0fb9784f 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -180,7 +180,6 @@ void icp_eoi(ICPState *icp, uint32_t xirr);
> >  void ics_simple_write_xive(ICSState *ics, int nr, int server,
> >                             uint8_t priority, uint8_t saved_priority);
> >  void ics_simple_set_irq(void *opaque, int srcno, int val);
> > -void ics_kvm_set_irq(void *opaque, int srcno, int val);
> >  
> >  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> >  void icp_pic_print_info(ICPState *icp, Monitor *mon);
> > @@ -201,5 +200,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp);
> >  void ics_get_kvm_state(ICSState *ics);
> >  int ics_set_kvm_state(ICSState *ics);
> >  void ics_synchronize_state(ICSState *ics);
> > +void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
> >  
> >  #endif /* XICS_H */
> >   
> 

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Greg Kurz]

On Fri, 15 Feb 2019 14:09:53 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 2:03 PM, Greg Kurz wrote:
> > On Fri, 15 Feb 2019 13:54:02 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 2/15/19 12:40 PM, Greg Kurz wrote:  
> >>> The realization of KVM ICP currently follows the parent_realize logic,
> >>> which is a bit overkill here. Also we want to get rid of the KVM ICP
> >>> class. Explicitely call icp_kvm_realize() from the base ICP realize
> >>> function.
> >>>
> >>> Note that ICPStateClass::parent_realize is retained because powernv
> >>> needs it.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>>
> >>> ---
> >>>  hw/intc/xics.c        |    8 ++++++++
> >>>  hw/intc/xics_kvm.c    |   10 +---------
> >>>  include/hw/ppc/xics.h |    1 +
> >>>  3 files changed, 10 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >>> index 822d367e6388..acd63ab5e0b9 100644
> >>> --- a/hw/intc/xics.c
> >>> +++ b/hw/intc/xics.c
> >>> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> >>>          return;
> >>>      }
> >>>  
> >>> +    if (kvm_irqchip_in_kernel()) {
> >>> +        icp_kvm_realize(dev, &err);    
> >>
> >> While we are at changing things, I would prefix all the KVM 
> >> backends routine with kvmppc_*. so that icp_kvm_realize() 
> >> becomes kvmppc_icp_realize()
> >>  
> > 
> > Well... kvmppc_* routines have historically been sitting under
> > target/ppc so I'm not sure we want to use the same prefix
> > elsewhere...  
> 
> Well, they could also be moved there but I think what is important 
> is that the kvmppc_* routine should be used under the kvm_enabled() 
> flag. 
> 
> Those under target/ppc have and extra dummy stub provided for the 
> !kvm_enabled() case. 
> 

Well, I don't really care but if we go this way (David?), I'd rather do it
globally in a followup patch.

> C.
> 
> 
> 
> >   
> >> Apart from that,
> >>
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>  
> >>> +        if (err) {
> >>> +            error_propagate(errp, err);
> >>> +            return;
> >>> +        }
> >>> +    }
> >>> +
> >>>      qemu_register_reset(icp_reset_handler, dev);
> >>>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> >>>  }
> >>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> >>> index 80321e9b75ab..4eebced516b6 100644
> >>> --- a/hw/intc/xics_kvm.c
> >>> +++ b/hw/intc/xics_kvm.c
> >>> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
> >>>      return 0;
> >>>  }
> >>>  
> >>> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>> +void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>>  {
> >>>      ICPState *icp = ICP(dev);
> >>> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> >>> -    Error *local_err = NULL;
> >>>      CPUState *cs;
> >>>      KVMEnabledICP *enabled_icp;
> >>>      unsigned long vcpu_id;
> >>> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>>          abort();
> >>>      }
> >>>  
> >>> -    icpc->parent_realize(dev, &local_err);
> >>> -    if (local_err) {
> >>> -        error_propagate(errp, local_err);
> >>> -        return;
> >>> -    }
> >>> -
> >>>      cs = icp->cs;
> >>>      vcpu_id = kvm_arch_vcpu_id(cs);
> >>>  
> >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >>> index e33282a576d0..ab61dc24010a 100644
> >>> --- a/include/hw/ppc/xics.h
> >>> +++ b/include/hw/ppc/xics.h
> >>> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> >>>  void icp_get_kvm_state(ICPState *icp);
> >>>  int icp_set_kvm_state(ICPState *icp);
> >>>  void icp_synchronize_state(ICPState *icp);
> >>> +void icp_kvm_realize(DeviceState *dev, Error **errp);
> >>>  
> >>>  #endif /* XICS_H */
> >>>     
> >>  
> >   
> 

Re: [Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[Greg Kurz]

On Fri, 15 Feb 2019 14:02:16 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 12:40 PM, Greg Kurz wrote:
> > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
> > it instead of the ICS KVM class.  
> 
> You are changing the type name. What about migration ? 
> 

Huh ?!? I don't see how the type name would relate to migration. AFAICT they
aren't being referred to in the vmstate descriptors in xics.c.

> Can't we move the xics_kvm_init() and xics_spapr_init() call under 
> spapr_ics_create() ? It would simplify a lot the routine I think
> if these were done before creating the ICSState. 
> 

I'll look into that if there's a v2 or else in a followup patch.

> C.
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr_irq.c |   15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index 9f43b7b3bf16..4aa8165307c7 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
> >   */
> >  
> >  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> > -                                  const char *type_ics,
> >                                    int nr_irqs, Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      Object *obj;
> >  
> > -    obj = object_new(type_ics);
> > +    obj = object_new(TYPE_ICS_SIMPLE);
> >      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> >      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
> >                                     &error_abort);
> > @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
> >  {
> >      MachineState *machine = MACHINE(spapr);
> >      Error *local_err = NULL;
> > +    bool xics_kvm = false;
> >  
> >      if (kvm_enabled()) {
> >          if (machine_kernel_irqchip_allowed(machine) &&
> >              !xics_kvm_init(spapr, &local_err)) {
> > -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> > -                                          &local_err);
> > +            xics_kvm = true;
> >          }
> > -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> > +        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
> >              error_prepend(&local_err,
> >                            "kernel_irqchip requested but unavailable: ");
> >              goto error;
> > @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
> >          local_err = NULL;
> >      }
> >  
> > -    if (!spapr->ics) {
> > +    if (!xics_kvm) {
> >          xics_spapr_init(spapr);
> > -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> > -                                      &local_err);
> >      }
> >  
> > +    spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err);
> > +
> >  error:
> >      error_propagate(errp, local_err);
> >  }
> >   
> 

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Cédric Le Goater]

On 2/15/19 2:18 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 13:55:53 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 2/15/19 12:40 PM, Greg Kurz wrote:
>>> The KVM ICP class isn't used anymore. Drop it.  
>>
>> Isn't migration complaining ?  If not,
>>
> 
> Hm.. no, but why would migration complain ?

You are changing the type name of the object being transferred:

"icp-kcm" -> "icp"

Isn't that an issue ? 

C.

> 
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  hw/intc/xics_kvm.c    |   18 ------------------
>>>  include/hw/ppc/xics.h |    3 ---
>>>  2 files changed, 21 deletions(-)
>>>
>>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>>> index 4eebced516b6..fae4ac431f2f 100644
>>> --- a/hw/intc/xics_kvm.c
>>> +++ b/hw/intc/xics_kvm.c
>>> @@ -152,23 +152,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
>>>  }
>>>  
>>> -static void icp_kvm_class_init(ObjectClass *klass, void *data)
>>> -{
>>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>> -    ICPStateClass *icpc = ICP_CLASS(klass);
>>> -
>>> -    device_class_set_parent_realize(dc, icp_kvm_realize,
>>> -                                    &icpc->parent_realize);
>>> -}
>>> -
>>> -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
>>>   */
>>> @@ -425,7 +408,6 @@ fail:
>>>  static void xics_kvm_register_types(void)
>>>  {
>>>      type_register_static(&ics_kvm_info);
>>> -    type_register_static(&icp_kvm_info);
>>>  }
>>>  
>>>  type_init(xics_kvm_register_types)
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index ab61dc24010a..fae54e6f28ae 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -50,9 +50,6 @@ typedef struct XICSFabric XICSFabric;
>>>  #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 TYPE_PNV_ICP "pnv-icp"
>>>  #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
>>>  
>>>   
>>
> 

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[Cédric Le Goater]

On 2/15/19 2:27 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 14:09:53 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 2/15/19 2:03 PM, Greg Kurz wrote:
>>> On Fri, 15 Feb 2019 13:54:02 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>   
>>>> On 2/15/19 12:40 PM, Greg Kurz wrote:  
>>>>> The realization of KVM ICP currently follows the parent_realize logic,
>>>>> which is a bit overkill here. Also we want to get rid of the KVM ICP
>>>>> class. Explicitely call icp_kvm_realize() from the base ICP realize
>>>>> function.
>>>>>
>>>>> Note that ICPStateClass::parent_realize is retained because powernv
>>>>> needs it.
>>>>>
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>>
>>>>> ---
>>>>>  hw/intc/xics.c        |    8 ++++++++
>>>>>  hw/intc/xics_kvm.c    |   10 +---------
>>>>>  include/hw/ppc/xics.h |    1 +
>>>>>  3 files changed, 10 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>>>> index 822d367e6388..acd63ab5e0b9 100644
>>>>> --- a/hw/intc/xics.c
>>>>> +++ b/hw/intc/xics.c
>>>>> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
>>>>>          return;
>>>>>      }
>>>>>  
>>>>> +    if (kvm_irqchip_in_kernel()) {
>>>>> +        icp_kvm_realize(dev, &err);    
>>>>
>>>> While we are at changing things, I would prefix all the KVM 
>>>> backends routine with kvmppc_*. so that icp_kvm_realize() 
>>>> becomes kvmppc_icp_realize()
>>>>  
>>>
>>> Well... kvmppc_* routines have historically been sitting under
>>> target/ppc so I'm not sure we want to use the same prefix
>>> elsewhere...  
>>
>> Well, they could also be moved there but I think what is important 
>> is that the kvmppc_* routine should be used under the kvm_enabled() 
>> flag. 
>>
>> Those under target/ppc have and extra dummy stub provided for the 
>> !kvm_enabled() case. 
>>
> 
> Well, I don't really care but if we go this way (David?), I'd rather do it
> globally in a followup patch.

yes. that's fine also.

C. 

> 
>> C.
>>
>>
>>
>>>   
>>>> Apart from that,
>>>>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> Thanks,
>>>>
>>>> C.
>>>>
>>>>  
>>>>> +        if (err) {
>>>>> +            error_propagate(errp, err);
>>>>> +            return;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>>      qemu_register_reset(icp_reset_handler, dev);
>>>>>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>>>>>  }
>>>>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>>>>> index 80321e9b75ab..4eebced516b6 100644
>>>>> --- a/hw/intc/xics_kvm.c
>>>>> +++ b/hw/intc/xics_kvm.c
>>>>> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
>>>>>      return 0;
>>>>>  }
>>>>>  
>>>>> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>>> +void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>>>  {
>>>>>      ICPState *icp = ICP(dev);
>>>>> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>>>>> -    Error *local_err = NULL;
>>>>>      CPUState *cs;
>>>>>      KVMEnabledICP *enabled_icp;
>>>>>      unsigned long vcpu_id;
>>>>> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
>>>>>          abort();
>>>>>      }
>>>>>  
>>>>> -    icpc->parent_realize(dev, &local_err);
>>>>> -    if (local_err) {
>>>>> -        error_propagate(errp, local_err);
>>>>> -        return;
>>>>> -    }
>>>>> -
>>>>>      cs = icp->cs;
>>>>>      vcpu_id = kvm_arch_vcpu_id(cs);
>>>>>  
>>>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>>>> index e33282a576d0..ab61dc24010a 100644
>>>>> --- a/include/hw/ppc/xics.h
>>>>> +++ b/include/hw/ppc/xics.h
>>>>> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>>>>>  void icp_get_kvm_state(ICPState *icp);
>>>>>  int icp_set_kvm_state(ICPState *icp);
>>>>>  void icp_synchronize_state(ICPState *icp);
>>>>> +void icp_kvm_realize(DeviceState *dev, Error **errp);
>>>>>  
>>>>>  #endif /* XICS_H */
>>>>>     
>>>>  
>>>   
>>
> 

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Greg Kurz]

On Fri, 15 Feb 2019 14:35:03 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 2/15/19 2:18 PM, Greg Kurz wrote:
> > On Fri, 15 Feb 2019 13:55:53 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 2/15/19 12:40 PM, Greg Kurz wrote:  
> >>> The KVM ICP class isn't used anymore. Drop it.    
> >>
> >> Isn't migration complaining ?  If not,
> >>  
> > 
> > Hm.. no, but why would migration complain ?  
> 
> You are changing the type name of the object being transferred:
> 
> "icp-kcm" -> "icp"
> 

AFAIK type names aren't part of the vmstate description.

> Isn't that an issue ? 
> 
> C.
> 
> >   
> >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>  
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>>  hw/intc/xics_kvm.c    |   18 ------------------
> >>>  include/hw/ppc/xics.h |    3 ---
> >>>  2 files changed, 21 deletions(-)
> >>>
> >>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> >>> index 4eebced516b6..fae4ac431f2f 100644
> >>> --- a/hw/intc/xics_kvm.c
> >>> +++ b/hw/intc/xics_kvm.c
> >>> @@ -152,23 +152,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
> >>>      QLIST_INSERT_HEAD(&kvm_enabled_icps, enabled_icp, node);
> >>>  }
> >>>  
> >>> -static void icp_kvm_class_init(ObjectClass *klass, void *data)
> >>> -{
> >>> -    DeviceClass *dc = DEVICE_CLASS(klass);
> >>> -    ICPStateClass *icpc = ICP_CLASS(klass);
> >>> -
> >>> -    device_class_set_parent_realize(dc, icp_kvm_realize,
> >>> -                                    &icpc->parent_realize);
> >>> -}
> >>> -
> >>> -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
> >>>   */
> >>> @@ -425,7 +408,6 @@ fail:
> >>>  static void xics_kvm_register_types(void)
> >>>  {
> >>>      type_register_static(&ics_kvm_info);
> >>> -    type_register_static(&icp_kvm_info);
> >>>  }
> >>>  
> >>>  type_init(xics_kvm_register_types)
> >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >>> index ab61dc24010a..fae54e6f28ae 100644
> >>> --- a/include/hw/ppc/xics.h
> >>> +++ b/include/hw/ppc/xics.h
> >>> @@ -50,9 +50,6 @@ typedef struct XICSFabric XICSFabric;
> >>>  #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 TYPE_PNV_ICP "pnv-icp"
> >>>  #define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
> >>>  
> >>>     
> >>  
> >   
> 

Re: [Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[Cédric Le Goater]

On 2/15/19 2:32 PM, Greg Kurz wrote:
> On Fri, 15 Feb 2019 14:02:16 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 2/15/19 12:40 PM, Greg Kurz wrote:
>>> The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
>>> it instead of the ICS KVM class.  
>>
>> You are changing the type name. What about migration ? 
>>
> 
> Huh ?!? I don't see how the type name would relate to migration. AFAICT they
> aren't being referred to in the vmstate descriptors in xics.c.

In that case all is fine.

C. 
>> Can't we move the xics_kvm_init() and xics_spapr_init() call under 
>> spapr_ics_create() ? It would simplify a lot the routine I think
>> if these were done before creating the ICSState. 
>>
> 
> I'll look into that if there's a v2 or else in a followup patch.
> 
>> C.
>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  hw/ppc/spapr_irq.c |   15 +++++++--------
>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>> index 9f43b7b3bf16..4aa8165307c7 100644
>>> --- a/hw/ppc/spapr_irq.c
>>> +++ b/hw/ppc/spapr_irq.c
>>> @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>>>   */
>>>  
>>>  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
>>> -                                  const char *type_ics,
>>>                                    int nr_irqs, Error **errp)
>>>  {
>>>      Error *local_err = NULL;
>>>      Object *obj;
>>>  
>>> -    obj = object_new(type_ics);
>>> +    obj = object_new(TYPE_ICS_SIMPLE);
>>>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>>>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>>>                                     &error_abort);
>>> @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>>>  {
>>>      MachineState *machine = MACHINE(spapr);
>>>      Error *local_err = NULL;
>>> +    bool xics_kvm = false;
>>>  
>>>      if (kvm_enabled()) {
>>>          if (machine_kernel_irqchip_allowed(machine) &&
>>>              !xics_kvm_init(spapr, &local_err)) {
>>> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
>>> -                                          &local_err);
>>> +            xics_kvm = true;
>>>          }
>>> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
>>> +        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
>>>              error_prepend(&local_err,
>>>                            "kernel_irqchip requested but unavailable: ");
>>>              goto error;
>>> @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>>>          local_err = NULL;
>>>      }
>>>  
>>> -    if (!spapr->ics) {
>>> +    if (!xics_kvm) {
>>>          xics_spapr_init(spapr);
>>> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
>>> -                                      &local_err);
>>>      }
>>>  
>>> +    spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err);
>>> +
>>>  error:
>>>      error_propagate(errp, local_err);
>>>  }
>>>   
>>
> 

Re: [Qemu-devel] [PATCH 01/10] xics: Explicitely call KVM ICP methods from the common code

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:39:48PM +0100, Greg Kurz wrote:
> The pre_save(), post_load() and synchronize_state() methods of the
> ICPStateClass type are really KVM only things. Make that obvious
> by dropping the indirections and directly calling the KVM functions
> instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics.c        |   24 +++++++++++-------------
>  hw/intc/xics_kvm.c    |   12 ++++--------
>  include/hw/ppc/xics.h |    9 +++++----
>  3 files changed, 20 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 16e8ffa2aaf7..988b53abd17d 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -37,18 +37,18 @@
>  #include "qapi/visitor.h"
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
> +#include "sysemu/kvm.h"
>  
>  void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  {
> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
>      int cpu_index = icp->cs ? icp->cs->cpu_index : -1;
>  
>      if (!icp->output) {
>          return;
>      }
>  
> -    if (icpc->synchronize_state) {
> -        icpc->synchronize_state(icp);
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_synchronize_state(icp);
>      }
>  
>      monitor_printf(mon, "CPU %d XIRR=%08x (%p) PP=%02x MFRR=%02x\n",
> @@ -252,25 +252,23 @@ static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority)
>      }
>  }
>  
> -static int icp_dispatch_pre_save(void *opaque)
> +static int icp_pre_save(void *opaque)
>  {
>      ICPState *icp = opaque;
> -    ICPStateClass *info = ICP_GET_CLASS(icp);
>  
> -    if (info->pre_save) {
> -        info->pre_save(icp);
> +    if (kvm_irqchip_in_kernel()) {
> +        icp_get_kvm_state(icp);
>      }
>  
>      return 0;
>  }
>  
> -static int icp_dispatch_post_load(void *opaque, int version_id)
> +static int icp_post_load(void *opaque, int version_id)
>  {
>      ICPState *icp = opaque;
> -    ICPStateClass *info = ICP_GET_CLASS(icp);
>  
> -    if (info->post_load) {
> -        return info->post_load(icp, version_id);
> +    if (kvm_irqchip_in_kernel()) {
> +        return icp_set_kvm_state(icp);
>      }
>  
>      return 0;
> @@ -280,8 +278,8 @@ static const VMStateDescription vmstate_icp_server = {
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = icp_dispatch_pre_save,
> -    .post_load = icp_dispatch_post_load,
> +    .pre_save = icp_pre_save,
> +    .post_load = icp_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32(xirr, ICPState),
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index dff13300504c..7efa99b8b434 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -54,7 +54,7 @@ static QLIST_HEAD(, KVMEnabledICP)
>  /*
>   * ICP-KVM
>   */
> -static void icp_get_kvm_state(ICPState *icp)
> +void icp_get_kvm_state(ICPState *icp)
>  {
>      uint64_t state;
>      int ret;
> @@ -83,14 +83,14 @@ static void do_icp_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
>      icp_get_kvm_state(arg.host_ptr);
>  }
>  
> -static void icp_synchronize_state(ICPState *icp)
> +void icp_synchronize_state(ICPState *icp)
>  {
>      if (icp->cs) {
>          run_on_cpu(icp->cs, do_icp_synchronize_state, RUN_ON_CPU_HOST_PTR(icp));
>      }
>  }
>  
> -static int icp_set_kvm_state(ICPState *icp, int version_id)
> +int icp_set_kvm_state(ICPState *icp)
>  {
>      uint64_t state;
>      int ret;
> @@ -121,7 +121,7 @@ static void icp_kvm_reset(DeviceState *dev)
>  
>      icpc->parent_reset(dev);
>  
> -    icp_set_kvm_state(ICP(dev), 1);
> +    icp_set_kvm_state(ICP(dev));
>  }
>  
>  static void icp_kvm_realize(DeviceState *dev, Error **errp)
> @@ -178,10 +178,6 @@ static void icp_kvm_class_init(ObjectClass *klass, void *data)
>                                      &icpc->parent_realize);
>      device_class_set_parent_reset(dc, icp_kvm_reset,
>                                    &icpc->parent_reset);
> -
> -    icpc->pre_save = icp_get_kvm_state;
> -    icpc->post_load = icp_set_kvm_state;
> -    icpc->synchronize_state = icp_synchronize_state;
>  }
>  
>  static const TypeInfo icp_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index fad786e8b22d..3236ccec924c 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -66,10 +66,6 @@ struct ICPStateClass {
>  
>      DeviceRealize parent_realize;
>      DeviceReset parent_reset;
> -
> -    void (*pre_save)(ICPState *icp);
> -    int (*post_load)(ICPState *icp, int version_id);
> -    void (*synchronize_state)(ICPState *icp);
>  };
>  
>  struct ICPState {
> @@ -203,4 +199,9 @@ void icp_resend(ICPState *ss);
>  Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>                     Error **errp);
>  
> +/* KVM */
> +void icp_get_kvm_state(ICPState *icp);
> +int icp_set_kvm_state(ICPState *icp);
> +void icp_synchronize_state(ICPState *icp);
> +
>  #endif /* XICS_H */
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 02/10] xics: Handle KVM ICP reset from the common code

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:39:54PM +0100, Greg Kurz wrote:
> The KVM ICP reset handler simply writes the ICP state to KVM. This
> doesn't need the overkill parent_reset logic we have today. Call
> icp_set_kvm_state() from the base ICP reset function instead.
> 
> Since there are no other users for ICPStateClass::parent_reset, and
> it isn't currently expected to change, drop it as well.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 03/10] xics: Handle KVM ICP realize from the common code

[David Gibson]

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

On Fri, Feb 15, 2019 at 02:27:41PM +0100, Greg Kurz wrote:
> On Fri, 15 Feb 2019 14:09:53 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 2/15/19 2:03 PM, Greg Kurz wrote:
> > > On Fri, 15 Feb 2019 13:54:02 +0100
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >   
> > >> On 2/15/19 12:40 PM, Greg Kurz wrote:  
> > >>> The realization of KVM ICP currently follows the parent_realize logic,
> > >>> which is a bit overkill here. Also we want to get rid of the KVM ICP
> > >>> class. Explicitely call icp_kvm_realize() from the base ICP realize
> > >>> function.
> > >>>
> > >>> Note that ICPStateClass::parent_realize is retained because powernv
> > >>> needs it.
> > >>>
> > >>> Signed-off-by: Greg Kurz <groug@kaod.org>>
> > >>> ---
> > >>>  hw/intc/xics.c        |    8 ++++++++
> > >>>  hw/intc/xics_kvm.c    |   10 +---------
> > >>>  include/hw/ppc/xics.h |    1 +
> > >>>  3 files changed, 10 insertions(+), 9 deletions(-)
> > >>>
> > >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > >>> index 822d367e6388..acd63ab5e0b9 100644
> > >>> --- a/hw/intc/xics.c
> > >>> +++ b/hw/intc/xics.c
> > >>> @@ -349,6 +349,14 @@ static void icp_realize(DeviceState *dev, Error **errp)
> > >>>          return;
> > >>>      }
> > >>>  
> > >>> +    if (kvm_irqchip_in_kernel()) {
> > >>> +        icp_kvm_realize(dev, &err);    
> > >>
> > >> While we are at changing things, I would prefix all the KVM 
> > >> backends routine with kvmppc_*. so that icp_kvm_realize() 
> > >> becomes kvmppc_icp_realize()
> > >>  
> > > 
> > > Well... kvmppc_* routines have historically been sitting under
> > > target/ppc so I'm not sure we want to use the same prefix
> > > elsewhere...  
> > 
> > Well, they could also be moved there but I think what is important 
> > is that the kvmppc_* routine should be used under the kvm_enabled() 
> > flag. 
> > 
> > Those under target/ppc have and extra dummy stub provided for the 
> > !kvm_enabled() case. 
> > 
> 
> Well, I don't really care but if we go this way (David?), I'd rather do it
> globally in a followup patch.

I concur.  I'm not all that fussed really, and I think it would be
done best as a followup.

> 
> > C.
> > 
> > 
> > 
> > >   
> > >> Apart from that,
> > >>
> > >> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > >>
> > >> Thanks,
> > >>
> > >> C.
> > >>
> > >>  
> > >>> +        if (err) {
> > >>> +            error_propagate(errp, err);
> > >>> +            return;
> > >>> +        }
> > >>> +    }
> > >>> +
> > >>>      qemu_register_reset(icp_reset_handler, dev);
> > >>>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> > >>>  }
> > >>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > >>> index 80321e9b75ab..4eebced516b6 100644
> > >>> --- a/hw/intc/xics_kvm.c
> > >>> +++ b/hw/intc/xics_kvm.c
> > >>> @@ -115,11 +115,9 @@ int icp_set_kvm_state(ICPState *icp)
> > >>>      return 0;
> > >>>  }
> > >>>  
> > >>> -static void icp_kvm_realize(DeviceState *dev, Error **errp)
> > >>> +void icp_kvm_realize(DeviceState *dev, Error **errp)
> > >>>  {
> > >>>      ICPState *icp = ICP(dev);
> > >>> -    ICPStateClass *icpc = ICP_GET_CLASS(icp);
> > >>> -    Error *local_err = NULL;
> > >>>      CPUState *cs;
> > >>>      KVMEnabledICP *enabled_icp;
> > >>>      unsigned long vcpu_id;
> > >>> @@ -129,12 +127,6 @@ static void icp_kvm_realize(DeviceState *dev, Error **errp)
> > >>>          abort();
> > >>>      }
> > >>>  
> > >>> -    icpc->parent_realize(dev, &local_err);
> > >>> -    if (local_err) {
> > >>> -        error_propagate(errp, local_err);
> > >>> -        return;
> > >>> -    }
> > >>> -
> > >>>      cs = icp->cs;
> > >>>      vcpu_id = kvm_arch_vcpu_id(cs);
> > >>>  
> > >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > >>> index e33282a576d0..ab61dc24010a 100644
> > >>> --- a/include/hw/ppc/xics.h
> > >>> +++ b/include/hw/ppc/xics.h
> > >>> @@ -202,5 +202,6 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > >>>  void icp_get_kvm_state(ICPState *icp);
> > >>>  int icp_set_kvm_state(ICPState *icp);
> > >>>  void icp_synchronize_state(ICPState *icp);
> > >>> +void icp_kvm_realize(DeviceState *dev, Error **errp);
> > >>>  
> > >>>  #endif /* XICS_H */
> > >>>     
> > >>  
> > >   
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 04/10] spapr/irq: Use the base ICP class for KVM

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:40:06PM +0100, Greg Kurz wrote:
> The base ICP class knows how to interact with KVM. Adapt sPAPR to use it
> instead of the ICP KVM class.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/ppc/spapr_irq.c     |    4 +---
>  include/hw/ppc/spapr.h |    1 -
>  2 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 48d6b2daed6e..e6893df61e76 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -102,7 +102,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>      if (kvm_enabled()) {
>          if (machine_kernel_irqchip_allowed(machine) &&
>              !xics_kvm_init(spapr, &local_err)) {
> -            spapr->icp_type = TYPE_KVM_ICP;
>              spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
>                                            &local_err);
>          }
> @@ -117,7 +116,6 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>  
>      if (!spapr->ics) {
>          xics_spapr_init(spapr);
> -        spapr->icp_type = TYPE_ICP;
>          spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
>                                        &local_err);
>      }
> @@ -199,7 +197,7 @@ static void spapr_irq_cpu_intc_create_xics(sPAPRMachineState *spapr,
>      Object *obj;
>      sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>  
> -    obj = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> +    obj = icp_create(OBJECT(cpu), TYPE_ICP, XICS_FABRIC(spapr),
>                       &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index cbd276ed2b6a..631fc5103b7b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -178,7 +178,6 @@ struct sPAPRMachineState {
>      /*< public >*/
>      char *kvm_type;
>  
> -    const char *icp_type;
>      int32_t irq_map_nr;
>      unsigned long *irq_map;
>      sPAPRXive  *xive;
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[David Gibson]

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

On Fri, Feb 15, 2019 at 02:35:03PM +0100, Cédric Le Goater wrote:
> On 2/15/19 2:18 PM, Greg Kurz wrote:
> > On Fri, 15 Feb 2019 13:55:53 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> >> On 2/15/19 12:40 PM, Greg Kurz wrote:
> >>> The KVM ICP class isn't used anymore. Drop it.  
> >>
> >> Isn't migration complaining ?  If not,
> >>
> > 
> > Hm.. no, but why would migration complain ?
> 
> You are changing the type name of the object being transferred:
> 
> "icp-kcm" -> "icp"

It's a little more complex than that.  The way migration works, the
state associated with the base class is transferred under the name
"icp" and the state associated with the derived class is transferred
under the name "icp-kvm".

> Isn't that an issue ?

It would be.. except that there is no extra state in the derived
class, which is why we got away with this not-very-good solution at
all in the first place.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 06/10] xics: Explicitely call KVM ICS methods from the common code

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:40:18PM +0100, Greg Kurz wrote:
> The pre_save(), post_load() and synchronize_state() methods of the
> ICSStateClass type are really KVM only things. Make that obvious
> by dropping the indirections and directly calling the KVM functions
> instead.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics.c        |   23 ++++++++++-------------
>  hw/intc/xics_kvm.c    |   12 ++++--------
>  include/hw/ppc/xics.h |    7 ++++---
>  3 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index acd63ab5e0b9..ae5d5ea135b8 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -58,7 +58,6 @@ void icp_pic_print_info(ICPState *icp, Monitor *mon)
>  
>  void ics_pic_print_info(ICSState *ics, Monitor *mon)
>  {
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
>      uint32_t i;
>  
>      monitor_printf(mon, "ICS %4x..%4x %p\n",
> @@ -68,8 +67,8 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon)
>          return;
>      }
>  
> -    if (icsc->synchronize_state) {
> -        icsc->synchronize_state(ics);
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_synchronize_state(ics);
>      }
>  
>      for (i = 0; i < ics->nr_irqs; i++) {
> @@ -647,25 +646,23 @@ static void ics_base_instance_init(Object *obj)
>      ics->offset = XICS_IRQ_BASE;
>  }
>  
> -static int ics_base_dispatch_pre_save(void *opaque)
> +static int ics_base_pre_save(void *opaque)
>  {
>      ICSState *ics = opaque;
> -    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>  
> -    if (info->pre_save) {
> -        info->pre_save(ics);
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_get_kvm_state(ics);
>      }
>  
>      return 0;
>  }
>  
> -static int ics_base_dispatch_post_load(void *opaque, int version_id)
> +static int ics_base_post_load(void *opaque, int version_id)
>  {
>      ICSState *ics = opaque;
> -    ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
>  
> -    if (info->post_load) {
> -        return info->post_load(ics, version_id);
> +    if (kvm_irqchip_in_kernel()) {
> +        return ics_set_kvm_state(ics);
>      }
>  
>      return 0;
> @@ -689,8 +686,8 @@ static const VMStateDescription vmstate_ics_base = {
>      .name = "ics",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_save = ics_base_dispatch_pre_save,
> -    .post_load = ics_base_dispatch_post_load,
> +    .pre_save = ics_base_pre_save,
> +    .post_load = ics_base_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index fae4ac431f2f..642351e5790f 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -155,7 +155,7 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
>  /*
>   * ICS-KVM
>   */
> -static void ics_get_kvm_state(ICSState *ics)
> +void ics_get_kvm_state(ICSState *ics)
>  {
>      uint64_t state;
>      int i;
> @@ -208,12 +208,12 @@ static void ics_get_kvm_state(ICSState *ics)
>      }
>  }
>  
> -static void ics_synchronize_state(ICSState *ics)
> +void ics_synchronize_state(ICSState *ics)
>  {
>      ics_get_kvm_state(ics);
>  }
>  
> -static int ics_set_kvm_state(ICSState *ics, int version_id)
> +int ics_set_kvm_state(ICSState *ics)
>  {
>      uint64_t state;
>      int i;
> @@ -286,7 +286,7 @@ static void ics_kvm_reset(DeviceState *dev)
>  
>      icsc->parent_reset(dev);
>  
> -    ics_set_kvm_state(ICS_KVM(dev), 1);
> +    ics_set_kvm_state(ICS_KVM(dev));
>  }
>  
>  static void ics_kvm_reset_handler(void *dev)
> @@ -318,10 +318,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>                                      &icsc->parent_realize);
>      device_class_set_parent_reset(dc, ics_kvm_reset,
>                                    &icsc->parent_reset);
> -
> -    icsc->pre_save = ics_get_kvm_state;
> -    icsc->post_load = ics_set_kvm_state;
> -    icsc->synchronize_state = ics_synchronize_state;
>  }
>  
>  static const TypeInfo ics_kvm_info = {
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index fae54e6f28ae..06e87128f88e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -109,12 +109,9 @@ struct ICSStateClass {
>      DeviceRealize parent_realize;
>      DeviceReset parent_reset;
>  
> -    void (*pre_save)(ICSState *s);
> -    int (*post_load)(ICSState *s, int version_id);
>      void (*reject)(ICSState *s, uint32_t irq);
>      void (*resend)(ICSState *s);
>      void (*eoi)(ICSState *s, uint32_t irq);
> -    void (*synchronize_state)(ICSState *s);
>  };
>  
>  struct ICSState {
> @@ -201,4 +198,8 @@ int icp_set_kvm_state(ICPState *icp);
>  void icp_synchronize_state(ICPState *icp);
>  void icp_kvm_realize(DeviceState *dev, Error **errp);
>  
> +void ics_get_kvm_state(ICSState *ics);
> +int ics_set_kvm_state(ICSState *ics);
> +void ics_synchronize_state(ICSState *ics);
> +
>  #endif /* XICS_H */
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 07/10] xics: Handle KVM ICS reset from the "simple" ICS code

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:40:24PM +0100, Greg Kurz wrote:
> The KVM ICS reset handler simply writes the ICS state to KVM. This
> doesn't need the overkill parent_reset logic we have today. Also
> we want to use the same ICS type for the KVM and non-KVM case with
> pseries.
> 
> Call icp_set_kvm_state() from the "simple" ICS reset function.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied, thanks.

> ---
>  hw/intc/xics.c     |    4 ++++
>  hw/intc/xics_kvm.c |   18 ------------------
>  2 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index ae5d5ea135b8..49401745c410 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -553,6 +553,10 @@ static void ics_simple_reset(DeviceState *dev)
>      ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>  
>      icsc->parent_reset(dev);
> +
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_set_kvm_state(ICS_BASE(dev));
> +    }
>  }
>  
>  static void ics_simple_reset_handler(void *dev)
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 642351e5790f..e7b8d4c29ce6 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -280,20 +280,6 @@ void ics_kvm_set_irq(void *opaque, int srcno, int val)
>      }
>  }
>  
> -static void ics_kvm_reset(DeviceState *dev)
> -{
> -    ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> -
> -    icsc->parent_reset(dev);
> -
> -    ics_set_kvm_state(ICS_KVM(dev));
> -}
> -
> -static void ics_kvm_reset_handler(void *dev)
> -{
> -    ics_kvm_reset(dev);
> -}
> -
>  static void ics_kvm_realize(DeviceState *dev, Error **errp)
>  {
>      ICSState *ics = ICS_KVM(dev);
> @@ -305,8 +291,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp)
>          error_propagate(errp, local_err);
>          return;
>      }
> -
> -    qemu_register_reset(ics_kvm_reset_handler, ics);
>  }
>  
>  static void ics_kvm_class_init(ObjectClass *klass, void *data)
> @@ -316,8 +300,6 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>  
>      device_class_set_parent_realize(dc, ics_kvm_realize,
>                                      &icsc->parent_realize);
> -    device_class_set_parent_reset(dc, ics_kvm_reset,
> -                                  &icsc->parent_reset);
>  }
>  
>  static const TypeInfo ics_kvm_info = {
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 08/10] xics: Handle KVM interrupt presentation from "simple" ICS code

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:40:30PM +0100, Greg Kurz wrote:
> We want to use the "simple" ICS type in both KVM and non-KVM setups.
> Teach the "simple" ICS how to present interrupts to KVM and adapt
> sPAPR accordingly.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

As Cédric points out, not strictly required to accomplish the series'
goals, but I think it's a nice cleanup, so applied.

> ---
>  hw/intc/xics.c        |    5 +++++
>  hw/intc/xics_kvm.c    |    3 +--
>  hw/ppc/spapr_irq.c    |    7 +------
>  include/hw/ppc/xics.h |    2 +-
>  4 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 49401745c410..3009fa747277 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -466,6 +466,11 @@ void ics_simple_set_irq(void *opaque, int srcno, int val)
>  {
>      ICSState *ics = (ICSState *)opaque;
>  
> +    if (kvm_irqchip_in_kernel()) {
> +        ics_kvm_set_irq(ics, srcno, val);
> +        return;
> +    }
> +
>      if (ics->irqs[srcno].flags & XICS_FLAGS_IRQ_LSI) {
>          ics_simple_set_irq_lsi(ics, srcno, val);
>      } else {
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index e7b8d4c29ce6..f34eacda03e7 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -259,9 +259,8 @@ int ics_set_kvm_state(ICSState *ics)
>      return 0;
>  }
>  
> -void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +void ics_kvm_set_irq(ICSState *ics, int srcno, int val)
>  {
> -    ICSState *ics = opaque;
>      struct kvm_irq_level args;
>      int rc;
>  
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index e6893df61e76..9f43b7b3bf16 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -222,13 +222,8 @@ static int spapr_irq_post_load_xics(sPAPRMachineState *spapr, int version_id)
>  static void spapr_irq_set_irq_xics(void *opaque, int srcno, int val)
>  {
>      sPAPRMachineState *spapr = opaque;
> -    MachineState *machine = MACHINE(opaque);
>  
> -    if (kvm_enabled() && machine_kernel_irqchip_allowed(machine)) {
> -        ics_kvm_set_irq(spapr->ics, srcno, val);
> -    } else {
> -        ics_simple_set_irq(spapr->ics, srcno, val);
> -    }
> +    ics_simple_set_irq(spapr->ics, srcno, val);
>  }
>  
>  static void spapr_irq_reset_xics(sPAPRMachineState *spapr, Error **errp)
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 06e87128f88e..61bd0fb9784f 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -180,7 +180,6 @@ void icp_eoi(ICPState *icp, uint32_t xirr);
>  void ics_simple_write_xive(ICSState *ics, int nr, int server,
>                             uint8_t priority, uint8_t saved_priority);
>  void ics_simple_set_irq(void *opaque, int srcno, int val);
> -void ics_kvm_set_irq(void *opaque, int srcno, int val);
>  
>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
>  void icp_pic_print_info(ICPState *icp, Monitor *mon);
> @@ -201,5 +200,6 @@ void icp_kvm_realize(DeviceState *dev, Error **errp);
>  void ics_get_kvm_state(ICSState *ics);
>  int ics_set_kvm_state(ICSState *ics);
>  void ics_synchronize_state(ICSState *ics);
> +void ics_kvm_set_irq(ICSState *ics, int srcno, int val);
>  
>  #endif /* XICS_H */
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[David Gibson]

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

On Fri, Feb 15, 2019 at 02:02:16PM +0100, Cédric Le Goater wrote:
> On 2/15/19 12:40 PM, Greg Kurz wrote:
> > The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
> > it instead of the ICS KVM class.
> 
> You are changing the type name. What about migration ?

As with ICP this would be a problem if the ics-kvm class had any extra
migration state, but it doesn't.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 09/10] spapr/irq: Use the "simple" ICS class for KVM

[David Gibson]

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

On Fri, Feb 15, 2019 at 12:40:35PM +0100, Greg Kurz wrote:
> The "simple" ICS class knows how to interract with KVM. Adapt sPAPR to use
> it instead of the ICS KVM class.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_irq.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 9f43b7b3bf16..4aa8165307c7 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -67,13 +67,12 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>   */
>  
>  static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> -                                  const char *type_ics,
>                                    int nr_irqs, Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
>  
> -    obj = object_new(type_ics);
> +    obj = object_new(TYPE_ICS_SIMPLE);
>      object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>      object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>                                     &error_abort);
> @@ -98,14 +97,14 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>  {
>      MachineState *machine = MACHINE(spapr);
>      Error *local_err = NULL;
> +    bool xics_kvm = false;
>  
>      if (kvm_enabled()) {
>          if (machine_kernel_irqchip_allowed(machine) &&
>              !xics_kvm_init(spapr, &local_err)) {
> -            spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> -                                          &local_err);
> +            xics_kvm = true;
>          }
> -        if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> +        if (machine_kernel_irqchip_required(machine) && !xics_kvm) {
>              error_prepend(&local_err,
>                            "kernel_irqchip requested but unavailable: ");
>              goto error;
> @@ -114,12 +113,12 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
>          local_err = NULL;
>      }
>  
> -    if (!spapr->ics) {
> +    if (!xics_kvm) {
>          xics_spapr_init(spapr);
> -        spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> -                                      &local_err);

As Cédric suggested, I think this would be nicer if xics_kvm_init()
and xics_spapr_init() were folded together with the KVM fallback
inside.

That can be a follow up patch, though,

Applied.

>      }
>  
> +    spapr->ics = spapr_ics_create(spapr, nr_irqs, &local_err);
> +
>  error:
>      error_propagate(errp, local_err);
>  }
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 05/10] xics: Drop the KVM ICP class

[Cédric Le Goater]

On 2/18/19 12:37 AM, David Gibson wrote:
> On Fri, Feb 15, 2019 at 02:35:03PM +0100, Cédric Le Goater wrote:
>> On 2/15/19 2:18 PM, Greg Kurz wrote:
>>> On Fri, 15 Feb 2019 13:55:53 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> On 2/15/19 12:40 PM, Greg Kurz wrote:
>>>>> The KVM ICP class isn't used anymore. Drop it.  
>>>>
>>>> Isn't migration complaining ?  If not,
>>>>
>>>
>>> Hm.. no, but why would migration complain ?
>>
>> You are changing the type name of the object being transferred:
>>
>> "icp-kcm" -> "icp"
> 
> It's a little more complex than that.  The way migration works, the
> state associated with the base class is transferred under the name
> "icp" and the state associated with the derived class is transferred
> under the name "icp-kvm".
> 
>> Isn't that an issue ?
> 
> It would be.. except that there is no extra state in the derived
> class, which is why we got away with this not-very-good solution at
> all in the first place.

Ah good. Another reason to get rid of the sub-class.

C.
  

Re: [Qemu-devel] [PATCH 01/10] xics: Explicitely call KVM ICP methods from the common code

[Eric Blake]

On 2/17/19 5:14 PM, David Gibson wrote:

In the subject line: s/Explicitely/Explicitly/

> On Fri, Feb 15, 2019 at 12:39:48PM +0100, Greg Kurz wrote:
>> The pre_save(), post_load() and synchronize_state() methods of the
>> ICPStateClass type are really KVM only things. Make that obvious
>> by dropping the indirections and directly calling the KVM functions
>> instead.
>>
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 01/10] xics: Explicitely call KVM ICP methods from the common code

[David Gibson]

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

On Mon, Feb 18, 2019 at 02:10:41PM -0600, Eric Blake wrote:
> On 2/17/19 5:14 PM, David Gibson wrote:
> 
> In the subject line: s/Explicitely/Explicitly/

I'm afraid I already sent that to Peter.  If the pull request gets
bounced I'll fix it up, otherwise, too bad.

> 
> > On Fri, Feb 15, 2019 at 12:39:48PM +0100, Greg Kurz wrote:
> >> The pre_save(), post_load() and synchronize_state() methods of the
> >> ICPStateClass type are really KVM only things. Make that obvious
> >> by dropping the indirections and directly calling the KVM functions
> >> instead.
> >>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]