qom-cpu: Wrap set_pc hook and use in bootloaders

PATCH Superseded Peter Crosthwaite 3 participants 9 messages Jun 15, 2015

Share & tools

Peter Crosthwaite

author

2015-06-15 03:48 UTC #0 lore raw

cover qom-cpu: Wrap set_pc hook and use in bootloaders

Wrap the CPUClass::set_pc fn hook in a caller helper to reduce verbosity of calls. Simplify the call from the gdbstub.

Then use the call to abstract away the PC env fields from the ARM and Microblaze bootloaders.

This moves towards the goal of minimising system level code of the CPU env (and one step closer to common-obj'ing the bootloaders). Theres a long way to go (at least for ARM, not so far for MB), but this is a small win in that direction.

This helps with multi-arch where the current thinking is to compile out the maximum content possible from cpu.h. This removes program counter definitions from the multi-arch cpu.h compile-in list.

Peter Crosthwaite (5):
qom: cpu: Add wrapper to the set-pc hook
gdbstub: Use cpu_set_pc helper
arm: Support thumb in set_pc routines
arm: boot: Use cpu_set_pc
microblaze: boot: Use cpu_set_pc

 gdbstub.c            |  5 +----
 hw/arm/boot.c        | 16 ++++------------
 hw/microblaze/boot.c |  2 +-
 include/qom/cpu.h    | 21 +++++++++++++++++++++
 target-arm/cpu.c     |  2 +-
 target-arm/cpu64.c   |  2 +-
 6 files changed, 29 insertions(+), 19 deletions(-)

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-15 03:48 UTC #1 lore raw

PATCH 1/5 qom: cpu: Add wrapper to the set-pc hook

Add a wrapper around the CPUClass::set_pc hook. Accepts an error pointer to report the case where the hook is not set.

Signed-off-by Peter Crosthwaite <[email protected]>

---
 include/qom/cpu.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

include/qom/cpu.h +21 -0

index 7db310e..97d4edf 100644
@@ -600,6 +600,27 @@ static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
 #endif
 
 /**
+ * cpu_set_pc:
+ * @cpu: The CPU to set the program counter for.
+ * @addr: Program counter value.
+ * @errp: Error pointer to populate in case of error.
+ *
+ * Set the program counter for a CPU. If there is no available implementation
+ * an error is raised.
+ */
+
+static inline void cpu_set_pc(CPUState *cpu, vaddr addr, Error **errp)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->set_pc) {
+        cc->set_pc(cpu, addr);
+    } else {
+        error_setg(errp, "CPU does not implement set PC");
+    }
+}
+
+/**
  * cpu_reset_interrupt:
  * @cpu: The CPU to clear the interrupt on.
  * @mask: The interrupt mask to clear.

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-15 03:48 UTC #2 lore raw

PATCH 2/5 gdbstub: Use cpu_set_pc helper

In reply to Peter Crosthwaite

Use the cpu_set_pc helper which will take care of CPUClass retrieval for us.

Signed-off-by Peter Crosthwaite <[email protected]>

---
 gdbstub.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

gdbstub.c +1 -4

index 75563db..ceb60ac 100644
@@ -754,12 +754,9 @@ static void gdb_breakpoint_remove_all(void)
 static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
 {
     CPUState *cpu = s->c_cpu;
-    CPUClass *cc = CPU_GET_CLASS(cpu);
 
     cpu_synchronize_state(cpu);
-    if (cc->set_pc) {
-        cc->set_pc(cpu, pc);
-    }
+    cpu_set_pc(cpu, pc, NULL);
 }
 
 static CPUState *find_cpu(uint32_t thread_id)

Signature · 1 line

1.9.1

Peter Crosthwaite

author

2015-06-15 03:48 UTC #3 lore raw

PATCH 3/5 arm: Support thumb in set_pc routines

In reply to Peter Crosthwaite

ARM program counters are always at least 16b aligned with the LSB being only used the indicate thumb mode in exchange situations. Mask this bit off in set_pc to ignore the exchange semantic (which must still be managed by the caller).

Signed-off-by Peter Crosthwaite <[email protected]>

---

---
 target-arm/cpu.c   | 2 +-
 target-arm/cpu64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

target-arm/cpu.c +1 -1

index 6181f28..5bb08a6 100644
@@ -35,7 +35,7 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
     ARMCPU *cpu = ARM_CPU(cs);
 
-    cpu->env.regs[15] = value;
+    cpu->env.regs[15] = value & 0xfffffffe;
 }
 
 static bool arm_cpu_has_work(CPUState *cs)

target-arm/cpu64.c +1 -1

index bf7dd68..1e26a48 100644
@@ -279,7 +279,7 @@ static void aarch64_cpu_set_pc(CPUState *cs, vaddr value)
     if (is_a64(&cpu->env)) {
         cpu->env.pc = value;
     } else {
-        cpu->env.regs[15] = value;
+        cpu->env.regs[15] = value & 0xfffffffe;
     }
 }

Signature · 1 line

1.9.1

Peter Maydell

2015-06-15 07:37 UTC #4 lore raw

Re: [Qemu-devel] [PATCH 3/5] arm: Support thumb in set_pc routines

Quoted 17 lines · Peter Crosthwaite

ARM program counters are always at least 16b aligned with the LSB being only used the indicate thumb mode in exchange situations. Mask this bit off in set_pc to ignore the exchange semantic (which must still be managed by the caller).
Signed-off-by Peter Crosthwaite <[email protected]>
---
---
 target-arm/cpu.c   | 2 +-
 target-arm/cpu64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
target-arm/cpu.c +0 -0
index 6181f28..5bb08a6 100644

target-arm/cpu.c static void arm_cpu_set_pc(CPUState *cs, vaddr value) · line 39

Quoted 6 lines

target-arm/cpu.c +1 -1

@@ -35,7 +35,7 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
     ARMCPU *cpu = ARM_CPU(cs);
 
-    cpu->env.regs[15] = value;
+    cpu->env.regs[15] = value & 0xfffffffe;
 }

This doesn't look right to me. There are two semantics that make sense for setting an ARM PC value:

(1) interworking-aware, where we set the Thumb bit from the
LS bit and r15 from everything else
(2) interworking-unaware, where we just set r15 (and it's
the caller's job to not pass us a misaligned value)

This seems to be an odd mix of both.

-- PMM

Peter Crosthwaite

2015-06-15 22:41 UTC #5 lore raw

Re: [Qemu-devel] [PATCH 3/5] arm: Support thumb in set_pc routines

Quoted 29 lines · Peter Maydell

Quoted 17 lines · Peter Crosthwaite
ARM program counters are always at least 16b aligned with the LSB being only used the indicate thumb mode in exchange situations. Mask this bit off in set_pc to ignore the exchange semantic (which must still be managed by the caller).
Signed-off-by Peter Crosthwaite <[email protected]>
---
---
 target-arm/cpu.c   | 2 +-
 target-arm/cpu64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
target-arm/cpu.c +0 -0
index 6181f28..5bb08a6 100644
target-arm/cpu.c static void arm_cpu_set_pc(CPUState *cs, vaddr value) · line 39
Quoted 6 lines
target-arm/cpu.c +1 -1
@@ -35,7 +35,7 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 {
     ARMCPU *cpu = ARM_CPU(cs);
 
-    cpu->env.regs[15] = value;
+    cpu->env.regs[15] = value & 0xfffffffe;
 }
This doesn't look right to me. There are two semantics that make sense for setting an ARM PC value:

Quoted 5 lines

(1) interworking-aware, where we set the Thumb bit from the
LS bit and r15 from everything else
(2) interworking-unaware, where we just set r15 (and it's
the caller's job to not pass us a misaligned value)

Actually I am just going to leave as-is and mask off in the caller. What I am really trying to do is remove usage of r[15] from boot.c and I can do that using strategy (2) still.

Regards,
Peter

Quoted 4 lines

This seems to be an odd mix of both.
-- PMM

Peter Crosthwaite

author

2015-06-15 03:48 UTC #6 lore raw

PATCH 4/5 arm: boot: Use cpu_set_pc

In reply to Peter Crosthwaite

Use cpu_set_pc across the board for setting program counters. This removes instances of system level code having to reach into the CPU env.

Signed-off-by Peter Crosthwaite <[email protected]>

---
 hw/arm/boot.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

hw/arm/boot.c +4 ~~-12~~

index d036624..324ba6d 100644
@@ -168,11 +168,9 @@ static void default_write_secondary(ARMCPU *cpu,
 static void default_reset_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
-    CPUARMState *env = &cpu->env;
-
     address_space_stl_notdirty(&address_space_memory, info->smp_bootreg_addr,
                                0, MEMTXATTRS_UNSPECIFIED, NULL);
-    env->regs[15] = info->smp_loader_start;
+    cpu_set_pc(CPU(cpu), info->smp_loader_start, &error_abort);
 }
 
 static inline bool have_dtb(const struct arm_boot_info *info)

@@ -452,12 +450,10 @@ static void do_cpu_reset(void *opaque)
     if (info) {
         if (!info->is_linux) {
             /* Jump to the entry point.  */
-            if (env->aarch64) {
-                env->pc = info->entry;
-            } else {
-                env->regs[15] = info->entry & 0xfffffffe;
+            if (!env->aarch64) {
                 env->thumb = info->entry & 1;
             }
+            cpu_set_pc(CPU(cpu), info->entry, &error_abort);
         } else {
             /* If we are booting Linux then we need to check whether we are
              * booting into secure or non-secure state and adjust the state

@@ -488,11 +484,7 @@ static void do_cpu_reset(void *opaque)
             }
 
             if (CPU(cpu) == first_cpu) {
-                if (env->aarch64) {
-                    env->pc = info->loader_start;
-                } else {
-                    env->regs[15] = info->loader_start;
-                }
+                cpu_set_pc(CPU(cpu), info->loader_start, &error_abort);
 
                 if (!have_dtb(info)) {
                     if (old_param) {

Signature · 1 line

1.9.1

Peter Maydell

2015-06-15 07:38 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 4/5] arm: boot: Use cpu_set_pc

Quoted 28 lines · Peter Crosthwaite

Use cpu_set_pc across the board for setting program counters. This removes instances of system level code having to reach into the CPU env.

Signed-off-by Peter Crosthwaite <[email protected]>

---
 hw/arm/boot.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

hw/arm/boot.c +1 -3

index d036624..324ba6d 100644
@@ -168,11 +168,9 @@ static void default_write_secondary(ARMCPU *cpu,
 static void default_reset_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
-    CPUARMState *env = &cpu->env;
-
     address_space_stl_notdirty(&address_space_memory, info->smp_bootreg_addr,
                                0, MEMTXATTRS_UNSPECIFIED, NULL);
-    env->regs[15] = info->smp_loader_start;
+    cpu_set_pc(CPU(cpu), info->smp_loader_start, &error_abort);
 }
 
 static inline bool have_dtb(const struct arm_boot_info *info)

@@ -452,12 +450,10 @@ static void do_cpu_reset(void *opaque)
     if (info) {

hw/arm/boot.c static void do_cpu_reset(void *opaque) · line 456

Quoted 10 lines

hw/arm/boot.c +2 -4

         if (!info->is_linux) {
             /* Jump to the entry point.  */
-            if (env->aarch64) {
-                env->pc = info->entry;
-            } else {
-                env->regs[15] = info->entry & 0xfffffffe;
+            if (!env->aarch64) {
                 env->thumb = info->entry & 1;
             }
+            cpu_set_pc(CPU(cpu), info->entry, &error_abort);

Code like this is really wanting to call a "set PC and Thumb" function, ie an interworking-aware set-pc.

-- PMM

Peter Crosthwaite

author

2015-06-15 03:48 UTC #8 lore raw

PATCH 5/5 microblaze: boot: Use cpu_set_pc

In reply to Peter Crosthwaite

Use cpu_set_pc for setting program counters when bootloading. This removes an instance of system level code having to reach into the CPU env.

Signed-off-by Peter Crosthwaite <[email protected]>

---
 hw/microblaze/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/microblaze/boot.c +1 -1

index 4c44317..ec68479 100644
@@ -54,7 +54,7 @@ static void main_cpu_reset(void *opaque)
     env->regs[5] = boot_info.cmdline;
     env->regs[6] = boot_info.initrd_start;
     env->regs[7] = boot_info.fdt;
-    env->sregs[SR_PC] = boot_info.bootstrap_pc;
+    cpu_set_pc(CPU(cpu), boot_info.bootstrap_pc, &error_abort);
     if (boot_info.machine_cpu_reset) {
         boot_info.machine_cpu_reset(cpu);
     }

Signature · 1 line

1.9.1