Re: [PATCH] powerpc/pseries: hcall_exit tracepoint retval should be signed

[Ravi Bangoria]

On 05/07/2018 06:33 PM, Michael Ellerman wrote:
> The hcall_exit() tracepoint has retval defined as unsigned long. That
> leads to humours results like:
>
>   bash-3686  [009] d..2   854.134094: hcall_entry: opcode=24
>   bash-3686  [009] d..2   854.134095: hcall_exit: opcode=24 
> retval=18446744073709551609
>
> It's normal for some hcalls to return negative values, displaying them
> as unsigned isn't very helpful. So change it to signed.
>
>   bash-3711  [001] d..2   471.691008: hcall_entry: opcode=24
>   bash-3711  [001] d..2   471.691008: hcall_exit: opcode=24 retval=-7
>
> Which can be more easily compared to H_NOT_FOUND in hvcall.h

Tested-by: Ravi Bangoria 

Thank,
Ravi

Re: [PATCH] powerpc/pseries: hcall_exit tracepoint retval should be signed

[Anton Blanchard]

Hi Michael,

> The hcall_exit() tracepoint has retval defined as unsigned long. That
> leads to humours results like:
> 
>   bash-3686  [009] d..2   854.134094: hcall_entry: opcode=24
>   bash-3686  [009] d..2   854.134095: hcall_exit: opcode=24
> retval=18446744073709551609
> 
> It's normal for some hcalls to return negative values, displaying them
> as unsigned isn't very helpful. So change it to signed.
> 
>   bash-3711  [001] d..2   471.691008: hcall_entry: opcode=24
>   bash-3711  [001] d..2   471.691008: hcall_exit: opcode=24 retval=-7
> 
> Which can be more easily compared to H_NOT_FOUND in hvcall.h

Much nicer.

Acked-by: Anton Blanchard 

Anton

> Signed-off-by: Michael Ellerman 
> ---
>  arch/powerpc/include/asm/asm-prototypes.h| 3 +--
>  arch/powerpc/include/asm/trace.h | 7 +++
>  arch/powerpc/platforms/pseries/hvCall_inst.c | 2 +-
>  arch/powerpc/platforms/pseries/lpar.c| 3 +--
>  4 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/asm-prototypes.h
> b/arch/powerpc/include/asm/asm-prototypes.h index
> d9713ad62e3c..068760d61e7e 100644 ---
> a/arch/powerpc/include/asm/asm-prototypes.h +++
> b/arch/powerpc/include/asm/asm-prototypes.h @@ -36,8 +36,7 @@ void
> kexec_copy_flush(struct kimage *image); /* pseries hcall tracing */
>  extern struct static_key hcall_tracepoint_key;
>  void __trace_hcall_entry(unsigned long opcode, unsigned long *args);
> -void __trace_hcall_exit(long opcode, unsigned long retval,
> - unsigned long *retbuf);
> +void __trace_hcall_exit(long opcode, long retval, unsigned long
> *retbuf); /* OPAL tracing */
>  #ifdef HAVE_JUMP_LABEL
>  extern struct static_key opal_tracepoint_key;
> diff --git a/arch/powerpc/include/asm/trace.h
> b/arch/powerpc/include/asm/trace.h index 33f3b479138b..d018e8602694
> 100644 --- a/arch/powerpc/include/asm/trace.h
> +++ b/arch/powerpc/include/asm/trace.h
> @@ -81,8 +81,7 @@ TRACE_EVENT_FN_COND(hcall_entry,
>  
>  TRACE_EVENT_FN_COND(hcall_exit,
>  
> - TP_PROTO(unsigned long opcode, unsigned long retval,
> - unsigned long *retbuf),
> + TP_PROTO(unsigned long opcode, long retval, unsigned long
> *retbuf), 
>   TP_ARGS(opcode, retval, retbuf),
>  
> @@ -90,7 +89,7 @@ TRACE_EVENT_FN_COND(hcall_exit,
>  
>   TP_STRUCT__entry(
>   __field(unsigned long, opcode)
> - __field(unsigned long, retval)
> + __field(long, retval)
>   ),
>  
>   TP_fast_assign(
> @@ -98,7 +97,7 @@ TRACE_EVENT_FN_COND(hcall_exit,
>   __entry->retval = retval;
>   ),
>  
> - TP_printk("opcode=%lu retval=%lu", __entry->opcode,
> __entry->retval),
> + TP_printk("opcode=%lu retval=%ld", __entry->opcode,
> __entry->retval), 
>   hcall_tracepoint_regfunc, hcall_tracepoint_unregfunc
>  );
> diff --git a/arch/powerpc/platforms/pseries/hvCall_inst.c
> b/arch/powerpc/platforms/pseries/hvCall_inst.c index
> 89b7ce807e70..6da320c786cd 100644 ---
> a/arch/powerpc/platforms/pseries/hvCall_inst.c +++
> b/arch/powerpc/platforms/pseries/hvCall_inst.c @@ -125,7 +125,7 @@
> static void probe_hcall_entry(void *ignored, unsigned long opcode,
> unsigned long h->purr_start = mfspr(SPRN_PURR); }
>  
> -static void probe_hcall_exit(void *ignored, unsigned long opcode,
> unsigned long retval, +static void probe_hcall_exit(void *ignored,
> unsigned long opcode, long retval, unsigned long *retbuf)
>  {
>   struct hcall_stats *h;
> diff --git a/arch/powerpc/platforms/pseries/lpar.c
> b/arch/powerpc/platforms/pseries/lpar.c index
> adb996ed51e1..5a392e40f3d2 100644 ---
> a/arch/powerpc/platforms/pseries/lpar.c +++
> b/arch/powerpc/platforms/pseries/lpar.c @@ -902,8 +902,7 @@ void
> __trace_hcall_entry(unsigned long opcode, unsigned long *args)
> local_irq_restore(flags); }
>  
> -void __trace_hcall_exit(long opcode, unsigned long retval,
> - unsigned long *retbuf)
> +void __trace_hcall_exit(long opcode, long retval, unsigned long
> *retbuf) {
>   unsigned long flags;
>   unsigned int *depth;

[PATCH] powerpc/pseries: hcall_exit tracepoint retval should be signed

[Michael Ellerman]

The hcall_exit() tracepoint has retval defined as unsigned long. That
leads to humours results like:

  bash-3686  [009] d..2   854.134094: hcall_entry: opcode=24
  bash-3686  [009] d..2   854.134095: hcall_exit: opcode=24 
retval=18446744073709551609

It's normal for some hcalls to return negative values, displaying them
as unsigned isn't very helpful. So change it to signed.

  bash-3711  [001] d..2   471.691008: hcall_entry: opcode=24
  bash-3711  [001] d..2   471.691008: hcall_exit: opcode=24 retval=-7

Which can be more easily compared to H_NOT_FOUND in hvcall.h

Signed-off-by: Michael Ellerman 
---
 arch/powerpc/include/asm/asm-prototypes.h| 3 +--
 arch/powerpc/include/asm/trace.h | 7 +++
 arch/powerpc/platforms/pseries/hvCall_inst.c | 2 +-
 arch/powerpc/platforms/pseries/lpar.c| 3 +--
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/asm-prototypes.h 
b/arch/powerpc/include/asm/asm-prototypes.h
index d9713ad62e3c..068760d61e7e 100644
--- a/arch/powerpc/include/asm/asm-prototypes.h
+++ b/arch/powerpc/include/asm/asm-prototypes.h
@@ -36,8 +36,7 @@ void kexec_copy_flush(struct kimage *image);
 /* pseries hcall tracing */
 extern struct static_key hcall_tracepoint_key;
 void __trace_hcall_entry(unsigned long opcode, unsigned long *args);
-void __trace_hcall_exit(long opcode, unsigned long retval,
-   unsigned long *retbuf);
+void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf);
 /* OPAL tracing */
 #ifdef HAVE_JUMP_LABEL
 extern struct static_key opal_tracepoint_key;
diff --git a/arch/powerpc/include/asm/trace.h b/arch/powerpc/include/asm/trace.h
index 33f3b479138b..d018e8602694 100644
--- a/arch/powerpc/include/asm/trace.h
+++ b/arch/powerpc/include/asm/trace.h
@@ -81,8 +81,7 @@ TRACE_EVENT_FN_COND(hcall_entry,
 
 TRACE_EVENT_FN_COND(hcall_exit,
 
-   TP_PROTO(unsigned long opcode, unsigned long retval,
-   unsigned long *retbuf),
+   TP_PROTO(unsigned long opcode, long retval, unsigned long *retbuf),
 
TP_ARGS(opcode, retval, retbuf),
 
@@ -90,7 +89,7 @@ TRACE_EVENT_FN_COND(hcall_exit,
 
TP_STRUCT__entry(
__field(unsigned long, opcode)
-   __field(unsigned long, retval)
+   __field(long, retval)
),
 
TP_fast_assign(
@@ -98,7 +97,7 @@ TRACE_EVENT_FN_COND(hcall_exit,
__entry->retval = retval;
),
 
-   TP_printk("opcode=%lu retval=%lu", __entry->opcode, __entry->retval),
+   TP_printk("opcode=%lu retval=%ld", __entry->opcode, __entry->retval),
 
hcall_tracepoint_regfunc, hcall_tracepoint_unregfunc
 );
diff --git a/arch/powerpc/platforms/pseries/hvCall_inst.c 
b/arch/powerpc/platforms/pseries/hvCall_inst.c
index 89b7ce807e70..6da320c786cd 100644
--- a/arch/powerpc/platforms/pseries/hvCall_inst.c
+++ b/arch/powerpc/platforms/pseries/hvCall_inst.c
@@ -125,7 +125,7 @@ static void probe_hcall_entry(void *ignored, unsigned long 
opcode, unsigned long
h->purr_start = mfspr(SPRN_PURR);
 }
 
-static void probe_hcall_exit(void *ignored, unsigned long opcode, unsigned 
long retval,
+static void probe_hcall_exit(void *ignored, unsigned long opcode, long retval,
 unsigned long *retbuf)
 {
struct hcall_stats *h;
diff --git a/arch/powerpc/platforms/pseries/lpar.c 
b/arch/powerpc/platforms/pseries/lpar.c
index adb996ed51e1..5a392e40f3d2 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -902,8 +902,7 @@ void __trace_hcall_entry(unsigned long opcode, unsigned 
long *args)
local_irq_restore(flags);
 }
 
-void __trace_hcall_exit(long opcode, unsigned long retval,
-   unsigned long *retbuf)
+void __trace_hcall_exit(long opcode, long retval, unsigned long *retbuf)
 {
unsigned long flags;
unsigned int *depth;
-- 
2.14.1