Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-11-11 Thread Paolo Bonzini 


On 11/11/2015 03:27, David Gibson wrote:
> Meaning one which adds the hook to the monitor, and another which
> implements it for ppc?  Maybe.
> 
> I'd like to take this, but I'm not sure whether it's reasonable for
> the small generic monitor change to go through the ppc tree.  Peter,
> Paolo, opinion?

I'm not a monitor maintainer, but I think it's okay since there is
none. :)

We probably should replace target_monitor_defs() with an implementation
of target_get_monitor_def on all targets.  It can just pass a MonitorDef
array to a common function, i.e.

int lookup_monitor_def(MonitorDef *def, void *base, const char *name, uint64_t 
*pval)

static int target_get_monitor_def(CPUState *cs, const char *name, uint64_t 
*pval)
{
return lookup_monitor_def(monitor_defs, cs->env_ptr, name, pval);
}

I don't want you to do it now, but we should add it to the wiki
BiteSizedTasks so we don't forget.

Paolo

Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-11-10 Thread David Gibson 
On Fri, Oct 02, 2015 at 04:16:13PM +1000, Alexey Kardashevskiy wrote:
> At the moment get_monitor_def() returns only registers from statically
> defined monitor_defs array. However there is a lot of BOOK3S SPRs
> which are not in the list and cannot be printed from the monitor.
> 
> This adds a new target platform hook - target_get_monitor_def().
> The hook is called if a register was not found in the static
> array returned by the target_monitor_defs() hook.
> 
> The hook is only defined for POWERPC, it returns registered
> SPRs and fails on unregistered ones providing the user with information
> on what is actually supported on the running CPU. The register value is
> saved as uint64_t as it is the biggest supported register size;
> target_ulong cannot be used because of the stub - it is in a "common"
> code and cannot include "cpu.h", etc; this is also why the hook prototype
> is redefined in the stub instead of being included from some header.
> 
> This replaces static descriptors for GPRs, FPRs, SRs with a helper which
> looks for a value in a corresponding array in the CPUPPCState.
> The immediate effect is that all 32 SRs can be printed now (instead of 16);
> later this can be reused for VSX or TM registers.
> 
> While we are here, this adds "cr" as a synonym of "ccr".

Sorry I've taken so long to look at this.

This is a big improvement over the current approach.

> Signed-off-by: Alexey Kardashevskiy 

Reviewed-by: David Gibson 

(although I note one small nit below)

> ---
> 
> Does it make sense to split it into two patches?

Meaning one which adds the hook to the monitor, and another which
implements it for ppc?  Maybe.

I'd like to take this, but I'm not sure whether it's reasonable for
the small generic monitor change to go through the ppc tree.  Peter,
Paolo, opinion?


[snip]
> diff --git a/target-ppc/monitor.c b/target-ppc/monitor.c
> index 9cb1fe9..98417f0 100644
> --- a/target-ppc/monitor.c
> +++ b/target-ppc/monitor.c
> @@ -76,176 +76,21 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
>  dump_mmu((FILE*)mon, (fprintf_function)monitor_printf, env1);
>  }
>  
> -
>  const MonitorDef monitor_defs[] = {
> -/* General purpose registers */
> -{ "r0", offsetof(CPUPPCState, gpr[0]) },
> -{ "r1", offsetof(CPUPPCState, gpr[1]) },
> -{ "r2", offsetof(CPUPPCState, gpr[2]) },
> -{ "r3", offsetof(CPUPPCState, gpr[3]) },
> -{ "r4", offsetof(CPUPPCState, gpr[4]) },
> -{ "r5", offsetof(CPUPPCState, gpr[5]) },
> -{ "r6", offsetof(CPUPPCState, gpr[6]) },
> -{ "r7", offsetof(CPUPPCState, gpr[7]) },
> -{ "r8", offsetof(CPUPPCState, gpr[8]) },
> -{ "r9", offsetof(CPUPPCState, gpr[9]) },
> -{ "r10", offsetof(CPUPPCState, gpr[10]) },
> -{ "r11", offsetof(CPUPPCState, gpr[11]) },
> -{ "r12", offsetof(CPUPPCState, gpr[12]) },
> -{ "r13", offsetof(CPUPPCState, gpr[13]) },
> -{ "r14", offsetof(CPUPPCState, gpr[14]) },
> -{ "r15", offsetof(CPUPPCState, gpr[15]) },
> -{ "r16", offsetof(CPUPPCState, gpr[16]) },
> -{ "r17", offsetof(CPUPPCState, gpr[17]) },
> -{ "r18", offsetof(CPUPPCState, gpr[18]) },
> -{ "r19", offsetof(CPUPPCState, gpr[19]) },
> -{ "r20", offsetof(CPUPPCState, gpr[20]) },
> -{ "r21", offsetof(CPUPPCState, gpr[21]) },
> -{ "r22", offsetof(CPUPPCState, gpr[22]) },
> -{ "r23", offsetof(CPUPPCState, gpr[23]) },
> -{ "r24", offsetof(CPUPPCState, gpr[24]) },
> -{ "r25", offsetof(CPUPPCState, gpr[25]) },
> -{ "r26", offsetof(CPUPPCState, gpr[26]) },
> -{ "r27", offsetof(CPUPPCState, gpr[27]) },
> -{ "r28", offsetof(CPUPPCState, gpr[28]) },
> -{ "r29", offsetof(CPUPPCState, gpr[29]) },
> -{ "r30", offsetof(CPUPPCState, gpr[30]) },
> -{ "r31", offsetof(CPUPPCState, gpr[31]) },
> -/* Floating point registers */
> -{ "f0", offsetof(CPUPPCState, fpr[0]) },
> -{ "f1", offsetof(CPUPPCState, fpr[1]) },
> -{ "f2", offsetof(CPUPPCState, fpr[2]) },
> -{ "f3", offsetof(CPUPPCState, fpr[3]) },
> -{ "f4", offsetof(CPUPPCState, fpr[4]) },
> -{ "f5", offsetof(CPUPPCState, fpr[5]) },
> -{ "f6", offsetof(CPUPPCState, fpr[6]) },
> -{ "f7", offsetof(CPUPPCState, fpr[7]) },
> -{ "f8", offsetof(CPUPPCState, fpr[8]) },
> -{ "f9", offsetof(CPUPPCState, fpr[9]) },
> -{ "f10", offsetof(CPUPPCState, fpr[10]) },
> -{ "f11", offsetof(CPUPPCState, fpr[11]) },
> -{ "f12", offsetof(CPUPPCState, fpr[12]) },
> -{ "f13", offsetof(CPUPPCState, fpr[13]) },
> -{ "f14", offsetof(CPUPPCState, fpr[14]) },
> -{ "f15", offsetof(CPUPPCState, fpr[15]) },
> -{ "f16", offsetof(CPUPPCState, fpr[16]) },
> -{ "f17", offsetof(CPUPPCState, fpr[17]) },
> -{ "f18", offsetof(CPUPPCState, fpr[18]) },
> -{ "f19", offsetof(CPUPPCState, fpr[19]) },
> -{ "f20", offsetof(CPUPPCState, fpr[20]) },
> -{ "f21", offsetof(CPUPPCState, fpr[21]) },
> -{ "f22", offsetof(CPUPPCState, fpr[22])

Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-10-23 Thread Benjamin Herrenschmidt 
On Thu, 2015-10-22 at 20:20 +1100, David Gibson wrote:
> I asked a while back if your patch or BenH's patch doing a similar
> thing was the one I should be looking at, and I never got a reply.

You must have missed it ... go for Alexey latest

Cheers,
Ben.

Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-10-22 Thread Alexey Kardashevskiy 

On 10/02/2015 04:16 PM, Alexey Kardashevskiy wrote:

At the moment get_monitor_def() returns only registers from statically
defined monitor_defs array. However there is a lot of BOOK3S SPRs
which are not in the list and cannot be printed from the monitor.

This adds a new target platform hook - target_get_monitor_def().
The hook is called if a register was not found in the static
array returned by the target_monitor_defs() hook.

The hook is only defined for POWERPC, it returns registered
SPRs and fails on unregistered ones providing the user with information
on what is actually supported on the running CPU. The register value is
saved as uint64_t as it is the biggest supported register size;
target_ulong cannot be used because of the stub - it is in a "common"
code and cannot include "cpu.h", etc; this is also why the hook prototype
is redefined in the stub instead of being included from some header.

This replaces static descriptors for GPRs, FPRs, SRs with a helper which
looks for a value in a corresponding array in the CPUPPCState.
The immediate effect is that all 32 SRs can be printed now (instead of 16);
later this can be reused for VSX or TM registers.

While we are here, this adds "cr" as a synonym of "ccr".

Signed-off-by: Alexey Kardashevskiy 
---

Does it make sense to split it into two patches?



Ping?




---
Changes:
v4:
* rebased on the current upstream which moved MonitorDef to target-ppc
* reverted the change for registers other than GPR/FPR/SPR, such as
FPSCR/PC/MSR/...
* removed CPU hook and made it a stub

v3:
* removed the check for endptr as strtoul() always initializes it
* check if there is any number after r/f/sr and fail if none
* added tolower()/strncasecmp() to support both r/f/sr and R/F/SR

v2:
* handles r**, f**, sr** if their numbers  were parsed completely and correctly
* added "cr" as synonym of "ccr"
---
  include/monitor/hmp-target.h   |   1 +
  monitor.c  |  10 +-
  stubs/Makefile.objs|   1 +
  stubs/target-get-monitor-def.c |  31 ++
  target-ppc/cpu-qom.h   |   2 +
  target-ppc/monitor.c   | 219 -
  6 files changed, 105 insertions(+), 159 deletions(-)
  create mode 100644 stubs/target-get-monitor-def.c

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 213566c..bc2c9c0 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_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);

  CPUArchState *mon_get_cpu_env(void);
  CPUState *mon_get_cpu(void);
diff --git a/monitor.c b/monitor.c
index 4f1ba2f..83e126a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2104,6 +2104,8 @@ static int get_monitor_def(target_long *pval, const char 
*name)
  {
  const MonitorDef *md = target_monitor_defs();
  void *ptr;
+uint64_t tmp = 0;
+int ret;

  if (md == NULL) {
  return -1;
@@ -2131,7 +2133,13 @@ static int get_monitor_def(target_long *pval, const char 
*name)
  return 0;
  }
  }
-return -1;
+
+ret = target_get_monitor_def(mon_get_cpu(), name, );
+if (!ret) {
+*pval = (target_long) tmp;
+}
+
+return ret;
  }

  static void next(void)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 85e4e81..91bcdbb 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-get-monitor-def.o
diff --git a/stubs/target-get-monitor-def.c b/stubs/target-get-monitor-def.c
new file mode 100644
index 000..711a9ae
--- /dev/null
+++ b/stubs/target-get-monitor-def.c
@@ -0,0 +1,31 @@
+/*
+ *  Stub for target_get_monitor_def.
+ *
+ *  Copyright IBM Corp., 2015
+ *
+ *  Author: Alexey Kardashevskiy 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see .
+ */
+
+#include "stdint.h"
+
+typedef struct CPUState CPUState;
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
+{
+return -1;
+}
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 6967a80..bc20504 100644

Re: [Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-10-22 Thread David Gibson 
On Thu, Oct 22, 2015 at 06:31:56PM +1100, Alexey Kardashevskiy wrote:
> On 10/02/2015 04:16 PM, Alexey Kardashevskiy wrote:
> >At the moment get_monitor_def() returns only registers from statically
> >defined monitor_defs array. However there is a lot of BOOK3S SPRs
> >which are not in the list and cannot be printed from the monitor.
> >
> >This adds a new target platform hook - target_get_monitor_def().
> >The hook is called if a register was not found in the static
> >array returned by the target_monitor_defs() hook.
> >
> >The hook is only defined for POWERPC, it returns registered
> >SPRs and fails on unregistered ones providing the user with information
> >on what is actually supported on the running CPU. The register value is
> >saved as uint64_t as it is the biggest supported register size;
> >target_ulong cannot be used because of the stub - it is in a "common"
> >code and cannot include "cpu.h", etc; this is also why the hook prototype
> >is redefined in the stub instead of being included from some header.
> >
> >This replaces static descriptors for GPRs, FPRs, SRs with a helper which
> >looks for a value in a corresponding array in the CPUPPCState.
> >The immediate effect is that all 32 SRs can be printed now (instead of 16);
> >later this can be reused for VSX or TM registers.
> >
> >While we are here, this adds "cr" as a synonym of "ccr".
> >
> >Signed-off-by: Alexey Kardashevskiy 
> >---
> >
> >Does it make sense to split it into two patches?

I asked a while back if your patch or BenH's patch doing a similar
thing was the one I should be looking at, and I never got a reply.

-- 
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


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH qemu v4] monitor/target-ppc: Define target_get_monitor_def

2015-10-02 Thread Alexey Kardashevskiy 
At the moment get_monitor_def() returns only registers from statically
defined monitor_defs array. However there is a lot of BOOK3S SPRs
which are not in the list and cannot be printed from the monitor.

This adds a new target platform hook - target_get_monitor_def().
The hook is called if a register was not found in the static
array returned by the target_monitor_defs() hook.

The hook is only defined for POWERPC, it returns registered
SPRs and fails on unregistered ones providing the user with information
on what is actually supported on the running CPU. The register value is
saved as uint64_t as it is the biggest supported register size;
target_ulong cannot be used because of the stub - it is in a "common"
code and cannot include "cpu.h", etc; this is also why the hook prototype
is redefined in the stub instead of being included from some header.

This replaces static descriptors for GPRs, FPRs, SRs with a helper which
looks for a value in a corresponding array in the CPUPPCState.
The immediate effect is that all 32 SRs can be printed now (instead of 16);
later this can be reused for VSX or TM registers.

While we are here, this adds "cr" as a synonym of "ccr".

Signed-off-by: Alexey Kardashevskiy 
---

Does it make sense to split it into two patches?

---
Changes:
v4:
* rebased on the current upstream which moved MonitorDef to target-ppc
* reverted the change for registers other than GPR/FPR/SPR, such as
FPSCR/PC/MSR/...
* removed CPU hook and made it a stub

v3:
* removed the check for endptr as strtoul() always initializes it
* check if there is any number after r/f/sr and fail if none
* added tolower()/strncasecmp() to support both r/f/sr and R/F/SR

v2:
* handles r**, f**, sr** if their numbers  were parsed completely and correctly
* added "cr" as synonym of "ccr"
---
 include/monitor/hmp-target.h   |   1 +
 monitor.c  |  10 +-
 stubs/Makefile.objs|   1 +
 stubs/target-get-monitor-def.c |  31 ++
 target-ppc/cpu-qom.h   |   2 +
 target-ppc/monitor.c   | 219 -
 6 files changed, 105 insertions(+), 159 deletions(-)
 create mode 100644 stubs/target-get-monitor-def.c

diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 213566c..bc2c9c0 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_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
 
 CPUArchState *mon_get_cpu_env(void);
 CPUState *mon_get_cpu(void);
diff --git a/monitor.c b/monitor.c
index 4f1ba2f..83e126a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2104,6 +2104,8 @@ static int get_monitor_def(target_long *pval, const char 
*name)
 {
 const MonitorDef *md = target_monitor_defs();
 void *ptr;
+uint64_t tmp = 0;
+int ret;
 
 if (md == NULL) {
 return -1;
@@ -2131,7 +2133,13 @@ static int get_monitor_def(target_long *pval, const char 
*name)
 return 0;
 }
 }
-return -1;
+
+ret = target_get_monitor_def(mon_get_cpu(), name, );
+if (!ret) {
+*pval = (target_long) tmp;
+}
+
+return ret;
 }
 
 static void next(void)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 85e4e81..91bcdbb 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-get-monitor-def.o
diff --git a/stubs/target-get-monitor-def.c b/stubs/target-get-monitor-def.c
new file mode 100644
index 000..711a9ae
--- /dev/null
+++ b/stubs/target-get-monitor-def.c
@@ -0,0 +1,31 @@
+/*
+ *  Stub for target_get_monitor_def.
+ *
+ *  Copyright IBM Corp., 2015
+ *
+ *  Author: Alexey Kardashevskiy 
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see .
+ */
+
+#include "stdint.h"
+
+typedef struct CPUState CPUState;
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
+
+int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval)
+{
+return -1;
+}
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index 6967a80..bc20504 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -118,6 +118,8 @@ void