Re: [Qemu-devel] [PATCH v3 2/3] hw/arm/virt: Add PMU node for virt machine

[Andrew Jones]

On Mon, Apr 25, 2016 at 07:37:13PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/4/25 17:22, Andrew Jones wrote:
> > On Mon, Apr 25, 2016 at 03:11:45PM +0800, Shannon Zhao wrote:
> >> > From: Shannon Zhao <address@hidden>
> >> > 
> >> > Add a virtual PMU device for virt machine while use PPI 7 for PMU
> >> > overflow interrupt number.
> >> > 
> >> > Signed-off-by: Shannon Zhao <address@hidden>
> >> > ---
> >> >  hw/arm/virt.c         | 34 ++++++++++++++++++++++++++++++++++
> >> >  include/hw/arm/virt.h |  4 ++++
> >> >  include/sysemu/kvm.h  |  1 +
> >> >  stubs/kvm.c           |  5 +++++
> >> >  target-arm/kvm64.c    | 39 +++++++++++++++++++++++++++++++++++++++
> >> >  5 files changed, 83 insertions(+)
> >> > 
> >> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> > index 56d35c7..c3632d8 100644
> >> > --- a/hw/arm/virt.c
> >> > +++ b/hw/arm/virt.c
> >> > @@ -428,6 +428,38 @@ static void fdt_add_gic_node(VirtBoardInfo *vbi, 
> >> > int type)
> >> >      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", 
> >> > vbi->gic_phandle);
> >> >  }
> >> >  
> >> > +static void fdt_add_pmu_nodes(const VirtBoardInfo *vbi, int gictype)
> >> > +{
> >> > +    CPUState *cpu;
> >> > +    ARMCPU *armcpu;
> >> > +    uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> >> > +
> >> > +    CPU_FOREACH(cpu) {
> >> > +        armcpu = ARM_CPU(cpu);
> >> > +        if (!armcpu->has_pmu) {
> >> > +            return;
> >> > +        }
> >> > +
> >> > +        kvm_arm_pmu_create(cpu, PPI(VIRTUAL_PMU_IRQ));
> > I think we need to return a failure code from kvm_arm_pmu_create, and
> > then do
> > 
> >            if (!kvm_arm_pmu_create(...)) {
> >                return;
> >            }
> > 
> > Otherwise we create a /pmu node for the guest that won't work.
> > 
> Ok, will update.
> 
> >> > +    }
> >> > +
> >> > +    if (gictype == 2) {
> >> > +        irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START,
> >> > +                             GIC_FDT_IRQ_PPI_CPU_WIDTH,
> >> > +                             (1 << vbi->smp_cpus) - 1);
> > So, if we're using gicv3, then we're level triggered and these cpu
> > mask bits aren't defined by the arm,gic-v3.txt bindings spec. Good.
> > 
> > If we're using gicv2, then we're edge triggered and this mask is
> > defined. Assuming the GIC implementation requires using edge
> > triggered interrupts (as the comment in fdt_add_timer_nodes says),
> > then OK.
> > 
> IIRC the comments in fdt_add_timer_nodes are not correct now since KVM
> makes PPI level triggered.
> 
>     /* Note that on A15 h/w these interrupts are level-triggered,
>      * but for the GIC implementation provided by both QEMU and KVM
>      * they are edge-triggered.
>      */

OK, in that case the comment should be updated to say something like
"used to", but I suspect it's too late for the timer to switch to
level at this point anyway, unless KVM can be probed to determine if
level is OK.

However, for PMU, we know that if the PMU feature exists, then level is
OK, so if we want a level triggered interrupt for PMU, then we should
change the giv2 irqflags assignment to an '|=' of the cpu mask.

Thanks,
drew