[Qemu-devel] [PATCH 0/6 v2] xics: reworks and in-kernel support

[Qemu-devel] [PATCH 0/6 v2] xics: reworks and in-kernel support

[Alexey Kardashevskiy]

Another try with XICS-KVM.

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

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


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

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

 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xics.c                    | 280 ++++++++++++++++------
 hw/intc/xics_kvm.c                | 479 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c                    |  27 ++-
 include/hw/ppc/xics.h             |  68 +++++-
 target-ppc/kvm.c                  |  14 ++
 target-ppc/kvm_ppc.h              |   7 +
 8 files changed, 803 insertions(+), 74 deletions(-)
 create mode 100644 hw/intc/xics_kvm.c

-- 
1.8.3.2

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

[Alexey Kardashevskiy]

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

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

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

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

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

[Andreas Färber]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent PowerKVM allows the kernel to intercept some RTAS calls from the
> guest directly.  This is used to implement the more efficient in-kernel
> XICS for example.  qemu is still responsible for assigning the RTAS token
> numbers however, and needs to tell the kernel which RTAS function name is
> assigned to a given token value.  This patch adds a convenience wrapper for
> the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this purpose.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Lacking your Sob, otherwise looks fine.

Andreas

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

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

[Alexey Kardashevskiy]

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

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

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

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

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

[Andreas Färber]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> The upcoming support of in-kernel XICS will redefine migration callbacks
> for both ICS and ICP so classes and callback pointers are added.
> 
> This adds a cpu_setup callback to the XICS device class (as XICS-KVM
> will do it different) and xics_dispatch_cpu_setup(). This also moves
> the place where xics_dispatch_cpu_setup() is called a bit further to
> have VCPU initialized (required for XICS-KVM).
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/intc/xics.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/ppc/spapr.c        |  4 ++--
>  include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 104 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 6b3c071..c5dad2f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -153,11 +153,35 @@ static void icp_irq(XICSState *icp, int server, int nr, uint8_t priority)
>      }
>  }
>  
> +static void icp_dispatch_pre_save(void *opaque)
> +{
> +    ICPState *ss = opaque;
> +    ICPStateClass *info = ICP_GET_CLASS(ss);
> +
> +    if (info->pre_save) {
> +        info->pre_save(ss);
> +    }
> +}
> +
> +static int icp_dispatch_post_load(void *opaque, int version_id)
> +{
> +    ICPState *ss = opaque;
> +    ICPStateClass *info = ICP_GET_CLASS(ss);
> +
> +    if (info->post_load) {
> +        return info->post_load(ss);
> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_icp_server = {
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> +    .pre_save = icp_dispatch_pre_save,
> +    .post_load = icp_dispatch_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32(xirr, ICPState),
> @@ -192,6 +216,7 @@ static TypeInfo icp_info = {
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICPState),
>      .class_init = icp_class_init,
> +    .class_size = sizeof(ICPStateClass),
>  };
>  
>  /*
> @@ -353,10 +378,9 @@ static void ics_reset(DeviceState *dev)
>      }
>  }
>  
> -static int ics_post_load(void *opaque, int version_id)
> +static int ics_post_load(ICSState *ics)
>  {
>      int i;
> -    ICSState *ics = opaque;
>  
>      for (i = 0; i < ics->icp->nr_servers; i++) {
>          icp_resend(ics->icp, i);
> @@ -365,6 +389,28 @@ static int ics_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void ics_dispatch_pre_save(void *opaque)
> +{
> +    ICSState *ics = opaque;
> +    ICSStateClass *info = ICS_GET_CLASS(ics);
> +
> +    if (info->pre_save) {
> +        info->pre_save(ics);
> +    }
> +}
> +
> +static int ics_dispatch_post_load(void *opaque, int version_id)
> +{
> +    ICSState *ics = opaque;
> +    ICSStateClass *info = ICS_GET_CLASS(ics);
> +
> +    if (info->post_load) {
> +        return info->post_load(ics);

Pass version_id through?

> +    }
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_ics_irq = {
>      .name = "ics/irq",
>      .version_id = 1,
> @@ -384,7 +430,8 @@ static const VMStateDescription vmstate_ics = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> -    .post_load = ics_post_load,
> +    .pre_save = ics_dispatch_pre_save,
> +    .post_load = ics_dispatch_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
> @@ -409,10 +456,12 @@ static int ics_realize(DeviceState *dev)
>  static void ics_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *k = ICS_CLASS(klass);

isc?

Otherwise looks fine.

Andreas

>  
>      dc->init = ics_realize;
>      dc->vmsd = &vmstate_ics;
>      dc->reset = ics_reset;
> +    k->post_load = ics_post_load;
>  }
>  
>  static TypeInfo ics_info = {
> @@ -420,6 +469,7 @@ static TypeInfo ics_info = {
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICSState),
>      .class_init = ics_class_init,
> +    .class_size = sizeof(ICSStateClass),
>  };
>  
>  /*
> @@ -612,7 +662,7 @@ static void xics_reset(DeviceState *d)
>      device_reset(DEVICE(icp->ics));
>  }
>  
> -void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> @@ -674,10 +724,12 @@ static Property xics_properties[] = {
>  static void xics_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *k = XICS_CLASS(oc);
>  
>      dc->realize = xics_realize;
>      dc->props = xics_properties;
>      dc->reset = xics_reset;
> +    k->cpu_setup = xics_cpu_setup;
>  
>      spapr_rtas_register("ibm,set-xive", rtas_set_xive);
>      spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> @@ -694,6 +746,7 @@ static const TypeInfo xics_info = {
>      .name          = TYPE_XICS,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(XICSState),
> +    .class_size = sizeof(XICSStateClass),
>      .class_init    = xics_class_init,
>      .instance_init = xics_initfn,
>  };
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 16bfab9..432f0d2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1155,8 +1155,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>          }
>          env = &cpu->env;
>  
> -        xics_cpu_setup(spapr->icp, cpu);
> -
>          /* Set time-base frequency to 512 MHz */
>          cpu_ppc_tb_init(env, TIMEBASE_FREQ);
>  
> @@ -1170,6 +1168,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>              kvmppc_set_papr(cpu);
>          }
>  
> +        xics_dispatch_cpu_setup(spapr->icp, cpu);
> +
>          qemu_register_reset(spapr_cpu_reset, cpu);
>      }
>  
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 66364c5..90ecaf8 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -32,6 +32,11 @@
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define XICS_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
> +#define XICS_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
> +
>  #define XICS_IPI        0x2
>  #define XICS_BUID       0x1
>  #define XICS_IRQ_BASE   (XICS_BUID << 12)
> @@ -41,11 +46,20 @@
>   * (the kernel implementation supports more but we don't exploit
>   *  that yet)
>   */
> +typedef struct XICSStateClass XICSStateClass;
>  typedef struct XICSState XICSState;
> +typedef struct ICPStateClass ICPStateClass;
>  typedef struct ICPState ICPState;
> +typedef struct ICSStateClass ICSStateClass;
>  typedef struct ICSState ICSState;
>  typedef struct ICSIRQState ICSIRQState;
>  
> +struct XICSStateClass {
> +    DeviceClass parent_class;
> +
> +    void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
> +};
> +
>  struct XICSState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -59,6 +73,18 @@ struct XICSState {
>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> +#define ICP_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
> +#define ICP_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(ICPStateClass, (obj), TYPE_ICP)
> +
> +struct ICPStateClass {
> +    DeviceClass parent_class;
> +
> +    void (*pre_save)(ICPState *s);
> +    int (*post_load)(ICPState *s);
> +};
> +
>  struct ICPState {
>      /*< private >*/
>      DeviceState parent_obj;
> @@ -72,6 +98,18 @@ struct ICPState {
>  #define TYPE_ICS "ics"
>  #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>  
> +#define ICS_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
> +#define ICS_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
> +
> +struct ICSStateClass {
> +    DeviceClass parent_class;
> +
> +    void (*pre_save)(ICSState *s);
> +    int (*post_load)(ICSState *s);
> +};
> +
>  struct ICSState {
>      /*< private >*/
>      DeviceState parent_obj;
> @@ -98,6 +136,12 @@ struct ICSIRQState {
>  qemu_irq xics_get_qirq(XICSState *icp, int irq);
>  void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
>  
> -void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
> +static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    XICSStateClass *info = XICS_GET_CLASS(icp);
> +
> +    assert(info->cpu_setup);
> +    info->cpu_setup(icp, cpu);
> +}
>  
>  #endif /* __XICS_H__ */
> 


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

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

[Alexey Kardashevskiy]

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

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

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c5dad2f..436c788 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -692,6 +692,17 @@ static void xics_realize(DeviceState *dev, Error **errp)
     ICSState *ics = icp->ics;
     int i;
 
+    /* Registration of global state belongs into realize */
+    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
+    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
+    spapr_rtas_register("ibm,int-off", rtas_int_off);
+    spapr_rtas_register("ibm,int-on", rtas_int_on);
+
+    spapr_register_hypercall(H_CPPR, h_cppr);
+    spapr_register_hypercall(H_IPI, h_ipi);
+    spapr_register_hypercall(H_XIRR, h_xirr);
+    spapr_register_hypercall(H_EOI, h_eoi);
+
     ics->nr_irqs = icp->nr_irqs;
     ics->offset = XICS_IRQ_BASE;
     ics->icp = icp;
@@ -730,16 +741,6 @@ static void xics_class_init(ObjectClass *oc, void *data)
     dc->props = xics_properties;
     dc->reset = xics_reset;
     k->cpu_setup = xics_cpu_setup;
-
-    spapr_rtas_register("ibm,set-xive", rtas_set_xive);
-    spapr_rtas_register("ibm,get-xive", rtas_get_xive);
-    spapr_rtas_register("ibm,int-off", rtas_int_off);
-    spapr_rtas_register("ibm,int-on", rtas_int_on);
-
-    spapr_register_hypercall(H_CPPR, h_cppr);
-    spapr_register_hypercall(H_IPI, h_ipi);
-    spapr_register_hypercall(H_XIRR, h_xirr);
-    spapr_register_hypercall(H_EOI, h_eoi);
 }
 
 static const TypeInfo xics_info = {
-- 
1.8.3.2

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

[Andreas Färber]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> Registration of global state belongs into realize so move it there.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

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

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

[Alexey Kardashevskiy]

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

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 436c788..20840e3 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -29,6 +29,7 @@
 #include "trace.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "qemu/error-report.h"
 
 /*
  * ICP: Presentation layer
@@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_icp_server;
 }
 
-static TypeInfo icp_info = {
+static const TypeInfo icp_info = {
     .name = TYPE_ICP,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICPState),
@@ -446,6 +447,7 @@ static int ics_realize(DeviceState *dev)
 {
     ICSState *ics = ICS(dev);
 
+    assert(ics->nr_irqs);
     ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
     ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
     ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
@@ -456,15 +458,15 @@ static int ics_realize(DeviceState *dev)
 static void ics_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    ICSStateClass *k = ICS_CLASS(klass);
+    ICSStateClass *icsc = ICS_CLASS(klass);
 
     dc->init = ics_realize;
     dc->vmsd = &vmstate_ics;
     dc->reset = ics_reset;
-    k->post_load = ics_post_load;
+    icsc->post_load = ics_post_load;
 }
 
-static TypeInfo ics_info = {
+static const TypeInfo ics_info = {
     .name = TYPE_ICS,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(ICSState),
@@ -680,8 +682,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
         break;
 
     default:
-        fprintf(stderr, "XICS interrupt controller does not support this CPU "
-                "bus model\n");
+        error_report("XICS interrupt controller does not support this CPU "
+                "bus model");
         abort();
     }
 }
@@ -690,6 +692,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
 {
     XICSState *icp = XICS(dev);
     ICSState *ics = icp->ics;
+    Error *error = NULL;
     int i;
 
     /* Registration of global state belongs into realize */
@@ -706,15 +709,24 @@ static void xics_realize(DeviceState *dev, Error **errp)
     ics->nr_irqs = icp->nr_irqs;
     ics->offset = XICS_IRQ_BASE;
     ics->icp = icp;
-    qdev_init_nofail(DEVICE(ics));
+    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
 
+    assert(icp->nr_servers);
     icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
         char buffer[32];
         object_initialize(&icp->ss[i], TYPE_ICP);
         snprintf(buffer, sizeof(buffer), "icp[%d]", i);
         object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
-        qdev_init_nofail(DEVICE(&icp->ss[i]));
+        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
+        if (error) {
+            error_propagate(errp, error);
+            return;
+        }
     }
 }
 
@@ -735,12 +747,12 @@ static Property xics_properties[] = {
 static void xics_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
-    XICSStateClass *k = XICS_CLASS(oc);
+    XICSStateClass *xsc = XICS_CLASS(oc);
 
     dc->realize = xics_realize;
     dc->props = xics_properties;
     dc->reset = xics_reset;
-    k->cpu_setup = xics_cpu_setup;
+    xsc->cpu_setup = xics_cpu_setup;
 }
 
 static const TypeInfo xics_info = {
-- 
1.8.3.2

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

[Andreas Färber]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> Before XICS-KVM comes, it makes sense to clean up the emulated XICS a bit.
> 
> This does:
> 1. add assert in ics_realize()
> 2. change variable names from "k" to more informative ones
> 3. add "const" to every TypeInfo
> 4. replace fprintf(stderr, ..."\n") with error_report
> 5. replace old style qdev_init_nofail() with new style
> object_property_set_bool().
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  hw/intc/xics.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 436c788..20840e3 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -29,6 +29,7 @@
>  #include "trace.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "qemu/error-report.h"
>  
>  /*
>   * ICP: Presentation layer
> @@ -211,7 +212,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
>      dc->vmsd = &vmstate_icp_server;
>  }
>  
> -static TypeInfo icp_info = {
> +static const TypeInfo icp_info = {
>      .name = TYPE_ICP,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICPState),
> @@ -446,6 +447,7 @@ static int ics_realize(DeviceState *dev)
>  {
>      ICSState *ics = ICS(dev);
>  
> +    assert(ics->nr_irqs);

Why assert here? As pointed out, this function is bogus, it should have
an Error **errp argument, so you should just do something like this:

    if (ics->nr_irqs == 0) {
        error_setg(errp, "Number of interrupts needs to be greater 0");
        return;
    }

which propagates the error to the caller and lets him decide how to
handle it, such as actually specifying a different property value and
trying realized = true again.

>      ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>      ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>      ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
> @@ -456,15 +458,15 @@ static int ics_realize(DeviceState *dev)
>  static void ics_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    ICSStateClass *k = ICS_CLASS(klass);
> +    ICSStateClass *icsc = ICS_CLASS(klass);
>  
>      dc->init = ics_realize;
>      dc->vmsd = &vmstate_ics;
>      dc->reset = ics_reset;
> -    k->post_load = ics_post_load;
> +    icsc->post_load = ics_post_load;
>  }
>  
> -static TypeInfo ics_info = {
> +static const TypeInfo ics_info = {
>      .name = TYPE_ICS,
>      .parent = TYPE_DEVICE,
>      .instance_size = sizeof(ICSState),
> @@ -680,8 +682,8 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>          break;
>  
>      default:
> -        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> -                "bus model\n");
> +        error_report("XICS interrupt controller does not support this CPU "
> +                "bus model");
>          abort();
>      }
>  }
> @@ -690,6 +692,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *icp = XICS(dev);
>      ICSState *ics = icp->ics;
> +    Error *error = NULL;
>      int i;
>  
>      /* Registration of global state belongs into realize */
> @@ -706,15 +709,24 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      ics->nr_irqs = icp->nr_irqs;
>      ics->offset = XICS_IRQ_BASE;
>      ics->icp = icp;
> -    qdev_init_nofail(DEVICE(ics));
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
>  
> +    assert(icp->nr_servers);

Ditto here - this one already has errp.

Otherwise good, thanks.

Andreas

>      icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>      for (i = 0; i < icp->nr_servers; i++) {
>          char buffer[32];
>          object_initialize(&icp->ss[i], TYPE_ICP);
>          snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>          object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> -        qdev_init_nofail(DEVICE(&icp->ss[i]));
> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            return;
> +        }
>      }
>  }
>  
> @@ -735,12 +747,12 @@ static Property xics_properties[] = {
>  static void xics_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> -    XICSStateClass *k = XICS_CLASS(oc);
> +    XICSStateClass *xsc = XICS_CLASS(oc);
>  
>      dc->realize = xics_realize;
>      dc->props = xics_properties;
>      dc->reset = xics_reset;
> -    k->cpu_setup = xics_cpu_setup;
> +    xsc->cpu_setup = xics_cpu_setup;
>  }
>  
>  static const TypeInfo xics_info = {

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

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

[Alexey Kardashevskiy]

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

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

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

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/intc/xics.c        | 190 +++++++++++++++++++++++++++++++++++---------------
 include/hw/ppc/xics.h |  11 ++-
 2 files changed, 143 insertions(+), 58 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 20840e3..95865aa 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -30,6 +30,112 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
 #include "qemu/error-report.h"
+#include "qapi/visitor.h"
+
+/*
+ * XICS Common class - parent for emulated XICS and KVM-XICS
+ */
+
+static void xics_common_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    ICPState *ss = &icp->ss[cs->cpu_index];
+
+    assert(cs->cpu_index < icp->nr_servers);
+
+    switch (PPC_INPUT(env)) {
+    case PPC_FLAGS_INPUT_POWER7:
+        ss->output = env->irq_inputs[POWER7_INPUT_INT];
+        break;
+
+    case PPC_FLAGS_INPUT_970:
+        ss->output = env->irq_inputs[PPC970_INPUT_INT];
+        break;
+
+    default:
+        error_report("XICS interrupt controller does not support this CPU "
+                "bus model");
+        abort();
+    }
+}
+
+void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
+                           void *opaque, const char *name, struct Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    assert(info->set_nr_irqs);
+    assert(!icp->nr_irqs);
+    assert(!icp->ics);
+    info->set_nr_irqs(icp, value);
+}
+
+void xics_prop_set_nr_servers(Object *obj, struct Visitor *v,
+                              void *opaque, const char *name, struct Error **errp)
+{
+    XICSState *icp = XICS_COMMON(obj);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    assert(info->set_nr_servers);
+    assert(!icp->nr_servers);
+    info->set_nr_servers(icp, value);
+}
+
+static void xics_common_initfn(Object *obj)
+{
+    object_property_add(obj, "nr_irqs", "int",
+                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
+    object_property_add(obj, "nr_servers", "int",
+                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);
+}
+
+static void xics_common_reset(DeviceState *d)
+{
+    XICSState *icp = XICS_COMMON(d);
+    int i;
+
+    for (i = 0; i < icp->nr_servers; i++) {
+        device_reset(DEVICE(&icp->ss[i]));
+    }
+
+    device_reset(DEVICE(icp->ics));
+}
+
+static void xics_common_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
+
+    dc->reset = xics_common_reset;
+    xsc->cpu_setup = xics_common_cpu_setup;
+}
+
+static const TypeInfo xics_common_info = {
+    .name          = TYPE_XICS_COMMON,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(XICSState),
+    .class_size    = sizeof(XICSStateClass),
+    .instance_init = xics_common_initfn,
+    .class_init    = xics_common_class_init,
+};
 
 /*
  * ICP: Presentation layer
@@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = {
     },
 };
 
+static void ics_initfn(Object *obj)
+{
+    ICSState *ics = ICS(obj);
+
+    ics->offset = XICS_IRQ_BASE;
+}
+
 static int ics_realize(DeviceState *dev)
 {
     ICSState *ics = ICS(dev);
@@ -472,6 +585,7 @@ static const TypeInfo ics_info = {
     .instance_size = sizeof(ICSState),
     .class_init = ics_class_init,
     .class_size = sizeof(ICSStateClass),
+    .instance_init = ics_initfn,
 };
 
 /*
@@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 /*
  * XICS
  */
+void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
+{
+    icp->ics = ICS(object_new(TYPE_ICS));
+    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
+    icp->ics->icp = icp;
+    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
+}
 
-static void xics_reset(DeviceState *d)
+void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers)
 {
-    XICSState *icp = XICS(d);
     int i;
 
+    icp->nr_servers = nr_servers;
+
+    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
-        device_reset(DEVICE(&icp->ss[i]));
-    }
-
-    device_reset(DEVICE(icp->ics));
-}
-
-static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
-{
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    ICPState *ss = &icp->ss[cs->cpu_index];
-
-    assert(cs->cpu_index < icp->nr_servers);
-
-    switch (PPC_INPUT(env)) {
-    case PPC_FLAGS_INPUT_POWER7:
-        ss->output = env->irq_inputs[POWER7_INPUT_INT];
-        break;
-
-    case PPC_FLAGS_INPUT_970:
-        ss->output = env->irq_inputs[PPC970_INPUT_INT];
-        break;
-
-    default:
-        error_report("XICS interrupt controller does not support this CPU "
-                "bus model");
-        abort();
+        char buffer[32];
+        object_initialize(&icp->ss[i], TYPE_ICP);
+        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
+        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
     }
 }
 
 static void xics_realize(DeviceState *dev, Error **errp)
 {
     XICSState *icp = XICS(dev);
-    ICSState *ics = icp->ics;
     Error *error = NULL;
     int i;
 
@@ -706,9 +805,6 @@ static void xics_realize(DeviceState *dev, Error **errp)
     spapr_register_hypercall(H_XIRR, h_xirr);
     spapr_register_hypercall(H_EOI, h_eoi);
 
-    ics->nr_irqs = icp->nr_irqs;
-    ics->offset = XICS_IRQ_BASE;
-    ics->icp = icp;
     object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
     if (error) {
         error_propagate(errp, error);
@@ -716,12 +812,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
     }
 
     assert(icp->nr_servers);
-    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
     for (i = 0; i < icp->nr_servers; i++) {
-        char buffer[32];
-        object_initialize(&icp->ss[i], TYPE_ICP);
-        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
-        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
         object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
         if (error) {
             error_propagate(errp, error);
@@ -730,42 +821,27 @@ static void xics_realize(DeviceState *dev, Error **errp)
     }
 }
 
-static void xics_initfn(Object *obj)
-{
-    XICSState *xics = XICS(obj);
-
-    xics->ics = ICS(object_new(TYPE_ICS));
-    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
-}
-
-static Property xics_properties[] = {
-    DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
-    DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void xics_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     XICSStateClass *xsc = XICS_CLASS(oc);
 
     dc->realize = xics_realize;
-    dc->props = xics_properties;
-    dc->reset = xics_reset;
-    xsc->cpu_setup = xics_cpu_setup;
+    xsc->set_nr_irqs = xics_set_nr_irqs;
+    xsc->set_nr_servers = xics_set_nr_servers;
 }
 
 static const TypeInfo xics_info = {
     .name          = TYPE_XICS,
-    .parent        = TYPE_SYS_BUS_DEVICE,
+    .parent        = TYPE_XICS_COMMON,
     .instance_size = sizeof(XICSState),
     .class_size = sizeof(XICSStateClass),
     .class_init    = xics_class_init,
-    .instance_init = xics_initfn,
 };
 
 static void xics_register_types(void)
 {
+    type_register_static(&xics_common_info);
     type_register_static(&xics_info);
     type_register_static(&ics_info);
     type_register_static(&icp_info);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 90ecaf8..1066f69 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -29,11 +29,18 @@
 
 #include "hw/sysbus.h"
 
+#define TYPE_XICS_COMMON "xics-common"
+#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMMON)
+
 #define TYPE_XICS "xics"
 #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
 
+#define XICS_COMMON_CLASS(klass) \
+     OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
 #define XICS_CLASS(klass) \
      OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
+#define XICS_COMMON_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS_COMMON)
 #define XICS_GET_CLASS(obj) \
      OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
 
@@ -58,6 +65,8 @@ struct XICSStateClass {
     DeviceClass parent_class;
 
     void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
+    void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs);
+    void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers);
 };
 
 struct XICSState {
@@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
 
 static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
-    XICSStateClass *info = XICS_GET_CLASS(icp);
+    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
 
     assert(info->cpu_setup);
     info->cpu_setup(icp, cpu);
-- 
1.8.3.2

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

[Andreas Färber]

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

This looks really good, except for error handling and introspection
support - minor nits.

> ---
>  hw/intc/xics.c        | 190 +++++++++++++++++++++++++++++++++++---------------
>  include/hw/ppc/xics.h |  11 ++-
>  2 files changed, 143 insertions(+), 58 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 20840e3..95865aa 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -30,6 +30,112 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
>  #include "qemu/error-report.h"
> +#include "qapi/visitor.h"
> +
> +/*
> + * XICS Common class - parent for emulated XICS and KVM-XICS
> + */
> +
> +static void xics_common_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    ICPState *ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> +
> +    case PPC_FLAGS_INPUT_970:
> +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> +
> +    default:
> +        error_report("XICS interrupt controller does not support this CPU "
> +                "bus model");

Indentation is off.

> +        abort();

I previously wondered whether it may make sense to add Error **errp
argument to avoid the abort, but this is only called from the machine
init, right?

> +    }
> +}
> +
> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
> +                           void *opaque, const char *name, struct Error **errp)

You should be able to drop both "struct"s.

> +{
> +    XICSState *icp = XICS_COMMON(obj);
> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    assert(info->set_nr_irqs);

> +    assert(!icp->nr_irqs);

For reasons of simplicity you're only implementing these as one-off
setters. I think that's acceptable, but since someone can easily try to
set this property via QMP qom-set you should do error_setg() rather than
assert() then. Asserting the class methods is fine as they are not
user-changeable.

> +    assert(!icp->ics);
> +    info->set_nr_irqs(icp, value);
> +}
> +
> +void xics_prop_set_nr_servers(Object *obj, struct Visitor *v,
> +                              void *opaque, const char *name, struct Error **errp)
> +{
> +    XICSState *icp = XICS_COMMON(obj);
> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
> +    Error *error = NULL;
> +    int64_t value;
> +
> +    visit_type_int(v, &value, name, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    assert(info->set_nr_servers);

> +    assert(!icp->nr_servers);

Ditto.

> +    info->set_nr_servers(icp, value);
> +}
> +
> +static void xics_common_initfn(Object *obj)
> +{
> +    object_property_add(obj, "nr_irqs", "int",
> +                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
> +    object_property_add(obj, "nr_servers", "int",
> +                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);

Since this property is visible in the QOM tree, it would be nice to
implement trivial getters returns the value from the state fields.

> +}
> +
> +static void xics_common_reset(DeviceState *d)
> +{
> +    XICSState *icp = XICS_COMMON(d);
> +    int i;
> +
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        device_reset(DEVICE(&icp->ss[i]));
> +    }
> +
> +    device_reset(DEVICE(icp->ics));
> +}
> +
> +static void xics_common_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
> +
> +    dc->reset = xics_common_reset;
> +    xsc->cpu_setup = xics_common_cpu_setup;
> +}
> +
> +static const TypeInfo xics_common_info = {
> +    .name          = TYPE_XICS_COMMON,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(XICSState),
> +    .class_size    = sizeof(XICSStateClass),
> +    .instance_init = xics_common_initfn,
> +    .class_init    = xics_common_class_init,
> +};
>  
>  /*
>   * ICP: Presentation layer
> @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = {
>      },
>  };
>  
> +static void ics_initfn(Object *obj)
> +{
> +    ICSState *ics = ICS(obj);
> +
> +    ics->offset = XICS_IRQ_BASE;
> +}
> +
>  static int ics_realize(DeviceState *dev)
>  {
>      ICSState *ics = ICS(dev);
> @@ -472,6 +585,7 @@ static const TypeInfo ics_info = {
>      .instance_size = sizeof(ICSState),
>      .class_init = ics_class_init,
>      .class_size = sizeof(ICSStateClass),
> +    .instance_init = ics_initfn,
>  };
>  
>  /*
> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>  /*
>   * XICS
>   */
> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
> +{
> +    icp->ics = ICS(object_new(TYPE_ICS));
> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);

Why create this single object in the setter? Can't you just create this
in the common type's instance_init similar to before but using
object_initialize() instead of object_new() and
object_property_set_bool() in the realizefn?

NULL above also shows that your callback should probably get an Error
**errp argument to propagate any errors to the property and its callers.

Common split-off, setters and hooks look good otherwise.

Thanks,
Andreas

> +    icp->ics->icp = icp;
> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
> +}
>  
> -static void xics_reset(DeviceState *d)
> +void xics_set_nr_servers(XICSState *icp, uint32_t nr_servers)
>  {
> -    XICSState *icp = XICS(d);
>      int i;
>  
> +    icp->nr_servers = nr_servers;
> +
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>      for (i = 0; i < icp->nr_servers; i++) {
> -        device_reset(DEVICE(&icp->ss[i]));
> -    }
> -
> -    device_reset(DEVICE(icp->ics));
> -}
> -
> -static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    ICPState *ss = &icp->ss[cs->cpu_index];
> -
> -    assert(cs->cpu_index < icp->nr_servers);
> -
> -    switch (PPC_INPUT(env)) {
> -    case PPC_FLAGS_INPUT_POWER7:
> -        ss->output = env->irq_inputs[POWER7_INPUT_INT];
> -        break;
> -
> -    case PPC_FLAGS_INPUT_970:
> -        ss->output = env->irq_inputs[PPC970_INPUT_INT];
> -        break;
> -
> -    default:
> -        error_report("XICS interrupt controller does not support this CPU "
> -                "bus model");
> -        abort();
> +        char buffer[32];
> +        object_initialize(&icp->ss[i], TYPE_ICP);
> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>      }
>  }
>  
>  static void xics_realize(DeviceState *dev, Error **errp)
>  {
>      XICSState *icp = XICS(dev);
> -    ICSState *ics = icp->ics;
>      Error *error = NULL;
>      int i;
>  
> @@ -706,9 +805,6 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      spapr_register_hypercall(H_XIRR, h_xirr);
>      spapr_register_hypercall(H_EOI, h_eoi);
>  
> -    ics->nr_irqs = icp->nr_irqs;
> -    ics->offset = XICS_IRQ_BASE;
> -    ics->icp = icp;
>      object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
>      if (error) {
>          error_propagate(errp, error);
> @@ -716,12 +812,7 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      }
>  
>      assert(icp->nr_servers);
> -    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>      for (i = 0; i < icp->nr_servers; i++) {
> -        char buffer[32];
> -        object_initialize(&icp->ss[i], TYPE_ICP);
> -        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> -        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>          object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
>          if (error) {
>              error_propagate(errp, error);
> @@ -730,42 +821,27 @@ static void xics_realize(DeviceState *dev, Error **errp)
>      }
>  }
>  
> -static void xics_initfn(Object *obj)
> -{
> -    XICSState *xics = XICS(obj);
> -
> -    xics->ics = ICS(object_new(TYPE_ICS));
> -    object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> -}
> -
> -static Property xics_properties[] = {
> -    DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
> -    DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void xics_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      XICSStateClass *xsc = XICS_CLASS(oc);
>  
>      dc->realize = xics_realize;
> -    dc->props = xics_properties;
> -    dc->reset = xics_reset;
> -    xsc->cpu_setup = xics_cpu_setup;
> +    xsc->set_nr_irqs = xics_set_nr_irqs;
> +    xsc->set_nr_servers = xics_set_nr_servers;
>  }
>  
>  static const TypeInfo xics_info = {
>      .name          = TYPE_XICS,
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .parent        = TYPE_XICS_COMMON,
>      .instance_size = sizeof(XICSState),
>      .class_size = sizeof(XICSStateClass),
>      .class_init    = xics_class_init,
> -    .instance_init = xics_initfn,
>  };
>  
>  static void xics_register_types(void)
>  {
> +    type_register_static(&xics_common_info);
>      type_register_static(&xics_info);
>      type_register_static(&ics_info);
>      type_register_static(&icp_info);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 90ecaf8..1066f69 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -29,11 +29,18 @@
>  
>  #include "hw/sysbus.h"
>  
> +#define TYPE_XICS_COMMON "xics-common"
> +#define XICS_COMMON(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS_COMMON)
> +
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define XICS_COMMON_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>  #define XICS_CLASS(klass) \
>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
> +#define XICS_COMMON_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS_COMMON)
>  #define XICS_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>  
> @@ -58,6 +65,8 @@ struct XICSStateClass {
>      DeviceClass parent_class;
>  
>      void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
> +    void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs);
> +    void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers);
>  };
>  
>  struct XICSState {
> @@ -138,7 +147,7 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
>  
>  static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>  {
> -    XICSStateClass *info = XICS_GET_CLASS(icp);
> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
>  
>      assert(info->cpu_setup);
>      info->cpu_setup(icp, cpu);
> 


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

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

[Alexey Kardashevskiy]

On 08/06/2013 07:53 PM, Andreas Färber wrote:
> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
>> The upcoming XICS-KVM support will use bits of emulated XICS code.
>> So this introduces new level of hierarchy - "xics-common" class. Both
>> emulated XICS and XICS-KVM will inherit from it and override class
>> callbacks when required.
>>
>> The new "xics-common" class implements:
>> 1. replaces static "nr_irqs" and "nr_servers" properties with
>> the dynamic ones and adds callbacks to be executed when properties
>> are set.
>> 2. xics_cpu_setup() callback renamed to xics_common_cpu_setup() as
>> it is a common part for both XICS'es
>> 3. xics_reset() renamed to xics_common_reset() for the same reason.
>>
>> The emulated XICS changes:
>> 1. instance_init() callback is replaced with "nr_irqs" property callback
>> as it creates ICS which needs the nr_irqs property set.
>> 2. the part of xics_realize() which creates ICPs is moved to
>> the "nr_servers" property callback as realize() is too late to
>> create/initialize devices and instance_init() is too early to create
>> devices as the number of child devices comes via the "nr_servers"
>> property.
>> 3. added ics_initfn() which does a little part of what xics_realize() did.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> This looks really good, except for error handling and introspection
> support - minor nits.
> 
>> ---
>>  hw/intc/xics.c        | 190 +++++++++++++++++++++++++++++++++++---------------
>>  include/hw/ppc/xics.h |  11 ++-
>>  2 files changed, 143 insertions(+), 58 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 20840e3..95865aa 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -30,6 +30,112 @@
>>  #include "hw/ppc/spapr.h"
>>  #include "hw/ppc/xics.h"
>>  #include "qemu/error-report.h"
>> +#include "qapi/visitor.h"
>> +
>> +/*
>> + * XICS Common class - parent for emulated XICS and KVM-XICS
>> + */
>> +
>> +static void xics_common_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    ICPState *ss = &icp->ss[cs->cpu_index];
>> +
>> +    assert(cs->cpu_index < icp->nr_servers);
>> +
>> +    switch (PPC_INPUT(env)) {
>> +    case PPC_FLAGS_INPUT_POWER7:
>> +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
>> +        break;
>> +
>> +    case PPC_FLAGS_INPUT_970:
>> +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
>> +        break;
>> +
>> +    default:
>> +        error_report("XICS interrupt controller does not support this CPU "
>> +                "bus model");
> 
> Indentation is off.
> 
>> +        abort();
> 
> I previously wondered whether it may make sense to add Error **errp
> argument to avoid the abort, but this is only called from the machine
> init, right?


Right. If it fails, nothing for the caller to decide.


>> +    }
>> +}
>> +
>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
>> +                           void *opaque, const char *name, struct Error **errp)
> 
> You should be able to drop both "struct"s.

Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined.

>> +{
>> +    XICSState *icp = XICS_COMMON(obj);
>> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
>> +    Error *error = NULL;
>> +    int64_t value;
>> +
>> +    visit_type_int(v, &value, name, &error);
>> +    if (error) {
>> +        error_propagate(errp, error);
>> +        return;
>> +    }
>> +
>> +    assert(info->set_nr_irqs);
> 
>> +    assert(!icp->nr_irqs);
> 
> For reasons of simplicity you're only implementing these as one-off
> setters. I think that's acceptable, but since someone can easily try to
> set this property via QMP qom-set you should do error_setg() rather than
> assert() then. Asserting the class methods is fine as they are not
> user-changeable.
>
>> +    assert(!icp->ics);
>> +    info->set_nr_irqs(icp, value);
>> +}
>> +
>> +void xics_prop_set_nr_servers(Object *obj, struct Visitor *v,
>> +                              void *opaque, const char *name, struct Error **errp)
>> +{
>> +    XICSState *icp = XICS_COMMON(obj);
>> +    XICSStateClass *info = XICS_COMMON_GET_CLASS(icp);
>> +    Error *error = NULL;
>> +    int64_t value;
>> +
>> +    visit_type_int(v, &value, name, &error);
>> +    if (error) {
>> +        error_propagate(errp, error);
>> +        return;
>> +    }
>> +
>> +    assert(info->set_nr_servers);
> 
>> +    assert(!icp->nr_servers);
> 
> Ditto.
> 
>> +    info->set_nr_servers(icp, value);
>> +}
>> +
>> +static void xics_common_initfn(Object *obj)
>> +{
>> +    object_property_add(obj, "nr_irqs", "int",
>> +                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
>> +    object_property_add(obj, "nr_servers", "int",
>> +                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);
> 
> Since this property is visible in the QOM tree, it would be nice to
> implement trivial getters returns the value from the state fields.

Added. How do I test it? "info qtree" prints only
DEVICE_CLASS(class)->props which is null in this case.


>> +}
>> +
>> +static void xics_common_reset(DeviceState *d)
>> +{
>> +    XICSState *icp = XICS_COMMON(d);
>> +    int i;
>> +
>> +    for (i = 0; i < icp->nr_servers; i++) {
>> +        device_reset(DEVICE(&icp->ss[i]));
>> +    }
>> +
>> +    device_reset(DEVICE(icp->ics));
>> +}
>> +
>> +static void xics_common_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
>> +
>> +    dc->reset = xics_common_reset;
>> +    xsc->cpu_setup = xics_common_cpu_setup;
>> +}
>> +
>> +static const TypeInfo xics_common_info = {
>> +    .name          = TYPE_XICS_COMMON,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(XICSState),
>> +    .class_size    = sizeof(XICSStateClass),
>> +    .instance_init = xics_common_initfn,
>> +    .class_init    = xics_common_class_init,
>> +};
>>  
>>  /*
>>   * ICP: Presentation layer
>> @@ -443,6 +549,13 @@ static const VMStateDescription vmstate_ics = {
>>      },
>>  };
>>  
>> +static void ics_initfn(Object *obj)
>> +{
>> +    ICSState *ics = ICS(obj);
>> +
>> +    ics->offset = XICS_IRQ_BASE;
>> +}
>> +
>>  static int ics_realize(DeviceState *dev)
>>  {
>>      ICSState *ics = ICS(dev);
>> @@ -472,6 +585,7 @@ static const TypeInfo ics_info = {
>>      .instance_size = sizeof(ICSState),
>>      .class_init = ics_class_init,
>>      .class_size = sizeof(ICSStateClass),
>> +    .instance_init = ics_initfn,
>>  };
>>  
>>  /*
>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>  /*
>>   * XICS
>>   */
>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>> +{
>> +    icp->ics = ICS(object_new(TYPE_ICS));
>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> 
> Why create this single object in the setter? Can't you just create this
> in the common type's instance_init similar to before but using
> object_initialize() instead of object_new() and
> object_property_set_bool() in the realizefn?

I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM
(oops, KVM is at the end, will fix it) types.

And I would really want not to create it in instance_init() as I want to
have the object created and all its properties initialized in one place.


> NULL above also shows that your callback should probably get an Error
> **errp argument to propagate any errors to the property and its callers.

Yep, will fix that.



-- 
Alexey

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

[Andreas Färber]

Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
> On 08/06/2013 07:53 PM, Andreas Färber wrote:
>> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
>>> +    }
>>> +}
>>> +
>>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
>>> +                           void *opaque, const char *name, struct Error **errp)
>>
>> You should be able to drop both "struct"s.
> 
> Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined.

Yeah, reason is some circular header dependencies. ;)

>>> +static void xics_common_initfn(Object *obj)
>>> +{
>>> +    object_property_add(obj, "nr_irqs", "int",
>>> +                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
>>> +    object_property_add(obj, "nr_servers", "int",
>>> +                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);
>>
>> Since this property is visible in the QOM tree, it would be nice to
>> implement trivial getters returns the value from the state fields.
> 
> Added. How do I test it?

./QMP/qom-list to find the path, if not fixed in code yet, and
./QMP/qom-get path.nr_servers to retrieve the value,
./QMP/qom-set path.nr_servers to verify it doesn't kill the process.

-qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side.

> "info qtree" prints only
> DEVICE_CLASS(class)->props which is null in this case.

True, info qtree is from qdev times and deprecated. I recently attached
a "qom-tree" script doing the equivalent that you could try.

I'll try to address -device xics,? showing them for 1.7. (Anthony
indicated on a KVM call that the expected way to discover properties was
to instantiate the type rather than looking at its class. Requires that
types are instantiatable without asserting KVM accelerator, which gets
initialized later.)

>>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>  /*
>>>   * XICS
>>>   */
>>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>>> +{
>>> +    icp->ics = ICS(object_new(TYPE_ICS));
>>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>>
>> Why create this single object in the setter? Can't you just create this
>> in the common type's instance_init similar to before but using
>> object_initialize() instead of object_new() and
>> object_property_set_bool() in the realizefn?
> 
> I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM
> (oops, KVM is at the end, will fix it) types.
> 
> And I would really want not to create it in instance_init() as I want to
> have the object created and all its properties initialized in one place.

Doing it in instance_init facilitates improving the setter to allow
multiple uses as a follow-up patch, since it must only be created once.
If you don't, then the child will not be present in the composition tree
before setting this seemingly unrelated property. That seems worse to me
than accessing a field in two places - instance_init will be run before
any property setter.

BTW these dynamic setters do not check automatically whether the object
has already been realized. If you want the setter to return an Error in
that case, you will need to check for DeviceState *dev = DEVICE(icp);
dev->realized yourself. Not sure if that's the semantics you desire, but
I guess so?

Andreas

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

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

[Alexey Kardashevskiy]

On 08/07/2013 05:03 PM, Andreas Färber wrote:
> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
>> On 08/06/2013 07:53 PM, Andreas Färber wrote:
>>> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
>>>> +    }
>>>> +}
>>>> +
>>>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
>>>> +                           void *opaque, const char *name, struct Error **errp)
>>>
>>> You should be able to drop both "struct"s.
>>
>> Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined.
> 
> Yeah, reason is some circular header dependencies. ;)
> 
>>>> +static void xics_common_initfn(Object *obj)
>>>> +{
>>>> +    object_property_add(obj, "nr_irqs", "int",
>>>> +                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
>>>> +    object_property_add(obj, "nr_servers", "int",
>>>> +                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);
>>>
>>> Since this property is visible in the QOM tree, it would be nice to
>>> implement trivial getters returns the value from the state fields.
>>
>> Added. How do I test it?
> 
> ./QMP/qom-list to find the path, if not fixed in code yet, and

Something is wrong. XICS does not have an id but it should not be a
problem, right (btw how to set it from the code?)?

[aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock
/
[aik@dyn232 ~]$

This is qtree:

(qemu) info qtree
bus: main-system-bus
  type System
  dev: spapr-pci-host-bridge, id ""
    index = 0
    buid = 0x800000020000000
    liobn = 0x80000000
    mem_win_addr = 0x100a0000000
    mem_win_size = 0x20000000
    io_win_addr = 0x10080000000
    io_win_size = 0x10000
    msi_win_addr = 0x10090000000
    irq 0
    bus: pci
      type PCI
  dev: spapr-vio-bridge, id ""
    irq 0
    bus: spapr-vio
      type spapr-vio-bus
      dev: spapr-vty, id "ser1"
        reg = 1895825409
        chardev = char1
        irq = 4102
      dev: spapr-nvram, id "nvram@71000000"
        reg = 1895825408
        drive = <null>
        irq = 4097
  dev: xics-kvm, id ""
    irq 0


> ./QMP/qom-get path.nr_servers to retrieve the value,
> ./QMP/qom-set path.nr_servers to verify it doesn't kill the process.
> 
> -qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side.




>> "info qtree" prints only
>> DEVICE_CLASS(class)->props which is null in this case.
> 
> True, info qtree is from qdev times and deprecated. I recently attached
> a "qom-tree" script doing the equivalent that you could try.
> 
> I'll try to address -device xics,? showing them for 1.7. (Anthony
> indicated on a KVM call that the expected way to discover properties was
> to instantiate the type rather than looking at its class. Requires that
> types are instantiatable without asserting KVM accelerator, which gets
> initialized later.)
> 
>>>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>>  /*
>>>>   * XICS
>>>>   */
>>>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>>>> +{
>>>> +    icp->ics = ICS(object_new(TYPE_ICS));
>>>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>>>
>>> Why create this single object in the setter? Can't you just create this
>>> in the common type's instance_init similar to before but using
>>> object_initialize() instead of object_new() and
>>> object_property_set_bool() in the realizefn?
>>
>> I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM
>> (oops, KVM is at the end, will fix it) types.
>>
>> And I would really want not to create it in instance_init() as I want to
>> have the object created and all its properties initialized in one place.
> 
> Doing it in instance_init facilitates improving the setter to allow
> multiple uses as a follow-up patch, since it must only be created once.
> If you don't, then the child will not be present in the composition tree
> before setting this seemingly unrelated property.

It will still be happening to ICP's and cannot be avoided. Why to make it
different for ICS and ICP? No IRQs number effectively means "no IRQ source".


> That seems worse to me
> than accessing a field in two places - instance_init will be run before
> any property setter.


> BTW these dynamic setters do not check automatically whether the object
> has already been realized. If you want the setter to return an Error in
> that case, you will need to check for DeviceState *dev = DEVICE(icp);
> dev->realized yourself. Not sure if that's the semantics you desire, but
> I guess so?


realize() will fail if a property is not set. Setters will fail if a
property is already set. By fail I mean that Error** thingy, not abort().
Do not really see why would we need something more here.



-- 
Alexey

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

[Andreas Färber]

Am 07.08.2013 09:26, schrieb Alexey Kardashevskiy:
> On 08/07/2013 05:03 PM, Andreas Färber wrote:
>> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
>>> [...] How do I test it?
>>
>> ./QMP/qom-list to find the path, if not fixed in code yet, and
> 
> Something is wrong. XICS does not have an id but it should not be a
> problem, right (btw how to set it from the code?)?
> 
> [aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock
> /

That's expected for lack of argument.

$ ./QMP/qom-list -s ./qmp-sock /machine
unattached/
peripheral/
peripheral-anon/
type

This confirms "path ... not fixed in code yet" (otherwise there would've
been an "spapr/" entry or anything else telling.

So you need to look through /machine/unassigned for the device[n] with
qom-get /machine/unassigned/device[n].type matching your type, possibly
device[0] as in my case. From there on you see your icp[0]/ and ics/
children without searching the unassigned list again.

Or simply use my qom-tree script:
http://lists.nongnu.org/archive/html/qemu-devel/2013-07/txte1WIbWzu5Z.txt

Setting a canonical path is done via object_property_add_child() after
instantiating and before realizing a device.
For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35"
for q35. Suggest to discuss with Anthony how to structure the
composition tree for sPAPR.

Andreas

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

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

[Alexey Kardashevskiy]

On 08/08/2013 12:22 AM, Andreas Färber wrote:
> Am 07.08.2013 09:26, schrieb Alexey Kardashevskiy:
>> On 08/07/2013 05:03 PM, Andreas Färber wrote:
>>> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
>>>> [...] How do I test it?
>>>
>>> ./QMP/qom-list to find the path, if not fixed in code yet, and
>>
>> Something is wrong. XICS does not have an id but it should not be a
>> problem, right (btw how to set it from the code?)?
>>
>> [aik@dyn232 ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock
>> /
> 
> That's expected for lack of argument.
> 
> $ ./QMP/qom-list -s ./qmp-sock /machine
> unattached/
> peripheral/
> peripheral-anon/
> type
> 
> This confirms "path ... not fixed in code yet" (otherwise there would've
> been an "spapr/" entry or anything else telling.
> 
> So you need to look through /machine/unassigned for the device[n] with
> qom-get /machine/unassigned/device[n].type matching your type, possibly
> device[0] as in my case. From there on you see your icp[0]/ and ics/
> children without searching the unassigned list again.
> 
> Or simply use my qom-tree script:
> http://lists.nongnu.org/archive/html/qemu-devel/2013-07/txte1WIbWzu5Z.txt

Yep. That works, thanks.

> Setting a canonical path is done via object_property_add_child() after
> instantiating and before realizing a device.
> For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35"
> for q35. Suggest to discuss with Anthony how to structure the
> composition tree for sPAPR.

I tried inserting this:
object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NULL);

in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is what
i440fx_init() does in the very beginning) but when I do that, spapr_phb
already has a parent of a "container" type so assert happens.

What is the aim of all of this? Is it not to have "unattached" in a path
starting with /machine?


btw I have added notes in the previous response against splitting ICS
initialization into 2 functions and you skipped it. Does this mean you do
not want to waste more of your time persuading me and this is the real
stopped or you just gave up? :) Thanks.


-- 
Alexey

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

[Andreas Färber]

Am 08.08.2013 05:10, schrieb Alexey Kardashevskiy:
> On 08/08/2013 12:22 AM, Andreas Färber wrote:
>> Setting a canonical path is done via object_property_add_child() after
>> instantiating and before realizing a device.
>> For x86 the northbridge is used as name, i.e. "i440fx" for pc and "q35"
>> for q35. Suggest to discuss with Anthony how to structure the
>> composition tree for sPAPR.
> 
> I tried inserting this:
> object_property_add_child(qdev_get_machine(), busname, OBJECT(dev), NULL);
> 
> in TYPE_SPAPR_PCI_HOST_BRIDGE's spapr_phb_init() (I thought this is what
> i440fx_init() does in the very beginning) but when I do that, spapr_phb
> already has a parent of a "container" type so assert happens.

See above: After instantiating (instance_init) and before realizing
(realize/init). It needs to be done by the parent, not by the device
itself - unless I'm completely misunderstanding what you're trying, in
absence of code to look at.

> What is the aim of all of this? Is it not to have "unattached" in a path
> starting with /machine?

Correct. To have a canonical path for management tools, qtest, etc.
similar to /sys filesystem in Linux.

> btw I have added notes in the previous response against splitting ICS
> initialization into 2 functions and you skipped it. Does this mean you do
> not want to waste more of your time persuading me and this is the real
> stopped or you just gave up? :) Thanks.

"Never give up, never surrender!" :P

I believe I had already answered to that: I have no clue what ICS, ICP,
XICS acronyms actually stand for, but I explained friendly and verbose
why doing QOM things the way I asked you to makes sense. Ultimately I
was told by Anthony when and how to do these things, and I do not have
all day to argue with you, so if you don't like my feedback then please
discuss with your IBM colleague directly.

There is by definition a functional split between instance_init and
realize, not my invention, and management tools should be able to tweak
properties of objects, such as you setting nr_servers to 1 on machine
init and QMP qom-set bumping it to 2. It is IMO not essential to
implement that from the start because having a performant interrupt
implementation is more important than our QOM'ish refactorings,
therefore I said error_setg() should be okay, but my feedback is
targeted at keeping our design future-proof, which means that
instantiation of a single object that was in instance_init before should
not be moved into a property setter that may be called multiple times
because it would then need to be moved back anyway.

Use of object_initialize() was requested by Anthony to have the QOM
composition reflected in the allocation model, i.e. in this case
g_try_malloc0(sizeof(XICSState)) including the ICS(?) thingy.
So since it was created once independent of properties before your
patch, I assume we should keep that behavior, while only changing the
storage location.

Still a whole week left for cleaning up 1.7 patches! :)

Andreas

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

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

[Alexey Kardashevskiy]

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

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

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

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 default-configs/ppc64-softmmu.mak |   1 +
 hw/intc/Makefile.objs             |   1 +
 hw/intc/xics_kvm.c                | 479 ++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c                    |  23 +-
 include/hw/ppc/xics.h             |  13 ++
 5 files changed, 515 insertions(+), 2 deletions(-)
 create mode 100644 hw/intc/xics_kvm.c

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

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

[Andreas Färber]

Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  default-configs/ppc64-softmmu.mak |   1 +
>  hw/intc/Makefile.objs             |   1 +
>  hw/intc/xics_kvm.c                | 479 ++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr.c                    |  23 +-
>  include/hw/ppc/xics.h             |  13 ++
>  5 files changed, 515 insertions(+), 2 deletions(-)
>  create mode 100644 hw/intc/xics_kvm.c
> 
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 7831c2b..116f4ca 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -47,6 +47,7 @@ CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For pSeries
>  CONFIG_XICS=$(CONFIG_PSERIES)
> +CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 2851eed..47ac442 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
>  obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
>  obj-$(CONFIG_SH4) += sh_intc.o
>  obj-$(CONFIG_XICS) += xics.o
> +obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..d9313d4
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
> @@ -0,0 +1,479 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
> + *
> + * Copyright (c) 2013 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "trace.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +
> +#include <sys/ioctl.h>
> +
> +typedef struct KVMXICSState {
> +    XICSState parent_obj;
> +
> +    uint32_t set_xive_token;
> +    uint32_t get_xive_token;
> +    uint32_t int_off_token;
> +    uint32_t int_on_token;
> +    int kernel_xics_fd;
> +} KVMXICSState;
> +
> +/*
> + * ICP-KVM
> + */
> +static void icp_get_kvm_state(ICPState *ss)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return;
> +    }
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        error_report("Unable to retrieve KVM interrupt controller state"
> +                " for CPU %d: %s", ss->cs->cpu_index, strerror(errno));
> +        exit(1);
> +    }
> +
> +    ss->xirr = state >> KVM_REG_PPC_ICP_XISR_SHIFT;
> +    ss->mfrr = (state >> KVM_REG_PPC_ICP_MFRR_SHIFT)
> +        & KVM_REG_PPC_ICP_MFRR_MASK;
> +    ss->pending_priority = (state >> KVM_REG_PPC_ICP_PPRI_SHIFT)
> +        & KVM_REG_PPC_ICP_PPRI_MASK;
> +}
> +
> +static int icp_set_kvm_state(ICPState *ss)
> +{
> +    uint64_t state;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_PPC_ICP_STATE,
> +        .addr = (uintptr_t)&state,
> +    };
> +    int ret;
> +
> +    /* ICP for this CPU thread is not in use, exiting */
> +    if (!ss->cs) {
> +        return 0;
> +    }
> +
> +    state = ((uint64_t)ss->xirr << KVM_REG_PPC_ICP_XISR_SHIFT)
> +        | ((uint64_t)ss->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
> +        | ((uint64_t)ss->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
> +
> +    ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, &reg);
> +    if (ret != 0) {
> +        error_report("Unable to restore KVM interrupt controller state (0x%"
> +                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static void icp_kvm_reset(DeviceState *dev)
> +{
> +    ICPState *icp = ICP(dev);
> +
> +    icp->xirr = 0;
> +    icp->pending_priority = 0xff;
> +    icp->mfrr = 0xff;
> +
> +    /* Make all outputs are deasserted */
> +    qemu_set_irq(icp->output, 0);
> +
> +    icp_set_kvm_state(icp);
> +}
> +
> +static void icp_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICPStateClass *icpc = ICP_CLASS(klass);
> +
> +    dc->reset = icp_kvm_reset;
> +    icpc->pre_save = icp_get_kvm_state;
> +    icpc->post_load = icp_set_kvm_state;
> +}
> +
> +static const TypeInfo icp_kvm_info = {
> +    .name = TYPE_ICP_KVM,
> +    .parent = TYPE_ICP,
> +    .instance_size = sizeof(ICPState),
> +    .class_init = icp_kvm_class_init,
> +    .class_size = sizeof(ICPStateClass),
> +};
> +
> +/*
> + * ICS-KVM
> + */
> +static void ics_get_kvm_state(ICSState *ics)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to retrieve KVM interrupt controller state"
> +                    " for IRQ %d: %s", i + ics->offset, strerror(errno));
> +            exit(1);
> +        }
> +
> +        irq->server = state & KVM_XICS_DESTINATION_MASK;
> +        irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
> +            & KVM_XICS_PRIORITY_MASK;
> +        /*
> +         * To be consistent with the software emulation in xics.c, we
> +         * split out the masked state + priority that we get from the
> +         * kernel into 'current priority' (0xff if masked) and
> +         * 'saved priority' (if masked, this is the priority the
> +         * interrupt had before it was masked).  Masking and unmasking
> +         * are done with the ibm,int-off and ibm,int-on RTAS calls.
> +         */
> +        if (state & KVM_XICS_MASKED) {
> +            irq->priority = 0xff;
> +        } else {
> +            irq->priority = irq->saved_priority;
> +        }
> +
> +        if (state & KVM_XICS_PENDING) {
> +            if (state & KVM_XICS_LEVEL_SENSITIVE) {
> +                irq->status |= XICS_STATUS_ASSERTED;
> +            } else {
> +                /*
> +                 * A pending edge-triggered interrupt (or MSI)
> +                 * must have been rejected previously when we
> +                 * first detected it and tried to deliver it,
> +                 * so mark it as pending and previously rejected
> +                 * for consistency with how xics.c works.
> +                 */
> +                irq->status |= XICS_STATUS_MASKED_PENDING
> +                    | XICS_STATUS_REJECTED;
> +            }
> +        }
> +    }
> +}
> +
> +static int ics_set_kvm_state(ICSState *ics)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(ics->icp);
> +    uint64_t state;
> +    struct kvm_device_attr attr = {
> +        .flags = 0,
> +        .group = KVM_DEV_XICS_GRP_SOURCES,
> +        .addr = (uint64_t)(uintptr_t)&state,
> +    };
> +    int i;
> +
> +    for (i = 0; i < ics->nr_irqs; i++) {
> +        ICSIRQState *irq = &ics->irqs[i];
> +        int ret;
> +
> +        attr.attr = i + ics->offset;
> +
> +        state = irq->server;
> +        state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
> +            << KVM_XICS_PRIORITY_SHIFT;
> +        if (irq->priority != irq->saved_priority) {
> +            assert(irq->priority == 0xff);
> +            state |= KVM_XICS_MASKED;
> +        }
> +
> +        if (ics->islsi[i]) {
> +            state |= KVM_XICS_LEVEL_SENSITIVE;
> +            if (irq->status & XICS_STATUS_ASSERTED) {
> +                state |= KVM_XICS_PENDING;
> +            }
> +        } else {
> +            if (irq->status & XICS_STATUS_MASKED_PENDING) {
> +                state |= KVM_XICS_PENDING;
> +            }
> +        }
> +
> +        ret = ioctl(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
> +        if (ret != 0) {
> +            error_report("Unable to restore KVM interrupt controller state"
> +                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +{
> +    ICSState *ics = opaque;
> +    struct kvm_irq_level args;
> +    int rc;
> +
> +    args.irq = srcno + ics->offset;
> +    if (!ics->islsi[srcno]) {
> +        if (!val) {
> +            return;
> +        }
> +        args.level = KVM_INTERRUPT_SET;
> +    } else {
> +        args.level = val ? KVM_INTERRUPT_SET_LEVEL : KVM_INTERRUPT_UNSET;
> +    }
> +    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> +    if (rc < 0) {
> +        perror("kvm_irq_line");
> +    }
> +}
> +
> +static void ics_kvm_reset(DeviceState *dev)
> +{
> +    ics_set_kvm_state(ICS(dev));
> +}
> +
> +static int ics_kvm_realize(DeviceState *dev)

This is still copying the realize error: It's an old-style initfn
disguised as realizefn.

> +{
> +    ICSState *ics = ICS(dev);
> +
> +    assert(ics->nr_irqs);

error_setg()

> +    ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> +    ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +
> +    return 0;
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ICSStateClass *icsc = ICS_CLASS(klass);
> +
> +    dc->init = ics_kvm_realize;
> +    dc->reset = ics_kvm_reset;
> +    icsc->pre_save = ics_get_kvm_state;
> +    icsc->post_load = ics_set_kvm_state;
> +}
> +
> +static const TypeInfo ics_kvm_info = {
> +    .name = TYPE_ICS_KVM,
> +    .parent = TYPE_ICS,
> +    .instance_size = sizeof(ICSState),
> +    .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> +    CPUState *cs;
> +    ICPState *ss;
> +    KVMXICSState *icpkvm = KVM_XICS(icp);
> +    XICSStateClass *xics_info = XICS_GET_PARENT_CLASS(OBJECT(icp));
> +
> +    cs = CPU(cpu);
> +    ss = &icp->ss[cs->cpu_index];
> +
> +    assert(cs->cpu_index < icp->nr_servers);
> +    if (icpkvm->kernel_xics_fd == -1) {
> +        abort();
> +    }
> +
> +    if (icpkvm->kernel_xics_fd != -1) {
> +        int ret;
> +        struct kvm_enable_cap xics_enable_cap = {
> +            .cap = KVM_CAP_IRQ_XICS,
> +            .flags = 0,
> +            .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
> +        };
> +
> +        ss->cs = cs;
> +
> +        ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
> +        if (ret < 0) {
> +            error_report("Unable to connect CPU%d to kernel XICS: %s\n",
> +                    cs->cpu_index, strerror(errno));
> +            exit(1);
> +        }
> +    }
> +
> +    /* Call emulated XICS implementation for consistency */
> +    assert(xics_info && xics_info->cpu_setup);
> +    xics_info->cpu_setup(icp, cpu);
> +}
> +
> +void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
> +{
> +    icp->ics = ICS(object_new(TYPE_ICS_KVM));
> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +    icp->ics->icp = icp;

instance_init + object_initialize()?

> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
> +}
> +
> +void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers)
> +{
> +    int i;
> +
> +    icp->nr_servers = nr_servers;
> +
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        char buffer[32];
> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> +    }
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                       uint32_t token,
> +                       uint32_t nargs, target_ulong args,
> +                       uint32_t nret, target_ulong rets)
> +{
> +    error_report("pseries: %s must never be called for in-kernel XICS\n",
> +            __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> +    KVMXICSState *icpkvm = KVM_XICS(dev);
> +    XICSState *icp = XICS_COMMON(dev);
> +    int i, rc;
> +    Error *error = NULL;
> +    struct kvm_create_device xics_create_device = {
> +        .type = KVM_DEV_TYPE_XICS,
> +        .flags = 0,
> +    };
> +
> +    assert(kvm_enabled());
> +    assert(kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS));

error_setg() - if device can be created without accel=kvm (which it
looks as if it can) then you should just error out the nice way here.

> +
> +    icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> +    icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> +    icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> +    icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> +        goto fail;
> +    }
> +
> +    rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> +    if (rc < 0) {
> +        error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
> +        goto fail;
> +    }
> +
> +    /* Create the kernel ICP */
> +    rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
> +    if (rc < 0) {
> +        error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> +        goto fail;
> +    }
> +
> +    icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> +    object_property_set_bool(OBJECT(icp->ics), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +            goto fail;

Indentation?

> +    }
> +
> +    assert(icp->nr_servers);
> +    for (i = 0; i < icp->nr_servers; i++) {
> +        object_property_set_bool(OBJECT(&icp->ss[i]), true, "realized", &error);
> +        if (error) {
> +            error_propagate(errp, error);
> +            goto fail;
> +        }
> +    }
> +    return;
> +
> +fail:
> +    kvmppc_define_rtas_token(0, "ibm,set-xive");
> +    kvmppc_define_rtas_token(0, "ibm,get-xive");
> +    kvmppc_define_rtas_token(0, "ibm,int-on");
> +    kvmppc_define_rtas_token(0, "ibm,int-off");
> +    return;

This return is not needed.

> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
> +
> +    dc->realize = xics_kvm_realize;
> +    xsc->cpu_setup = xics_kvm_cpu_setup;
> +    xsc->set_nr_irqs = xics_kvm_set_nr_irqs;
> +    xsc->set_nr_servers = xics_kvm_set_nr_servers;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> +    .name          = TYPE_KVM_XICS,
> +    .parent        = TYPE_XICS_COMMON,
> +    .instance_size = sizeof(KVMXICSState),
> +    .class_init    = xics_kvm_class_init,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> +    type_register_static(&xics_kvm_info);
> +    type_register_static(&ics_kvm_info);
> +    type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..8dc8565 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -141,14 +141,33 @@ static XICSState *try_create_xics(const char *type, int nr_servers,
>          return NULL;
>      }
>  
> -    return XICS(dev);
> +    return XICS_COMMON(dev);
>  }
>  
>  static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>  {
>      XICSState *icp = NULL;
>  
> -    icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    if (kvm_enabled()) {
> +        QemuOpts *machine_opts = qemu_get_machine_opts();
> +        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> +                                                "kernel_irqchip", true);
> +        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> +                                                  "kernel_irqchip", false);
> +        if (irqchip_allowed) {
> +            icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
> +        }
> +
> +        if (irqchip_required && !icp) {
> +            perror("Failed to create in-kernel XICS\n");
> +            abort();
> +        }
> +    }
> +
> +    if (!icp) {
> +        icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> +    }
> +
>      if (!icp) {
>          perror("Failed to create XICS\n");
>          abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1066f69..e5889e9 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -35,6 +35,9 @@
>  #define TYPE_XICS "xics"
>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>  
> +#define TYPE_KVM_XICS "xics-kvm"
> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
> +
>  #define XICS_COMMON_CLASS(klass) \
>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>  #define XICS_CLASS(klass) \
> @@ -44,6 +47,9 @@
>  #define XICS_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>  
> +#define XICS_GET_PARENT_CLASS(obj) \
> +    (XICSStateClass *) object_class_get_parent(object_get_class(obj))

This is dangerous in that it takes the parent class of whatever type the
obj argument turns out to have. This has been discussed in lengths in
the context of Peter C.'s proposal for ISA/ARM realize cleanup.

This should therefore be a macro per type specifying the TYPE_*, either
for object_class_by_name() or to my proposed macro which abstracts the
implementation to core QOM code.

XICS_CLASS() would be nicer than open-coding, but why cast here?
DeviceClass can be needed just as well.

Also please check error_report() indentation and that they don't contain
trailing \n.

Regards,
Andreas

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


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

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

[Alexey Kardashevskiy]

On 08/06/2013 08:12 PM, Andreas Färber wrote:

>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 1066f69..e5889e9 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -35,6 +35,9 @@
>>  #define TYPE_XICS "xics"
>>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>>  
>> +#define TYPE_KVM_XICS "xics-kvm"
>> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
>> +
>>  #define XICS_COMMON_CLASS(klass) \
>>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>>  #define XICS_CLASS(klass) \
>> @@ -44,6 +47,9 @@
>>  #define XICS_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>>  
>> +#define XICS_GET_PARENT_CLASS(obj) \
>> +    (XICSStateClass *) object_class_get_parent(object_get_class(obj))
> 
> This is dangerous in that it takes the parent class of whatever type the
> obj argument turns out to have. This has been discussed in lengths in
> the context of Peter C.'s proposal for ISA/ARM realize cleanup.

How is it dangerous? I perfectly know what I call it with. And simple run
will crash QEMU if something is wrong.

> This should therefore be a macro per type specifying the TYPE_*, either
> for object_class_by_name() or to my proposed macro which abstracts the
> implementation to core QOM code.

Please, be more specific. What type should be used in macro here -
XICS_COMMON or KVM_XICS? I asked already in the other thread but somehow
you missed it. Neither really makes sense for me as I believe that
get_parent is exactly for the cases (and this is the case) when I do not
want to know exact parent type and I already know the current type. Thanks.

> XICS_CLASS() would be nicer than open-coding, but why cast here?
> DeviceClass can be needed just as well.

I do not need DeviceClass, at all, anywhere. I need a pointer to a my
specific cpu_setup callback, this is all about it.



-- 
Alexey

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

[Andreas Färber]

Am 06.08.2013 14:06, schrieb Alexey Kardashevskiy:
> On 08/06/2013 08:12 PM, Andreas Färber wrote:
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index 1066f69..e5889e9 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -35,6 +35,9 @@
>>>  #define TYPE_XICS "xics"
>>>  #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>>>  
>>> +#define TYPE_KVM_XICS "xics-kvm"
>>> +#define KVM_XICS(obj) OBJECT_CHECK(KVMXICSState, (obj), TYPE_KVM_XICS)
>>> +
>>>  #define XICS_COMMON_CLASS(klass) \
>>>       OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS_COMMON)
>>>  #define XICS_CLASS(klass) \
>>> @@ -44,6 +47,9 @@
>>>  #define XICS_GET_CLASS(obj) \
>>>       OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
>>>  
>>> +#define XICS_GET_PARENT_CLASS(obj) \
>>> +    (XICSStateClass *) object_class_get_parent(object_get_class(obj))
>>
>> This is dangerous in that it takes the parent class of whatever type the
>> obj argument turns out to have. This has been discussed in lengths in
>> the context of Peter C.'s proposal for ISA/ARM realize cleanup.
> 
> How is it dangerous? I perfectly know what I call it with. And simple run
> will crash QEMU if something is wrong.

I did not say it was wrong or an error, I said it's dangerous. The
reason is that unlike C++ we do not have VTables in QOM:
KVM_XICS(obj) === XICS(obj) === obj.
I.e., object_get_class() always returns the actual, most specific type
rather than a type corresponding to the variable you are using it through.

So if someone derives TYPE_MY_KVM_XICS from your TYPE_KVM_XICS then the
function will silently do something different than it was doing before.
Same if someone inserts TYPE_VIRTUAL_XICS between TYPE_XICS and
TYPE_KVM_XICS.

The thought of both my macro (and Peter C.'s last version) and my
comments here is to keep the type information in a central place and to
make, say, a kvm_xics_realize C function behave like a KVMXICS::realize
C++ function would.

>> This should therefore be a macro per type specifying the TYPE_*, either
>> for object_class_by_name() or to my proposed macro which abstracts the
>> implementation to core QOM code.
> 
> Please, be more specific. What type should be used in macro here -
> XICS_COMMON or KVM_XICS? I asked already in the other thread but somehow
> you missed it.

Answered it now! ;)

> Neither really makes sense for me as I believe that
> get_parent is exactly for the cases (and this is the case) when I do not
> want to know exact parent type and I already know the current type. Thanks.

Correct. You do not know, you are making implicit assumptions that I
would like to avoid for safety reasons in favor of explicit
parent-of-my-type. That allows both to derive further types and to
insert intermediate types when using object_class_get_parent() the way I do.

>> XICS_CLASS() would be nicer than open-coding, but why cast here?
>> DeviceClass can be needed just as well.
> 
> I do not need DeviceClass, at all, anywhere. I need a pointer to a my
> specific cpu_setup callback, this is all about it.

You might not do so now - you might if there is common stuff to do in
DeviceClass::realize().

But that is something I suggest to discuss on my macro patch instead, as
it is a general design question for our parent-related macros. I also
suggest to search the archives for Peter's last two proposals that got
some discussion of corner cases and intentions as well.

Andreas

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

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

[Alexey Kardashevskiy]

On 08/06/2013 08:12 PM, Andreas Färber wrote:
>> +    /* Call emulated XICS implementation for consistency */
>> +    assert(xics_info && xics_info->cpu_setup);
>> +    xics_info->cpu_setup(icp, cpu);
>> +}
>> +
>> +void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>> +{
>> +    icp->ics = ICS(object_new(TYPE_ICS_KVM));
>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>> +    icp->ics->icp = icp;
> 
> instance_init + object_initialize()?

Create ICS_KVM in instance_init of XICS_KVM and initialize it here? Why split?

> 
>> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
>> +}
>> +
>> +void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers)
>> +{
>> +    int i;
>> +
>> +    icp->nr_servers = nr_servers;
>> +
>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>> +    for (i = 0; i < icp->nr_servers; i++) {
>> +        char buffer[32];
>> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>> +    }
>> +}
>> +
>> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +                       uint32_t token,
>> +                       uint32_t nargs, target_ulong args,
>> +                       uint32_t nret, target_ulong rets)
>> +{
>> +    error_report("pseries: %s must never be called for in-kernel XICS\n",
>> +            __func__);
>> +}
>> +
>> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
>> +{
>> +    KVMXICSState *icpkvm = KVM_XICS(dev);
>> +    XICSState *icp = XICS_COMMON(dev);
>> +    int i, rc;
>> +    Error *error = NULL;
>> +    struct kvm_create_device xics_create_device = {
>> +        .type = KVM_DEV_TYPE_XICS,
>> +        .flags = 0,
>> +    };
>> +
>> +    assert(kvm_enabled());
>> +    assert(kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS));
> 
> error_setg() - if device can be created without accel=kvm (which it
> looks as if it can) then you should just error out the nice way here.


I check kvm_enabled() (I thought I check both but was wrong, will fix it)
where I try to create XICS_KVM so we should not be here if that check failed.


-- 
Alexey

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

[Andreas Färber]

Am 07.08.2013 09:03, schrieb Alexey Kardashevskiy:
> On 08/06/2013 08:12 PM, Andreas Färber wrote:
>>> +    /* Call emulated XICS implementation for consistency */
>>> +    assert(xics_info && xics_info->cpu_setup);
>>> +    xics_info->cpu_setup(icp, cpu);
>>> +}
>>> +
>>> +void xics_kvm_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>>> +{
>>> +    icp->ics = ICS(object_new(TYPE_ICS_KVM));
>>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>>> +    icp->ics->icp = icp;
>>
>> instance_init + object_initialize()?
> 
> Create ICS_KVM in instance_init of XICS_KVM and initialize it here? Why split?

Answered on 5/6 (mail bounced at first due to kernel.crashing.org).

>>> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
>>> +}
>>> +
>>> +void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers)
>>> +{
>>> +    int i;
>>> +
>>> +    icp->nr_servers = nr_servers;
>>> +
>>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>> +    for (i = 0; i < icp->nr_servers; i++) {
>>> +        char buffer[32];
>>> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>> +    }
>>> +}
[...]
>>> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    KVMXICSState *icpkvm = KVM_XICS(dev);
>>> +    XICSState *icp = XICS_COMMON(dev);
>>> +    int i, rc;
>>> +    Error *error = NULL;
>>> +    struct kvm_create_device xics_create_device = {
>>> +        .type = KVM_DEV_TYPE_XICS,
>>> +        .flags = 0,
>>> +    };
>>> +
>>> +    assert(kvm_enabled());
>>> +    assert(kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS));
>>
>> error_setg() - if device can be created without accel=kvm (which it
>> looks as if it can) then you should just error out the nice way here.
> 
> 
> I check kvm_enabled() (I thought I check both but was wrong, will fix it)
> where I try to create XICS_KVM so we should not be here if that check failed.

-device, -object and patch Anthony's patch QMP are other ways to
instantiate the same type. :)

Andreas

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

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

[Alexey Kardashevskiy]

On 08/07/2013 05:08 PM, Andreas Färber wrote:
>>>> +    icp->nr_irqs = icp->ics->nr_irqs = nr_irqs;
>>>> +}
>>>> +
>>>> +void xics_kvm_set_nr_servers(XICSState *icp, uint32_t nr_servers)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    icp->nr_servers = nr_servers;
>>>> +
>>>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>> +    for (i = 0; i < icp->nr_servers; i++) {
>>>> +        char buffer[32];
>>>> +        object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>> +        snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>> +        object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>> +    }
>>>> +}
> [...]
>>>> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    KVMXICSState *icpkvm = KVM_XICS(dev);
>>>> +    XICSState *icp = XICS_COMMON(dev);
>>>> +    int i, rc;
>>>> +    Error *error = NULL;
>>>> +    struct kvm_create_device xics_create_device = {
>>>> +        .type = KVM_DEV_TYPE_XICS,
>>>> +        .flags = 0,
>>>> +    };
>>>> +
>>>> +    assert(kvm_enabled());
>>>> +    assert(kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS));
>>>
>>> error_setg() - if device can be created without accel=kvm (which it
>>> looks as if it can) then you should just error out the nice way here.
>>
>>
>> I check kvm_enabled() (I thought I check both but was wrong, will fix it)
>> where I try to create XICS_KVM so we should not be here if that check failed.
> 
> -device, -object and patch Anthony's patch QMP are other ways to
> instantiate the same type. :)

Hmm. Nice. I thought "-device xics,nr_irqs=1,nr_servers=2" won't work but
it does (well, fails, but lot further, in realize()) :)



-- 
Alexey