Re: [PATCH v3] xen: arm: enable perf counters

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2015-01-14 at 15:07 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 14/01/15 14:33, Ian Campbell wrote:
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 25ecf1d..604fc81 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -179,7 +179,12 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
> >  {
> >      struct irq_desc *desc = irq_to_desc(irq);
> >  
> > -    /* TODO: perfc_incr(irqs); */
> > +    perfc_incr(irqs);
> > +
> > +    if (irq < 32) /* SGIs do not come down this path */
> 
> if ( irq < 32 )
> 
> Maybe an ASSERT(irq >= 16) to check the validity of the comment.

OK.

> > +            perfc_incr(psci_cpu_on);
> 
> I find confusing to use psci_cpu_on because it may refer to the host psci.
> 
> Maybe renaming to vpsci_ would be better?

Yes, good idea.

> 
> [..]
> 
> > @@ -2113,6 +2146,7 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >  
> >      default:
> >   bad_trap:
> > +        perfc_incr(trap_bad);
> 
> I don't think this perf counters is useful. When the CPU is receiving a
> bad trap Xen will panic in do_unexpected_trap.

Good point, I'll drop.

> > +#endif
> 
> Missing the Emacs local variables.

Will add.

> 
> > diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> > new file mode 100644
> > index 0000000..df86879
> > --- /dev/null
> > +++ b/xen/include/asm-arm/perfc_defn.h
> > @@ -0,0 +1,74 @@
> > +/* This file is legiimately included multiple times. */
> 
> Legacy for xen/include/perfc_defn.h?

No, since this file is included by that. perfc_defn.* contain lists
which are included multiple times using different definitions of
PERFCOUNTER etc to produce different bits of the require infrastructure
(definitions vs declarations). asm-x86/perfc_defn.h is identical in this
respect.

> > +/*#ifndef __XEN_PERFC_DEFN_H__*/
> 
> All headers in asm-arm use __ARCH_ARM_* or __ARM_*
> 
> This would avoid the strange reason to not use guard in xen/perfc_defn.h.

No it wouldn't.
> 
> [..]
> 
> > +/*#endif*/ /* __XEN_PERFC_DEFN_H__ */
> 
> Missing local variables for emacs.

Will add.

Ian.