All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8)
@ 2017-03-28  7:32 Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU Cédric Le Goater
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

Hello,

Here is a series adding support for the interrupt controller as found
on a POWER8 system. POWER9 uses a different interrupt controller
called XIVE, still to be worked on.

The initial patches are more cleanups of the XICS layer which move the
IRQ 'server' number mapping under the machine handlers. The PowerNV
machine is then extended with the Interrupt Source Control (ICS), the
Interrupt Control Presenter (ICP) objects and the Interrupt Management
area.

To test, grab a kernel and a rootfs image here :

  https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/zImage.epapr
  https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/rootfs.cpio.xz

The full patchset is available here :

   https://github.com/legoater/qemu/commits/powernv-ipmi-2.9

Thanks,

C.

Changes since v2:

 - removed the ICS list from the PowerNV machine
 - changed the 'icp' backlink type to be an 'Object'

Changes since v1:

 - introduced PnvICPState to hold the ICP memory region
 - handled pir-to-cpu_index mapping under the machine icp_get handler
 - added multichip support
 - removed ics_eoi handler (came from a bug in PHB3_MSI)
 - kept PSI and OCC model for later, when this part is done.

Cédric Le Goater (8):
  ppc/xics: introduce an 'icp' backlink under PowerPCCPU
  spapr: move the IRQ server number mapping under the machine
  ppc/xics: add a realize() handler to ICPStateClass
  ppc/pnv: add a PnvICPState object
  ppc/pnv: create the ICP and ICS objects under the machine
  ppc/pnv: add a helper to calculate MMIO addresses registers
  ppc/pnv: link the CPUs to the machine XICSFabric
  ppc/pnv: add memory regions for the ICP registers

 hw/intc/Makefile.objs   |   1 +
 hw/intc/xics.c          |   9 ++-
 hw/intc/xics_pnv.c      | 180 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/intc/xics_spapr.c    |  25 ++-----
 hw/ppc/pnv.c            | 179 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/pnv_core.c       |  20 ++++--
 hw/ppc/spapr.c          |   3 +-
 hw/ppc/spapr_cpu_core.c |   5 +-
 include/hw/ppc/pnv.h    |  34 ++++++++-
 include/hw/ppc/xics.h   |  13 ++++
 target/ppc/cpu.h        |   1 +
 11 files changed, 443 insertions(+), 27 deletions(-)
 create mode 100644 hw/intc/xics_pnv.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-29  4:11   ` David Gibson
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 2/8] spapr: move the IRQ server number mapping under the machine Cédric Le Goater
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

Today, the ICPState array of the sPAPR machine is indexed with
'cpu_index' of the CPUState. This numbering of CPUs is internal to
QEMU and the guest only knows about what is exposed in the device
tree, that is the 'cpu_dt_id'. This is why sPAPR uses the helper
xics_get_cpu_index_by_dt_id() to do the mapping in a couple of places.

To provide a more generic XICS layer, we need to abstract the IRQ
'server' number and remove any assumption made on its nature. It
should not be used as a 'cpu_index' for lookups like xics_cpu_setup()
and xics_cpu_destroy() do.

To reach that goal, we choose to introduce an 'icp' backlink under
PowerPCCPU, and let the machine core init routine do the ICPState
lookup. The resulting object is stored under PowerPCCPU which is
passed on to xics_cpu_setup(). The IRQ 'server' number in XICS is now
generic. sPAPR uses 'cpu_dt_id' and PowerNV will use 'PIR' number.

This also has the benefit of simplifying the sPAPR hcall routines
which do not need to do any ICPState lookups anymore.

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

Changes since v2:

 - changed the 'icp' backlink type to be an 'Object'

 hw/intc/xics.c          |  4 ++--
 hw/intc/xics_spapr.c    | 20 +++++---------------
 hw/ppc/spapr_cpu_core.c |  5 ++++-
 target/ppc/cpu.h        |  1 +
 4 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e740989a1162..bb485cc5b078 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -52,7 +52,7 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
 void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
+    ICPState *icp = ICP(cpu->icp);
 
     assert(icp);
     assert(cs == icp->cs);
@@ -65,7 +65,7 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
+    ICPState *icp = ICP(cpu->icp);
     ICPStateClass *icpc;
 
     assert(icp);
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 84d24b2837a7..6144f9876ae3 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -43,11 +43,9 @@
 static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
     target_ulong cppr = args[0];
 
-    icp_set_cppr(icp, cppr);
+    icp_set_cppr(ICP(cpu->icp), cppr);
     return H_SUCCESS;
 }
 
@@ -69,9 +67,7 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
-    uint32_t xirr = icp_accept(icp);
+    uint32_t xirr = icp_accept(ICP(cpu->icp));
 
     args[0] = xirr;
     return H_SUCCESS;
@@ -80,9 +76,7 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                              target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
-    uint32_t xirr = icp_accept(icp);
+    uint32_t xirr = icp_accept(ICP(cpu->icp));
 
     args[0] = xirr;
     args[1] = cpu_get_host_ticks();
@@ -92,21 +86,17 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
     target_ulong xirr = args[0];
 
-    icp_eoi(icp, xirr);
+    icp_eoi(ICP(cpu->icp), xirr);
     return H_SUCCESS;
 }
 
 static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                             target_ulong opcode, target_ulong *args)
 {
-    CPUState *cs = CPU(cpu);
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
     uint32_t mfrr;
-    uint32_t xirr = icp_ipoll(icp, &mfrr);
+    uint32_t xirr = icp_ipoll(ICP(cpu->icp), &mfrr);
 
     args[0] = xirr;
     args[1] = mfrr;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 6883f0991ae9..f9ca3f09a0f8 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -63,6 +63,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
                            Error **errp)
 {
     CPUPPCState *env = &cpu->env;
+    XICSFabric *xi = XICS_FABRIC(spapr);
+    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
@@ -80,7 +82,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
         }
     }
 
-    xics_cpu_setup(XICS_FABRIC(spapr), cpu);
+    cpu->icp = OBJECT(icp);
+    xics_cpu_setup(xi, cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
     spapr_cpu_reset(cpu);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 5ee33b3fd315..774f2d717831 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1196,6 +1196,7 @@ struct PowerPCCPU {
     uint32_t max_compat;
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
+    Object *icp;
 
     /* Fields related to migration compatibility hacks */
     bool pre_2_8_migration;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 2/8] spapr: move the IRQ server number mapping under the machine
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 3/8] ppc/xics: add a realize() handler to ICPStateClass Cédric Le Goater
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

This is the second step to abstract the IRQ 'server' number of the
XICS layer. Now that the prereq cleanups have been done in the
previous patch, we can move down the 'cpu_dt_id' to 'cpu_index'
mapping in the sPAPR machine handler.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics_spapr.c    | 5 ++---
 hw/ppc/spapr.c          | 3 ++-
 hw/ppc/spapr_cpu_core.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 6144f9876ae3..5d685769ad0c 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -52,9 +52,8 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    target_ulong server = xics_get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
-    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), server);
+    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), args[0]);
 
     if (!icp) {
         return H_PARAMETER;
@@ -122,7 +121,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     }
 
     nr = rtas_ld(args, 0);
-    server = xics_get_cpu_index_by_dt_id(rtas_ld(args, 1));
+    server = rtas_ld(args, 1);
     priority = rtas_ld(args, 2);
 
     if (!ics_valid_irq(ics, nr) || !xics_icp_get(XICS_FABRIC(spapr), server)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8aecea3dd10c..b9f7f8607869 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3024,9 +3024,10 @@ static void spapr_ics_resend(XICSFabric *dev)
     ics_resend(spapr->ics);
 }
 
-static ICPState *spapr_icp_get(XICSFabric *xi, int server)
+static ICPState *spapr_icp_get(XICSFabric *xi, int cpu_dt_id)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
+    int server = xics_get_cpu_index_by_dt_id(cpu_dt_id);
 
     return (server < spapr->nr_servers) ? &spapr->icps[server] : NULL;
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index f9ca3f09a0f8..4bd7449a7c88 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -64,7 +64,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
 {
     CPUPPCState *env = &cpu->env;
     XICSFabric *xi = XICS_FABRIC(spapr);
-    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
+    ICPState *icp = xics_icp_get(xi, cpu->cpu_dt_id);
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 3/8] ppc/xics: add a realize() handler to ICPStateClass
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 2/8] spapr: move the IRQ server number mapping under the machine Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 4/8] ppc/pnv: add a PnvICPState object Cédric Le Goater
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

It will be used by derived classes in PowerNV for customization.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c        | 5 +++++
 include/hw/ppc/xics.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index bb485cc5b078..bf0fa22b2ddd 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -348,6 +348,7 @@ static void icp_reset(void *dev)
 static void icp_realize(DeviceState *dev, Error **errp)
 {
     ICPState *icp = ICP(dev);
+    ICPStateClass *icpc = ICP_GET_CLASS(dev);
     Object *obj;
     Error *err = NULL;
 
@@ -360,6 +361,10 @@ static void icp_realize(DeviceState *dev, Error **errp)
 
     icp->xics = XICS_FABRIC(obj);
 
+    if (icpc->realize) {
+        icpc->realize(dev, errp);
+    }
+
     qemu_register_reset(icp_reset, dev);
 }
 
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 9a5e715fe553..0863e3a079f5 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -60,6 +60,7 @@ typedef struct XICSFabric XICSFabric;
 struct ICPStateClass {
     DeviceClass parent_class;
 
+    void (*realize)(DeviceState *dev, Error **errp);
     void (*pre_save)(ICPState *s);
     int (*post_load)(ICPState *s, int version_id);
     void (*cpu_setup)(ICPState *icp, PowerPCCPU *cpu);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 4/8] ppc/pnv: add a PnvICPState object
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
                   ` (2 preceding siblings ...)
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 3/8] ppc/xics: add a realize() handler to ICPStateClass Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine Cédric Le Goater
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

This provides a new ICPState object for the PowerNV machine (POWER8).
Access to the Interrupt Management area is done though a memory
region. It contains the registers of the Interrupt Control Presenters
of each thread which are used to accept, return, forward interrupts in
the system.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---

 Changes since v1:

 - moved the memory region from PnvCore to a specific PnvICPState object

 hw/intc/Makefile.objs |   1 +
 hw/intc/xics_pnv.c    | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/xics.h |  12 ++++
 3 files changed, 193 insertions(+)
 create mode 100644 hw/intc/xics_pnv.c

diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index adedd0da5fd8..78426a7dafcd 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -35,6 +35,7 @@ obj-$(CONFIG_SH4) += sh_intc.o
 obj-$(CONFIG_XICS) += xics.o
 obj-$(CONFIG_XICS_SPAPR) += xics_spapr.o
 obj-$(CONFIG_XICS_KVM) += xics_kvm.o
+obj-$(CONFIG_POWERNV) += xics_pnv.o
 obj-$(CONFIG_ALLWINNER_A10_PIC) += allwinner-a10-pic.o
 obj-$(CONFIG_S390_FLIC) += s390_flic.o
 obj-$(CONFIG_S390_FLIC_KVM) += s390_flic_kvm.o
diff --git a/hw/intc/xics_pnv.c b/hw/intc/xics_pnv.c
new file mode 100644
index 000000000000..b1f300ca6d2e
--- /dev/null
+++ b/hw/intc/xics_pnv.c
@@ -0,0 +1,180 @@
+/*
+ * QEMU PowerPC PowerNV Interrupt Control Presenter (ICP) model
+ *
+ * Copyright (c) 2017, IBM Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "hw/ppc/xics.h"
+
+static uint64_t pnv_icp_read(void *opaque, hwaddr addr, unsigned width)
+{
+    ICPState *icp = ICP(opaque);
+    PnvICPState *picp = PNV_ICP(opaque);
+    bool byte0 = (width == 1 && (addr & 0x3) == 0);
+    uint64_t val = 0xffffffff;
+
+    switch (addr & 0xffc) {
+    case 0: /* poll */
+        val = icp_ipoll(icp, NULL);
+        if (byte0) {
+            val >>= 24;
+        } else if (width != 4) {
+            goto bad_access;
+        }
+        break;
+    case 4: /* xirr */
+        if (byte0) {
+            val = icp_ipoll(icp, NULL) >> 24;
+        } else if (width == 4) {
+            val = icp_accept(icp);
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 12:
+        if (byte0) {
+            val = icp->mfrr;
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 16:
+        if (width == 4) {
+            val = picp->links[0];
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 20:
+        if (width == 4) {
+            val = picp->links[1];
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 24:
+        if (width == 4) {
+            val = picp->links[2];
+        } else {
+            goto bad_access;
+        }
+        break;
+    default:
+bad_access:
+        qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
+                      HWADDR_PRIx"/%d\n", addr, width);
+    }
+
+    return val;
+}
+
+static void pnv_icp_write(void *opaque, hwaddr addr, uint64_t val,
+                              unsigned width)
+{
+    ICPState *icp = ICP(opaque);
+    PnvICPState *picp = PNV_ICP(opaque);
+    bool byte0 = (width == 1 && (addr & 0x3) == 0);
+
+    switch (addr & 0xffc) {
+    case 4: /* xirr */
+        if (byte0) {
+            icp_set_cppr(icp, val);
+        } else if (width == 4) {
+            icp_eoi(icp, val);
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 12:
+        if (byte0) {
+            icp_set_mfrr(icp, val);
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 16:
+        if (width == 4) {
+            picp->links[0] = val;
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 20:
+        if (width == 4) {
+            picp->links[1] = val;
+        } else {
+            goto bad_access;
+        }
+        break;
+    case 24:
+        if (width == 4) {
+            picp->links[2] = val;
+        } else {
+            goto bad_access;
+        }
+        break;
+    default:
+bad_access:
+        qemu_log_mask(LOG_GUEST_ERROR, "XICS: Bad ICP access 0x%"
+                      HWADDR_PRIx"/%d\n", addr, width);
+    }
+}
+
+static const MemoryRegionOps pnv_icp_ops = {
+    .read = pnv_icp_read,
+    .write = pnv_icp_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 4,
+    .endianness = DEVICE_BIG_ENDIAN,
+};
+
+static void pnv_icp_realize(DeviceState *dev, Error **errp)
+{
+    PnvICPState *icp = PNV_ICP(dev);
+
+    memory_region_init_io(&icp->mmio, OBJECT(dev), &pnv_icp_ops,
+                          icp, "icp-thread", 0x1000);
+}
+
+static void pnv_icp_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ICPStateClass *icpc = ICP_CLASS(klass);
+
+    icpc->realize = pnv_icp_realize;
+    dc->desc = "PowerNV ICP";
+}
+
+static const TypeInfo pnv_icp_info = {
+    .name          = TYPE_PNV_ICP,
+    .parent        = TYPE_ICP,
+    .instance_size = sizeof(PnvICPState),
+    .class_init    = pnv_icp_class_init,
+    .class_size    = sizeof(ICPStateClass),
+};
+
+static void pnv_icp_register_types(void)
+{
+    type_register_static(&pnv_icp_info);
+}
+
+type_init(pnv_icp_register_types)
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 0863e3a079f5..cfcf7ecece69 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -41,10 +41,12 @@
  */
 typedef struct ICPStateClass ICPStateClass;
 typedef struct ICPState ICPState;
+typedef struct PnvICPState PnvICPState;
 typedef struct ICSStateClass ICSStateClass;
 typedef struct ICSState ICSState;
 typedef struct ICSIRQState ICSIRQState;
 typedef struct XICSFabric XICSFabric;
+typedef struct PowerPCCPU PowerPCCPU;
 
 #define TYPE_ICP "icp"
 #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
@@ -52,6 +54,9 @@ typedef struct XICSFabric XICSFabric;
 #define TYPE_KVM_ICP "icp-kvm"
 #define KVM_ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_KVM_ICP)
 
+#define TYPE_PNV_ICP "pnv-icp"
+#define PNV_ICP(obj) OBJECT_CHECK(PnvICPState, (obj), TYPE_PNV_ICP)
+
 #define ICP_CLASS(klass) \
      OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
 #define ICP_GET_CLASS(obj) \
@@ -81,6 +86,13 @@ struct ICPState {
     XICSFabric *xics;
 };
 
+struct PnvICPState {
+    ICPState parent_obj;
+
+    MemoryRegion mmio;
+    uint32_t links[3];
+};
+
 #define TYPE_ICS_BASE "ics-base"
 #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
                   ` (3 preceding siblings ...)
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 4/8] ppc/pnv: add a PnvICPState object Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-29  5:18   ` David Gibson
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 6/8] ppc/pnv: add a helper to calculate MMIO addresses registers Cédric Le Goater
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

Like this is done for the sPAPR machine, we use a simple array under
the PowerNV machine to store the Interrupt Control Presenters (ICP)
objects, one for each vCPU. This array is indexed by 'cpu_index' of
the CPUState but the users will provide a core PIR number. The mapping
is done in the icp_get() handler of the machine and is transparent to
XICS.

The Interrupt Control Sources (ICS), Processor Service Interface and
PCI-E interface models, will be introduced in subsequent patches. For
now, we have none, so we just prepare ground with place holders.

Finally, to interface with the XICS layer which manipulates the ICP
and ICS objects, we extend the PowerNV machine with an XICSFabric
interface and its associated handlers.

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

 Changes since v2:

 - removed the list of ICS. The handlers will iterate on the chips to
   use the available ICS.

 Changes since v1:

 - handled pir-to-cpu_index mapping under icp_get 
 - removed ics_eio handler
 - changed ICP name indexing
 - removed sysbus parenting of the ICP object

 hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h |  3 ++
 2 files changed, 99 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 3fa722af82e6..e441b8ac1cad 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -33,7 +33,10 @@
 #include "exec/address-spaces.h"
 #include "qemu/cutils.h"
 #include "qapi/visitor.h"
+#include "monitor/monitor.h"
+#include "hw/intc/intc.h"
 
+#include "hw/ppc/xics.h"
 #include "hw/ppc/pnv_xscom.h"
 
 #include "hw/isa/isa.h"
@@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
         machine->cpu_model = "POWER8";
     }
 
+    /* Create the Interrupt Control Presenters before the vCPUs */
+    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
+    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
+    for (i = 0; i < pnv->nr_servers; i++) {
+        PnvICPState *icp = &pnv->icps[i];
+        char name[32];
+
+        /* TODO: fix ICP object name to be in sync with the core name */
+        snprintf(name, sizeof(name), "icp[%d]", i);
+        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
+        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
+                                  &error_fatal);
+        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
+                                       &error_fatal);
+        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
+    }
+
     /* Create the processor chips */
     chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
     if (!object_class_by_name(chip_typename)) {
@@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
     .abstract      = true,
 };
 
+static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
+{
+    PnvMachineState *pnv = POWERNV_MACHINE(xi);
+    int i;
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        /* place holder */
+    }
+    return NULL;
+}
+
+static void pnv_ics_resend(XICSFabric *xi)
+{
+    PnvMachineState *pnv = POWERNV_MACHINE(xi);
+    int i;
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        /* place holder */
+    }
+}
+
+static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        CPUPPCState *env = &cpu->env;
+
+        if (env->spr_cb[SPR_PIR].default_value == pir) {
+            return cpu;
+        }
+    }
+
+    return NULL;
+}
+
+static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
+{
+    PnvMachineState *pnv = POWERNV_MACHINE(xi);
+    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
+
+    if (!cpu) {
+        return NULL;
+    }
+
+    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
+    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
+}
+
+static void pnv_pic_print_info(InterruptStatsProvider *obj,
+                               Monitor *mon)
+{
+    PnvMachineState *pnv = POWERNV_MACHINE(obj);
+    int i;
+
+    for (i = 0; i < pnv->nr_servers; i++) {
+        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
+    }
+
+    for (i = 0; i < pnv->num_chips; i++) {
+        /* place holder */
+    }
+}
+
 static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
                               void *opaque, Error **errp)
 {
@@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
 static void powernv_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
+    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
 
     mc->desc = "IBM PowerNV (Non-Virtualized)";
     mc->init = ppc_powernv_init;
@@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
     mc->default_boot_order = NULL;
     mc->default_ram_size = 1 * G_BYTE;
+    xic->icp_get = pnv_icp_get;
+    xic->ics_get = pnv_ics_get;
+    xic->ics_resend = pnv_ics_resend;
+    ispc->print_info = pnv_pic_print_info;
 
     powernv_machine_class_props_init(oc);
 }
@@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
     .instance_size = sizeof(PnvMachineState),
     .instance_init = powernv_machine_initfn,
     .class_init    = powernv_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_XICS_FABRIC },
+        { TYPE_INTERRUPT_STATS_PROVIDER },
+        { },
+    },
 };
 
 static void powernv_machine_register_types(void)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index df98a72006e4..1ca197d2ec83 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -22,6 +22,7 @@
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "hw/ppc/pnv_lpc.h"
+#include "hw/ppc/xics.h"
 
 #define TYPE_PNV_CHIP "powernv-chip"
 #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
@@ -114,6 +115,8 @@ typedef struct PnvMachineState {
     PnvChip      **chips;
 
     ISABus       *isa_bus;
+    PnvICPState  *icps;
+    uint32_t     nr_servers;
 } PnvMachineState;
 
 #define PNV_FDT_ADDR          0x01000000
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 6/8] ppc/pnv: add a helper to calculate MMIO addresses registers
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
                   ` (4 preceding siblings ...)
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric Cédric Le Goater
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 8/8] ppc/pnv: add memory regions for the ICP registers Cédric Le Goater
  7 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

Some controllers (ICP, PSI) have a base register address which is
calculated using the chip id.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/hw/ppc/pnv.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 1ca197d2ec83..4e28d5d8f03a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -92,14 +92,24 @@ typedef struct PnvChipClass {
     OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER9)
 
 /*
- * This generates a HW chip id depending on an index:
+ * This generates a HW chip id depending on an index, as found on a
+ * two socket system with dual chip modules :
  *
  *    0x0, 0x1, 0x10, 0x11
  *
  * 4 chips should be the maximum
+ *
+ * TODO: use a machine property to define the chip ids
  */
 #define PNV_CHIP_HWID(i) ((((i) & 0x3e) << 3) | ((i) & 0x1))
 
+/*
+ * Converts back a HW chip id to an index. This is useful to calculate
+ * the MMIO addresses of some controllers which depend on the chip id.
+ */
+#define PNV_CHIP_INDEX(chip)                                    \
+    (((chip)->chip_id >> 2) * 2 + ((chip)->chip_id & 0x3))
+
 #define TYPE_POWERNV_MACHINE       MACHINE_TYPE_NAME("powernv")
 #define POWERNV_MACHINE(obj) \
     OBJECT_CHECK(PnvMachineState, (obj), TYPE_POWERNV_MACHINE)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
                   ` (5 preceding siblings ...)
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 6/8] ppc/pnv: add a helper to calculate MMIO addresses registers Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  2017-03-29  5:20   ` David Gibson
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 8/8] ppc/pnv: add memory regions for the ICP registers Cédric Le Goater
  7 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

This assigns the ICPState object to the CPU using the PIR number for
lookups before calling the XICS layer to finish the job.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv.c      |  2 ++
 hw/ppc/pnv_core.c | 20 ++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index e441b8ac1cad..ae894834892f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -711,6 +711,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
         object_property_set_int(OBJECT(pnv_core),
                                 pcc->core_pir(chip, core_hwid),
                                 "pir", &error_fatal);
+        object_property_add_const_link(OBJECT(pnv_core), "xics",
+                                       qdev_get_machine(), &error_fatal);
         object_property_set_bool(OBJECT(pnv_core), true, "realized",
                                  &error_fatal);
         object_unref(OBJECT(pnv_core));
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index d79d530b4881..a5e9614dac7d 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -25,6 +25,7 @@
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_core.h"
 #include "hw/ppc/pnv_xscom.h"
+#include "hw/ppc/xics.h"
 
 static void powernv_cpu_reset(void *opaque)
 {
@@ -43,7 +44,7 @@ static void powernv_cpu_reset(void *opaque)
     env->msr |= MSR_HVB; /* Hypervisor mode */
 }
 
-static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
+static void powernv_cpu_init(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     int core_pir;
@@ -63,6 +64,9 @@ static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
     cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
 
     qemu_register_reset(powernv_cpu_reset, cpu);
+
+    cpu->icp = OBJECT(xics_icp_get(xi, pir->default_value));
+    xics_cpu_setup(xi, cpu);
 }
 
 /*
@@ -110,7 +114,7 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_core_realize_child(Object *child, Error **errp)
+static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
 {
     Error *local_err = NULL;
     CPUState *cs = CPU(child);
@@ -122,7 +126,7 @@ static void pnv_core_realize_child(Object *child, Error **errp)
         return;
     }
 
-    powernv_cpu_init(cpu, &local_err);
+    powernv_cpu_init(cpu, xi, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -140,6 +144,14 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     void *obj;
     int i, j;
     char name[32];
+    Object *xi;
+
+    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
+    if (!xi) {
+        error_setg(errp, "%s: required link 'xics' not found: %s",
+                   __func__, error_get_pretty(local_err));
+        return;
+    }
 
     pc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -160,7 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     for (j = 0; j < cc->nr_threads; j++) {
         obj = pc->threads + j * size;
 
-        pnv_core_realize_child(obj, &local_err);
+        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
         if (local_err) {
             goto err;
         }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v3 8/8] ppc/pnv: add memory regions for the ICP registers
  2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
                   ` (6 preceding siblings ...)
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric Cédric Le Goater
@ 2017-03-28  7:32 ` Cédric Le Goater
  7 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-28  7:32 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel, Cédric Le Goater

This provides to a PowerNV chip (POWER8) access to the Interrupt
Management area, which contains the registers of the Interrupt Control
Presenters of each thread. These are used to accept, return, forward
interrupts in the system.

This area is modeled with a per-chip container memory region holding
all the ICP registers. Each thread of a chip is then associated with
its ICP registers using a memory subregion indexed by its PIR number
in the overall region.

The device tree is populated accordingly.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---

 Changes since v1:

 - added multichip support
 - adapted to use PnvICPState object

 hw/ppc/pnv.c         | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/pnv.h | 19 ++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index ae894834892f..534cf625e29c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -218,6 +218,43 @@ static void powernv_create_core_node(PnvChip *chip, PnvCore *pc, void *fdt)
                        servers_prop, sizeof(servers_prop))));
 }
 
+static void powernv_populate_icp(PnvChip *chip, void *fdt, uint32_t pir,
+                                 uint32_t nr_threads)
+{
+    uint64_t addr = PNV_ICP_BASE(chip) | (pir << 12);
+    char *name;
+    const char compat[] = "IBM,power8-icp\0IBM,ppc-xicp";
+    uint32_t irange[2], i, rsize;
+    uint64_t *reg;
+    int offset;
+
+    irange[0] = cpu_to_be32(pir);
+    irange[1] = cpu_to_be32(nr_threads);
+
+    rsize = sizeof(uint64_t) * 2 * nr_threads;
+    reg = g_malloc(rsize);
+    for (i = 0; i < nr_threads; i++) {
+        reg[i * 2] = cpu_to_be64(addr | ((pir + i) * 0x1000));
+        reg[i * 2 + 1] = cpu_to_be64(0x1000);
+    }
+
+    name = g_strdup_printf("interrupt-controller@%"PRIX64, addr);
+    offset = fdt_add_subnode(fdt, 0, name);
+    _FDT(offset);
+    g_free(name);
+
+    _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
+    _FDT((fdt_setprop(fdt, offset, "reg", reg, rsize)));
+    _FDT((fdt_setprop_string(fdt, offset, "device_type",
+                              "PowerPC-External-Interrupt-Presentation")));
+    _FDT((fdt_setprop(fdt, offset, "interrupt-controller", NULL, 0)));
+    _FDT((fdt_setprop(fdt, offset, "ibm,interrupt-server-ranges",
+                       irange, sizeof(irange))));
+    _FDT((fdt_setprop_cell(fdt, offset, "#interrupt-cells", 1)));
+    _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 0)));
+    g_free(reg);
+}
+
 static void powernv_populate_chip(PnvChip *chip, void *fdt)
 {
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
@@ -231,6 +268,10 @@ static void powernv_populate_chip(PnvChip *chip, void *fdt)
         PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
 
         powernv_create_core_node(chip, pnv_core, fdt);
+
+        /* Interrupt Control Presenters (ICP). One per core. */
+        powernv_populate_icp(chip, fdt, pnv_core->pir,
+                             CPU_CORE(pnv_core)->nr_threads);
     }
 
     if (chip->ram_size) {
@@ -660,6 +701,38 @@ static void pnv_chip_init(Object *obj)
     object_property_add_child(obj, "lpc", OBJECT(&chip->lpc), NULL);
 }
 
+static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
+{
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
+    char *typename = pnv_core_typename(pcc->cpu_model);
+    size_t typesize = object_type_get_instance_size(typename);
+    int i, j;
+    char *name;
+    XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
+
+    name = g_strdup_printf("icp-%x", chip->chip_id);
+    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
+    g_free(name);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 1, PNV_ICP_BASE(chip));
+
+    /* Map the ICP registers for each thread */
+    for (i = 0; i < chip->nr_cores; i++) {
+        PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
+        int core_hwid = CPU_CORE(pnv_core)->core_id;
+
+        for (j = 0; j < CPU_CORE(pnv_core)->nr_threads; j++) {
+            uint32_t pir = pcc->core_pir(chip, core_hwid) + j;
+            PnvICPState *icp = PNV_ICP(xics_icp_get(xi, pir));
+
+            memory_region_add_subregion(&chip->icp_mmio, pir << 12, &icp->mmio);
+        }
+    }
+
+    g_free(typename);
+}
+
 static void pnv_chip_realize(DeviceState *dev, Error **errp)
 {
     PnvChip *chip = PNV_CHIP(dev);
@@ -730,6 +803,14 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
     object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
                              &error_fatal);
     pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
+
+    /* Interrupt Management Area. This is the memory region holding
+     * all the Interrupt Control Presenter (ICP) registers */
+    pnv_chip_icp_realize(chip, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
 }
 
 static Property pnv_chip_properties[] = {
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 4e28d5d8f03a..f5801034b920 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -55,6 +55,7 @@ typedef struct PnvChip {
     MemoryRegion xscom_mmio;
     MemoryRegion xscom;
     AddressSpace xscom_as;
+    MemoryRegion icp_mmio;
 
     PnvLpcController lpc;
 } PnvChip;
@@ -139,4 +140,22 @@ typedef struct PnvMachineState {
 #define PNV_XSCOM_BASE(chip)                                            \
     (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
 
+/*
+ * XSCOM 0x20109CA defines the ICP BAR:
+ *
+ * 0:29   : bits 14 to 43 of address to define 1 MB region.
+ * 30     : 1 to enable ICP to receive loads/stores against its BAR region
+ * 31:63  : Constant 0
+ *
+ * Usually defined as :
+ *
+ *      0xffffe00200000000 -> 0x0003ffff80000000
+ *      0xffffe00600000000 -> 0x0003ffff80100000
+ *      0xffffe02200000000 -> 0x0003ffff80800000
+ *      0xffffe02600000000 -> 0x0003ffff80900000
+ */
+#define PNV_ICP_SIZE         0x0000000000100000ull
+#define PNV_ICP_BASE(chip)                                              \
+    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
+
 #endif /* _PPC_PNV_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU Cédric Le Goater
@ 2017-03-29  4:11   ` David Gibson
  2017-03-29  7:14     ` Cédric Le Goater
  0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2017-03-29  4:11 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Tue, Mar 28, 2017 at 09:32:25AM +0200, Cédric Le Goater wrote:
> Today, the ICPState array of the sPAPR machine is indexed with
> 'cpu_index' of the CPUState. This numbering of CPUs is internal to
> QEMU and the guest only knows about what is exposed in the device
> tree, that is the 'cpu_dt_id'. This is why sPAPR uses the helper
> xics_get_cpu_index_by_dt_id() to do the mapping in a couple of places.
> 
> To provide a more generic XICS layer, we need to abstract the IRQ
> 'server' number and remove any assumption made on its nature. It
> should not be used as a 'cpu_index' for lookups like xics_cpu_setup()
> and xics_cpu_destroy() do.
> 
> To reach that goal, we choose to introduce an 'icp' backlink under
> PowerPCCPU, and let the machine core init routine do the ICPState
> lookup. The resulting object is stored under PowerPCCPU which is
> passed on to xics_cpu_setup(). The IRQ 'server' number in XICS is now
> generic. sPAPR uses 'cpu_dt_id' and PowerNV will use 'PIR' number.
> 
> This also has the benefit of simplifying the sPAPR hcall routines
> which do not need to do any ICPState lookups anymore.

Since you've changed the type to a generic Object *, the name needs to
be changed to something generic as well.  Maybe 'intc' or
'irq_private'.

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
> Changes since v2:
> 
>  - changed the 'icp' backlink type to be an 'Object'
> 
>  hw/intc/xics.c          |  4 ++--
>  hw/intc/xics_spapr.c    | 20 +++++---------------
>  hw/ppc/spapr_cpu_core.c |  5 ++++-
>  target/ppc/cpu.h        |  1 +
>  4 files changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e740989a1162..bb485cc5b078 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -52,7 +52,7 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
> +    ICPState *icp = ICP(cpu->icp);
>  
>      assert(icp);
>      assert(cs == icp->cs);
> @@ -65,7 +65,7 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
> +    ICPState *icp = ICP(cpu->icp);
>      ICPStateClass *icpc;
>  
>      assert(icp);
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 84d24b2837a7..6144f9876ae3 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -43,11 +43,9 @@
>  static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>      target_ulong cppr = args[0];
>  
> -    icp_set_cppr(icp, cppr);
> +    icp_set_cppr(ICP(cpu->icp), cppr);
>      return H_SUCCESS;
>  }
>  
> @@ -69,9 +67,7 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> -    uint32_t xirr = icp_accept(icp);
> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
>  
>      args[0] = xirr;
>      return H_SUCCESS;
> @@ -80,9 +76,7 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                               target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> -    uint32_t xirr = icp_accept(icp);
> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
>  
>      args[0] = xirr;
>      args[1] = cpu_get_host_ticks();
> @@ -92,21 +86,17 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>      target_ulong xirr = args[0];
>  
> -    icp_eoi(icp, xirr);
> +    icp_eoi(ICP(cpu->icp), xirr);
>      return H_SUCCESS;
>  }
>  
>  static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
> -    CPUState *cs = CPU(cpu);
> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>      uint32_t mfrr;
> -    uint32_t xirr = icp_ipoll(icp, &mfrr);
> +    uint32_t xirr = icp_ipoll(ICP(cpu->icp), &mfrr);
>  
>      args[0] = xirr;
>      args[1] = mfrr;
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 6883f0991ae9..f9ca3f09a0f8 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -63,6 +63,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>                             Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> +    XICSFabric *xi = XICS_FABRIC(spapr);
> +    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> @@ -80,7 +82,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>          }
>      }
>  
> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu);
> +    cpu->icp = OBJECT(icp);
> +    xics_cpu_setup(xi, cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5ee33b3fd315..774f2d717831 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1196,6 +1196,7 @@ struct PowerPCCPU {
>      uint32_t max_compat;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> +    Object *icp;
>  
>      /* Fields related to migration compatibility hacks */
>      bool pre_2_8_migration;

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine Cédric Le Goater
@ 2017-03-29  5:18   ` David Gibson
  2017-03-29  8:13     ` Cédric Le Goater
  0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2017-03-29  5:18 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Tue, Mar 28, 2017 at 09:32:29AM +0200, Cédric Le Goater wrote:
> Like this is done for the sPAPR machine, we use a simple array under
> the PowerNV machine to store the Interrupt Control Presenters (ICP)
> objects, one for each vCPU. This array is indexed by 'cpu_index' of
> the CPUState but the users will provide a core PIR number. The mapping
> is done in the icp_get() handler of the machine and is transparent to
> XICS.
> 
> The Interrupt Control Sources (ICS), Processor Service Interface and
> PCI-E interface models, will be introduced in subsequent patches. For
> now, we have none, so we just prepare ground with place holders.
> 
> Finally, to interface with the XICS layer which manipulates the ICP
> and ICS objects, we extend the PowerNV machine with an XICSFabric
> interface and its associated handlers.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Changes since v2:
> 
>  - removed the list of ICS. The handlers will iterate on the chips to
>    use the available ICS.
> 
>  Changes since v1:
> 
>  - handled pir-to-cpu_index mapping under icp_get 
>  - removed ics_eio handler
>  - changed ICP name indexing
>  - removed sysbus parenting of the ICP object
> 
>  hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/pnv.h |  3 ++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 3fa722af82e6..e441b8ac1cad 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -33,7 +33,10 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/cutils.h"
>  #include "qapi/visitor.h"
> +#include "monitor/monitor.h"
> +#include "hw/intc/intc.h"
>  
> +#include "hw/ppc/xics.h"
>  #include "hw/ppc/pnv_xscom.h"
>  
>  #include "hw/isa/isa.h"
> @@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
>          machine->cpu_model = "POWER8";
>      }
>  
> +    /* Create the Interrupt Control Presenters before the vCPUs */
> +    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
> +    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
> +    for (i = 0; i < pnv->nr_servers; i++) {
> +        PnvICPState *icp = &pnv->icps[i];
> +        char name[32];
> +
> +        /* TODO: fix ICP object name to be in sync with the core name */
> +        snprintf(name, sizeof(name), "icp[%d]", i);

It may end up being the same value, but since the qom name is exposed
to the outside, it would be better to have it be the PIR, rather than
the cpu_index.

> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
> +        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
> +                                  &error_fatal);
> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
> +                                       &error_fatal);
> +        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
> +    }
> +
>      /* Create the processor chips */
>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
>      if (!object_class_by_name(chip_typename)) {
> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
>      .abstract      = true,
>  };
>  
> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
> +{
> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> +    int i;
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        /* place holder */
> +    }
> +    return NULL;
> +}
> +
> +static void pnv_ics_resend(XICSFabric *xi)
> +{
> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> +    int i;
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        /* place holder */
> +    }
> +}

Seems like the above two functions belong in a later patch.

> +
> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +        CPUPPCState *env = &cpu->env;
> +
> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
> +            return cpu;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> +{
> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> +
> +    if (!cpu) {
> +        return NULL;
> +    }
> +
> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);

Should use CPU() instead of parent_obj here.

> +}
> +
> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
> +                               Monitor *mon)
> +{
> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> +    int i;
> +
> +    for (i = 0; i < pnv->nr_servers; i++) {
> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
> +    }
> +
> +    for (i = 0; i < pnv->num_chips; i++) {
> +        /* place holder */
> +    }
> +}
> +
>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
>                                void *opaque, Error **errp)
>  {
> @@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = ppc_powernv_init;
> @@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_boot_order = NULL;
>      mc->default_ram_size = 1 * G_BYTE;
> +    xic->icp_get = pnv_icp_get;
> +    xic->ics_get = pnv_ics_get;
> +    xic->ics_resend = pnv_ics_resend;
> +    ispc->print_info = pnv_pic_print_info;
>  
>      powernv_machine_class_props_init(oc);
>  }
> @@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
>      .instance_size = sizeof(PnvMachineState),
>      .instance_init = powernv_machine_initfn,
>      .class_init    = powernv_machine_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_XICS_FABRIC },
> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> +        { },
> +    },
>  };
>  
>  static void powernv_machine_register_types(void)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index df98a72006e4..1ca197d2ec83 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -22,6 +22,7 @@
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  #include "hw/ppc/pnv_lpc.h"
> +#include "hw/ppc/xics.h"
>  
>  #define TYPE_PNV_CHIP "powernv-chip"
>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> @@ -114,6 +115,8 @@ typedef struct PnvMachineState {
>      PnvChip      **chips;
>  
>      ISABus       *isa_bus;
> +    PnvICPState  *icps;
> +    uint32_t     nr_servers;
>  } PnvMachineState;
>  
>  #define PNV_FDT_ADDR          0x01000000

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric
  2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric Cédric Le Goater
@ 2017-03-29  5:20   ` David Gibson
  2017-03-29  7:16     ` Cédric Le Goater
  0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2017-03-29  5:20 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Tue, Mar 28, 2017 at 09:32:31AM +0200, Cédric Le Goater wrote:
> This assigns the ICPState object to the CPU using the PIR number for
> lookups before calling the XICS layer to finish the job.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv.c      |  2 ++
>  hw/ppc/pnv_core.c | 20 ++++++++++++++++----
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index e441b8ac1cad..ae894834892f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -711,6 +711,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>          object_property_set_int(OBJECT(pnv_core),
>                                  pcc->core_pir(chip, core_hwid),
>                                  "pir", &error_fatal);
> +        object_property_add_const_link(OBJECT(pnv_core), "xics",
> +                                       qdev_get_machine(), &error_fatal);
>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>                                   &error_fatal);
>          object_unref(OBJECT(pnv_core));
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index d79d530b4881..a5e9614dac7d 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -25,6 +25,7 @@
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_core.h"
>  #include "hw/ppc/pnv_xscom.h"
> +#include "hw/ppc/xics.h"
>  
>  static void powernv_cpu_reset(void *opaque)
>  {
> @@ -43,7 +44,7 @@ static void powernv_cpu_reset(void *opaque)
>      env->msr |= MSR_HVB; /* Hypervisor mode */
>  }
>  
> -static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
> +static void powernv_cpu_init(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
>      int core_pir;
> @@ -63,6 +64,9 @@ static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
>      cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
>  
>      qemu_register_reset(powernv_cpu_reset, cpu);
> +
> +    cpu->icp = OBJECT(xics_icp_get(xi, pir->default_value));
> +    xics_cpu_setup(xi, cpu);

Hmm.. seems like xics_cpu_setup() should probably set the cpu->icp link..

>  }
>  
>  /*
> @@ -110,7 +114,7 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void pnv_core_realize_child(Object *child, Error **errp)
> +static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>  {
>      Error *local_err = NULL;
>      CPUState *cs = CPU(child);
> @@ -122,7 +126,7 @@ static void pnv_core_realize_child(Object *child, Error **errp)
>          return;
>      }
>  
> -    powernv_cpu_init(cpu, &local_err);
> +    powernv_cpu_init(cpu, xi, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -140,6 +144,14 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      void *obj;
>      int i, j;
>      char name[32];
> +    Object *xi;
> +
> +    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> +    if (!xi) {
> +        error_setg(errp, "%s: required link 'xics' not found: %s",
> +                   __func__, error_get_pretty(local_err));
> +        return;
> +    }
>  
>      pc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -160,7 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      for (j = 0; j < cc->nr_threads; j++) {
>          obj = pc->threads + j * size;
>  
> -        pnv_core_realize_child(obj, &local_err);
> +        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
>          if (local_err) {
>              goto err;
>          }

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU
  2017-03-29  4:11   ` David Gibson
@ 2017-03-29  7:14     ` Cédric Le Goater
  2017-03-30  1:26       ` David Gibson
  0 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-29  7:14 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel

On 03/29/2017 06:11 AM, David Gibson wrote:
> On Tue, Mar 28, 2017 at 09:32:25AM +0200, Cédric Le Goater wrote:
>> Today, the ICPState array of the sPAPR machine is indexed with
>> 'cpu_index' of the CPUState. This numbering of CPUs is internal to
>> QEMU and the guest only knows about what is exposed in the device
>> tree, that is the 'cpu_dt_id'. This is why sPAPR uses the helper
>> xics_get_cpu_index_by_dt_id() to do the mapping in a couple of places.
>>
>> To provide a more generic XICS layer, we need to abstract the IRQ
>> 'server' number and remove any assumption made on its nature. It
>> should not be used as a 'cpu_index' for lookups like xics_cpu_setup()
>> and xics_cpu_destroy() do.
>>
>> To reach that goal, we choose to introduce an 'icp' backlink under
>> PowerPCCPU, and let the machine core init routine do the ICPState
>> lookup. The resulting object is stored under PowerPCCPU which is
>> passed on to xics_cpu_setup(). The IRQ 'server' number in XICS is now
>> generic. sPAPR uses 'cpu_dt_id' and PowerNV will use 'PIR' number.
>>
>> This also has the benefit of simplifying the sPAPR hcall routines
>> which do not need to do any ICPState lookups anymore.
> 
> Since you've changed the type to a generic Object *, the name needs to
> be changed to something generic as well.  Maybe 'intc' or
> 'irq_private'.

yes. I think 'intc' is a good choice, unless we make the type 
'void *' and in that case 'irq_private' would be better.

I took a quick look at the other machines under ppc. It is not 
obvious how we could use that 'intc' pointer. May be for the 
objects TYPE_OPENPIC and heathrow_pic.

The ppc405 machines could use it to store 'qemu_irq *pic' but 
I am not sure there is much benefit doing that. To be studied.

Anyhow I will change the name.

Thanks,

C. 

>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>> Changes since v2:
>>
>>  - changed the 'icp' backlink type to be an 'Object'
>>
>>  hw/intc/xics.c          |  4 ++--
>>  hw/intc/xics_spapr.c    | 20 +++++---------------
>>  hw/ppc/spapr_cpu_core.c |  5 ++++-
>>  target/ppc/cpu.h        |  1 +
>>  4 files changed, 12 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index e740989a1162..bb485cc5b078 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -52,7 +52,7 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
>> +    ICPState *icp = ICP(cpu->icp);
>>  
>>      assert(icp);
>>      assert(cs == icp->cs);
>> @@ -65,7 +65,7 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu)
>>  {
>>      CPUState *cs = CPU(cpu);
>>      CPUPPCState *env = &cpu->env;
>> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
>> +    ICPState *icp = ICP(cpu->icp);
>>      ICPStateClass *icpc;
>>  
>>      assert(icp);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 84d24b2837a7..6144f9876ae3 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -43,11 +43,9 @@
>>  static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                             target_ulong opcode, target_ulong *args)
>>  {
>> -    CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>>      target_ulong cppr = args[0];
>>  
>> -    icp_set_cppr(icp, cppr);
>> +    icp_set_cppr(ICP(cpu->icp), cppr);
>>      return H_SUCCESS;
>>  }
>>  
>> @@ -69,9 +67,7 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                             target_ulong opcode, target_ulong *args)
>>  {
>> -    CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> -    uint32_t xirr = icp_accept(icp);
>> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
>>  
>>      args[0] = xirr;
>>      return H_SUCCESS;
>> @@ -80,9 +76,7 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                               target_ulong opcode, target_ulong *args)
>>  {
>> -    CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> -    uint32_t xirr = icp_accept(icp);
>> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
>>  
>>      args[0] = xirr;
>>      args[1] = cpu_get_host_ticks();
>> @@ -92,21 +86,17 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                            target_ulong opcode, target_ulong *args)
>>  {
>> -    CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>>      target_ulong xirr = args[0];
>>  
>> -    icp_eoi(icp, xirr);
>> +    icp_eoi(ICP(cpu->icp), xirr);
>>      return H_SUCCESS;
>>  }
>>  
>>  static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>                              target_ulong opcode, target_ulong *args)
>>  {
>> -    CPUState *cs = CPU(cpu);
>> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>>      uint32_t mfrr;
>> -    uint32_t xirr = icp_ipoll(icp, &mfrr);
>> +    uint32_t xirr = icp_ipoll(ICP(cpu->icp), &mfrr);
>>  
>>      args[0] = xirr;
>>      args[1] = mfrr;
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 6883f0991ae9..f9ca3f09a0f8 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -63,6 +63,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>                             Error **errp)
>>  {
>>      CPUPPCState *env = &cpu->env;
>> +    XICSFabric *xi = XICS_FABRIC(spapr);
>> +    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
>>  
>>      /* Set time-base frequency to 512 MHz */
>>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>> @@ -80,7 +82,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>>          }
>>      }
>>  
>> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu);
>> +    cpu->icp = OBJECT(icp);
>> +    xics_cpu_setup(xi, cpu);
>>  
>>      qemu_register_reset(spapr_cpu_reset, cpu);
>>      spapr_cpu_reset(cpu);
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 5ee33b3fd315..774f2d717831 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1196,6 +1196,7 @@ struct PowerPCCPU {
>>      uint32_t max_compat;
>>      uint32_t compat_pvr;
>>      PPCVirtualHypervisor *vhyp;
>> +    Object *icp;
>>  
>>      /* Fields related to migration compatibility hacks */
>>      bool pre_2_8_migration;
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric
  2017-03-29  5:20   ` David Gibson
@ 2017-03-29  7:16     ` Cédric Le Goater
  0 siblings, 0 replies; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-29  7:16 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel

On 03/29/2017 07:20 AM, David Gibson wrote:
> On Tue, Mar 28, 2017 at 09:32:31AM +0200, Cédric Le Goater wrote:
>> This assigns the ICPState object to the CPU using the PIR number for
>> lookups before calling the XICS layer to finish the job.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/pnv.c      |  2 ++
>>  hw/ppc/pnv_core.c | 20 ++++++++++++++++----
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index e441b8ac1cad..ae894834892f 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -711,6 +711,8 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>>          object_property_set_int(OBJECT(pnv_core),
>>                                  pcc->core_pir(chip, core_hwid),
>>                                  "pir", &error_fatal);
>> +        object_property_add_const_link(OBJECT(pnv_core), "xics",
>> +                                       qdev_get_machine(), &error_fatal);
>>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>>                                   &error_fatal);
>>          object_unref(OBJECT(pnv_core));
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index d79d530b4881..a5e9614dac7d 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -25,6 +25,7 @@
>>  #include "hw/ppc/pnv.h"
>>  #include "hw/ppc/pnv_core.h"
>>  #include "hw/ppc/pnv_xscom.h"
>> +#include "hw/ppc/xics.h"
>>  
>>  static void powernv_cpu_reset(void *opaque)
>>  {
>> @@ -43,7 +44,7 @@ static void powernv_cpu_reset(void *opaque)
>>      env->msr |= MSR_HVB; /* Hypervisor mode */
>>  }
>>  
>> -static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
>> +static void powernv_cpu_init(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>>  {
>>      CPUPPCState *env = &cpu->env;
>>      int core_pir;
>> @@ -63,6 +64,9 @@ static void powernv_cpu_init(PowerPCCPU *cpu, Error **errp)
>>      cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
>>  
>>      qemu_register_reset(powernv_cpu_reset, cpu);
>> +
>> +    cpu->icp = OBJECT(xics_icp_get(xi, pir->default_value));
>> +    xics_cpu_setup(xi, cpu);
> 
> Hmm.. seems like xics_cpu_setup() should probably set the cpu->icp link..

OK. It is not problem, and I have to change 'PATCH 1' anyhow.

Thanks,

C.
 
> 
>>  }
>>  
>>  /*
>> @@ -110,7 +114,7 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>>      .endianness = DEVICE_BIG_ENDIAN,
>>  };
>>  
>> -static void pnv_core_realize_child(Object *child, Error **errp)
>> +static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>>  {
>>      Error *local_err = NULL;
>>      CPUState *cs = CPU(child);
>> @@ -122,7 +126,7 @@ static void pnv_core_realize_child(Object *child, Error **errp)
>>          return;
>>      }
>>  
>> -    powernv_cpu_init(cpu, &local_err);
>> +    powernv_cpu_init(cpu, xi, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>> @@ -140,6 +144,14 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>      void *obj;
>>      int i, j;
>>      char name[32];
>> +    Object *xi;
>> +
>> +    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
>> +    if (!xi) {
>> +        error_setg(errp, "%s: required link 'xics' not found: %s",
>> +                   __func__, error_get_pretty(local_err));
>> +        return;
>> +    }
>>  
>>      pc->threads = g_malloc0(size * cc->nr_threads);
>>      for (i = 0; i < cc->nr_threads; i++) {
>> @@ -160,7 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>      for (j = 0; j < cc->nr_threads; j++) {
>>          obj = pc->threads + j * size;
>>  
>> -        pnv_core_realize_child(obj, &local_err);
>> +        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
>>          if (local_err) {
>>              goto err;
>>          }
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-29  5:18   ` David Gibson
@ 2017-03-29  8:13     ` Cédric Le Goater
  2017-03-30  1:55       ` David Gibson
  0 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-29  8:13 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel

On 03/29/2017 07:18 AM, David Gibson wrote:
> On Tue, Mar 28, 2017 at 09:32:29AM +0200, Cédric Le Goater wrote:
>> Like this is done for the sPAPR machine, we use a simple array under
>> the PowerNV machine to store the Interrupt Control Presenters (ICP)
>> objects, one for each vCPU. This array is indexed by 'cpu_index' of
>> the CPUState but the users will provide a core PIR number. The mapping
>> is done in the icp_get() handler of the machine and is transparent to
>> XICS.
>>
>> The Interrupt Control Sources (ICS), Processor Service Interface and
>> PCI-E interface models, will be introduced in subsequent patches. For
>> now, we have none, so we just prepare ground with place holders.
>>
>> Finally, to interface with the XICS layer which manipulates the ICP
>> and ICS objects, we extend the PowerNV machine with an XICSFabric
>> interface and its associated handlers.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Changes since v2:
>>
>>  - removed the list of ICS. The handlers will iterate on the chips to
>>    use the available ICS.
>>
>>  Changes since v1:
>>
>>  - handled pir-to-cpu_index mapping under icp_get 
>>  - removed ics_eio handler
>>  - changed ICP name indexing
>>  - removed sysbus parenting of the ICP object
>>
>>  hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h |  3 ++
>>  2 files changed, 99 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3fa722af82e6..e441b8ac1cad 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -33,7 +33,10 @@
>>  #include "exec/address-spaces.h"
>>  #include "qemu/cutils.h"
>>  #include "qapi/visitor.h"
>> +#include "monitor/monitor.h"
>> +#include "hw/intc/intc.h"
>>  
>> +#include "hw/ppc/xics.h"
>>  #include "hw/ppc/pnv_xscom.h"
>>  
>>  #include "hw/isa/isa.h"
>> @@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
>>          machine->cpu_model = "POWER8";
>>      }
>>  
>> +    /* Create the Interrupt Control Presenters before the vCPUs */
>> +    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
>> +    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
>> +    for (i = 0; i < pnv->nr_servers; i++) {
>> +        PnvICPState *icp = &pnv->icps[i];
>> +        char name[32];
>> +
>> +        /* TODO: fix ICP object name to be in sync with the core name */
>> +        snprintf(name, sizeof(name), "icp[%d]", i);
> 
> It may end up being the same value, but since the qom name is exposed
> to the outside, it would be better to have it be the PIR, rather than
> the cpu_index.

The problem is that the ICPState objects are created before the PnvChip
objects. The PnvChip sanitizes the core layout in terms HW id and then 
creates the PnvCore objects. The core creates a PowerPCCPU object per 
thread and, in the realize function, uses the XICSFabric to look for
a matching ICP to link the CPU with. 

So it is a little complex to do what you ask for ...

What would really simplify the code, would be to allocate a TYPE_PNV_ICP
object when the PowerPCCPU object is realized and store it under the 
'icp/intc' backlink. The XICSFabric handler icp_get() would not need 
the 'icps' array anymore. 

How's that proposal ? 

>> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
>> +        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
>> +                                  &error_fatal);
>> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
>> +                                       &error_fatal);
>> +        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
>> +    }
>> +
>>      /* Create the processor chips */
>>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
>>      if (!object_class_by_name(chip_typename)) {
>> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
>>      .abstract      = true,
>>  };
>>  
>> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>> +{
>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>> +    int i;
>> +
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        /* place holder */
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static void pnv_ics_resend(XICSFabric *xi)
>> +{
>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>> +    int i;
>> +
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        /* place holder */
>> +    }
>> +}
> 
> Seems like the above two functions belong in a later patch.

OK. I guess we can add these handlers in the next patchset introducing PSI. 
I will check that the tests still run because the XICS layer uses the 
XICSFabric handlers blindly without any checks. 
 
>> +
>> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
>> +{
>> +    CPUState *cs;
>> +
>> +    CPU_FOREACH(cs) {
>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>> +        CPUPPCState *env = &cpu->env;
>> +
>> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
>> +            return cpu;
>> +        }
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
>> +{
>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>> +
>> +    if (!cpu) {
>> +        return NULL;
>> +    }
>> +
>> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
>> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
> 
> Should use CPU() instead of parent_obj here.

OK. I might just remove the array though.

Thanks,

C.


>> +}
>> +
>> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
>> +                               Monitor *mon)
>> +{
>> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
>> +    int i;
>> +
>> +    for (i = 0; i < pnv->nr_servers; i++) {
>> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
>> +    }
>> +
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        /* place holder */
>> +    }
>> +}
>> +
>>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
>>                                void *opaque, Error **errp)
>>  {
>> @@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>>  {
>>      MachineClass *mc = MACHINE_CLASS(oc);
>> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
>> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>>  
>>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>>      mc->init = ppc_powernv_init;
>> @@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
>>      mc->no_parallel = 1;
>>      mc->default_boot_order = NULL;
>>      mc->default_ram_size = 1 * G_BYTE;
>> +    xic->icp_get = pnv_icp_get;
>> +    xic->ics_get = pnv_ics_get;
>> +    xic->ics_resend = pnv_ics_resend;
>> +    ispc->print_info = pnv_pic_print_info;
>>  
>>      powernv_machine_class_props_init(oc);
>>  }
>> @@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
>>      .instance_size = sizeof(PnvMachineState),
>>      .instance_init = powernv_machine_initfn,
>>      .class_init    = powernv_machine_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_XICS_FABRIC },
>> +        { TYPE_INTERRUPT_STATS_PROVIDER },
>> +        { },
>> +    },
>>  };
>>  
>>  static void powernv_machine_register_types(void)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index df98a72006e4..1ca197d2ec83 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -22,6 +22,7 @@
>>  #include "hw/boards.h"
>>  #include "hw/sysbus.h"
>>  #include "hw/ppc/pnv_lpc.h"
>> +#include "hw/ppc/xics.h"
>>  
>>  #define TYPE_PNV_CHIP "powernv-chip"
>>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>> @@ -114,6 +115,8 @@ typedef struct PnvMachineState {
>>      PnvChip      **chips;
>>  
>>      ISABus       *isa_bus;
>> +    PnvICPState  *icps;
>> +    uint32_t     nr_servers;
>>  } PnvMachineState;
>>  
>>  #define PNV_FDT_ADDR          0x01000000
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU
  2017-03-29  7:14     ` Cédric Le Goater
@ 2017-03-30  1:26       ` David Gibson
  0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2017-03-30  1:26 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Wed, Mar 29, 2017 at 09:14:01AM +0200, Cédric Le Goater wrote:
> On 03/29/2017 06:11 AM, David Gibson wrote:
> > On Tue, Mar 28, 2017 at 09:32:25AM +0200, Cédric Le Goater wrote:
> >> Today, the ICPState array of the sPAPR machine is indexed with
> >> 'cpu_index' of the CPUState. This numbering of CPUs is internal to
> >> QEMU and the guest only knows about what is exposed in the device
> >> tree, that is the 'cpu_dt_id'. This is why sPAPR uses the helper
> >> xics_get_cpu_index_by_dt_id() to do the mapping in a couple of places.
> >>
> >> To provide a more generic XICS layer, we need to abstract the IRQ
> >> 'server' number and remove any assumption made on its nature. It
> >> should not be used as a 'cpu_index' for lookups like xics_cpu_setup()
> >> and xics_cpu_destroy() do.
> >>
> >> To reach that goal, we choose to introduce an 'icp' backlink under
> >> PowerPCCPU, and let the machine core init routine do the ICPState
> >> lookup. The resulting object is stored under PowerPCCPU which is
> >> passed on to xics_cpu_setup(). The IRQ 'server' number in XICS is now
> >> generic. sPAPR uses 'cpu_dt_id' and PowerNV will use 'PIR' number.
> >>
> >> This also has the benefit of simplifying the sPAPR hcall routines
> >> which do not need to do any ICPState lookups anymore.
> > 
> > Since you've changed the type to a generic Object *, the name needs to
> > be changed to something generic as well.  Maybe 'intc' or
> > 'irq_private'.
> 
> yes. I think 'intc' is a good choice, unless we make the type 
> 'void *' and in that case 'irq_private' would be better.
> 
> I took a quick look at the other machines under ppc. It is not 
> obvious how we could use that 'intc' pointer. May be for the 
> objects TYPE_OPENPIC and heathrow_pic.
> 
> The ppc405 machines could use it to store 'qemu_irq *pic' but 
> I am not sure there is much benefit doing that. To be studied.

Right.  I don't know that it will lead to any easy, quick conversions
of existing platforms.  It will mostly be of use for modern, high
performance type intcs (like XICS, or IO-APIC/LAPIC) - older style
simpler intcs may not have a use for it.  We'll probably want
something like it when we come to implement POWER9's XIVE, though.

> 
> Anyhow I will change the name.
> 
> Thanks,
> 
> C. 
> 
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >> Changes since v2:
> >>
> >>  - changed the 'icp' backlink type to be an 'Object'
> >>
> >>  hw/intc/xics.c          |  4 ++--
> >>  hw/intc/xics_spapr.c    | 20 +++++---------------
> >>  hw/ppc/spapr_cpu_core.c |  5 ++++-
> >>  target/ppc/cpu.h        |  1 +
> >>  4 files changed, 12 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >> index e740989a1162..bb485cc5b078 100644
> >> --- a/hw/intc/xics.c
> >> +++ b/hw/intc/xics.c
> >> @@ -52,7 +52,7 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
> >>  void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
> >>  {
> >>      CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
> >> +    ICPState *icp = ICP(cpu->icp);
> >>  
> >>      assert(icp);
> >>      assert(cs == icp->cs);
> >> @@ -65,7 +65,7 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu)
> >>  {
> >>      CPUState *cs = CPU(cpu);
> >>      CPUPPCState *env = &cpu->env;
> >> -    ICPState *icp = xics_icp_get(xi, cs->cpu_index);
> >> +    ICPState *icp = ICP(cpu->icp);
> >>      ICPStateClass *icpc;
> >>  
> >>      assert(icp);
> >> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> >> index 84d24b2837a7..6144f9876ae3 100644
> >> --- a/hw/intc/xics_spapr.c
> >> +++ b/hw/intc/xics_spapr.c
> >> @@ -43,11 +43,9 @@
> >>  static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                             target_ulong opcode, target_ulong *args)
> >>  {
> >> -    CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> >>      target_ulong cppr = args[0];
> >>  
> >> -    icp_set_cppr(icp, cppr);
> >> +    icp_set_cppr(ICP(cpu->icp), cppr);
> >>      return H_SUCCESS;
> >>  }
> >>  
> >> @@ -69,9 +67,7 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                             target_ulong opcode, target_ulong *args)
> >>  {
> >> -    CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> >> -    uint32_t xirr = icp_accept(icp);
> >> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
> >>  
> >>      args[0] = xirr;
> >>      return H_SUCCESS;
> >> @@ -80,9 +76,7 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                               target_ulong opcode, target_ulong *args)
> >>  {
> >> -    CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> >> -    uint32_t xirr = icp_accept(icp);
> >> +    uint32_t xirr = icp_accept(ICP(cpu->icp));
> >>  
> >>      args[0] = xirr;
> >>      args[1] = cpu_get_host_ticks();
> >> @@ -92,21 +86,17 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                            target_ulong opcode, target_ulong *args)
> >>  {
> >> -    CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> >>      target_ulong xirr = args[0];
> >>  
> >> -    icp_eoi(icp, xirr);
> >> +    icp_eoi(ICP(cpu->icp), xirr);
> >>      return H_SUCCESS;
> >>  }
> >>  
> >>  static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >>                              target_ulong opcode, target_ulong *args)
> >>  {
> >> -    CPUState *cs = CPU(cpu);
> >> -    ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
> >>      uint32_t mfrr;
> >> -    uint32_t xirr = icp_ipoll(icp, &mfrr);
> >> +    uint32_t xirr = icp_ipoll(ICP(cpu->icp), &mfrr);
> >>  
> >>      args[0] = xirr;
> >>      args[1] = mfrr;
> >> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> >> index 6883f0991ae9..f9ca3f09a0f8 100644
> >> --- a/hw/ppc/spapr_cpu_core.c
> >> +++ b/hw/ppc/spapr_cpu_core.c
> >> @@ -63,6 +63,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >>                             Error **errp)
> >>  {
> >>      CPUPPCState *env = &cpu->env;
> >> +    XICSFabric *xi = XICS_FABRIC(spapr);
> >> +    ICPState *icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
> >>  
> >>      /* Set time-base frequency to 512 MHz */
> >>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> >> @@ -80,7 +82,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >>          }
> >>      }
> >>  
> >> -    xics_cpu_setup(XICS_FABRIC(spapr), cpu);
> >> +    cpu->icp = OBJECT(icp);
> >> +    xics_cpu_setup(xi, cpu);
> >>  
> >>      qemu_register_reset(spapr_cpu_reset, cpu);
> >>      spapr_cpu_reset(cpu);
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 5ee33b3fd315..774f2d717831 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -1196,6 +1196,7 @@ struct PowerPCCPU {
> >>      uint32_t max_compat;
> >>      uint32_t compat_pvr;
> >>      PPCVirtualHypervisor *vhyp;
> >> +    Object *icp;
> >>  
> >>      /* Fields related to migration compatibility hacks */
> >>      bool pre_2_8_migration;
> > 
> 

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-29  8:13     ` Cédric Le Goater
@ 2017-03-30  1:55       ` David Gibson
  2017-03-30  8:15         ` Cédric Le Goater
  0 siblings, 1 reply; 19+ messages in thread
From: David Gibson @ 2017-03-30  1:55 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Wed, Mar 29, 2017 at 10:13:59AM +0200, Cédric Le Goater wrote:
> On 03/29/2017 07:18 AM, David Gibson wrote:
> > On Tue, Mar 28, 2017 at 09:32:29AM +0200, Cédric Le Goater wrote:
> >> Like this is done for the sPAPR machine, we use a simple array under
> >> the PowerNV machine to store the Interrupt Control Presenters (ICP)
> >> objects, one for each vCPU. This array is indexed by 'cpu_index' of
> >> the CPUState but the users will provide a core PIR number. The mapping
> >> is done in the icp_get() handler of the machine and is transparent to
> >> XICS.
> >>
> >> The Interrupt Control Sources (ICS), Processor Service Interface and
> >> PCI-E interface models, will be introduced in subsequent patches. For
> >> now, we have none, so we just prepare ground with place holders.
> >>
> >> Finally, to interface with the XICS layer which manipulates the ICP
> >> and ICS objects, we extend the PowerNV machine with an XICSFabric
> >> interface and its associated handlers.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>
> >>  Changes since v2:
> >>
> >>  - removed the list of ICS. The handlers will iterate on the chips to
> >>    use the available ICS.
> >>
> >>  Changes since v1:
> >>
> >>  - handled pir-to-cpu_index mapping under icp_get 
> >>  - removed ics_eio handler
> >>  - changed ICP name indexing
> >>  - removed sysbus parenting of the ICP object
> >>
> >>  hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h |  3 ++
> >>  2 files changed, 99 insertions(+)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 3fa722af82e6..e441b8ac1cad 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -33,7 +33,10 @@
> >>  #include "exec/address-spaces.h"
> >>  #include "qemu/cutils.h"
> >>  #include "qapi/visitor.h"
> >> +#include "monitor/monitor.h"
> >> +#include "hw/intc/intc.h"
> >>  
> >> +#include "hw/ppc/xics.h"
> >>  #include "hw/ppc/pnv_xscom.h"
> >>  
> >>  #include "hw/isa/isa.h"
> >> @@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
> >>          machine->cpu_model = "POWER8";
> >>      }
> >>  
> >> +    /* Create the Interrupt Control Presenters before the vCPUs */
> >> +    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
> >> +    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
> >> +    for (i = 0; i < pnv->nr_servers; i++) {
> >> +        PnvICPState *icp = &pnv->icps[i];
> >> +        char name[32];
> >> +
> >> +        /* TODO: fix ICP object name to be in sync with the core name */
> >> +        snprintf(name, sizeof(name), "icp[%d]", i);
> > 
> > It may end up being the same value, but since the qom name is exposed
> > to the outside, it would be better to have it be the PIR, rather than
> > the cpu_index.
> 
> The problem is that the ICPState objects are created before the PnvChip
> objects. The PnvChip sanitizes the core layout in terms HW id and then 
> creates the PnvCore objects. The core creates a PowerPCCPU object per 
> thread and, in the realize function, uses the XICSFabric to look for
> a matching ICP to link the CPU with. 
> 
> So it is a little complex to do what you ask for ...
> 
> What would really simplify the code, would be to allocate a TYPE_PNV_ICP
> object when the PowerPCCPU object is realized and store it under the 
> 'icp/intc' backlink. The XICSFabric handler icp_get() would not need 
> the 'icps' array anymore. 
> 
> How's that proposal ?

Sounds workable.  You could do that from the core realize function,
couldn't you?

> 
> >> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
> >> +        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
> >> +                                  &error_fatal);
> >> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
> >> +                                       &error_fatal);
> >> +        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
> >> +    }
> >> +
> >>      /* Create the processor chips */
> >>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> >>      if (!object_class_by_name(chip_typename)) {
> >> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
> >>      .abstract      = true,
> >>  };
> >>  
> >> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +    return NULL;
> >> +}
> >> +
> >> +static void pnv_ics_resend(XICSFabric *xi)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +}
> > 
> > Seems like the above two functions belong in a later patch.
> 
> OK. I guess we can add these handlers in the next patchset introducing PSI. 
> I will check that the tests still run because the XICS layer uses the 
> XICSFabric handlers blindly without any checks.

Well, sure, but the whole XICS isn't going to work without real
implementations of the ICS callbacks, so I don't see that it matters.

>  
> >> +
> >> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
> >> +{
> >> +    CPUState *cs;
> >> +
> >> +    CPU_FOREACH(cs) {
> >> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +        CPUPPCState *env = &cpu->env;
> >> +
> >> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
> >> +            return cpu;
> >> +        }
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> >> +
> >> +    if (!cpu) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
> >> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
> > 
> > Should use CPU() instead of parent_obj here.
> 
> OK. I might just remove the array though.
> 
> Thanks,
> 
> C.
> 
> 
> >> +}
> >> +
> >> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
> >> +                               Monitor *mon)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->nr_servers; i++) {
> >> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
> >> +    }
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +}
> >> +
> >>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> >>                                void *opaque, Error **errp)
> >>  {
> >> @@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
> >>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
> >>  {
> >>      MachineClass *mc = MACHINE_CLASS(oc);
> >> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> >> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> >>  
> >>      mc->desc = "IBM PowerNV (Non-Virtualized)";
> >>      mc->init = ppc_powernv_init;
> >> @@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
> >>      mc->no_parallel = 1;
> >>      mc->default_boot_order = NULL;
> >>      mc->default_ram_size = 1 * G_BYTE;
> >> +    xic->icp_get = pnv_icp_get;
> >> +    xic->ics_get = pnv_ics_get;
> >> +    xic->ics_resend = pnv_ics_resend;
> >> +    ispc->print_info = pnv_pic_print_info;
> >>  
> >>      powernv_machine_class_props_init(oc);
> >>  }
> >> @@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
> >>      .instance_size = sizeof(PnvMachineState),
> >>      .instance_init = powernv_machine_initfn,
> >>      .class_init    = powernv_machine_class_init,
> >> +    .interfaces = (InterfaceInfo[]) {
> >> +        { TYPE_XICS_FABRIC },
> >> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> >> +        { },
> >> +    },
> >>  };
> >>  
> >>  static void powernv_machine_register_types(void)
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index df98a72006e4..1ca197d2ec83 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -22,6 +22,7 @@
> >>  #include "hw/boards.h"
> >>  #include "hw/sysbus.h"
> >>  #include "hw/ppc/pnv_lpc.h"
> >> +#include "hw/ppc/xics.h"
> >>  
> >>  #define TYPE_PNV_CHIP "powernv-chip"
> >>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> >> @@ -114,6 +115,8 @@ typedef struct PnvMachineState {
> >>      PnvChip      **chips;
> >>  
> >>      ISABus       *isa_bus;
> >> +    PnvICPState  *icps;
> >> +    uint32_t     nr_servers;
> >>  } PnvMachineState;
> >>  
> >>  #define PNV_FDT_ADDR          0x01000000
> > 
> 

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-30  1:55       ` David Gibson
@ 2017-03-30  8:15         ` Cédric Le Goater
  2017-04-02  6:11           ` David Gibson
  0 siblings, 1 reply; 19+ messages in thread
From: Cédric Le Goater @ 2017-03-30  8:15 UTC (permalink / raw)
  To: David Gibson; +Cc: qemu-ppc, qemu-devel

On 03/30/2017 03:55 AM, David Gibson wrote:
> On Wed, Mar 29, 2017 at 10:13:59AM +0200, Cédric Le Goater wrote:
>> On 03/29/2017 07:18 AM, David Gibson wrote:
>>> On Tue, Mar 28, 2017 at 09:32:29AM +0200, Cédric Le Goater wrote:
>>>> Like this is done for the sPAPR machine, we use a simple array under
>>>> the PowerNV machine to store the Interrupt Control Presenters (ICP)
>>>> objects, one for each vCPU. This array is indexed by 'cpu_index' of
>>>> the CPUState but the users will provide a core PIR number. The mapping
>>>> is done in the icp_get() handler of the machine and is transparent to
>>>> XICS.
>>>>
>>>> The Interrupt Control Sources (ICS), Processor Service Interface and
>>>> PCI-E interface models, will be introduced in subsequent patches. For
>>>> now, we have none, so we just prepare ground with place holders.
>>>>
>>>> Finally, to interface with the XICS layer which manipulates the ICP
>>>> and ICS objects, we extend the PowerNV machine with an XICSFabric
>>>> interface and its associated handlers.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>
>>>>  Changes since v2:
>>>>
>>>>  - removed the list of ICS. The handlers will iterate on the chips to
>>>>    use the available ICS.
>>>>
>>>>  Changes since v1:
>>>>
>>>>  - handled pir-to-cpu_index mapping under icp_get 
>>>>  - removed ics_eio handler
>>>>  - changed ICP name indexing
>>>>  - removed sysbus parenting of the ICP object
>>>>
>>>>  hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/ppc/pnv.h |  3 ++
>>>>  2 files changed, 99 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>> index 3fa722af82e6..e441b8ac1cad 100644
>>>> --- a/hw/ppc/pnv.c
>>>> +++ b/hw/ppc/pnv.c
>>>> @@ -33,7 +33,10 @@
>>>>  #include "exec/address-spaces.h"
>>>>  #include "qemu/cutils.h"
>>>>  #include "qapi/visitor.h"
>>>> +#include "monitor/monitor.h"
>>>> +#include "hw/intc/intc.h"
>>>>  
>>>> +#include "hw/ppc/xics.h"
>>>>  #include "hw/ppc/pnv_xscom.h"
>>>>  
>>>>  #include "hw/isa/isa.h"
>>>> @@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
>>>>          machine->cpu_model = "POWER8";
>>>>      }
>>>>  
>>>> +    /* Create the Interrupt Control Presenters before the vCPUs */
>>>> +    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
>>>> +    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
>>>> +    for (i = 0; i < pnv->nr_servers; i++) {
>>>> +        PnvICPState *icp = &pnv->icps[i];
>>>> +        char name[32];
>>>> +
>>>> +        /* TODO: fix ICP object name to be in sync with the core name */
>>>> +        snprintf(name, sizeof(name), "icp[%d]", i);
>>>
>>> It may end up being the same value, but since the qom name is exposed
>>> to the outside, it would be better to have it be the PIR, rather than
>>> the cpu_index.
>>
>> The problem is that the ICPState objects are created before the PnvChip
>> objects. The PnvChip sanitizes the core layout in terms HW id and then 
>> creates the PnvCore objects. The core creates a PowerPCCPU object per 
>> thread and, in the realize function, uses the XICSFabric to look for
>> a matching ICP to link the CPU with. 
>>
>> So it is a little complex to do what you ask for ...
>>
>> What would really simplify the code, would be to allocate a TYPE_PNV_ICP
>> object when the PowerPCCPU object is realized and store it under the 
>> 'icp/intc' backlink. The XICSFabric handler icp_get() would not need 
>> the 'icps' array anymore. 
>>
>> How's that proposal ?
> 
> Sounds workable.  You could do that from the core realize function,
> couldn't you?

OK, it would look better from a QOM perspective, I agree, as we would 
not "realize" the PowerPCCPU object before creating the ICP object.
I will send an update of this patch for you to review : 

	[PATCH v4 5/9] ppc/pnv: create the ICP object under PnvCore


We could follow the exact same pattern for the sPAPR machine if we 
knew which ICP type to use (KVM or not). May be a new XICSFabric 
handler :

	const char *icp_type(XICSFabric *xi)

would help for that ? I don't think we can use a property because of 
the hotplug mechanism.

 
>>>> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
>>>> +        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
>>>> +                                  &error_fatal);
>>>> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
>>>> +                                       &error_fatal);
>>>> +        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
>>>> +    }
>>>> +
>>>>      /* Create the processor chips */
>>>>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
>>>>      if (!object_class_by_name(chip_typename)) {
>>>> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
>>>>      .abstract      = true,
>>>>  };
>>>>  
>>>> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>>>> +{
>>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < pnv->num_chips; i++) {
>>>> +        /* place holder */
>>>> +    }
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +static void pnv_ics_resend(XICSFabric *xi)
>>>> +{
>>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < pnv->num_chips; i++) {
>>>> +        /* place holder */
>>>> +    }
>>>> +}
>>>
>>> Seems like the above two functions belong in a later patch.
>>
>> OK. I guess we can add these handlers in the next patchset introducing PSI. 
>> I will check that the tests still run because the XICS layer uses the 
>> XICSFabric handlers blindly without any checks.
> 
> Well, sure, but the whole XICS isn't going to work without real
> implementations of the ICS callbacks, so I don't see that it matters.

Just to be very clear, what I meant is that we need to define all
the handlers at once because the XICS layer does not check the 
handler validity and we would end up calling NULL. In patchset v4,
the handlers are empty routines. 

Thanks,

C. 
 
>>  
>>>> +
>>>> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
>>>> +{
>>>> +    CPUState *cs;
>>>> +
>>>> +    CPU_FOREACH(cs) {
>>>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>> +        CPUPPCState *env = &cpu->env;
>>>> +
>>>> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
>>>> +            return cpu;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
>>>> +{
>>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
>>>> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>>>> +
>>>> +    if (!cpu) {
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
>>>> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
>>>
>>> Should use CPU() instead of parent_obj here.
>>
>> OK. I might just remove the array though.
>>
>> Thanks,
>>
>> C.
>>
>>
>>>> +}
>>>> +
>>>> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
>>>> +                               Monitor *mon)
>>>> +{
>>>> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < pnv->nr_servers; i++) {
>>>> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
>>>> +    }
>>>> +
>>>> +    for (i = 0; i < pnv->num_chips; i++) {
>>>> +        /* place holder */
>>>> +    }
>>>> +}
>>>> +
>>>>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
>>>>                                void *opaque, Error **errp)
>>>>  {
>>>> @@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
>>>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>>>>  {
>>>>      MachineClass *mc = MACHINE_CLASS(oc);
>>>> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
>>>> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>>>>  
>>>>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>>>>      mc->init = ppc_powernv_init;
>>>> @@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
>>>>      mc->no_parallel = 1;
>>>>      mc->default_boot_order = NULL;
>>>>      mc->default_ram_size = 1 * G_BYTE;
>>>> +    xic->icp_get = pnv_icp_get;
>>>> +    xic->ics_get = pnv_ics_get;
>>>> +    xic->ics_resend = pnv_ics_resend;
>>>> +    ispc->print_info = pnv_pic_print_info;
>>>>  
>>>>      powernv_machine_class_props_init(oc);
>>>>  }
>>>> @@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
>>>>      .instance_size = sizeof(PnvMachineState),
>>>>      .instance_init = powernv_machine_initfn,
>>>>      .class_init    = powernv_machine_class_init,
>>>> +    .interfaces = (InterfaceInfo[]) {
>>>> +        { TYPE_XICS_FABRIC },
>>>> +        { TYPE_INTERRUPT_STATS_PROVIDER },
>>>> +        { },
>>>> +    },
>>>>  };
>>>>  
>>>>  static void powernv_machine_register_types(void)
>>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>>> index df98a72006e4..1ca197d2ec83 100644
>>>> --- a/include/hw/ppc/pnv.h
>>>> +++ b/include/hw/ppc/pnv.h
>>>> @@ -22,6 +22,7 @@
>>>>  #include "hw/boards.h"
>>>>  #include "hw/sysbus.h"
>>>>  #include "hw/ppc/pnv_lpc.h"
>>>> +#include "hw/ppc/xics.h"
>>>>  
>>>>  #define TYPE_PNV_CHIP "powernv-chip"
>>>>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
>>>> @@ -114,6 +115,8 @@ typedef struct PnvMachineState {
>>>>      PnvChip      **chips;
>>>>  
>>>>      ISABus       *isa_bus;
>>>> +    PnvICPState  *icps;
>>>> +    uint32_t     nr_servers;
>>>>  } PnvMachineState;
>>>>  
>>>>  #define PNV_FDT_ADDR          0x01000000
>>>
>>
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine
  2017-03-30  8:15         ` Cédric Le Goater
@ 2017-04-02  6:11           ` David Gibson
  0 siblings, 0 replies; 19+ messages in thread
From: David Gibson @ 2017-04-02  6:11 UTC (permalink / raw)
  To: Cédric Le Goater; +Cc: qemu-ppc, qemu-devel

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

On Thu, Mar 30, 2017 at 10:15:11AM +0200, Cédric Le Goater wrote:
> On 03/30/2017 03:55 AM, David Gibson wrote:
> > On Wed, Mar 29, 2017 at 10:13:59AM +0200, Cédric Le Goater wrote:
> >> On 03/29/2017 07:18 AM, David Gibson wrote:
> >>> On Tue, Mar 28, 2017 at 09:32:29AM +0200, Cédric Le Goater wrote:
> >>>> Like this is done for the sPAPR machine, we use a simple array under
> >>>> the PowerNV machine to store the Interrupt Control Presenters (ICP)
> >>>> objects, one for each vCPU. This array is indexed by 'cpu_index' of
> >>>> the CPUState but the users will provide a core PIR number. The mapping
> >>>> is done in the icp_get() handler of the machine and is transparent to
> >>>> XICS.
> >>>>
> >>>> The Interrupt Control Sources (ICS), Processor Service Interface and
> >>>> PCI-E interface models, will be introduced in subsequent patches. For
> >>>> now, we have none, so we just prepare ground with place holders.
> >>>>
> >>>> Finally, to interface with the XICS layer which manipulates the ICP
> >>>> and ICS objects, we extend the PowerNV machine with an XICSFabric
> >>>> interface and its associated handlers.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >>>> ---
> >>>>
> >>>>  Changes since v2:
> >>>>
> >>>>  - removed the list of ICS. The handlers will iterate on the chips to
> >>>>    use the available ICS.
> >>>>
> >>>>  Changes since v1:
> >>>>
> >>>>  - handled pir-to-cpu_index mapping under icp_get 
> >>>>  - removed ics_eio handler
> >>>>  - changed ICP name indexing
> >>>>  - removed sysbus parenting of the ICP object
> >>>>
> >>>>  hw/ppc/pnv.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/hw/ppc/pnv.h |  3 ++
> >>>>  2 files changed, 99 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >>>> index 3fa722af82e6..e441b8ac1cad 100644
> >>>> --- a/hw/ppc/pnv.c
> >>>> +++ b/hw/ppc/pnv.c
> >>>> @@ -33,7 +33,10 @@
> >>>>  #include "exec/address-spaces.h"
> >>>>  #include "qemu/cutils.h"
> >>>>  #include "qapi/visitor.h"
> >>>> +#include "monitor/monitor.h"
> >>>> +#include "hw/intc/intc.h"
> >>>>  
> >>>> +#include "hw/ppc/xics.h"
> >>>>  #include "hw/ppc/pnv_xscom.h"
> >>>>  
> >>>>  #include "hw/isa/isa.h"
> >>>> @@ -417,6 +420,23 @@ static void ppc_powernv_init(MachineState *machine)
> >>>>          machine->cpu_model = "POWER8";
> >>>>      }
> >>>>  
> >>>> +    /* Create the Interrupt Control Presenters before the vCPUs */
> >>>> +    pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads;
> >>>> +    pnv->icps = g_new0(PnvICPState, pnv->nr_servers);
> >>>> +    for (i = 0; i < pnv->nr_servers; i++) {
> >>>> +        PnvICPState *icp = &pnv->icps[i];
> >>>> +        char name[32];
> >>>> +
> >>>> +        /* TODO: fix ICP object name to be in sync with the core name */
> >>>> +        snprintf(name, sizeof(name), "icp[%d]", i);
> >>>
> >>> It may end up being the same value, but since the qom name is exposed
> >>> to the outside, it would be better to have it be the PIR, rather than
> >>> the cpu_index.
> >>
> >> The problem is that the ICPState objects are created before the PnvChip
> >> objects. The PnvChip sanitizes the core layout in terms HW id and then 
> >> creates the PnvCore objects. The core creates a PowerPCCPU object per 
> >> thread and, in the realize function, uses the XICSFabric to look for
> >> a matching ICP to link the CPU with. 
> >>
> >> So it is a little complex to do what you ask for ...
> >>
> >> What would really simplify the code, would be to allocate a TYPE_PNV_ICP
> >> object when the PowerPCCPU object is realized and store it under the 
> >> 'icp/intc' backlink. The XICSFabric handler icp_get() would not need 
> >> the 'icps' array anymore. 
> >>
> >> How's that proposal ?
> > 
> > Sounds workable.  You could do that from the core realize function,
> > couldn't you?
> 
> OK, it would look better from a QOM perspective, I agree, as we would 
> not "realize" the PowerPCCPU object before creating the ICP object.
> I will send an update of this patch for you to review : 
> 
> 	[PATCH v4 5/9] ppc/pnv: create the ICP object under PnvCore
> 
> 
> We could follow the exact same pattern for the sPAPR machine if we 
> knew which ICP type to use (KVM or not). May be a new XICSFabric 
> handler :
> 
> 	const char *icp_type(XICSFabric *xi)
> 
> would help for that ? I don't think we can use a property because of 
> the hotplug mechanism.

Hmm, couldn't we just move our existing logic for deciding the ICP
type into the spapr core object?

> 
>  
> >>>> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
> >>>> +        object_property_add_child(OBJECT(pnv), name, OBJECT(icp),
> >>>> +                                  &error_fatal);
> >>>> +        object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv),
> >>>> +                                       &error_fatal);
> >>>> +        object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal);
> >>>> +    }
> >>>> +
> >>>>      /* Create the processor chips */
> >>>>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model);
> >>>>      if (!object_class_by_name(chip_typename)) {
> >>>> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
> >>>>      .abstract      = true,
> >>>>  };
> >>>>  
> >>>> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
> >>>> +{
> >>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < pnv->num_chips; i++) {
> >>>> +        /* place holder */
> >>>> +    }
> >>>> +    return NULL;
> >>>> +}
> >>>> +
> >>>> +static void pnv_ics_resend(XICSFabric *xi)
> >>>> +{
> >>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < pnv->num_chips; i++) {
> >>>> +        /* place holder */
> >>>> +    }
> >>>> +}
> >>>
> >>> Seems like the above two functions belong in a later patch.
> >>
> >> OK. I guess we can add these handlers in the next patchset introducing PSI. 
> >> I will check that the tests still run because the XICS layer uses the 
> >> XICSFabric handlers blindly without any checks.
> > 
> > Well, sure, but the whole XICS isn't going to work without real
> > implementations of the ICS callbacks, so I don't see that it matters.
> 
> Just to be very clear, what I meant is that we need to define all
> the handlers at once because the XICS layer does not check the 
> handler validity and we would end up calling NULL. In patchset v4,
> the handlers are empty routines. 
> 
> Thanks,
> 
> C. 
>  
> >>  
> >>>> +
> >>>> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
> >>>> +{
> >>>> +    CPUState *cs;
> >>>> +
> >>>> +    CPU_FOREACH(cs) {
> >>>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>>> +        CPUPPCState *env = &cpu->env;
> >>>> +
> >>>> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
> >>>> +            return cpu;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    return NULL;
> >>>> +}
> >>>> +
> >>>> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> >>>> +{
> >>>> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >>>> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> >>>> +
> >>>> +    if (!cpu) {
> >>>> +        return NULL;
> >>>> +    }
> >>>> +
> >>>> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
> >>>> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
> >>>
> >>> Should use CPU() instead of parent_obj here.
> >>
> >> OK. I might just remove the array though.
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>>> +}
> >>>> +
> >>>> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
> >>>> +                               Monitor *mon)
> >>>> +{
> >>>> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> >>>> +    int i;
> >>>> +
> >>>> +    for (i = 0; i < pnv->nr_servers; i++) {
> >>>> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
> >>>> +    }
> >>>> +
> >>>> +    for (i = 0; i < pnv->num_chips; i++) {
> >>>> +        /* place holder */
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> >>>>                                void *opaque, Error **errp)
> >>>>  {
> >>>> @@ -787,6 +872,8 @@ static void powernv_machine_class_props_init(ObjectClass *oc)
> >>>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
> >>>>  {
> >>>>      MachineClass *mc = MACHINE_CLASS(oc);
> >>>> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> >>>> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> >>>>  
> >>>>      mc->desc = "IBM PowerNV (Non-Virtualized)";
> >>>>      mc->init = ppc_powernv_init;
> >>>> @@ -797,6 +884,10 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data)
> >>>>      mc->no_parallel = 1;
> >>>>      mc->default_boot_order = NULL;
> >>>>      mc->default_ram_size = 1 * G_BYTE;
> >>>> +    xic->icp_get = pnv_icp_get;
> >>>> +    xic->ics_get = pnv_ics_get;
> >>>> +    xic->ics_resend = pnv_ics_resend;
> >>>> +    ispc->print_info = pnv_pic_print_info;
> >>>>  
> >>>>      powernv_machine_class_props_init(oc);
> >>>>  }
> >>>> @@ -807,6 +898,11 @@ static const TypeInfo powernv_machine_info = {
> >>>>      .instance_size = sizeof(PnvMachineState),
> >>>>      .instance_init = powernv_machine_initfn,
> >>>>      .class_init    = powernv_machine_class_init,
> >>>> +    .interfaces = (InterfaceInfo[]) {
> >>>> +        { TYPE_XICS_FABRIC },
> >>>> +        { TYPE_INTERRUPT_STATS_PROVIDER },
> >>>> +        { },
> >>>> +    },
> >>>>  };
> >>>>  
> >>>>  static void powernv_machine_register_types(void)
> >>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >>>> index df98a72006e4..1ca197d2ec83 100644
> >>>> --- a/include/hw/ppc/pnv.h
> >>>> +++ b/include/hw/ppc/pnv.h
> >>>> @@ -22,6 +22,7 @@
> >>>>  #include "hw/boards.h"
> >>>>  #include "hw/sysbus.h"
> >>>>  #include "hw/ppc/pnv_lpc.h"
> >>>> +#include "hw/ppc/xics.h"
> >>>>  
> >>>>  #define TYPE_PNV_CHIP "powernv-chip"
> >>>>  #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP)
> >>>> @@ -114,6 +115,8 @@ typedef struct PnvMachineState {
> >>>>      PnvChip      **chips;
> >>>>  
> >>>>      ISABus       *isa_bus;
> >>>> +    PnvICPState  *icps;
> >>>> +    uint32_t     nr_servers;
> >>>>  } PnvMachineState;
> >>>>  
> >>>>  #define PNV_FDT_ADDR          0x01000000
> >>>
> >>
> > 
> 

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

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-04-02  6:35 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28  7:32 [Qemu-devel] [PATCH v3 0/8] ppc/pnv: interrupt controller (POWER8) Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 1/8] ppc/xics: introduce an 'icp' backlink under PowerPCCPU Cédric Le Goater
2017-03-29  4:11   ` David Gibson
2017-03-29  7:14     ` Cédric Le Goater
2017-03-30  1:26       ` David Gibson
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 2/8] spapr: move the IRQ server number mapping under the machine Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 3/8] ppc/xics: add a realize() handler to ICPStateClass Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 4/8] ppc/pnv: add a PnvICPState object Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine Cédric Le Goater
2017-03-29  5:18   ` David Gibson
2017-03-29  8:13     ` Cédric Le Goater
2017-03-30  1:55       ` David Gibson
2017-03-30  8:15         ` Cédric Le Goater
2017-04-02  6:11           ` David Gibson
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 6/8] ppc/pnv: add a helper to calculate MMIO addresses registers Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 7/8] ppc/pnv: link the CPUs to the machine XICSFabric Cédric Le Goater
2017-03-29  5:20   ` David Gibson
2017-03-29  7:16     ` Cédric Le Goater
2017-03-28  7:32 ` [Qemu-devel] [PATCH v3 8/8] ppc/pnv: add memory regions for the ICP registers Cédric Le Goater

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.