diff mbox

[RFC,qom-cpu,04/41] cpu: Introduce CPUClass::set_pc() for gdb_set_cpu_pc()

Message ID 1372536117-28167-5-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber June 29, 2013, 8:01 p.m. UTC 
This moves setting the Program Counter from gdbstub into target code.
Use uint64_t type as maximum replacement for target_ulong.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 gdbstub.c                   | 39 ++++++---------------------------------
 include/qom/cpu.h           |  1 +
 target-alpha/cpu.c          |  8 ++++++++
 target-arm/cpu.c            |  8 ++++++++
 target-cris/cpu.c           |  8 ++++++++
 target-i386/cpu.c           |  8 ++++++++
 target-lm32/cpu.c           |  8 ++++++++
 target-microblaze/cpu.c     |  8 ++++++++
 target-mips/cpu.c           | 14 ++++++++++++++
 target-openrisc/cpu.c       |  8 ++++++++
 target-ppc/translate_init.c |  8 ++++++++
 target-s390x/cpu.c          |  8 ++++++++
 target-sh4/cpu.c            |  8 ++++++++
 target-sparc/cpu.c          |  9 +++++++++
 target-xtensa/cpu.c         |  8 ++++++++
 15 files changed, 118 insertions(+), 33 deletions(-)

Comments

Richard Henderson July 1, 2013, 5:09 p.m. UTC | #1 
On 06/29/2013 01:01 PM, Andreas Färber wrote:
> This moves setting the Program Counter from gdbstub into target code.
> Use uint64_t type as maximum replacement for target_ulong.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  gdbstub.c                   | 39 ++++++---------------------------------
>  include/qom/cpu.h           |  1 +
>  target-alpha/cpu.c          |  8 ++++++++
>  target-arm/cpu.c            |  8 ++++++++
>  target-cris/cpu.c           |  8 ++++++++
>  target-i386/cpu.c           |  8 ++++++++
>  target-lm32/cpu.c           |  8 ++++++++
>  target-microblaze/cpu.c     |  8 ++++++++
>  target-mips/cpu.c           | 14 ++++++++++++++
>  target-openrisc/cpu.c       |  8 ++++++++
>  target-ppc/translate_init.c |  8 ++++++++
>  target-s390x/cpu.c          |  8 ++++++++
>  target-sh4/cpu.c            |  8 ++++++++
>  target-sparc/cpu.c          |  9 +++++++++
>  target-xtensa/cpu.c         |  8 ++++++++
>  15 files changed, 118 insertions(+), 33 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~
Andreas Färber July 1, 2013, 5:25 p.m. UTC | #2 
Richard or anyone,

Am 01.07.2013 19:09, schrieb Richard Henderson:
> On 06/29/2013 01:01 PM, Andreas Färber wrote:
>> This moves setting the Program Counter from gdbstub into target code.
>> Use uint64_t type as maximum replacement for target_ulong.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  gdbstub.c                   | 39 ++++++---------------------------------
>>  include/qom/cpu.h           |  1 +
>>  target-alpha/cpu.c          |  8 ++++++++
>>  target-arm/cpu.c            |  8 ++++++++
>>  target-cris/cpu.c           |  8 ++++++++
>>  target-i386/cpu.c           |  8 ++++++++
>>  target-lm32/cpu.c           |  8 ++++++++
>>  target-microblaze/cpu.c     |  8 ++++++++
>>  target-mips/cpu.c           | 14 ++++++++++++++
>>  target-openrisc/cpu.c       |  8 ++++++++
>>  target-ppc/translate_init.c |  8 ++++++++
>>  target-s390x/cpu.c          |  8 ++++++++
>>  target-sh4/cpu.c            |  8 ++++++++
>>  target-sparc/cpu.c          |  9 +++++++++
>>  target-xtensa/cpu.c         |  8 ++++++++
>>  15 files changed, 118 insertions(+), 33 deletions(-)
> 
> Reviewed-by: Richard Henderson <rth@twiddle.net>

This is the first case where I am proposing the use of uint64_t in place
of target_ulong. In this case a gdb command using such a hook is not
performance-sensitive. Do you see this as an acceptable path for adding
further CPUClass hooks such as MMU fault handling?

Should we introduce some ulong-target-max typedef similar to hwaddr or
use plain uint64_t as done here?

Thanks,
Andreas
Richard Henderson July 1, 2013, 7:03 p.m. UTC | #3 
On 07/01/2013 10:25 AM, Andreas Färber wrote:
> This is the first case where I am proposing the use of uint64_t in place
> of target_ulong. In this case a gdb command using such a hook is not
> performance-sensitive. Do you see this as an acceptable path for adding
> further CPUClass hooks such as MMU fault handling?

I think that would be appropriate.  MMU faulting can't be much more performance
sensitive than device emulation, which is already standardizing on 64-bit
addresses.

> Should we introduce some ulong-target-max typedef similar to hwaddr or
> use plain uint64_t as done here?

I would think hwaddr wouldn't be appropriate, since that's supposed to be
talking about physical addresses, and that's not the case here.  The name (and
admittedly minimal documentation for) ram_addr_t sounds right, but it seems to
be sized wrong, so I don't know what it's actually supposed to be.

Unless someone has a better suggestion, I'd stay with uint64_t.


r~
Peter Maydell July 1, 2013, 7:41 p.m. UTC | #4 
On 1 July 2013 20:03, Richard Henderson <rth@twiddle.net> wrote:
> I would think hwaddr wouldn't be appropriate, since that's supposed to be
> talking about physical addresses, and that's not the case here.  The name (and
> admittedly minimal documentation for) ram_addr_t sounds right, but it seems to
> be sized wrong, so I don't know what it's actually supposed to be.

ram_addr_t is for indexes into guest RAM, ie into what you get by concatenating
all the RAMBlocks together. (This is why it's uintptr_t sized -- you know you
can't have more RAM total than will fit into the host's address space.)
It usually isn't what you want. Feel free to suggest improvements to
HACKING -- I tried to describe this in the bit that goes "ram_addr_t is
a QEMU internal address space ..." but since I already knew what it did
I probably failed to explain sufficiently.

Returning to the point, what we're after here is "a type which will hold
a guest virtual address but whose size doesn't depend on the target the
way target_ulong does", right? My inclination is to suggest that we have
a 'vaddr' typedef by analogy with 'hwaddr'; it seems like that might make
code dealing with guest addresses a little more self documenting. I don't
feel very strongly about it though so if people think it's pointless/will
cause problems that's fine.

thanks
-- PMM
Richard Henderson July 1, 2013, 8:20 p.m. UTC | #5 
On 07/01/2013 12:41 PM, Peter Maydell wrote:
> Returning to the point, what we're after here is "a type which will hold
> a guest virtual address but whose size doesn't depend on the target the
> way target_ulong does", right? My inclination is to suggest that we have
> a 'vaddr' typedef by analogy with 'hwaddr'; it seems like that might make
> code dealing with guest addresses a little more self documenting. I don't
> feel very strongly about it though so if people think it's pointless/will
> cause problems that's fine.
> 

I agree that vaddr would probably be a good name.


r~
diff mbox

Patch

diff --git a/gdbstub.c b/gdbstub.c
index 4a0d04e..bdf4496 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2042,40 +2042,13 @@  static void gdb_breakpoint_remove_all(void)
 
 static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 {
-    cpu_synchronize_state(ENV_GET_CPU(s->c_cpu));
-#if defined(TARGET_I386)
-    s->c_cpu->eip = pc;
-#elif defined (TARGET_PPC)
-    s->c_cpu->nip = pc;
-#elif defined (TARGET_SPARC)
-    s->c_cpu->pc = pc;
-    s->c_cpu->npc = pc + 4;
-#elif defined (TARGET_ARM)
-    s->c_cpu->regs[15] = pc;
-#elif defined (TARGET_SH4)
-    s->c_cpu->pc = pc;
-#elif defined (TARGET_MIPS)
-    s->c_cpu->active_tc.PC = pc & ~(target_ulong)1;
-    if (pc & 1) {
-        s->c_cpu->hflags |= MIPS_HFLAG_M16;
-    } else {
-        s->c_cpu->hflags &= ~(MIPS_HFLAG_M16);
+    CPUState *cpu = ENV_GET_CPU(s->c_cpu);
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    cpu_synchronize_state(cpu);
+    if (cc->set_pc) {
+        cc->set_pc(cpu, pc);
     }
-#elif defined (TARGET_MICROBLAZE)
-    s->c_cpu->sregs[SR_PC] = pc;
-#elif defined(TARGET_OPENRISC)
-    s->c_cpu->pc = pc;
-#elif defined (TARGET_CRIS)
-    s->c_cpu->pc = pc;
-#elif defined (TARGET_ALPHA)
-    s->c_cpu->pc = pc;
-#elif defined (TARGET_S390X)
-    s->c_cpu->psw.addr = pc;
-#elif defined (TARGET_LM32)
-    s->c_cpu->pc = pc;
-#elif defined(TARGET_XTENSA)
-    s->c_cpu->pc = pc;
-#endif
 }
 
 static CPUArchState *find_cpu(uint32_t thread_id)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 147c256..3701eb1 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -84,6 +84,7 @@  typedef struct CPUClass {
     bool (*get_paging_enabled)(const CPUState *cpu);
     void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
                                Error **errp);
+    void (*set_pc)(CPUState *cpu, uint64_t value);
 
     const struct VMStateDescription *vmsd;
     int (*write_elf64_note)(WriteCoreDumpFunction f, CPUState *cpu,
diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index 2670805..9679ac4 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -24,6 +24,13 @@ 
 #include "migration/vmstate.h"
 
 
+static void alpha_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    AlphaCPU *cpu = ALPHA_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 static void alpha_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     AlphaCPUClass *acc = ALPHA_CPU_GET_CLASS(dev);
@@ -264,6 +271,7 @@  static void alpha_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = alpha_cpu_do_interrupt;
     cc->dump_state = alpha_cpu_dump_state;
     cpu_class_set_do_unassigned_access(cc, alpha_cpu_unassigned_access);
+    cc->set_pc = alpha_cpu_set_pc;
     device_class_set_vmsd(dc, &vmstate_alpha_cpu);
 }
 
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index be26acc..281e252 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -25,6 +25,13 @@ 
 #endif
 #include "sysemu/sysemu.h"
 
+static void arm_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    cpu->env.regs[15] = value;
+}
+
 static void cp_reg_reset(gpointer key, gpointer value, gpointer opaque)
 {
     /* Reset a single ARMCPRegInfo register */
@@ -811,6 +818,7 @@  static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = arm_cpu_class_by_name;
     cc->do_interrupt = arm_cpu_do_interrupt;
     cc->dump_state = arm_cpu_dump_state;
+    cc->set_pc = arm_cpu_set_pc;
     cpu_class_set_vmsd(cc, &vmstate_arm_cpu);
 }
 
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 2abb57f..f7e8c2a 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -26,6 +26,13 @@ 
 #include "mmu.h"
 
 
+static void cris_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    CRISCPU *cpu = CRIS_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 /* CPUClass::reset() */
 static void cris_cpu_reset(CPUState *s)
 {
@@ -247,6 +254,7 @@  static void cris_cpu_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = cris_cpu_class_by_name;
     cc->do_interrupt = cris_cpu_do_interrupt;
     cc->dump_state = cris_cpu_dump_state;
+    cc->set_pc = cris_cpu_set_pc;
 }
 
 static const TypeInfo cris_cpu_type_info = {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e3f75a8..83584e6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2506,6 +2506,13 @@  static bool x86_cpu_get_paging_enabled(const CPUState *cs)
     return cpu->env.cr[0] & CR0_PG_MASK;
 }
 
+static void x86_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    X86CPU *cpu = X86_CPU(cs);
+
+    cpu->env.eip = value;
+}
+
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 {
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
@@ -2522,6 +2529,7 @@  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     cc->do_interrupt = x86_cpu_do_interrupt;
     cc->dump_state = x86_cpu_dump_state;
+    cc->set_pc = x86_cpu_set_pc;
     cc->get_arch_id = x86_cpu_get_arch_id;
     cc->get_paging_enabled = x86_cpu_get_paging_enabled;
 #ifndef CONFIG_USER_ONLY
diff --git a/target-lm32/cpu.c b/target-lm32/cpu.c
index 04327ac..5a0b809 100644
--- a/target-lm32/cpu.c
+++ b/target-lm32/cpu.c
@@ -22,6 +22,13 @@ 
 #include "qemu-common.h"
 
 
+static void lm32_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    LM32CPU *cpu = LM32_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 /* CPUClass::reset() */
 static void lm32_cpu_reset(CPUState *s)
 {
@@ -79,6 +86,7 @@  static void lm32_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->do_interrupt = lm32_cpu_do_interrupt;
     cc->dump_state = lm32_cpu_dump_state;
+    cc->set_pc = lm32_cpu_set_pc;
     cpu_class_set_vmsd(cc, &vmstate_lm32_cpu);
 }
 
diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
index dce1c7e..5fa6f63 100644
--- a/target-microblaze/cpu.c
+++ b/target-microblaze/cpu.c
@@ -26,6 +26,13 @@ 
 #include "migration/vmstate.h"
 
 
+static void mb_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
+
+    cpu->env.sregs[SR_PC] = value;
+}
+
 /* CPUClass::reset() */
 static void mb_cpu_reset(CPUState *s)
 {
@@ -134,6 +141,7 @@  static void mb_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = mb_cpu_do_interrupt;
     cc->dump_state = mb_cpu_dump_state;
     cpu_class_set_do_unassigned_access(cc, mb_cpu_unassigned_access);
+    cc->set_pc = mb_cpu_set_pc;
     dc->vmsd = &vmstate_mb_cpu;
     dc->props = mb_properties;
 }
diff --git a/target-mips/cpu.c b/target-mips/cpu.c
index 60a3faf..9e6b7d5 100644
--- a/target-mips/cpu.c
+++ b/target-mips/cpu.c
@@ -22,6 +22,19 @@ 
 #include "qemu-common.h"
 
 
+static void mips_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    MIPSCPU *cpu = MIPS_CPU(cs);
+    CPUMIPSState *env = &cpu->env;
+
+    env->active_tc.PC = value & ~(target_ulong)1;
+    if (value & 1) {
+        env->hflags |= MIPS_HFLAG_M16;
+    } else {
+        env->hflags &= ~(MIPS_HFLAG_M16);
+    }
+}
+
 /* CPUClass::reset() */
 static void mips_cpu_reset(CPUState *s)
 {
@@ -76,6 +89,7 @@  static void mips_cpu_class_init(ObjectClass *c, void *data)
     cc->do_interrupt = mips_cpu_do_interrupt;
     cc->dump_state = mips_cpu_dump_state;
     cpu_class_set_do_unassigned_access(cc, mips_cpu_unassigned_access);
+    cc->set_pc = mips_cpu_set_pc;
 }
 
 static const TypeInfo mips_cpu_type_info = {
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index 8bced98..aa79420 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -20,6 +20,13 @@ 
 #include "cpu.h"
 #include "qemu-common.h"
 
+static void openrisc_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    OpenRISCCPU *cpu = OPENRISC_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 /* CPUClass::reset() */
 static void openrisc_cpu_reset(CPUState *s)
 {
@@ -144,6 +151,7 @@  static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = openrisc_cpu_class_by_name;
     cc->do_interrupt = openrisc_cpu_do_interrupt;
     cc->dump_state = openrisc_cpu_dump_state;
+    cc->set_pc = openrisc_cpu_set_pc;
     device_class_set_vmsd(dc, &vmstate_openrisc_cpu);
 }
 
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 95b0b21..815ce0c 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8178,6 +8178,13 @@  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
+static void ppc_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+    cpu->env.nip = value;
+}
+
 /* CPUClass::reset() */
 static void ppc_cpu_reset(CPUState *s)
 {
@@ -8304,6 +8311,7 @@  static void ppc_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = ppc_cpu_do_interrupt;
     cc->dump_state = ppc_cpu_dump_state;
     cc->dump_statistics = ppc_cpu_dump_statistics;
+    cc->set_pc = ppc_cpu_set_pc;
 }
 
 static const TypeInfo ppc_cpu_type_info = {
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 1ef2fc0..6fd9bdd 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -58,6 +58,13 @@  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 }
 #endif
 
+static void s390_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    S390CPU *cpu = S390_CPU(cs);
+
+    cpu->env.psw.addr = value;
+}
+
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
 {
@@ -165,6 +172,7 @@  static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->do_interrupt = s390_cpu_do_interrupt;
     cc->dump_state = s390_cpu_dump_state;
+    cc->set_pc = s390_cpu_set_pc;
     dc->vmsd = &vmstate_s390_cpu;
 }
 
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index 03487eb..75fcf5c 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -24,6 +24,13 @@ 
 #include "migration/vmstate.h"
 
 
+static void superh_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    SuperHCPU *cpu = SUPERH_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 /* CPUClass::reset() */
 static void superh_cpu_reset(CPUState *s)
 {
@@ -269,6 +276,7 @@  static void superh_cpu_class_init(ObjectClass *oc, void *data)
     cc->class_by_name = superh_cpu_class_by_name;
     cc->do_interrupt = superh_cpu_do_interrupt;
     cc->dump_state = superh_cpu_dump_state;
+    cc->set_pc = superh_cpu_set_pc;
     dc->vmsd = &vmstate_sh_cpu;
 }
 
diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 87c3a50..0cb5c6b 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -723,6 +723,14 @@  void sparc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
     cpu_fprintf(f, "\n");
 }
 
+static void sparc_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    SPARCCPU *cpu = SPARC_CPU(cs);
+
+    cpu->env.pc = value;
+    cpu->env.npc = value + 4;
+}
+
 static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
@@ -767,6 +775,7 @@  static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = sparc_cpu_do_interrupt;
     cc->dump_state = sparc_cpu_dump_state;
     cpu_class_set_do_unassigned_access(cc, sparc_cpu_unassigned_access);
+    cc->set_pc = sparc_cpu_set_pc;
 }
 
 static const TypeInfo sparc_cpu_type_info = {
diff --git a/target-xtensa/cpu.c b/target-xtensa/cpu.c
index 0488984..a388c6f 100644
--- a/target-xtensa/cpu.c
+++ b/target-xtensa/cpu.c
@@ -33,6 +33,13 @@ 
 #include "migration/vmstate.h"
 
 
+static void xtensa_cpu_set_pc(CPUState *cs, uint64_t value)
+{
+    XtensaCPU *cpu = XTENSA_CPU(cs);
+
+    cpu->env.pc = value;
+}
+
 /* CPUClass::reset() */
 static void xtensa_cpu_reset(CPUState *s)
 {
@@ -100,6 +107,7 @@  static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
 
     cc->do_interrupt = xtensa_cpu_do_interrupt;
     cc->dump_state = xtensa_cpu_dump_state;
+    cc->set_pc = xtensa_cpu_set_pc;
     dc->vmsd = &vmstate_xtensa_cpu;
 }