Re: [PATCH v5 1/2] perf,kvm/ppc: Add kvm_perf.h for powerpc

[Scott Wood <scottwood@freescale.com>]

On Wed, 2015-07-29 at 16:07 +0530, Hemant Kumar wrote:
> Hi Scott,
> 
> On 07/17/2015 01:40 AM, Scott Wood wrote:
> > On Thu, 2015-07-16 at 21:18 +0530, Hemant Kumar wrote:
> > > To analyze the exit events with perf, we need kvm_perf.h to be added in
> > > the arch/powerpc directory, where the kvm tracepoints needed to trace
> > > the KVM exit events are defined.
> > > 
> > > This patch adds "kvm_perf_book3s.h" to indicate that the tracepoints are
> > > book3s specific. Generic "kvm_perf.h" then can just include
> > > "kvm_perf_book3s.h".
> > > 
> > > Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
> > > ---
> > > Changes:
> > > - Not exporting the exit reasons compared to previous patchset 
> > > (suggested
> > > by Paul)
> > > 
> > >   arch/powerpc/include/uapi/asm/kvm_perf.h        |  6 ++++++
> > >   arch/powerpc/include/uapi/asm/kvm_perf_book3s.h | 14 ++++++++++++++
> > >   2 files changed, 20 insertions(+)
> > >   create mode 100644 arch/powerpc/include/uapi/asm/kvm_perf.h
> > >   create mode 100644 arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
> > > 
> > > diff --git a/arch/powerpc/include/uapi/asm/kvm_perf.h
> > > b/arch/powerpc/include/uapi/asm/kvm_perf.h
> > > new file mode 100644
> > > index 0000000..5ed2ff3
> > > --- /dev/null
> > > +++ b/arch/powerpc/include/uapi/asm/kvm_perf.h
> > > @@ -0,0 +1,6 @@
> > > +#ifndef _ASM_POWERPC_KVM_PERF_H
> > > +#define _ASM_POWERPC_KVM_PERF_H
> > > +
> > > +#include <asm/kvm_perf_book3s.h>
> > > +
> > > +#endif
> > > diff --git a/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
> > > b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
> > > new file mode 100644
> > > index 0000000..8c8d8c2
> > > --- /dev/null
> > > +++ b/arch/powerpc/include/uapi/asm/kvm_perf_book3s.h
> > > @@ -0,0 +1,14 @@
> > > +#ifndef _ASM_POWERPC_KVM_PERF_BOOK3S_H
> > > +#define _ASM_POWERPC_KVM_PERF_BOOK3S_H
> > > +
> > > +#include <asm/kvm.h>
> > > +
> > > +#define DECODE_STR_LEN 20
> > > +
> > > +#define VCPU_ID "vcpu_id"
> > > +
> > > +#define KVM_ENTRY_TRACE "kvm_hv:kvm_guest_enter"
> > > +#define KVM_EXIT_TRACE "kvm_hv:kvm_guest_exit"
> > > +#define KVM_EXIT_REASON "trap"
> > > +
> > > +#endif /* _ASM_POWERPC_KVM_PERF_BOOK3S_H */
> > Again, why is book3s stuff being presented via uapi as generic
> > <asm/kvm_perf.h> with generic symbol names?
> > 
> > -Scott
> 
> Ok.
> 
> We can change the KVM_ENTRY_TRACE macro to something like
> KVM_BOOK3S_ENTRY_TRACE and likewise for KVM_EXIT_TRACE
> and KVM_EXIT_REASON

What about DECODE_STR_LEN and VCPU_ID?

Where is this API documented?

>  and then, to resolve the issue of generic
> macro names in the userspace side, we can handle it using __weak
> modifier.

Does userspace get built differently for book3s versus book3e?  For now it'd 

be fine for userspace to check for book3s and not use the feature if it's 

book3e.  If and when book3e gains this feature, then userspace can be changed.

> What would you suggest?

Another option would be to explain this interface so that we can figure out 
if book3e would even want different values for these, and if not, move it to 
asm/kvm.h.

-Scott