[Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Benjamin Herrenschmidt]

We already have a table with all supported SPRs along with their names,
so let's use that rather than a duplicate table that is perpetually
out of sync in the monitor code.

This adds a new monitor hook target_extra_monitor_def() which is called
if nothing is found is the normal table. We still use the old mechanism
for anything that isn't an SPR.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 include/monitor/hmp-target.h     |  1 +
 monitor.c                        |  8 +++-
 stubs/Makefile.objs              |  1 +
 stubs/target-extra-monitor-def.c | 10 +++++
 target-ppc/monitor.c             | 93 +++++++++-------------------------------
 5 files changed, 39 insertions(+), 74 deletions(-)
 create mode 100644 stubs/target-extra-monitor-def.c

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 213566c..b946e32 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -35,6 +35,7 @@ struct MonitorDef {
 };
 
 const MonitorDef *target_monitor_defs(void);
+int target_extra_monitor_def(uint64_t *pval, const char *name);
 
 CPUArchState *mon_get_cpu_env(void);
 CPUState *mon_get_cpu(void);
diff --git a/monitor.c b/monitor.c
index 881d869..43cfcf8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2106,7 +2106,9 @@ expr_error(Monitor *mon, const char *fmt, ...)
 static int get_monitor_def(target_long *pval, const char *name)
 {
     const MonitorDef *md = target_monitor_defs();
+    uint64_t val;
     void *ptr;
+    int rc;
 
     if (md == NULL) {
         return -1;
@@ -2134,7 +2136,11 @@ static int get_monitor_def(target_long *pval, const char *name)
             return 0;
         }
     }
-    return -1;
+    rc = target_extra_monitor_def(&val, name);
+    if (rc >= 0) {
+        *pval = val;
+    }
+    return rc;
 }
 
 static void next(void)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 85e4e81..c042f73 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,4 @@ stub-obj-y += cpus.o
 stub-obj-y += kvm.o
 stub-obj-y += qmp_pc_dimm_device_list.o
 stub-obj-y += target-monitor-defs.o
+stub-obj-y += target-extra-monitor-def.o
diff --git a/stubs/target-extra-monitor-def.c b/stubs/target-extra-monitor-def.c
new file mode 100644
index 0000000..6f108f3
--- /dev/null
+++ b/stubs/target-extra-monitor-def.c
@@ -0,0 +1,10 @@
+#include "stddef.h"
+#include "qemu/typedefs.h"
+#include <stdint.h>
+
+int target_extra_monitor_def(uint64_t *pval, const char *name);
+
+int target_extra_monitor_def(uint64_t *pval, const char *name)
+{
+    return -1;
+}
diff --git a/target-ppc/monitor.c b/target-ppc/monitor.c
index 9cb1fe9..f52dbc9 100644
--- a/target-ppc/monitor.c
+++ b/target-ppc/monitor.c
@@ -157,7 +157,6 @@ const MonitorDef monitor_defs[] = {
     { "tbu", 0, &monitor_get_tbu, },
     { "tbl", 0, &monitor_get_tbl, },
     /* Segment registers */
-    { "sdr1", offsetof(CPUPPCState, spr[SPR_SDR1]) },
     { "sr0", offsetof(CPUPPCState, sr[0]) },
     { "sr1", offsetof(CPUPPCState, sr[1]) },
     { "sr2", offsetof(CPUPPCState, sr[2]) },
@@ -174,78 +173,6 @@ const MonitorDef monitor_defs[] = {
     { "sr13", offsetof(CPUPPCState, sr[13]) },
     { "sr14", offsetof(CPUPPCState, sr[14]) },
     { "sr15", offsetof(CPUPPCState, sr[15]) },
-    /* Too lazy to put BATs... */
-    { "pvr", offsetof(CPUPPCState, spr[SPR_PVR]) },
-
-    { "srr0", offsetof(CPUPPCState, spr[SPR_SRR0]) },
-    { "srr1", offsetof(CPUPPCState, spr[SPR_SRR1]) },
-    { "dar", offsetof(CPUPPCState, spr[SPR_DAR]) },
-    { "dsisr", offsetof(CPUPPCState, spr[SPR_DSISR]) },
-    { "cfar", offsetof(CPUPPCState, spr[SPR_CFAR]) },
-    { "sprg0", offsetof(CPUPPCState, spr[SPR_SPRG0]) },
-    { "sprg1", offsetof(CPUPPCState, spr[SPR_SPRG1]) },
-    { "sprg2", offsetof(CPUPPCState, spr[SPR_SPRG2]) },
-    { "sprg3", offsetof(CPUPPCState, spr[SPR_SPRG3]) },
-    { "sprg4", offsetof(CPUPPCState, spr[SPR_SPRG4]) },
-    { "sprg5", offsetof(CPUPPCState, spr[SPR_SPRG5]) },
-    { "sprg6", offsetof(CPUPPCState, spr[SPR_SPRG6]) },
-    { "sprg7", offsetof(CPUPPCState, spr[SPR_SPRG7]) },
-    { "pid", offsetof(CPUPPCState, spr[SPR_BOOKE_PID]) },
-    { "csrr0", offsetof(CPUPPCState, spr[SPR_BOOKE_CSRR0]) },
-    { "csrr1", offsetof(CPUPPCState, spr[SPR_BOOKE_CSRR1]) },
-    { "esr", offsetof(CPUPPCState, spr[SPR_BOOKE_ESR]) },
-    { "dear", offsetof(CPUPPCState, spr[SPR_BOOKE_DEAR]) },
-    { "mcsr", offsetof(CPUPPCState, spr[SPR_BOOKE_MCSR]) },
-    { "tsr", offsetof(CPUPPCState, spr[SPR_BOOKE_TSR]) },
-    { "tcr", offsetof(CPUPPCState, spr[SPR_BOOKE_TCR]) },
-    { "vrsave", offsetof(CPUPPCState, spr[SPR_VRSAVE]) },
-    { "pir", offsetof(CPUPPCState, spr[SPR_BOOKE_PIR]) },
-    { "mcsrr0", offsetof(CPUPPCState, spr[SPR_BOOKE_MCSRR0]) },
-    { "mcsrr1", offsetof(CPUPPCState, spr[SPR_BOOKE_MCSRR1]) },
-    { "decar", offsetof(CPUPPCState, spr[SPR_BOOKE_DECAR]) },
-    { "ivpr", offsetof(CPUPPCState, spr[SPR_BOOKE_IVPR]) },
-    { "epcr", offsetof(CPUPPCState, spr[SPR_BOOKE_EPCR]) },
-    { "sprg8", offsetof(CPUPPCState, spr[SPR_BOOKE_SPRG8]) },
-    { "ivor0", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR0]) },
-    { "ivor1", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR1]) },
-    { "ivor2", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR2]) },
-    { "ivor3", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR3]) },
-    { "ivor4", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR4]) },
-    { "ivor5", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR5]) },
-    { "ivor6", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR6]) },
-    { "ivor7", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR7]) },
-    { "ivor8", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR8]) },
-    { "ivor9", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR9]) },
-    { "ivor10", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR10]) },
-    { "ivor11", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR11]) },
-    { "ivor12", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR12]) },
-    { "ivor13", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR13]) },
-    { "ivor14", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR14]) },
-    { "ivor15", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR15]) },
-    { "ivor32", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR32]) },
-    { "ivor33", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR33]) },
-    { "ivor34", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR34]) },
-    { "ivor35", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR35]) },
-    { "ivor36", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR36]) },
-    { "ivor37", offsetof(CPUPPCState, spr[SPR_BOOKE_IVOR37]) },
-    { "mas0", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS0]) },
-    { "mas1", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS1]) },
-    { "mas2", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS2]) },
-    { "mas3", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS3]) },
-    { "mas4", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS4]) },
-    { "mas6", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS6]) },
-    { "mas7", offsetof(CPUPPCState, spr[SPR_BOOKE_MAS7]) },
-    { "mmucfg", offsetof(CPUPPCState, spr[SPR_MMUCFG]) },
-    { "tlb0cfg", offsetof(CPUPPCState, spr[SPR_BOOKE_TLB0CFG]) },
-    { "tlb1cfg", offsetof(CPUPPCState, spr[SPR_BOOKE_TLB1CFG]) },
-    { "epr", offsetof(CPUPPCState, spr[SPR_BOOKE_EPR]) },
-    { "eplc", offsetof(CPUPPCState, spr[SPR_BOOKE_EPLC]) },
-    { "epsc", offsetof(CPUPPCState, spr[SPR_BOOKE_EPSC]) },
-    { "svr", offsetof(CPUPPCState, spr[SPR_E500_SVR]) },
-    { "mcar", offsetof(CPUPPCState, spr[SPR_Exxx_MCAR]) },
-    { "pid1", offsetof(CPUPPCState, spr[SPR_BOOKE_PID1]) },
-    { "pid2", offsetof(CPUPPCState, spr[SPR_BOOKE_PID2]) },
-    { "hid0", offsetof(CPUPPCState, spr[SPR_HID0]) },
     { NULL },
 };
 
@@ -253,3 +180,23 @@ const MonitorDef *target_monitor_defs(void)
 {
     return monitor_defs;
 }
+
+int target_extra_monitor_def(uint64_t *pval, const char *name)
+{
+     /* On ppc, search through the SPRs so we can print any of them */
+    {
+        CPUArchState *env = mon_get_cpu_env();
+        ppc_spr_t *spr_cb = env->spr_cb;
+        int i;
+
+        for (i = 0; i < 1024; i++) {
+            if (!spr_cb[i].name || strcasecmp(name, spr_cb[i].name)) {
+                continue;
+            }
+            *pval = env->spr[i];
+            return 0;
+        }
+    }
+    return -1;
+}
+

Re: [Qemu-devel] [Qemu-ppc] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

On Sun, Sep 27, 2015 at 04:31:16PM +1000, Benjamin Herrenschmidt wrote:
> We already have a table with all supported SPRs along with their names,
> so let's use that rather than a duplicate table that is perpetually
> out of sync in the monitor code.
> 
> This adds a new monitor hook target_extra_monitor_def() which is called
> if nothing is found is the normal table. We still use the old mechanism
> for anything that isn't an SPR.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

This looks like a good idea, but it seems to be a slightly different
approach from the one taken by some rather similar patches Alexey
posted recently.

Would you care to co-ordinate on which of those approaches to go ahead
with?

[snip]
> @@ -253,3 +180,23 @@ const MonitorDef *target_monitor_defs(void)
>  {
>      return monitor_defs;
>  }
> +
> +int target_extra_monitor_def(uint64_t *pval, const char *name)
> +{
> +     /* On ppc, search through the SPRs so we can print any of them */
> +    {
       ^
Also, this appears to be a redundant set of braces.

> +        CPUArchState *env = mon_get_cpu_env();
> +        ppc_spr_t *spr_cb = env->spr_cb;
> +        int i;
> +
> +        for (i = 0; i < 1024; i++) {
> +            if (!spr_cb[i].name || strcasecmp(name, spr_cb[i].name)) {
> +                continue;
> +            }
> +            *pval = env->spr[i];
> +            return 0;
> +        }
> +    }
> +    return -1;
> +}
> +
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Benjamin Herrenschmidt]

On Wed, 2015-09-30 at 16:03 +1000, David Gibson wrote:
> On Sun, Sep 27, 2015 at 04:31:16PM +1000, Benjamin Herrenschmidt wrote:
> > We already have a table with all supported SPRs along with their names,
> > so let's use that rather than a duplicate table that is perpetually
> > out of sync in the monitor code.
> > 
> > This adds a new monitor hook target_extra_monitor_def() which is called
> > if nothing is found is the normal table. We still use the old mechanism
> > for anything that isn't an SPR.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This looks like a good idea, but it seems to be a slightly different
> approach from the one taken by some rather similar patches Alexey
> posted recently.
> 
> Would you care to co-ordinate on which of those approaches to go ahead
> with?

The code upstream has changed quite a bit...

> [snip]
> > @@ -253,3 +180,23 @@ const MonitorDef *target_monitor_defs(void)
> >  {
> >      return monitor_defs;
> >  }
> > +
> > +int target_extra_monitor_def(uint64_t *pval, const char *name)
> > +{
> > +     /* On ppc, search through the SPRs so we can print any of them */
> > +    {
>        ^
> Also, this appears to be a redundant set of braces.

Ah right, that used to be inside the caller (monitor_defs()) and I
moved it to a hook and forgot to take out the extra braces.

I'll respin.

>  +        CPUArchState *env = mon_get_cpu_env();
> > +        ppc_spr_t *spr_cb = env->spr_cb;
> > +        int i;
> > +
> > +        for (i = 0; i < 1024; i++) {
> > +            if (!spr_cb[i].name || strcasecmp(name, spr_cb[i].name)) {
> > +                continue;
> > +            }
> > +            *pval = env->spr[i];
> > +            return 0;
> > +        }
> > +    }
> > +    return -1;
> > +}
> > +
> > 
> > 
> > 
> 

Re: [Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Peter Maydell]

On 27 September 2015 at 07:31, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We already have a table with all supported SPRs along with their names,
> so let's use that rather than a duplicate table that is perpetually
> out of sync in the monitor code.
>
> This adds a new monitor hook target_extra_monitor_def() which is called
> if nothing is found is the normal table. We still use the old mechanism
> for anything that isn't an SPR.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  include/monitor/hmp-target.h     |  1 +
>  monitor.c                        |  8 +++-
>  stubs/Makefile.objs              |  1 +
>  stubs/target-extra-monitor-def.c | 10 +++++
>  target-ppc/monitor.c             | 93 +++++++++-------------------------------
>  5 files changed, 39 insertions(+), 74 deletions(-)
>  create mode 100644 stubs/target-extra-monitor-def.c
>
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index 213566c..b946e32 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -35,6 +35,7 @@ struct MonitorDef {
>  };
>
>  const MonitorDef *target_monitor_defs(void);
> +int target_extra_monitor_def(uint64_t *pval, const char *name);

This would be a good place to put a doc comment documenting
the semantics of this new hook.

MonitorDef structs treat the value to be obtained as
a target_long, but this uses uint64_t, which is a bit
inconsistent.

It might be better to:
 (a) fix the core monitor code to deal in int64_t rather
 than target_long
 (b) consider whether it would be better to have the ppc
 code generate a bunch of MonitorDef structs to return for the
 SPRs rather than having an extra hook function

> --- /dev/null
> +++ b/stubs/target-extra-monitor-def.c
> @@ -0,0 +1,10 @@
> +#include "stddef.h"
> +#include "qemu/typedefs.h"
> +#include <stdint.h>
> +
> +int target_extra_monitor_def(uint64_t *pval, const char *name);
> +
> +int target_extra_monitor_def(uint64_t *pval, const char *name)
> +{
> +    return -1;
> +}

It would be better to put the prototype for the hook somewhere
the stub file can include it rather than having it just rewritten
here.

thanks
-- PMM

Re: [Qemu-devel] [RFC/PATCH] monitor/ppc: Access all SPRs from the monitor

[Benjamin Herrenschmidt]

On Wed, 2015-09-30 at 12:47 +0100, Peter Maydell wrote:

> >  const MonitorDef *target_monitor_defs(void);
> > +int target_extra_monitor_def(uint64_t *pval, const char *name);
> 
> This would be a good place to put a doc comment documenting
> the semantics of this new hook.
> 
> MonitorDef structs treat the value to be obtained as
> a target_long, but this uses uint64_t, which is a bit
> inconsistent.

I couldn't get the definition of target_ulong in the stubs.

[ Note: Alexey has a different approach which completely replaces
monitor_defs() with a CPU specific one via a method in the CPU class.
I don't care which way you prefer as long as the functionality is there
]

> It might be better to:
>  (a) fix the core monitor code to deal in int64_t rather
>  than target_long
>  (b) consider whether it would be better to have the ppc
>  code generate a bunch of MonitorDef structs to return for the
>  SPRs rather than having an extra hook function

That sounds like the most bloated way to handle this :)

> > --- /dev/null
> > +++ b/stubs/target-extra-monitor-def.c
> > @@ -0,0 +1,10 @@
> > +#include "stddef.h"
> > +#include "qemu/typedefs.h"
> > +#include 
> > +
> > +int target_extra_monitor_def(uint64_t *pval, const char *name);
> > +
> > +int target_extra_monitor_def(uint64_t *pval, const char *name)
> > +{
> > +    return -1;
> > +}
> 
> It would be better to put the prototype for the hook somewhere
> the stub file can include it rather than having it just rewritten
> here.

I just copied the existing practice in get_monitor_defs()... but yes, I
agree.

Ben.

> thanks
> -- PMM