[v2,1/4] hw/arm/virt: add pmu interrupt state

Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
CPU state.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c    | 3 +++
 target/arm/cpu.c | 2 ++
 target/arm/cpu.h | 2 ++
 3 files changed, 7 insertions(+)

On Wed, Jul 19, 2017 at 09:39:54AM -0400, Andrew Jones wrote:
> Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
> CPU state.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/virt.c    | 3 +++
>  target/arm/cpu.c | 2 ++
>  target/arm/cpu.h | 2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 31739d75a3e0..ea26f0c473c2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -610,6 +610,9 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
>          qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
>                                      qdev_get_gpio_in(gicdev, ppibase
>                                                       + ARCH_GICV3_MAINT_IRQ));
> +        qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
> +                                    qdev_get_gpio_in(gicdev, ppibase
> +                                                     + VIRTUAL_PMU_IRQ));

I know Peter reviewed this, but isn't it a bit strange to create the
pmu-interrupt when creating the gic (as this isn't an output from the
GIC like the maintenance interrupt is) ?

Thanks,
-Christoffer

>  
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
>          sysbus_connect_irq(gicbusdev, i + smp_cpus,
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 96d1f840301f..fd82c7944840 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -499,6 +499,8 @@ static void arm_cpu_initfn(Object *obj)
>  
>      qdev_init_gpio_out_named(DEVICE(cpu), &cpu->gicv3_maintenance_interrupt,
>                               "gicv3-maintenance-interrupt", 1);
> +    qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt,
> +                             "pmu-interrupt", 1);
>  #endif
>  
>      /* DTB consumers generally don't in fact care what the 'compatible'
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 102c58afac52..8d91166eb97b 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -584,6 +584,8 @@ struct ARMCPU {
>      qemu_irq gt_timer_outputs[NUM_GTIMERS];
>      /* GPIO output for GICv3 maintenance interrupt signal */
>      qemu_irq gicv3_maintenance_interrupt;
> +    /* GPIO output for the PMU interrupt */
> +    qemu_irq pmu_interrupt;
>  
>      /* MemoryRegion to use for secure physical accesses */
>      MemoryRegion *secure_memory;
> -- 
> 1.8.3.1
>

On Fri, Jul 21, 2017 at 01:16:07PM +0200, Christoffer Dall wrote:
> On Wed, Jul 19, 2017 at 09:39:54AM -0400, Andrew Jones wrote:
> > Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
> > CPU state.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  hw/arm/virt.c    | 3 +++
> >  target/arm/cpu.c | 2 ++
> >  target/arm/cpu.h | 2 ++
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 31739d75a3e0..ea26f0c473c2 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -610,6 +610,9 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
> >          qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
> >                                      qdev_get_gpio_in(gicdev, ppibase
> >                                                       + ARCH_GICV3_MAINT_IRQ));
> > +        qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
> > +                                    qdev_get_gpio_in(gicdev, ppibase
> > +                                                     + VIRTUAL_PMU_IRQ));
> 
> I know Peter reviewed this, but isn't it a bit strange to create the
> pmu-interrupt when creating the gic (as this isn't an output from the
> GIC like the maintenance interrupt is) ?
>

Above the gic maintenance interrupt connection the timer irqs are also
connected. So, while the function name implies we only create the gic,
its function appears to be both its creation and the wiring up of CPU
inputs and outputs.

Thanks,
drew

On Fri, Jul 21, 2017 at 01:35:46PM +0200, Andrew Jones wrote:
> On Fri, Jul 21, 2017 at 01:16:07PM +0200, Christoffer Dall wrote:
> > On Wed, Jul 19, 2017 at 09:39:54AM -0400, Andrew Jones wrote:
> > > Mimicking gicv3-maintenance-interrupt, add the PMU's interrupt to
> > > CPU state.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > > ---
> > >  hw/arm/virt.c    | 3 +++
> > >  target/arm/cpu.c | 2 ++
> > >  target/arm/cpu.h | 2 ++
> > >  3 files changed, 7 insertions(+)
> > > 
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 31739d75a3e0..ea26f0c473c2 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -610,6 +610,9 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
> > >          qdev_connect_gpio_out_named(cpudev, "gicv3-maintenance-interrupt", 0,
> > >                                      qdev_get_gpio_in(gicdev, ppibase
> > >                                                       + ARCH_GICV3_MAINT_IRQ));
> > > +        qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
> > > +                                    qdev_get_gpio_in(gicdev, ppibase
> > > +                                                     + VIRTUAL_PMU_IRQ));
> > 
> > I know Peter reviewed this, but isn't it a bit strange to create the
> > pmu-interrupt when creating the gic (as this isn't an output from the
> > GIC like the maintenance interrupt is) ?
> >
> 
> Above the gic maintenance interrupt connection the timer irqs are also
> connected. So, while the function name implies we only create the gic,
> its function appears to be both its creation and the wiring up of CPU
> inputs and outputs.
> 

ok, I didn't see that.

Otherwise this patch looks good to me.

-Christoffer