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

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(-)

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]);
>       }
>

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]);
> >       }
> > 
>