[PATCH v5 0/7] ppc: reset the interrupt presenter from the CPU reset handler

[PATCH v5 0/7] ppc: reset the interrupt presenter from the CPU reset handler

[Cédric Le Goater]

Hello,

On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reseted
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reseted. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.

Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.

Set the OS CAM line when the interrupt presenter of the sPAPR core is
reseted. This will also cover the case of hot-plugged CPUs.

Thanks,

C.

Changes in v5:

 - Removed useless PNV_CHIP() cast
 
Changes in v4:

 - Introduce a PnvCore reset handler
 - Add PnvChip pointer to PnvCore

Changes in v3:

 - Introduced a DeviceClass::reset for the CPU (Greg)
 - add support for PowerNV
 
Changes in v2:

 - removed property
 - simplified reset handlers

Cédric Le Goater (6):
  spapr: move CPU reset after presenter creation
  ppc/pnv: Introduce a PnvCore reset handler
  ppc/pnv: Add a PnvChip pointer to PnvCore
  ppc: Reset the interrupt presenter from the CPU reset handler
  ppc/pnv: Fix naming of routines realizing the CPUs
  spapr/xive: Set the OS CAM line at reset

Greg Kurz (1):
  spapr_cpu_core: Implement DeviceClass::reset

 include/hw/ppc/pnv.h        |  1 +
 include/hw/ppc/pnv_core.h   |  3 +++
 include/hw/ppc/spapr_irq.h  |  2 ++
 include/hw/ppc/spapr_xive.h |  1 -
 include/hw/ppc/xics.h       |  1 +
 include/hw/ppc/xive.h       |  1 +
 hw/intc/spapr_xive.c        | 53 +++++++++++++++++--------------------
 hw/intc/xics.c              |  8 ++----
 hw/intc/xics_spapr.c        |  7 +++++
 hw/intc/xive.c              | 12 +--------
 hw/ppc/pnv.c                | 18 +++++++++++++
 hw/ppc/pnv_core.c           | 31 ++++++++++++++++------
 hw/ppc/spapr_cpu_core.c     | 44 +++++++++++++++++++++++-------
 hw/ppc/spapr_irq.c          | 14 ++++++++++
 14 files changed, 131 insertions(+), 65 deletions(-)

-- 
2.21.0

[PATCH v5 1/7] spapr: move CPU reset after presenter creation

[Cédric Le Goater]

This change prepares ground for future changes which will reset the
interrupt presenter in the reset handler of the sPAPR and PowerNV
cores.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_cpu_core.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3e4302c7d596..2b21285d2009 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -234,13 +234,16 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
     cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
     kvmppc_set_papr(cpu);
 
-    qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
-
     if (spapr_irq_cpu_intc_create(spapr, cpu, &local_err) < 0) {
-        goto error_unregister;
+        goto error_intc_create;
     }
 
+    /*
+     * FIXME: Hot-plugged CPUs are not reset. Do it at realize.
+     */
+    qemu_register_reset(spapr_cpu_reset, cpu);
+    spapr_cpu_reset(cpu);
+
     if (!sc->pre_3_0_migration) {
         vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
                          cpu->machine_data);
@@ -248,8 +251,7 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
     return;
 
-error_unregister:
-    qemu_unregister_reset(spapr_cpu_reset, cpu);
+error_intc_create:
     cpu_remove_sync(CPU(cpu));
 error:
     error_propagate(errp, local_err);
-- 
2.21.0

[PATCH v5 2/7] spapr_cpu_core: Implement DeviceClass::reset

[Cédric Le Goater]

From: Greg Kurz <groug@kaod.org>

Since vCPUs aren't plugged into a bus, we manually register a reset
handler for each vCPU. We also call this handler at realize time
to ensure hot plugged vCPUs are reset before being exposed to the
guest. This results in vCPUs being reset twice at machine reset.
It doesn't break anything but it is slightly suboptimal and above
all confusing.

The hotplug path in device_set_realized() already knows how to reset
a hotplugged device if the device reset handler is present. Implement
one for sPAPR CPU cores that resets all vCPUs under a core.

While here rename spapr_cpu_reset() to spapr_reset_vcpu() for
consistency with spapr_realize_vcpu() and spapr_unrealize_vcpu().

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
[clg: add documentation on the reset helper usage ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/spapr_cpu_core.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2b21285d2009..2e34832d0ea2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -25,9 +25,8 @@
 #include "sysemu/hw_accel.h"
 #include "qemu/error-report.h"
 
-static void spapr_cpu_reset(void *opaque)
+static void spapr_reset_vcpu(PowerPCCPU *cpu)
 {
-    PowerPCCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -193,7 +192,6 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
     if (!sc->pre_3_0_migration) {
         vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
     }
-    qemu_unregister_reset(spapr_cpu_reset, cpu);
     if (spapr_cpu_state(cpu)->icp) {
         object_unparent(OBJECT(spapr_cpu_state(cpu)->icp));
     }
@@ -204,12 +202,36 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
     object_unparent(OBJECT(cpu));
 }
 
+/*
+ * Called when CPUs are hot-plugged.
+ */
+static void spapr_cpu_core_reset(DeviceState *dev)
+{
+    CPUCore *cc = CPU_CORE(dev);
+    SpaprCpuCore *sc = SPAPR_CPU_CORE(dev);
+    int i;
+
+    for (i = 0; i < cc->nr_threads; i++) {
+        spapr_reset_vcpu(sc->threads[i]);
+    }
+}
+
+/*
+ * Called by the machine reset.
+ */
+static void spapr_cpu_core_reset_handler(void *opaque)
+{
+    spapr_cpu_core_reset(opaque);
+}
+
 static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
 {
     SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
     int i;
 
+    qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
+
     for (i = 0; i < cc->nr_threads; i++) {
         spapr_unrealize_vcpu(sc->threads[i], sc);
     }
@@ -238,12 +260,6 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, SpaprMachineState *spapr,
         goto error_intc_create;
     }
 
-    /*
-     * FIXME: Hot-plugged CPUs are not reset. Do it at realize.
-     */
-    qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
-
     if (!sc->pre_3_0_migration) {
         vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
                          cpu->machine_data);
@@ -338,6 +354,8 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
             goto err_unrealize;
         }
     }
+
+    qemu_register_reset(spapr_cpu_core_reset_handler, sc);
     return;
 
 err_unrealize:
@@ -366,6 +384,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
 
     dc->realize = spapr_cpu_core_realize;
     dc->unrealize = spapr_cpu_core_unrealize;
+    dc->reset = spapr_cpu_core_reset;
     dc->props = spapr_cpu_core_properties;
     scc->cpu_type = data;
 }
-- 
2.21.0

[PATCH v5 3/7] ppc/pnv: Introduce a PnvCore reset handler

[Cédric Le Goater]

in which individual CPUs are reset. It will ease the introduction of
future change reseting the interrupt presenter from the CPU reset
handler.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv_core.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index b1a7489e7abf..9f981a4940e6 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -40,9 +40,8 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
     return cpu_type;
 }
 
-static void pnv_cpu_reset(void *opaque)
+static void pnv_core_cpu_reset(PowerPCCPU *cpu)
 {
-    PowerPCCPU *cpu = opaque;
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
 
@@ -192,8 +191,17 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
+}
+
+static void pnv_core_reset(void *dev)
+{
+    CPUCore *cc = CPU_CORE(dev);
+    PnvCore *pc = PNV_CORE(dev);
+    int i;
 
-    qemu_register_reset(pnv_cpu_reset, cpu);
+    for (i = 0; i < cc->nr_threads; i++) {
+        pnv_core_cpu_reset(pc->threads[i]);
+    }
 }
 
 static void pnv_core_realize(DeviceState *dev, Error **errp)
@@ -244,6 +252,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
     pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
                           pc, name, PNV_XSCOM_EX_SIZE);
+
+    qemu_register_reset(pnv_core_reset, pc);
     return;
 
 err:
@@ -259,7 +269,6 @@ static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
 {
     PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
 
-    qemu_unregister_reset(pnv_cpu_reset, cpu);
     object_unparent(OBJECT(pnv_cpu_state(cpu)->intc));
     cpu_remove_sync(CPU(cpu));
     cpu->machine_data = NULL;
@@ -273,6 +282,8 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
     CPUCore *cc = CPU_CORE(dev);
     int i;
 
+    qemu_unregister_reset(pnv_core_reset, pc);
+
     for (i = 0; i < cc->nr_threads; i++) {
         pnv_unrealize_vcpu(pc->threads[i]);
     }
-- 
2.21.0

Re: [PATCH v5 3/7] ppc/pnv: Introduce a PnvCore reset handler

[Philippe Mathieu-Daudé]

Hi Cédric,

On 10/22/19 6:38 PM, Cédric Le Goater wrote:
> in which individual CPUs are reset. It will ease the introduction of
> future change reseting the interrupt presenter from the CPU reset
> handler.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/pnv_core.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index b1a7489e7abf..9f981a4940e6 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -40,9 +40,8 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
>       return cpu_type;
>   }
>   
> -static void pnv_cpu_reset(void *opaque)
> +static void pnv_core_cpu_reset(PowerPCCPU *cpu)
>   {
> -    PowerPCCPU *cpu = opaque;
>       CPUState *cs = CPU(cpu);
>       CPUPPCState *env = &cpu->env;
>   
> @@ -192,8 +191,17 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>   
>       /* Set time-base frequency to 512 MHz */
>       cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> +}
> +
> +static void pnv_core_reset(void *dev)

Here the opaque pointer is a 'PnvCore *pc'.
If you don't want to call it 'opaque', maybe 'pc' is better.

> +{
> +    CPUCore *cc = CPU_CORE(dev);
> +    PnvCore *pc = PNV_CORE(dev);

This type conversion is not necessary.

What about:

    static void pnv_core_reset(void *opaque)
    {
        PnvCore *pc = opaque;
        CPUCore *cc = CPU_CORE(pc);

> +    int i;
>   
> -    qemu_register_reset(pnv_cpu_reset, cpu);
> +    for (i = 0; i < cc->nr_threads; i++) {
> +        pnv_core_cpu_reset(pc->threads[i]);
> +    }
>   }
>   
>   static void pnv_core_realize(DeviceState *dev, Error **errp)
> @@ -244,6 +252,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>       snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
>       pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
>                             pc, name, PNV_XSCOM_EX_SIZE);
> +
> +    qemu_register_reset(pnv_core_reset, pc);
>       return;
>   
>   err:
> @@ -259,7 +269,6 @@ static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
>   {
>       PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
>   
> -    qemu_unregister_reset(pnv_cpu_reset, cpu);
>       object_unparent(OBJECT(pnv_cpu_state(cpu)->intc));
>       cpu_remove_sync(CPU(cpu));
>       cpu->machine_data = NULL;
> @@ -273,6 +282,8 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
>       CPUCore *cc = CPU_CORE(dev);
>       int i;
>   
> +    qemu_unregister_reset(pnv_core_reset, pc);
> +
>       for (i = 0; i < cc->nr_threads; i++) {
>           pnv_unrealize_vcpu(pc->threads[i]);
>       }
> 

Re: [PATCH v5 3/7] ppc/pnv: Introduce a PnvCore reset handler

[David Gibson]

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

On Wed, Oct 23, 2019 at 01:18:06PM +0200, Philippe Mathieu-Daudé wrote:
> Hi Cédric,
> 
> On 10/22/19 6:38 PM, Cédric Le Goater wrote:
> > in which individual CPUs are reset. It will ease the introduction of
> > future change reseting the interrupt presenter from the CPU reset
> > handler.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> >   hw/ppc/pnv_core.c | 19 +++++++++++++++----
> >   1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index b1a7489e7abf..9f981a4940e6 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -40,9 +40,8 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
> >       return cpu_type;
> >   }
> > -static void pnv_cpu_reset(void *opaque)
> > +static void pnv_core_cpu_reset(PowerPCCPU *cpu)
> >   {
> > -    PowerPCCPU *cpu = opaque;
> >       CPUState *cs = CPU(cpu);
> >       CPUPPCState *env = &cpu->env;
> > @@ -192,8 +191,17 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
> >       /* Set time-base frequency to 512 MHz */
> >       cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> > +}
> > +
> > +static void pnv_core_reset(void *dev)
> 
> Here the opaque pointer is a 'PnvCore *pc'.
> If you don't want to call it 'opaque', maybe 'pc' is better.
> 
> > +{
> > +    CPUCore *cc = CPU_CORE(dev);
> > +    PnvCore *pc = PNV_CORE(dev);
> 
> This type conversion is not necessary.
> 
> What about:
> 
>    static void pnv_core_reset(void *opaque)
>    {
>        PnvCore *pc = opaque;
>        CPUCore *cc = CPU_CORE(pc);

Those suggestions look good to me, but they can be done as a follow up.

> 
> > +    int i;
> > -    qemu_register_reset(pnv_cpu_reset, cpu);
> > +    for (i = 0; i < cc->nr_threads; i++) {
> > +        pnv_core_cpu_reset(pc->threads[i]);
> > +    }
> >   }
> >   static void pnv_core_realize(DeviceState *dev, Error **errp)
> > @@ -244,6 +252,8 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >       snprintf(name, sizeof(name), "xscom-core.%d", cc->core_id);
> >       pnv_xscom_region_init(&pc->xscom_regs, OBJECT(dev), pcc->xscom_ops,
> >                             pc, name, PNV_XSCOM_EX_SIZE);
> > +
> > +    qemu_register_reset(pnv_core_reset, pc);
> >       return;
> >   err:
> > @@ -259,7 +269,6 @@ static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> >   {
> >       PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> > -    qemu_unregister_reset(pnv_cpu_reset, cpu);
> >       object_unparent(OBJECT(pnv_cpu_state(cpu)->intc));
> >       cpu_remove_sync(CPU(cpu));
> >       cpu->machine_data = NULL;
> > @@ -273,6 +282,8 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
> >       CPUCore *cc = CPU_CORE(dev);
> >       int i;
> > +    qemu_unregister_reset(pnv_core_reset, pc);
> > +
> >       for (i = 0; i < cc->nr_threads; i++) {
> >           pnv_unrealize_vcpu(pc->threads[i]);
> >       }
> > 
> 

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

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

[PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Cédric Le Goater]

We will use it to reset the interrupt presenter from the CPU reset
handler.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/pnv_core.h | 3 +++
 hw/ppc/pnv_core.c         | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index bfbd2ec42aa6..55eee95104da 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -31,6 +31,8 @@
 #define PNV_CORE_GET_CLASS(obj) \
      OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
 
+typedef struct PnvChip PnvChip;
+
 typedef struct PnvCore {
     /*< private >*/
     CPUCore parent_obj;
@@ -38,6 +40,7 @@ typedef struct PnvCore {
     /*< public >*/
     PowerPCCPU **threads;
     uint32_t pir;
+    PnvChip *chip;
 
     MemoryRegion xscom_regs;
 } PnvCore;
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 9f981a4940e6..cc17bbfed829 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
                                 "required link 'chip' not found: ");
         return;
     }
+    pc->chip = PNV_CHIP(chip);
 
     pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
+        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
         if (local_err) {
             goto err;
         }
-- 
2.21.0

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Philippe Mathieu-Daudé]

On 10/22/19 6:38 PM, Cédric Le Goater wrote:
> We will use it to reset the interrupt presenter from the CPU reset
> handler.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>   include/hw/ppc/pnv_core.h | 3 +++
>   hw/ppc/pnv_core.c         | 3 ++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index bfbd2ec42aa6..55eee95104da 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -31,6 +31,8 @@
>   #define PNV_CORE_GET_CLASS(obj) \
>        OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
>   
> +typedef struct PnvChip PnvChip;
> +
>   typedef struct PnvCore {
>       /*< private >*/
>       CPUCore parent_obj;
> @@ -38,6 +40,7 @@ typedef struct PnvCore {
>       /*< public >*/
>       PowerPCCPU **threads;
>       uint32_t pir;
> +    PnvChip *chip;
>   
>       MemoryRegion xscom_regs;
>   } PnvCore;
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 9f981a4940e6..cc17bbfed829 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>                                   "required link 'chip' not found: ");
>           return;
>       }
> +    pc->chip = PNV_CHIP(chip);
>   
>       pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>       for (i = 0; i < cc->nr_threads; i++) {
> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>       }
>   
>       for (j = 0; j < cc->nr_threads; j++) {
> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
>           if (local_err) {
>               goto err;
>           }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[David Gibson]

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

On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> We will use it to reset the interrupt presenter from the CPU reset
> handler.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  include/hw/ppc/pnv_core.h | 3 +++
>  hw/ppc/pnv_core.c         | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index bfbd2ec42aa6..55eee95104da 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -31,6 +31,8 @@
>  #define PNV_CORE_GET_CLASS(obj) \
>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
>  
> +typedef struct PnvChip PnvChip;
> +
>  typedef struct PnvCore {
>      /*< private >*/
>      CPUCore parent_obj;
> @@ -38,6 +40,7 @@ typedef struct PnvCore {
>      /*< public >*/
>      PowerPCCPU **threads;
>      uint32_t pir;
> +    PnvChip *chip;

I don't love having this as a redundant encoding of the information
already in the property, since it raises the possibility of confusing
bugs if they ever got out of sync.

It's not a huge deal, but it would be nice to at least to at least
consider either a) grabbing the property everywhere you need it (if
there aren't too many places) or b) customizing the property
definition so it's written directly into that field.

>  
>      MemoryRegion xscom_regs;
>  } PnvCore;
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 9f981a4940e6..cc17bbfed829 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>                                  "required link 'chip' not found: ");
>          return;
>      }
> +    pc->chip = PNV_CHIP(chip);
>  
>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &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: 833 bytes --]

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Cédric Le Goater]

On 24/10/2019 04:38, David Gibson wrote:
> On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
>> We will use it to reset the interrupt presenter from the CPU reset
>> handler.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> ---
>>  include/hw/ppc/pnv_core.h | 3 +++
>>  hw/ppc/pnv_core.c         | 3 ++-
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
>> index bfbd2ec42aa6..55eee95104da 100644
>> --- a/include/hw/ppc/pnv_core.h
>> +++ b/include/hw/ppc/pnv_core.h
>> @@ -31,6 +31,8 @@
>>  #define PNV_CORE_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
>>  
>> +typedef struct PnvChip PnvChip;
>> +
>>  typedef struct PnvCore {
>>      /*< private >*/
>>      CPUCore parent_obj;
>> @@ -38,6 +40,7 @@ typedef struct PnvCore {
>>      /*< public >*/
>>      PowerPCCPU **threads;
>>      uint32_t pir;
>> +    PnvChip *chip;
> 
> I don't love having this as a redundant encoding of the information
> already in the property, since it raises the possibility of confusing
> bugs if they ever got out of sync.

Indeed.

> It's not a huge deal, but it would be nice to at least to at least
> consider either a) grabbing the property everywhere you need it (if
> there aren't too many places) 

We need the chip at core creation and core reset to call the 
interrupt chip handlers. These are not hot path but the pointer
seemed practical.

> or b) customizing the property
> definition so it's written directly into that field.

OK. That is better than just a link.

C. 

> 
>>  
>>      MemoryRegion xscom_regs;
>>  } PnvCore;
>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>> index 9f981a4940e6..cc17bbfed829 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>                                  "required link 'chip' not found: ");
>>          return;
>>      }
>> +    pc->chip = PNV_CHIP(chip);
>>  
>>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>>      for (i = 0; i < cc->nr_threads; i++) {
>> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>      }
>>  
>>      for (j = 0; j < cc->nr_threads; j++) {
>> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
>> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
>>          if (local_err) {
>>              goto err;
>>          }
> 

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Greg Kurz]

On Thu, 24 Oct 2019 11:57:05 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 24/10/2019 04:38, David Gibson wrote:
> > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> >> We will use it to reset the interrupt presenter from the CPU reset
> >> handler.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >> ---
> >>  include/hw/ppc/pnv_core.h | 3 +++
> >>  hw/ppc/pnv_core.c         | 3 ++-
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> index bfbd2ec42aa6..55eee95104da 100644
> >> --- a/include/hw/ppc/pnv_core.h
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -31,6 +31,8 @@
> >>  #define PNV_CORE_GET_CLASS(obj) \
> >>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> >>  
> >> +typedef struct PnvChip PnvChip;
> >> +
> >>  typedef struct PnvCore {
> >>      /*< private >*/
> >>      CPUCore parent_obj;
> >> @@ -38,6 +40,7 @@ typedef struct PnvCore {
> >>      /*< public >*/
> >>      PowerPCCPU **threads;
> >>      uint32_t pir;
> >> +    PnvChip *chip;
> > 
> > I don't love having this as a redundant encoding of the information
> > already in the property, since it raises the possibility of confusing
> > bugs if they ever got out of sync.
> 
> Indeed.
> 
> > It's not a huge deal, but it would be nice to at least to at least
> > consider either a) grabbing the property everywhere you need it (if
> > there aren't too many places) 
> 
> We need the chip at core creation and core reset to call the 
> interrupt chip handlers. These are not hot path but the pointer
> seemed practical.
> 

FWIW this is also used at core destruction in this patch:

[1/3] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
https://patchwork.ozlabs.org/patch/1183158/

> > or b) customizing the property
> > definition so it's written directly into that field.
> 
> OK. That is better than just a link.
> 

I think David is suggesting to use object_property_add_link()
instead of object_property_add_const_link() actually. Something
like that:

diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index 55eee95104da..fce6d8d9b31b 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -36,11 +36,11 @@ typedef struct PnvChip PnvChip;
 typedef struct PnvCore {
     /*< private >*/
     CPUCore parent_obj;
+    PnvChip *chip;
 
     /*< public >*/
     PowerPCCPU **threads;
     uint32_t pir;
-    PnvChip *chip;
 
     MemoryRegion xscom_regs;
 } PnvCore;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 68cc3c81aa75..90449d33e422 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1312,8 +1312,8 @@ static void pnv_chip_core_realize(PnvChip *chip, 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), "chip",
-                                       OBJECT(chip), &error_fatal);
+        object_property_set_link(OBJECT(pnv_core), OBJECT(chip), "chip",
+                                 &error_abort);
         object_property_set_bool(OBJECT(pnv_core), true, "realized",
                                  &error_fatal);
 
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 61b3d3ce2250..8562a9961845 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -217,15 +217,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     void *obj;
     int i, j;
     char name[32];
-    Object *chip;
-
-    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
-    if (!chip) {
-        error_propagate_prepend(errp, local_err,
-                                "required link 'chip' not found: ");
-        return;
-    }
-    pc->chip = PNV_CHIP(chip);
 
     pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
@@ -323,6 +314,14 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
     dc->props = pnv_core_properties;
 }
 
+static void pnv_core_instance_init(Object *obj)
+{
+    object_property_add_link(obj, "chip", TYPE_PNV_CHIP,
+                             (Object **) &PNV_CORE(obj)->chip,
+                             qdev_prop_allow_set_link_before_realize,
+                             0, &error_abort);
+}
+
 #define DEFINE_PNV_CORE_TYPE(family, cpu_model) \
     {                                           \
         .parent = TYPE_PNV_CORE,                \
@@ -335,6 +334,7 @@ static const TypeInfo pnv_core_infos[] = {
         .name           = TYPE_PNV_CORE,
         .parent         = TYPE_CPU_CORE,
         .instance_size  = sizeof(PnvCore),
+        .instance_init  = pnv_core_instance_init,
         .class_size     = sizeof(PnvCoreClass),
         .class_init = pnv_core_class_init,
         .abstract       = true,
----

> C. 
> 
> > 
> >>  
> >>      MemoryRegion xscom_regs;
> >>  } PnvCore;
> >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> >> index 9f981a4940e6..cc17bbfed829 100644
> >> --- a/hw/ppc/pnv_core.c
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >>                                  "required link 'chip' not found: ");
> >>          return;
> >>      }
> >> +    pc->chip = PNV_CHIP(chip);
> >>  
> >>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >>      for (i = 0; i < cc->nr_threads; i++) {
> >> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >>      }
> >>  
> >>      for (j = 0; j < cc->nr_threads; j++) {
> >> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> >> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
> >>          if (local_err) {
> >>              goto err;
> >>          }
> > 
> 

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[David Gibson]

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

On Thu, Oct 24, 2019 at 06:48:23PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 11:57:05 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > On 24/10/2019 04:38, David Gibson wrote:
> > > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> > >> We will use it to reset the interrupt presenter from the CPU reset
> > >> handler.
> > >>
> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > >> Reviewed-by: Greg Kurz <groug@kaod.org>
> > >> ---
> > >>  include/hw/ppc/pnv_core.h | 3 +++
> > >>  hw/ppc/pnv_core.c         | 3 ++-
> > >>  2 files changed, 5 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > >> index bfbd2ec42aa6..55eee95104da 100644
> > >> --- a/include/hw/ppc/pnv_core.h
> > >> +++ b/include/hw/ppc/pnv_core.h
> > >> @@ -31,6 +31,8 @@
> > >>  #define PNV_CORE_GET_CLASS(obj) \
> > >>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> > >>  
> > >> +typedef struct PnvChip PnvChip;
> > >> +
> > >>  typedef struct PnvCore {
> > >>      /*< private >*/
> > >>      CPUCore parent_obj;
> > >> @@ -38,6 +40,7 @@ typedef struct PnvCore {
> > >>      /*< public >*/
> > >>      PowerPCCPU **threads;
> > >>      uint32_t pir;
> > >> +    PnvChip *chip;
> > > 
> > > I don't love having this as a redundant encoding of the information
> > > already in the property, since it raises the possibility of confusing
> > > bugs if they ever got out of sync.
> > 
> > Indeed.
> > 
> > > It's not a huge deal, but it would be nice to at least to at least
> > > consider either a) grabbing the property everywhere you need it (if
> > > there aren't too many places) 
> > 
> > We need the chip at core creation and core reset to call the 
> > interrupt chip handlers. These are not hot path but the pointer
> > seemed practical.
> > 
> 
> FWIW this is also used at core destruction in this patch:
> 
> [1/3] ppc: Add intc_destroy() handlers to SpaprInterruptController/PnvChip
> https://patchwork.ozlabs.org/patch/1183158/
> 
> > > or b) customizing the property
> > > definition so it's written directly into that field.
> > 
> > OK. That is better than just a link.
> > 
> 
> I think David is suggesting to use object_property_add_link()
> instead of object_property_add_const_link() actually. Something
> like that:

TBH, I hadn't looked into the mechanics of how to do this that
closely.  Change below looks pretty reasonable, though.

> 
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 55eee95104da..fce6d8d9b31b 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -36,11 +36,11 @@ typedef struct PnvChip PnvChip;
>  typedef struct PnvCore {
>      /*< private >*/
>      CPUCore parent_obj;
> +    PnvChip *chip;
>  
>      /*< public >*/
>      PowerPCCPU **threads;
>      uint32_t pir;
> -    PnvChip *chip;
>  
>      MemoryRegion xscom_regs;
>  } PnvCore;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 68cc3c81aa75..90449d33e422 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1312,8 +1312,8 @@ static void pnv_chip_core_realize(PnvChip *chip, 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), "chip",
> -                                       OBJECT(chip), &error_fatal);
> +        object_property_set_link(OBJECT(pnv_core), OBJECT(chip), "chip",
> +                                 &error_abort);
>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>                                   &error_fatal);
>  
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 61b3d3ce2250..8562a9961845 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -217,15 +217,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      void *obj;
>      int i, j;
>      char name[32];
> -    Object *chip;
> -
> -    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
> -    if (!chip) {
> -        error_propagate_prepend(errp, local_err,
> -                                "required link 'chip' not found: ");
> -        return;
> -    }
> -    pc->chip = PNV_CHIP(chip);
>  
>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -323,6 +314,14 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
>      dc->props = pnv_core_properties;
>  }
>  
> +static void pnv_core_instance_init(Object *obj)
> +{
> +    object_property_add_link(obj, "chip", TYPE_PNV_CHIP,
> +                             (Object **) &PNV_CORE(obj)->chip,
> +                             qdev_prop_allow_set_link_before_realize,
> +                             0, &error_abort);
> +}
> +
>  #define DEFINE_PNV_CORE_TYPE(family, cpu_model) \
>      {                                           \
>          .parent = TYPE_PNV_CORE,                \
> @@ -335,6 +334,7 @@ static const TypeInfo pnv_core_infos[] = {
>          .name           = TYPE_PNV_CORE,
>          .parent         = TYPE_CPU_CORE,
>          .instance_size  = sizeof(PnvCore),
> +        .instance_init  = pnv_core_instance_init,
>          .class_size     = sizeof(PnvCoreClass),
>          .class_init = pnv_core_class_init,
>          .abstract       = true,
> ----
> 
> > C. 
> > 
> > > 
> > >>  
> > >>      MemoryRegion xscom_regs;
> > >>  } PnvCore;
> > >> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > >> index 9f981a4940e6..cc17bbfed829 100644
> > >> --- a/hw/ppc/pnv_core.c
> > >> +++ b/hw/ppc/pnv_core.c
> > >> @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> > >>                                  "required link 'chip' not found: ");
> > >>          return;
> > >>      }
> > >> +    pc->chip = PNV_CHIP(chip);
> > >>  
> > >>      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> > >>      for (i = 0; i < cc->nr_threads; i++) {
> > >> @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> > >>      }
> > >>  
> > >>      for (j = 0; j < cc->nr_threads; j++) {
> > >> -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> > >> +        pnv_realize_vcpu(pc->threads[j], pc->chip, &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: 833 bytes --]

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[David Gibson]

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

On Thu, Oct 24, 2019 at 11:57:05AM +0200, Cédric Le Goater wrote:
> On 24/10/2019 04:38, David Gibson wrote:
> > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> >> We will use it to reset the interrupt presenter from the CPU reset
> >> handler.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >> ---
> >>  include/hw/ppc/pnv_core.h | 3 +++
> >>  hw/ppc/pnv_core.c         | 3 ++-
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> >> index bfbd2ec42aa6..55eee95104da 100644
> >> --- a/include/hw/ppc/pnv_core.h
> >> +++ b/include/hw/ppc/pnv_core.h
> >> @@ -31,6 +31,8 @@
> >>  #define PNV_CORE_GET_CLASS(obj) \
> >>       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> >>  
> >> +typedef struct PnvChip PnvChip;
> >> +
> >>  typedef struct PnvCore {
> >>      /*< private >*/
> >>      CPUCore parent_obj;
> >> @@ -38,6 +40,7 @@ typedef struct PnvCore {
> >>      /*< public >*/
> >>      PowerPCCPU **threads;
> >>      uint32_t pir;
> >> +    PnvChip *chip;
> > 
> > I don't love having this as a redundant encoding of the information
> > already in the property, since it raises the possibility of confusing
> > bugs if they ever got out of sync.
> 
> Indeed.
> 
> > It's not a huge deal, but it would be nice to at least to at least
> > consider either a) grabbing the property everywhere you need it (if
> > there aren't too many places) 
> 
> We need the chip at core creation and core reset to call the 
> interrupt chip handlers. These are not hot path but the pointer
> seemed practical.
> 
> > or b) customizing the property
> > definition so it's written directly into that field.
> 
> OK. That is better than just a link.

I guess.  If there are only two non hot path callers, it seems like it
might be simpler to just pull it out of the property at those places.

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

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

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[Greg Kurz]

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

On Thu, 24 Oct 2019 13:38:12 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> > We will use it to reset the interrupt presenter from the CPU reset
> > handler.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > ---
> >  include/hw/ppc/pnv_core.h | 3 +++
> >  hw/ppc/pnv_core.c         | 3 ++-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > index bfbd2ec42aa6..55eee95104da 100644
> > --- a/include/hw/ppc/pnv_core.h
> > +++ b/include/hw/ppc/pnv_core.h
> > @@ -31,6 +31,8 @@
> >  #define PNV_CORE_GET_CLASS(obj) \
> >       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> >  
> > +typedef struct PnvChip PnvChip;
> > +
> >  typedef struct PnvCore {
> >      /*< private >*/
> >      CPUCore parent_obj;
> > @@ -38,6 +40,7 @@ typedef struct PnvCore {
> >      /*< public >*/
> >      PowerPCCPU **threads;
> >      uint32_t pir;
> > +    PnvChip *chip;
> 
> I don't love having this as a redundant encoding of the information
> already in the property, since it raises the possibility of confusing
> bugs if they ever got out of sync.
> 

Ouch, we also have this pattern in xive_tctx_realize(). The XiveTCXT
object has both a "cpu" property and a pointer to the vCPU.

> It's not a huge deal, but it would be nice to at least to at least
> consider either a) grabbing the property everywhere you need it (if
> there aren't too many places) or b) customizing the property
> definition so it's written directly into that field.
> 

The pointer to the vCPU is used among other things to get the
value of the PIR, which is needed by the presenting logic to
match physical CAM lines. This is a _hot_ path so it's probably
better to go for b).

> >  
> >      MemoryRegion xscom_regs;
> >  } PnvCore;
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 9f981a4940e6..cc17bbfed829 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -222,6 +222,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >                                  "required link 'chip' not found: ");
> >          return;
> >      }
> > +    pc->chip = PNV_CHIP(chip);
> >  
> >      pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > @@ -243,7 +244,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
> > +        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> 


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

Re: [PATCH v5 4/7] ppc/pnv: Add a PnvChip pointer to PnvCore

[David Gibson]

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

On Thu, Oct 24, 2019 at 07:30:30PM +0200, Greg Kurz wrote:
> On Thu, 24 Oct 2019 13:38:12 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Oct 22, 2019 at 06:38:09PM +0200, Cédric Le Goater wrote:
> > > We will use it to reset the interrupt presenter from the CPU reset
> > > handler.
> > > 
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  include/hw/ppc/pnv_core.h | 3 +++
> > >  hw/ppc/pnv_core.c         | 3 ++-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > > index bfbd2ec42aa6..55eee95104da 100644
> > > --- a/include/hw/ppc/pnv_core.h
> > > +++ b/include/hw/ppc/pnv_core.h
> > > @@ -31,6 +31,8 @@
> > >  #define PNV_CORE_GET_CLASS(obj) \
> > >       OBJECT_GET_CLASS(PnvCoreClass, (obj), TYPE_PNV_CORE)
> > >  
> > > +typedef struct PnvChip PnvChip;
> > > +
> > >  typedef struct PnvCore {
> > >      /*< private >*/
> > >      CPUCore parent_obj;
> > > @@ -38,6 +40,7 @@ typedef struct PnvCore {
> > >      /*< public >*/
> > >      PowerPCCPU **threads;
> > >      uint32_t pir;
> > > +    PnvChip *chip;
> > 
> > I don't love having this as a redundant encoding of the information
> > already in the property, since it raises the possibility of confusing
> > bugs if they ever got out of sync.
> > 
> 
> Ouch, we also have this pattern in xive_tctx_realize(). The XiveTCXT
> object has both a "cpu" property and a pointer to the vCPU.
> 
> > It's not a huge deal, but it would be nice to at least to at least
> > consider either a) grabbing the property everywhere you need it (if
> > there aren't too many places) or b) customizing the property
> > definition so it's written directly into that field.
> > 
> 
> The pointer to the vCPU is used among other things to get the
> value of the PIR, which is needed by the presenting logic to
> match physical CAM lines. This is a _hot_ path so it's probably
> better to go for b).

Agreed.

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

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

[PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler

[Cédric Le Goater]

On the sPAPR machine and PowerNV machine, the interrupt presenters are
created by a machine handler at the core level and are reset
independently. This is not consistent and it raises issues when it
comes to handle hot-plugged CPUs. In that case, the presenters are not
reset. This is less of an issue in XICS, although a zero MFFR could
be a concern, but in XIVE, the OS CAM line is not set and this breaks
the presenting algorithm. The current code has workarounds which need
a global cleanup.

Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
cpu_intc_reset() handler called by the CPU reset handler and remove
the XiveTCTX reset handler which is now redundant.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h       |  1 +
 include/hw/ppc/spapr_irq.h |  2 ++
 include/hw/ppc/xics.h      |  1 +
 include/hw/ppc/xive.h      |  1 +
 hw/intc/spapr_xive.c       |  9 +++++++++
 hw/intc/xics.c             |  8 ++------
 hw/intc/xics_spapr.c       |  7 +++++++
 hw/intc/xive.c             | 12 +-----------
 hw/ppc/pnv.c               | 18 ++++++++++++++++++
 hw/ppc/pnv_core.c          |  7 +++++--
 hw/ppc/spapr_cpu_core.c    |  5 ++++-
 hw/ppc/spapr_irq.c         | 14 ++++++++++++++
 12 files changed, 65 insertions(+), 20 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 1cdbe55bf86c..2a780e633f23 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -111,6 +111,7 @@ typedef struct PnvChipClass {
 
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
     void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
+    void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
     ISABus *(*isa_create)(PnvChip *chip, Error **errp);
     void (*dt_populate)(PnvChip *chip, void *fdt);
     void (*pic_print_info)(PnvChip *chip, Monitor *mon);
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index 5e150a667902..09232999b07e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
      */
     int (*cpu_intc_create)(SpaprInterruptController *intc,
                             PowerPCCPU *cpu, Error **errp);
+    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
     int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
                      Error **errp);
     void (*free_irq)(SpaprInterruptController *intc, int irq);
@@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
 
 int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
                               PowerPCCPU *cpu, Error **errp);
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
 void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
 void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
                   void *fdt, uint32_t phandle);
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 1e6a9300eb2b..602173c12250 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
 uint32_t icp_accept(ICPState *ss);
 uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
 void icp_eoi(ICPState *icp, uint32_t xirr);
+void icp_reset(ICPState *icp);
 
 void ics_write_xive(ICSState *ics, int nr, int server,
                     uint8_t priority, uint8_t saved_priority);
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
index fd3319bd3202..99381639f50c 100644
--- a/include/hw/ppc/xive.h
+++ b/include/hw/ppc/xive.h
@@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
 
 void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
 Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
+void xive_tctx_reset(XiveTCTX *tctx);
 
 static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
 {
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index ba32d2cc5b0f..20a8d8285f64 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu)
+{
+    XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+
+    xive_tctx_reset(tctx);
+}
+
 static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
 {
     SpaprXive *xive = SPAPR_XIVE(intc);
@@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
     sicc->activate = spapr_xive_activate;
     sicc->deactivate = spapr_xive_deactivate;
     sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
+    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
     sicc->claim_irq = spapr_xive_claim_irq;
     sicc->free_irq = spapr_xive_free_irq;
     sicc->set_irq = spapr_xive_set_irq;
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index b5ac408f7b74..6da05763f9db 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
     },
 };
 
-static void icp_reset_handler(void *dev)
+void icp_reset(ICPState *icp)
 {
-    ICPState *icp = ICP(dev);
-
     icp->xirr = 0;
     icp->pending_priority = 0xff;
     icp->mfrr = 0xff;
@@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
     if (kvm_irqchip_in_kernel()) {
         Error *local_err = NULL;
 
-        icp_set_kvm_state(ICP(dev), &local_err);
+        icp_set_kvm_state(icp, &local_err);
         if (local_err) {
             error_report_err(local_err);
         }
@@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    qemu_register_reset(icp_reset_handler, dev);
     vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
@@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
     ICPState *icp = ICP(dev);
 
     vmstate_unregister(NULL, &vmstate_icp_server, icp);
-    qemu_unregister_reset(icp_reset_handler, dev);
 }
 
 static void icp_class_init(ObjectClass *klass, void *data)
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 4f64b9a9fc66..7418fb9f370c 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
     return 0;
 }
 
+static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
+                                     PowerPCCPU *cpu)
+{
+    icp_reset(spapr_cpu_state(cpu)->icp);
+}
+
 static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
                                 bool lsi, Error **errp)
 {
@@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
     sicc->activate = xics_spapr_activate;
     sicc->deactivate = xics_spapr_deactivate;
     sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
+    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
     sicc->claim_irq = xics_spapr_claim_irq;
     sicc->free_irq = xics_spapr_free_irq;
     sicc->set_irq = xics_spapr_set_irq;
diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index d420c6571e14..f066be5eb5e3 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
     }
 }
 
-static void xive_tctx_reset(void *dev)
+void xive_tctx_reset(XiveTCTX *tctx)
 {
-    XiveTCTX *tctx = XIVE_TCTX(dev);
-
     memset(tctx->regs, 0, sizeof(tctx->regs));
 
     /* Set some defaults */
@@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-
-    qemu_register_reset(xive_tctx_reset, dev);
-}
-
-static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
-{
-    qemu_unregister_reset(xive_tctx_reset, dev);
 }
 
 static int vmstate_xive_tctx_pre_save(void *opaque)
@@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "XIVE Interrupt Thread Context";
     dc->realize = xive_tctx_realize;
-    dc->unrealize = xive_tctx_unrealize;
     dc->vmsd = &vmstate_xive_tctx;
     /*
      * Reason: part of XIVE interrupt controller, needs to be wired up
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7cf64b6d2533..4a51fb65a834 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -778,6 +778,13 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
     pnv_cpu->intc = obj;
 }
 
+static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
+{
+    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+
+    icp_reset(ICP(pnv_cpu->intc));
+}
+
 /*
  *    0:48  Reserved - Read as zeroes
  *   49:52  Node ID
@@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
     pnv_cpu->intc = obj;
 }
 
+static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
+{
+    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+
+    xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
+}
+
 /*
  * Allowed core identifiers on a POWER8 Processor Chip :
  *
@@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->intc_reset = pnv_chip_power8_intc_reset;
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->intc_reset = pnv_chip_power8_intc_reset;
     k->isa_create = pnv_chip_power8_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->intc_reset = pnv_chip_power8_intc_reset;
     k->isa_create = pnv_chip_power8nvl_isa_create;
     k->dt_populate = pnv_chip_power8_dt_populate;
     k->pic_print_info = pnv_chip_power8_pic_print_info;
@@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
     k->intc_create = pnv_chip_power9_intc_create;
+    k->intc_reset = pnv_chip_power9_intc_reset;
     k->isa_create = pnv_chip_power9_isa_create;
     k->dt_populate = pnv_chip_power9_dt_populate;
     k->pic_print_info = pnv_chip_power9_pic_print_info;
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index cc17bbfed829..be0310ac0340 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -40,10 +40,11 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
     return cpu_type;
 }
 
-static void pnv_core_cpu_reset(PowerPCCPU *cpu)
+static void pnv_core_cpu_reset(PowerPCCPU *cpu, PnvChip *chip)
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
     cpu_reset(cs);
 
@@ -54,6 +55,8 @@ static void pnv_core_cpu_reset(PowerPCCPU *cpu)
     env->gpr[3] = PNV_FDT_ADDR;
     env->nip = 0x10;
     env->msr |= MSR_HVB; /* Hypervisor mode */
+
+    pcc->intc_reset(chip, cpu);
 }
 
 /*
@@ -200,7 +203,7 @@ static void pnv_core_reset(void *dev)
     int i;
 
     for (i = 0; i < cc->nr_threads; i++) {
-        pnv_core_cpu_reset(pc->threads[i]);
+        pnv_core_cpu_reset(pc->threads[i], pc->chip);
     }
 }
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 2e34832d0ea2..ef7b27a66d56 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong lpcr;
+    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
 
     cpu_reset(cs);
 
@@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
     spapr_cpu->dtl_addr = 0;
     spapr_cpu->dtl_size = 0;
 
-    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
+    spapr_caps_cpu_apply(spapr, cpu);
 
     kvm_check_mmu(cpu, &error_fatal);
+
+    spapr_irq_cpu_intc_reset(spapr, cpu);
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 234d1073e518..b941608b69ba 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
     return 0;
 }
 
+void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
+{
+    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
+        SpaprInterruptController *intc = intcs[i];
+        if (intc) {
+            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
+            sicc->cpu_intc_reset(intc, cpu);
+        }
+    }
+}
+
 static void spapr_set_irq(void *opaque, int irq, int level)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
-- 
2.21.0

Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler

[Greg Kurz]

On Tue, 22 Oct 2019 18:38:10 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reset
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reset. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
> 
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  include/hw/ppc/pnv.h       |  1 +
>  include/hw/ppc/spapr_irq.h |  2 ++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  9 +++++++++
>  hw/intc/xics.c             |  8 ++------
>  hw/intc/xics_spapr.c       |  7 +++++++
>  hw/intc/xive.c             | 12 +-----------
>  hw/ppc/pnv.c               | 18 ++++++++++++++++++
>  hw/ppc/pnv_core.c          |  7 +++++--
>  hw/ppc/spapr_cpu_core.c    |  5 ++++-
>  hw/ppc/spapr_irq.c         | 14 ++++++++++++++
>  12 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,7 @@ typedef struct PnvChipClass {
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>      void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> +    void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>      void (*dt_populate)(PnvChip *chip, void *fdt);
>      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
>  void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
>  void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>                    void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>  uint32_t icp_accept(ICPState *ss);
>  uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>  void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>  
>  void ics_write_xive(ICSState *ics, int nr, int server,
>                      uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>  
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>  Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>  
>  static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>  {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> +    xive_tctx_reset(tctx);
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>      sicc->activate = spapr_xive_activate;
>      sicc->deactivate = spapr_xive_deactivate;
>      sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> +    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>      sicc->claim_irq = spapr_xive_claim_irq;
>      sicc->free_irq = spapr_xive_free_irq;
>      sicc->set_irq = spapr_xive_set_irq;
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>      },
>  };
>  
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
>  {
> -    ICPState *icp = ICP(dev);
> -
>      icp->xirr = 0;
>      icp->pending_priority = 0xff;
>      icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
>  
> -        icp_set_kvm_state(ICP(dev), &local_err);
> +        icp_set_kvm_state(icp, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>          }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(icp_reset_handler, dev);
>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>      ICPState *icp = ICP(dev);
>  
>      vmstate_unregister(NULL, &vmstate_icp_server, icp);
> -    qemu_unregister_reset(icp_reset_handler, dev);
>  }
>  
>  static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>      sicc->activate = xics_spapr_activate;
>      sicc->deactivate = xics_spapr_deactivate;
>      sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> +    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>      sicc->claim_irq = xics_spapr_claim_irq;
>      sicc->free_irq = xics_spapr_free_irq;
>      sicc->set_irq = xics_spapr_set_irq;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
>  {
> -    XiveTCTX *tctx = XIVE_TCTX(dev);
> -
>      memset(tctx->regs, 0, sizeof(tctx->regs));
>  
>      /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> -
> -    qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> -    qemu_unregister_reset(xive_tctx_reset, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc = "XIVE Interrupt Thread Context";
>      dc->realize = xive_tctx_realize;
> -    dc->unrealize = xive_tctx_unrealize;
>      dc->vmsd = &vmstate_xive_tctx;
>      /*
>       * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..4a51fb65a834 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -778,6 +778,13 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>      pnv_cpu->intc = obj;
>  }
>  
> +static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    icp_reset(ICP(pnv_cpu->intc));
> +}
> +
>  /*
>   *    0:48  Reserved - Read as zeroes
>   *   49:52  Node ID
> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>      pnv_cpu->intc = obj;
>  }
>  
> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
> +}
> +
>  /*
>   * Allowed core identifiers on a POWER8 Processor Chip :
>   *
> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8nvl_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
>      k->intc_create = pnv_chip_power9_intc_create;
> +    k->intc_reset = pnv_chip_power9_intc_reset;
>      k->isa_create = pnv_chip_power9_isa_create;
>      k->dt_populate = pnv_chip_power9_dt_populate;
>      k->pic_print_info = pnv_chip_power9_pic_print_info;
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index cc17bbfed829..be0310ac0340 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -40,10 +40,11 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
>      return cpu_type;
>  }
>  
> -static void pnv_core_cpu_reset(PowerPCCPU *cpu)
> +static void pnv_core_cpu_reset(PowerPCCPU *cpu, PnvChip *chip)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
>      cpu_reset(cs);
>  
> @@ -54,6 +55,8 @@ static void pnv_core_cpu_reset(PowerPCCPU *cpu)
>      env->gpr[3] = PNV_FDT_ADDR;
>      env->nip = 0x10;
>      env->msr |= MSR_HVB; /* Hypervisor mode */
> +
> +    pcc->intc_reset(chip, cpu);
>  }
>  
>  /*
> @@ -200,7 +203,7 @@ static void pnv_core_reset(void *dev)
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        pnv_core_cpu_reset(pc->threads[i]);
> +        pnv_core_cpu_reset(pc->threads[i], pc->chip);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2e34832d0ea2..ef7b27a66d56 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong lpcr;
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  
>      cpu_reset(cs);
>  
> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>      spapr_cpu->dtl_addr = 0;
>      spapr_cpu->dtl_size = 0;
>  
> -    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> +    spapr_caps_cpu_apply(spapr, cpu);
>  
>      kvm_check_mmu(cpu, &error_fatal);
> +
> +    spapr_irq_cpu_intc_reset(spapr, cpu);
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            sicc->cpu_intc_reset(intc, cpu);
> +        }
> +    }
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);

Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler

[Philippe Mathieu-Daudé]

On 10/22/19 6:38 PM, Cédric Le Goater wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reset
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reset. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
> 
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   include/hw/ppc/pnv.h       |  1 +
>   include/hw/ppc/spapr_irq.h |  2 ++
>   include/hw/ppc/xics.h      |  1 +
>   include/hw/ppc/xive.h      |  1 +
>   hw/intc/spapr_xive.c       |  9 +++++++++
>   hw/intc/xics.c             |  8 ++------
>   hw/intc/xics_spapr.c       |  7 +++++++
>   hw/intc/xive.c             | 12 +-----------
>   hw/ppc/pnv.c               | 18 ++++++++++++++++++
>   hw/ppc/pnv_core.c          |  7 +++++--
>   hw/ppc/spapr_cpu_core.c    |  5 ++++-
>   hw/ppc/spapr_irq.c         | 14 ++++++++++++++
>   12 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,7 @@ typedef struct PnvChipClass {
>   
>       uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>       void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> +    void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
>       ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>       void (*dt_populate)(PnvChip *chip, void *fdt);
>       void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>        */
>       int (*cpu_intc_create)(SpaprInterruptController *intc,
>                               PowerPCCPU *cpu, Error **errp);
> +    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>       int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                        Error **errp);
>       void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>   
>   int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                 PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
>   void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
>   void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>                     void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>   uint32_t icp_accept(ICPState *ss);
>   uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>   void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>   
>   void ics_write_xive(ICSState *ics, int nr, int server,
>                       uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>   
>   void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>   Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>   
>   static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>   {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>       return 0;
>   }
>   
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)

Indent off :|

> +{
> +    XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> +    xive_tctx_reset(tctx);
> +}
> +
>   static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>   {
>       SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>       sicc->activate = spapr_xive_activate;
>       sicc->deactivate = spapr_xive_deactivate;
>       sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> +    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>       sicc->claim_irq = spapr_xive_claim_irq;
>       sicc->free_irq = spapr_xive_free_irq;
>       sicc->set_irq = spapr_xive_set_irq;
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>       },
>   };
>   
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
>   {
> -    ICPState *icp = ICP(dev);
> -
>       icp->xirr = 0;
>       icp->pending_priority = 0xff;
>       icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>       if (kvm_irqchip_in_kernel()) {
>           Error *local_err = NULL;
>   
> -        icp_set_kvm_state(ICP(dev), &local_err);
> +        icp_set_kvm_state(icp, &local_err);
>           if (local_err) {
>               error_report_err(local_err);
>           }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    qemu_register_reset(icp_reset_handler, dev);
>       vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>   }
>   
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>       ICPState *icp = ICP(dev);
>   
>       vmstate_unregister(NULL, &vmstate_icp_server, icp);
> -    qemu_unregister_reset(icp_reset_handler, dev);
>   }
>   
>   static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>       return 0;
>   }
>   
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)

Ditto.

> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
>   static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                   bool lsi, Error **errp)
>   {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>       sicc->activate = xics_spapr_activate;
>       sicc->deactivate = xics_spapr_deactivate;
>       sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> +    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>       sicc->claim_irq = xics_spapr_claim_irq;
>       sicc->free_irq = xics_spapr_free_irq;
>       sicc->set_irq = xics_spapr_set_irq;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>       }
>   }
>   
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
>   {
> -    XiveTCTX *tctx = XIVE_TCTX(dev);
> -
>       memset(tctx->regs, 0, sizeof(tctx->regs));
>   
>       /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>       }
> -
> -    qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> -    qemu_unregister_reset(xive_tctx_reset, dev);
>   }
>   
>   static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>   
>       dc->desc = "XIVE Interrupt Thread Context";
>       dc->realize = xive_tctx_realize;
> -    dc->unrealize = xive_tctx_unrealize;
>       dc->vmsd = &vmstate_xive_tctx;
>       /*
>        * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..4a51fb65a834 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -778,6 +778,13 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>       pnv_cpu->intc = obj;
>   }
>   
> +static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    icp_reset(ICP(pnv_cpu->intc));
> +}
> +
>   /*
>    *    0:48  Reserved - Read as zeroes
>    *   49:52  Node ID
> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>       pnv_cpu->intc = obj;
>   }
>   
> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
> +}
> +
>   /*
>    * Allowed core identifiers on a POWER8 Processor Chip :
>    *
> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>       k->cores_mask = POWER8E_CORE_MASK;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>       k->isa_create = pnv_chip_power8_isa_create;
>       k->dt_populate = pnv_chip_power8_dt_populate;
>       k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>       k->cores_mask = POWER8_CORE_MASK;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>       k->isa_create = pnv_chip_power8_isa_create;
>       k->dt_populate = pnv_chip_power8_dt_populate;
>       k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>       k->cores_mask = POWER8_CORE_MASK;
>       k->core_pir = pnv_chip_core_pir_p8;
>       k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>       k->isa_create = pnv_chip_power8nvl_isa_create;
>       k->dt_populate = pnv_chip_power8_dt_populate;
>       k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>       k->cores_mask = POWER9_CORE_MASK;
>       k->core_pir = pnv_chip_core_pir_p9;
>       k->intc_create = pnv_chip_power9_intc_create;
> +    k->intc_reset = pnv_chip_power9_intc_reset;
>       k->isa_create = pnv_chip_power9_isa_create;
>       k->dt_populate = pnv_chip_power9_dt_populate;
>       k->pic_print_info = pnv_chip_power9_pic_print_info;
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index cc17bbfed829..be0310ac0340 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -40,10 +40,11 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
>       return cpu_type;
>   }
>   
> -static void pnv_core_cpu_reset(PowerPCCPU *cpu)
> +static void pnv_core_cpu_reset(PowerPCCPU *cpu, PnvChip *chip)
>   {
>       CPUState *cs = CPU(cpu);
>       CPUPPCState *env = &cpu->env;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>   
>       cpu_reset(cs);
>   
> @@ -54,6 +55,8 @@ static void pnv_core_cpu_reset(PowerPCCPU *cpu)
>       env->gpr[3] = PNV_FDT_ADDR;
>       env->nip = 0x10;
>       env->msr |= MSR_HVB; /* Hypervisor mode */
> +
> +    pcc->intc_reset(chip, cpu);
>   }
>   
>   /*
> @@ -200,7 +203,7 @@ static void pnv_core_reset(void *dev)
>       int i;
>   
>       for (i = 0; i < cc->nr_threads; i++) {
> -        pnv_core_cpu_reset(pc->threads[i]);
> +        pnv_core_cpu_reset(pc->threads[i], pc->chip);
>       }
>   }
>   
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2e34832d0ea2..ef7b27a66d56 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>       SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>       target_ulong lpcr;
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>   
>       cpu_reset(cs);
>   
> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>       spapr_cpu->dtl_addr = 0;
>       spapr_cpu->dtl_size = 0;
>   
> -    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> +    spapr_caps_cpu_apply(spapr, cpu);
>   
>       kvm_check_mmu(cpu, &error_fatal);
> +
> +    spapr_irq_cpu_intc_reset(spapr, cpu);
>   }
>   
>   void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>       return 0;
>   }
>   
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            sicc->cpu_intc_reset(intc, cpu);
> +        }
> +    }
> +}
> +
>   static void spapr_set_irq(void *opaque, int irq, int level)
>   {
>       SpaprMachineState *spapr = SPAPR_MACHINE(opaque);
> 

Way cleaner.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v5 5/7] ppc: Reset the interrupt presenter from the CPU reset handler

[David Gibson]

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

On Tue, Oct 22, 2019 at 06:38:10PM +0200, Cédric Le Goater wrote:
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reset
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reset. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
> 
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Both XiveTCTX and ICPState are QOM subclasses of TYPE_DEVICE.  I think
that means you could avoid the new hook by using dc->reset on those
classes instead.  It wouldn't get called automatically, of course,
since it's not on a bus, but you could call it explicitly via
device_reset() from the place you currently call the new hook.

> ---
>  include/hw/ppc/pnv.h       |  1 +
>  include/hw/ppc/spapr_irq.h |  2 ++
>  include/hw/ppc/xics.h      |  1 +
>  include/hw/ppc/xive.h      |  1 +
>  hw/intc/spapr_xive.c       |  9 +++++++++
>  hw/intc/xics.c             |  8 ++------
>  hw/intc/xics_spapr.c       |  7 +++++++
>  hw/intc/xive.c             | 12 +-----------
>  hw/ppc/pnv.c               | 18 ++++++++++++++++++
>  hw/ppc/pnv_core.c          |  7 +++++--
>  hw/ppc/spapr_cpu_core.c    |  5 ++++-
>  hw/ppc/spapr_irq.c         | 14 ++++++++++++++
>  12 files changed, 65 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 1cdbe55bf86c..2a780e633f23 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -111,6 +111,7 @@ typedef struct PnvChipClass {
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>      void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
> +    void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>      void (*dt_populate)(PnvChip *chip, void *fdt);
>      void (*pic_print_info)(PnvChip *chip, Monitor *mon);
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 5e150a667902..09232999b07e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,6 +52,7 @@ typedef struct SpaprInterruptControllerClass {
>       */
>      int (*cpu_intc_create)(SpaprInterruptController *intc,
>                              PowerPCCPU *cpu, Error **errp);
> +    void (*cpu_intc_reset)(SpaprInterruptController *intc, PowerPCCPU *cpu);
>      int (*claim_irq)(SpaprInterruptController *intc, int irq, bool lsi,
>                       Error **errp);
>      void (*free_irq)(SpaprInterruptController *intc, int irq);
> @@ -68,6 +69,7 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr);
>  
>  int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>                                PowerPCCPU *cpu, Error **errp);
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu);
>  void spapr_irq_print_info(SpaprMachineState *spapr, Monitor *mon);
>  void spapr_irq_dt(SpaprMachineState *spapr, uint32_t nr_servers,
>                    void *fdt, uint32_t phandle);
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 1e6a9300eb2b..602173c12250 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -161,6 +161,7 @@ void icp_set_mfrr(ICPState *icp, uint8_t mfrr);
>  uint32_t icp_accept(ICPState *ss);
>  uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr);
>  void icp_eoi(ICPState *icp, uint32_t xirr);
> +void icp_reset(ICPState *icp);
>  
>  void ics_write_xive(ICSState *ics, int nr, int server,
>                      uint8_t priority, uint8_t saved_priority);
> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> index fd3319bd3202..99381639f50c 100644
> --- a/include/hw/ppc/xive.h
> +++ b/include/hw/ppc/xive.h
> @@ -415,6 +415,7 @@ uint64_t xive_tctx_tm_read(XiveTCTX *tctx, hwaddr offset, unsigned size);
>  
>  void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon);
>  Object *xive_tctx_create(Object *cpu, XiveRouter *xrtr, Error **errp);
> +void xive_tctx_reset(XiveTCTX *tctx);
>  
>  static inline uint32_t xive_nvt_cam_line(uint8_t nvt_blk, uint32_t nvt_idx)
>  {
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index ba32d2cc5b0f..20a8d8285f64 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -553,6 +553,14 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +
> +    xive_tctx_reset(tctx);
> +}
> +
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> @@ -697,6 +705,7 @@ static void spapr_xive_class_init(ObjectClass *klass, void *data)
>      sicc->activate = spapr_xive_activate;
>      sicc->deactivate = spapr_xive_deactivate;
>      sicc->cpu_intc_create = spapr_xive_cpu_intc_create;
> +    sicc->cpu_intc_reset = spapr_xive_cpu_intc_reset;
>      sicc->claim_irq = spapr_xive_claim_irq;
>      sicc->free_irq = spapr_xive_free_irq;
>      sicc->set_irq = spapr_xive_set_irq;
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index b5ac408f7b74..6da05763f9db 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -274,10 +274,8 @@ static const VMStateDescription vmstate_icp_server = {
>      },
>  };
>  
> -static void icp_reset_handler(void *dev)
> +void icp_reset(ICPState *icp)
>  {
> -    ICPState *icp = ICP(dev);
> -
>      icp->xirr = 0;
>      icp->pending_priority = 0xff;
>      icp->mfrr = 0xff;
> @@ -288,7 +286,7 @@ static void icp_reset_handler(void *dev)
>      if (kvm_irqchip_in_kernel()) {
>          Error *local_err = NULL;
>  
> -        icp_set_kvm_state(ICP(dev), &local_err);
> +        icp_set_kvm_state(icp, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>          }
> @@ -351,7 +349,6 @@ static void icp_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    qemu_register_reset(icp_reset_handler, dev);
>      vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
> @@ -360,7 +357,6 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
>      ICPState *icp = ICP(dev);
>  
>      vmstate_unregister(NULL, &vmstate_icp_server, icp);
> -    qemu_unregister_reset(icp_reset_handler, dev);
>  }
>  
>  static void icp_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 4f64b9a9fc66..7418fb9f370c 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -346,6 +346,12 @@ static int xics_spapr_cpu_intc_create(SpaprInterruptController *intc,
>      return 0;
>  }
>  
> +static void xics_spapr_cpu_intc_reset(SpaprInterruptController *intc,
> +                                     PowerPCCPU *cpu)
> +{
> +    icp_reset(spapr_cpu_state(cpu)->icp);
> +}
> +
>  static int xics_spapr_claim_irq(SpaprInterruptController *intc, int irq,
>                                  bool lsi, Error **errp)
>  {
> @@ -433,6 +439,7 @@ static void ics_spapr_class_init(ObjectClass *klass, void *data)
>      sicc->activate = xics_spapr_activate;
>      sicc->deactivate = xics_spapr_deactivate;
>      sicc->cpu_intc_create = xics_spapr_cpu_intc_create;
> +    sicc->cpu_intc_reset = xics_spapr_cpu_intc_reset;
>      sicc->claim_irq = xics_spapr_claim_irq;
>      sicc->free_irq = xics_spapr_free_irq;
>      sicc->set_irq = xics_spapr_set_irq;
> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
> index d420c6571e14..f066be5eb5e3 100644
> --- a/hw/intc/xive.c
> +++ b/hw/intc/xive.c
> @@ -547,10 +547,8 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon)
>      }
>  }
>  
> -static void xive_tctx_reset(void *dev)
> +void xive_tctx_reset(XiveTCTX *tctx)
>  {
> -    XiveTCTX *tctx = XIVE_TCTX(dev);
> -
>      memset(tctx->regs, 0, sizeof(tctx->regs));
>  
>      /* Set some defaults */
> @@ -607,13 +605,6 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> -
> -    qemu_register_reset(xive_tctx_reset, dev);
> -}
> -
> -static void xive_tctx_unrealize(DeviceState *dev, Error **errp)
> -{
> -    qemu_unregister_reset(xive_tctx_reset, dev);
>  }
>  
>  static int vmstate_xive_tctx_pre_save(void *opaque)
> @@ -668,7 +659,6 @@ static void xive_tctx_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc = "XIVE Interrupt Thread Context";
>      dc->realize = xive_tctx_realize;
> -    dc->unrealize = xive_tctx_unrealize;
>      dc->vmsd = &vmstate_xive_tctx;
>      /*
>       * Reason: part of XIVE interrupt controller, needs to be wired up
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7cf64b6d2533..4a51fb65a834 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -778,6 +778,13 @@ static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>      pnv_cpu->intc = obj;
>  }
>  
> +static void pnv_chip_power8_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    icp_reset(ICP(pnv_cpu->intc));
> +}
> +
>  /*
>   *    0:48  Reserved - Read as zeroes
>   *   49:52  Node ID
> @@ -815,6 +822,13 @@ static void pnv_chip_power9_intc_create(PnvChip *chip, PowerPCCPU *cpu,
>      pnv_cpu->intc = obj;
>  }
>  
> +static void pnv_chip_power9_intc_reset(PnvChip *chip, PowerPCCPU *cpu)
> +{
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
> +    xive_tctx_reset(XIVE_TCTX(pnv_cpu->intc));
> +}
> +
>  /*
>   * Allowed core identifiers on a POWER8 Processor Chip :
>   *
> @@ -984,6 +998,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1003,6 +1018,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1022,6 +1038,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->intc_reset = pnv_chip_power8_intc_reset;
>      k->isa_create = pnv_chip_power8nvl_isa_create;
>      k->dt_populate = pnv_chip_power8_dt_populate;
>      k->pic_print_info = pnv_chip_power8_pic_print_info;
> @@ -1191,6 +1208,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
>      k->intc_create = pnv_chip_power9_intc_create;
> +    k->intc_reset = pnv_chip_power9_intc_reset;
>      k->isa_create = pnv_chip_power9_isa_create;
>      k->dt_populate = pnv_chip_power9_dt_populate;
>      k->pic_print_info = pnv_chip_power9_pic_print_info;
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index cc17bbfed829..be0310ac0340 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -40,10 +40,11 @@ static const char *pnv_core_cpu_typename(PnvCore *pc)
>      return cpu_type;
>  }
>  
> -static void pnv_core_cpu_reset(PowerPCCPU *cpu)
> +static void pnv_core_cpu_reset(PowerPCCPU *cpu, PnvChip *chip)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
>      cpu_reset(cs);
>  
> @@ -54,6 +55,8 @@ static void pnv_core_cpu_reset(PowerPCCPU *cpu)
>      env->gpr[3] = PNV_FDT_ADDR;
>      env->nip = 0x10;
>      env->msr |= MSR_HVB; /* Hypervisor mode */
> +
> +    pcc->intc_reset(chip, cpu);
>  }
>  
>  /*
> @@ -200,7 +203,7 @@ static void pnv_core_reset(void *dev)
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        pnv_core_cpu_reset(pc->threads[i]);
> +        pnv_core_cpu_reset(pc->threads[i], pc->chip);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 2e34832d0ea2..ef7b27a66d56 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -32,6 +32,7 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong lpcr;
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>  
>      cpu_reset(cs);
>  
> @@ -76,9 +77,11 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>      spapr_cpu->dtl_addr = 0;
>      spapr_cpu->dtl_size = 0;
>  
> -    spapr_caps_cpu_apply(SPAPR_MACHINE(qdev_get_machine()), cpu);
> +    spapr_caps_cpu_apply(spapr, cpu);
>  
>      kvm_check_mmu(cpu, &error_fatal);
> +
> +    spapr_irq_cpu_intc_reset(spapr, cpu);
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 234d1073e518..b941608b69ba 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -220,6 +220,20 @@ int spapr_irq_cpu_intc_create(SpaprMachineState *spapr,
>      return 0;
>  }
>  
> +void spapr_irq_cpu_intc_reset(SpaprMachineState *spapr, PowerPCCPU *cpu)
> +{
> +    SpaprInterruptController *intcs[] = ALL_INTCS(spapr);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(intcs); i++) {
> +        SpaprInterruptController *intc = intcs[i];
> +        if (intc) {
> +            SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(intc);
> +            sicc->cpu_intc_reset(intc, cpu);
> +        }
> +    }
> +}
> +
>  static void spapr_set_irq(void *opaque, int irq, int level)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(opaque);

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

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

[PATCH v5 6/7] ppc/pnv: Fix naming of routines realizing the CPUs

[Cédric Le Goater]

The 'vcpu' suffix is inherited from the sPAPR machine. Use better
names for PowerNV.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/pnv_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index be0310ac0340..e81cd3a3e047 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -162,7 +162,7 @@ static const MemoryRegionOps pnv_core_power9_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
+static void pnv_core_cpu_realize(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
 {
     CPUPPCState *env = &cpu->env;
     int core_pir;
@@ -247,7 +247,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
+        pnv_core_cpu_realize(pc->threads[j], pc->chip, &local_err);
         if (local_err) {
             goto err;
         }
@@ -269,7 +269,7 @@ err:
     error_propagate(errp, local_err);
 }
 
-static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
+static void pnv_core_cpu_unrealize(PowerPCCPU *cpu)
 {
     PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
 
@@ -289,7 +289,7 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
     qemu_unregister_reset(pnv_core_reset, pc);
 
     for (i = 0; i < cc->nr_threads; i++) {
-        pnv_unrealize_vcpu(pc->threads[i]);
+        pnv_core_cpu_unrealize(pc->threads[i]);
     }
     g_free(pc->threads);
 }
-- 
2.21.0

Re: [PATCH v5 6/7] ppc/pnv: Fix naming of routines realizing the CPUs

[Philippe Mathieu-Daudé]

On 10/22/19 6:38 PM, Cédric Le Goater wrote:
> The 'vcpu' suffix is inherited from the sPAPR machine. Use better
> names for PowerNV.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>   hw/ppc/pnv_core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index be0310ac0340..e81cd3a3e047 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -162,7 +162,7 @@ static const MemoryRegionOps pnv_core_power9_xscom_ops = {
>       .endianness = DEVICE_BIG_ENDIAN,
>   };
>   
> -static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
> +static void pnv_core_cpu_realize(PowerPCCPU *cpu, PnvChip *chip, Error **errp)
>   {
>       CPUPPCState *env = &cpu->env;
>       int core_pir;
> @@ -247,7 +247,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>       }
>   
>       for (j = 0; j < cc->nr_threads; j++) {
> -        pnv_realize_vcpu(pc->threads[j], pc->chip, &local_err);
> +        pnv_core_cpu_realize(pc->threads[j], pc->chip, &local_err);
>           if (local_err) {
>               goto err;
>           }
> @@ -269,7 +269,7 @@ err:
>       error_propagate(errp, local_err);
>   }
>   
> -static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> +static void pnv_core_cpu_unrealize(PowerPCCPU *cpu)
>   {
>       PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
>   
> @@ -289,7 +289,7 @@ static void pnv_core_unrealize(DeviceState *dev, Error **errp)
>       qemu_unregister_reset(pnv_core_reset, pc);
>   
>       for (i = 0; i < cc->nr_threads; i++) {
> -        pnv_unrealize_vcpu(pc->threads[i]);
> +        pnv_core_cpu_unrealize(pc->threads[i]);
>       }
>       g_free(pc->threads);
>   }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH v5 7/7] spapr/xive: Set the OS CAM line at reset

[Cédric Le Goater]

When a Virtual Processor is scheduled to run on a HW thread, the
hypervisor pushes its identifier in the OS CAM line. When running with
kernel_irqchip=off, QEMU needs to emulate the same behavior.

Set the OS CAM line when the interrupt presenter of the sPAPR core is
reset. This will also cover the case of hot-plugged CPUs.

This change also has the benefit to remove the use of CPU_FOREACH()
which can be unsafe.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 include/hw/ppc/spapr_xive.h |  1 -
 hw/intc/spapr_xive.c        | 48 +++++++++++++------------------------
 2 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index d84bd5c229f0..742b7e834f2a 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -57,7 +57,6 @@ typedef struct SpaprXive {
 void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
 
 void spapr_xive_hcall_init(SpaprMachineState *spapr);
-void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
 void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
 void spapr_xive_map_mmio(SpaprXive *xive);
 
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 20a8d8285f64..d8e1291905c3 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -205,23 +205,6 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
     memory_region_set_enabled(&xive->end_source.esb_mmio, false);
 }
 
-/*
- * When a Virtual Processor is scheduled to run on a HW thread, the
- * hypervisor pushes its identifier in the OS CAM line. Emulate the
- * same behavior under QEMU.
- */
-void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
-{
-    uint8_t  nvt_blk;
-    uint32_t nvt_idx;
-    uint32_t nvt_cam;
-
-    spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
-
-    nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk, nvt_idx));
-    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
-}
-
 static void spapr_xive_end_reset(XiveEND *end)
 {
     memset(end, 0, sizeof(*end));
@@ -544,21 +527,32 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
     }
 
     spapr_cpu->tctx = XIVE_TCTX(obj);
-
-    /*
-     * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
-     * don't beneficiate from the reset of the XIVE IRQ backend
-     */
-    spapr_xive_set_tctx_os_cam(spapr_cpu->tctx);
     return 0;
 }
 
+static void xive_tctx_set_os_cam(XiveTCTX *tctx, uint32_t os_cam)
+{
+    uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | os_cam);
+    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
+}
+
 static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
                                      PowerPCCPU *cpu)
 {
     XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
+    uint8_t  nvt_blk;
+    uint32_t nvt_idx;
 
     xive_tctx_reset(tctx);
+
+    /*
+     * When a Virtual Processor is scheduled to run on a HW thread,
+     * the hypervisor pushes its identifier in the OS CAM line.
+     * Emulate the same behavior under QEMU.
+     */
+    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
+
+    xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx));
 }
 
 static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
@@ -651,14 +645,6 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
 static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
 {
     SpaprXive *xive = SPAPR_XIVE(intc);
-    CPUState *cs;
-
-    CPU_FOREACH(cs) {
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-        /* (TCG) Set the OS CAM line of the thread interrupt context. */
-        spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
-    }
 
     if (kvm_enabled()) {
         int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);
-- 
2.21.0

Re: [PATCH v5 7/7] spapr/xive: Set the OS CAM line at reset

[David Gibson]

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

On Tue, Oct 22, 2019 at 06:38:12PM +0200, Cédric Le Goater wrote:
> When a Virtual Processor is scheduled to run on a HW thread, the
> hypervisor pushes its identifier in the OS CAM line. When running with
> kernel_irqchip=off, QEMU needs to emulate the same behavior.
> 
> Set the OS CAM line when the interrupt presenter of the sPAPR core is
> reset. This will also cover the case of hot-plugged CPUs.
> 
> This change also has the benefit to remove the use of CPU_FOREACH()
> which can be unsafe.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Since the values here should remain constant for the lifetime of a
(PAPR) guest, it kind of seems like this belongs more in realize()
than reset.  But this definitely fixes a real problem, so that's
somethine we can tweak later.

> ---
>  include/hw/ppc/spapr_xive.h |  1 -
>  hw/intc/spapr_xive.c        | 48 +++++++++++++------------------------
>  2 files changed, 17 insertions(+), 32 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index d84bd5c229f0..742b7e834f2a 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -57,7 +57,6 @@ typedef struct SpaprXive {
>  void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon);
>  
>  void spapr_xive_hcall_init(SpaprMachineState *spapr);
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx);
>  void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable);
>  void spapr_xive_map_mmio(SpaprXive *xive);
>  
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 20a8d8285f64..d8e1291905c3 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -205,23 +205,6 @@ void spapr_xive_mmio_set_enabled(SpaprXive *xive, bool enable)
>      memory_region_set_enabled(&xive->end_source.esb_mmio, false);
>  }
>  
> -/*
> - * When a Virtual Processor is scheduled to run on a HW thread, the
> - * hypervisor pushes its identifier in the OS CAM line. Emulate the
> - * same behavior under QEMU.
> - */
> -void spapr_xive_set_tctx_os_cam(XiveTCTX *tctx)
> -{
> -    uint8_t  nvt_blk;
> -    uint32_t nvt_idx;
> -    uint32_t nvt_cam;
> -
> -    spapr_xive_cpu_to_nvt(POWERPC_CPU(tctx->cs), &nvt_blk, &nvt_idx);
> -
> -    nvt_cam = cpu_to_be32(TM_QW1W2_VO | xive_nvt_cam_line(nvt_blk, nvt_idx));
> -    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &nvt_cam, 4);
> -}
> -
>  static void spapr_xive_end_reset(XiveEND *end)
>  {
>      memset(end, 0, sizeof(*end));
> @@ -544,21 +527,32 @@ static int spapr_xive_cpu_intc_create(SpaprInterruptController *intc,
>      }
>  
>      spapr_cpu->tctx = XIVE_TCTX(obj);
> -
> -    /*
> -     * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
> -     * don't beneficiate from the reset of the XIVE IRQ backend
> -     */
> -    spapr_xive_set_tctx_os_cam(spapr_cpu->tctx);
>      return 0;
>  }
>  
> +static void xive_tctx_set_os_cam(XiveTCTX *tctx, uint32_t os_cam)
> +{
> +    uint32_t qw1w2 = cpu_to_be32(TM_QW1W2_VO | os_cam);
> +    memcpy(&tctx->regs[TM_QW1_OS + TM_WORD2], &qw1w2, 4);
> +}
> +
>  static void spapr_xive_cpu_intc_reset(SpaprInterruptController *intc,
>                                       PowerPCCPU *cpu)
>  {
>      XiveTCTX *tctx = spapr_cpu_state(cpu)->tctx;
> +    uint8_t  nvt_blk;
> +    uint32_t nvt_idx;
>  
>      xive_tctx_reset(tctx);
> +
> +    /*
> +     * When a Virtual Processor is scheduled to run on a HW thread,
> +     * the hypervisor pushes its identifier in the OS CAM line.
> +     * Emulate the same behavior under QEMU.
> +     */
> +    spapr_xive_cpu_to_nvt(cpu, &nvt_blk, &nvt_idx);
> +
> +    xive_tctx_set_os_cam(tctx, xive_nvt_cam_line(nvt_blk, nvt_idx));
>  }
>  
>  static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val)
> @@ -651,14 +645,6 @@ static void spapr_xive_dt(SpaprInterruptController *intc, uint32_t nr_servers,
>  static int spapr_xive_activate(SpaprInterruptController *intc, Error **errp)
>  {
>      SpaprXive *xive = SPAPR_XIVE(intc);
> -    CPUState *cs;
> -
> -    CPU_FOREACH(cs) {
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> -        /* (TCG) Set the OS CAM line of the thread interrupt context. */
> -        spapr_xive_set_tctx_os_cam(spapr_cpu_state(cpu)->tctx);
> -    }
>  
>      if (kvm_enabled()) {
>          int rc = spapr_irq_init_kvm(kvmppc_xive_connect, intc, errp);

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

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

Re: [PATCH v5 0/7] ppc: reset the interrupt presenter from the CPU reset handler

[David Gibson]

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

On Tue, Oct 22, 2019 at 06:38:05PM +0200, Cédric Le Goater wrote:
> Hello,
> 
> On the sPAPR machine and PowerNV machine, the interrupt presenters are
> created by a machine handler at the core level and are reseted
> independently. This is not consistent and it raises issues when it
> comes to handle hot-plugged CPUs. In that case, the presenters are not
> reseted. This is less of an issue in XICS, although a zero MFFR could
> be a concern, but in XIVE, the OS CAM line is not set and this breaks
> the presenting algorithm. The current code has workarounds which need
> a global cleanup.
> 
> Extend the sPAPR IRQ backend and the PowerNV Chip class with a new
> cpu_intc_reset() handler called by the CPU reset handler and remove
> the XiveTCTX reset handler which is now redundant.
> 
> Set the OS CAM line when the interrupt presenter of the sPAPR core is
> reseted. This will also cover the case of hot-plugged CPUs.

I'm not totally convinced whether every step here is done the best it
can be.  But it addresses real problems, so I've applied anyway.  I
will make some comments inline, which could be addressed in follow up
patches.

> 
> Thanks,
> 
> C.
> 
> Changes in v5:
> 
>  - Removed useless PNV_CHIP() cast
>  
> Changes in v4:
> 
>  - Introduce a PnvCore reset handler
>  - Add PnvChip pointer to PnvCore
> 
> Changes in v3:
> 
>  - Introduced a DeviceClass::reset for the CPU (Greg)
>  - add support for PowerNV
>  
> Changes in v2:
> 
>  - removed property
>  - simplified reset handlers
> 
> Cédric Le Goater (6):
>   spapr: move CPU reset after presenter creation
>   ppc/pnv: Introduce a PnvCore reset handler
>   ppc/pnv: Add a PnvChip pointer to PnvCore
>   ppc: Reset the interrupt presenter from the CPU reset handler
>   ppc/pnv: Fix naming of routines realizing the CPUs
>   spapr/xive: Set the OS CAM line at reset
> 
> Greg Kurz (1):
>   spapr_cpu_core: Implement DeviceClass::reset
> 
>  include/hw/ppc/pnv.h        |  1 +
>  include/hw/ppc/pnv_core.h   |  3 +++
>  include/hw/ppc/spapr_irq.h  |  2 ++
>  include/hw/ppc/spapr_xive.h |  1 -
>  include/hw/ppc/xics.h       |  1 +
>  include/hw/ppc/xive.h       |  1 +
>  hw/intc/spapr_xive.c        | 53 +++++++++++++++++--------------------
>  hw/intc/xics.c              |  8 ++----
>  hw/intc/xics_spapr.c        |  7 +++++
>  hw/intc/xive.c              | 12 +--------
>  hw/ppc/pnv.c                | 18 +++++++++++++
>  hw/ppc/pnv_core.c           | 31 ++++++++++++++++------
>  hw/ppc/spapr_cpu_core.c     | 44 +++++++++++++++++++++++-------
>  hw/ppc/spapr_irq.c          | 14 ++++++++++
>  14 files changed, 131 insertions(+), 65 deletions(-)
> 

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

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