qom-cpu: Wrap set_pc hook and use in bootloaders

Add a wrapper around the CPUClass::set_pc() hook.

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

---
changed since v2:
drop "qom" from commit message subject.
Add () to functions in commit messages.
Drop error argument

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

s/set-pc/set_pc/

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 11 lines

Add a wrapper around the CPUClass::set_pc() hook.

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

---
changed since v2:
drop "qom" from commit message subject.
Add () to functions in commit messages.
Drop error argument

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

Queuing on qom-cpu-next with the following change:

include/qom/cpu.h +0 -1

@@ -604,16 +604,14 @@ static inline void cpu_unaligned_access(CPUState
 *cpu, vaddr addr,
  * @cpu: The CPU to set the program counter for.
  * @addr: Program counter value.
  *
- * Set the program counter for a CPU. If there is no available

implementation
- * no action occurs.
+ * Sets the program counter for a CPU.
*/
static inline void cpu_set_pc(CPUState *cpu, vaddr addr)

 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

Quoted 33 lines

s/set-pc/set_pc/

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 11 lines

Add a wrapper around the CPUClass::set_pc() hook.

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

---
changed since v2:
drop "qom" from commit message subject.
Add () to functions in commit messages.
Drop error argument

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

Queuing on qom-cpu-next with the following change:

include/qom/cpu.h +0 -1

@@ -604,16 +604,14 @@ static inline void cpu_unaligned_access(CPUState
 *cpu, vaddr addr,
  * @cpu: The CPU to set the program counter for.
  * @addr: Program counter value.
  *
- * Set the program counter for a CPU. If there is no available

implementation
- * no action occurs.
+ * Sets the program counter for a CPU.
*/
static inline void cpu_set_pc(CPUState *cpu, vaddr addr)

 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

include/qom/cpu.h

Quoted 6 lines

include/qom/cpu.h +2 -3

-    if (cc->set_pc) {
-        cc->set_pc(cpu, addr);
-    }
+    g_assert(cc->set_pc != NULL);
+    cc->set_pc(cpu, addr);
 }

Do we need this assert? If it would have fired then we'll just crash immediately calling the null pointer, so it's not like it's guarding against a more subtle failure at a later point...

Am 24.06.2015 um 21:11 schrieb Peter Maydell:

Quoted 9 lines · Andreas Färber

Review

Quoted 3 lines

(quoted diff) +2 -0

+    g_assert(cc->set_pc != NULL);
+    cc->set_pc(cpu, addr);
 }

Do we need this assert? If it would have fired then we'll just crash immediately calling the null pointer, so it's not like it's guarding against a more subtle failure at a later point...

There seemed uncertainty whether all corner cases of the 17 targets implement set_pc for all subclasses. By my reading, g_assert() calls g_assertion_message_expr(), which is marked G_GNUC_NORETURN - and I assume it to abort after printing the message, raising a signal and either exiting the process or falling back to an attached debugger.

It may be unnecessary, but I don't see it calling the null pointer here.

Quoted 11 lines

Am 24.06.2015 um 21:11 schrieb Peter Maydell:

Quoted 9 lines · Andreas Färber

Review

Quoted 3 lines

(quoted diff) +2 -0

+    g_assert(cc->set_pc != NULL);
+    cc->set_pc(cpu, addr);
 }

Do we need this assert? If it would have fired then we'll just crash immediately calling the null pointer, so it's not like it's guarding against a more subtle failure at a later point...

Quoted 7 lines

There seemed uncertainty whether all corner cases of the 17 targets implement set_pc for all subclasses. By my reading, g_assert() calls g_assertion_message_expr(), which is marked G_GNUC_NORETURN - and I assume it to abort after printing the message, raising a signal and either exiting the process or falling back to an attached debugger.

It may be unnecessary, but I don't see it calling the null pointer here.

What I mean is:
* with the assert, QEMU will die in this function if cc->set_pc is NULL, in an easily debuggable way
* without the assert, QEMU will still die in this function if cc->set_pc is NULL, in an easily debuggable way

So the assert doesn't hurt, but it doesn't really gain anything IMHO. Assertions are most useful when they turn something that would be a really confusing failure much later in execution into an easily debuggable crash earlier on, I think.

Still, this is a very minor thing, so it's a personal taste/style question, as you say. You can leave the assert in or remove it, whichever you prefer.

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.

Reviewed-by Peter Maydell <[email protected]>

Reviewed-by Andreas Färber <[email protected]>

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

---
Changed since v2:
Add () to fn names in commit msg
Drop error argument
Changed since v1:
Lease thumb masking in boot.c

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

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 13 lines

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.

Reviewed-by Peter Maydell <[email protected]>

Reviewed-by Andreas Färber <[email protected]>

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

---
Changed since v2:
Add () to fn names in commit msg
Drop error argument
Changed since v1:
Lease thumb masking in boot.c

Quoted 3 lines

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

Any objection to avoiding repeated casts as follows?

hw/arm/boot.c +3 -1

@@ -168,9 +168,11 @@ static void default_write_secondary(ARMCPU *cpu,
 static void default_reset_secondary(ARMCPU *cpu,
                                     const struct arm_boot_info *info)
 {
+    CPUState *cs = CPU(cpu);
+
     address_space_stl_notdirty(&address_space_memory,
 info->smp_bootreg_addr,
                                0, MEMTXATTRS_UNSPECIFIED, NULL);
-    cpu_set_pc(CPU(cpu), info->smp_loader_start);
+    cpu_set_pc(cs, info->smp_loader_start);
 }
 

static inline bool have_dtb(const struct arm_boot_info *info)

hw/arm/boot.c +5 -4

@@ -443,10 +445,11 @@ fail:
 static void do_cpu_reset(void *opaque)
 {
     ARMCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
     CPUARMState *env = &cpu->env;
     const struct arm_boot_info *info = env->boot_info;
 
-    cpu_reset(CPU(cpu));
+    cpu_reset(cs);
     if (info) {
         if (!info->is_linux) {
             /* Jump to the entry point.  */

@@ -456,7 +459,7 @@ static void do_cpu_reset(void *opaque)
                 env->thumb = info->entry & 1;
                 entry &= 0xfffffffe;
             }
-            cpu_set_pc(CPU(cpu), entry);
+            cpu_set_pc(cs, entry);
         } 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

@@ -486,8 +489,8 @@ static void do_cpu_reset(void *opaque)
                 }
             }
 
-            if (CPU(cpu) == first_cpu) {
-                cpu_set_pc(CPU(cpu), info->loader_start);
+            if (cs == first_cpu) {
+                cpu_set_pc(cs, info->loader_start);
 
                 if (!have_dtb(info)) {
                     if (old_param) {

Regards,
Andreas

--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 13 lines

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.

Reviewed-by Peter Maydell <[email protected]>

Reviewed-by Andreas Färber <[email protected]>

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

---
Changed since v2:
Add () to fn names in commit msg
Drop error argument
Changed since v1:
Lease thumb masking in boot.c

Quoted 3 lines

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

Any objection to avoiding repeated casts as follows?

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.

Reviewed-by Andreas Färber <[email protected]>

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

---
changed since v2:
Add () to function names in commit messages

---
 dtc                  | 2 +-
 hw/microblaze/boot.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

dtc +0 -0

index 65cc4d2..bc895d6 160000

hw/microblaze/boot.c +1 -1

index 4c44317..9f4698a 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);
     if (boot_info.machine_cpu_reset) {
         boot_info.machine_cpu_reset(cpu);
     }

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 14 lines

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.

Reviewed-by Andreas Färber <[email protected]>

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

---
changed since v2:
Add () to function names in commit messages

---
 dtc                  | 2 +-
 hw/microblaze/boot.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

dtc line 1

Quoted 7 lines

dtc +1 -1

index 65cc4d2..bc895d6 160000
@@ -1 +1 @@
-Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
+Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde

Submodule strikes again. Preparing to queue.

Am 24.06.2015 um 20:00 schrieb Andreas Färber:

Quoted 15 lines

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:

Quoted 14 lines

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.

Reviewed-by Andreas Färber <[email protected]>

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

---
changed since v2:
Add () to function names in commit messages

---
 dtc                  | 2 +-
 hw/microblaze/boot.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Quoted 9 lines

dtc line 1

Quoted 7 lines

dtc +1 -1

index 65cc4d2..bc895d6 160000
@@ -1 +1 @@
-Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
+Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde

Submodule strikes again. Preparing to queue.

Will squash the following deduplication:

hw/microblaze/boot.c +3 -2

index 9f4698a..3e8820f 100644
@@ -48,13 +48,14 @@ static struct
 static void main_cpu_reset(void *opaque)
 {
     MicroBlazeCPU *cpu = opaque;
+    CPUState *cs = CPU(cpu);
     CPUMBState *env = &cpu->env;
 
-    cpu_reset(CPU(cpu));
+    cpu_reset(cs);
     env->regs[5] = boot_info.cmdline;
     env->regs[6] = boot_info.initrd_start;
     env->regs[7] = boot_info.fdt;
-    cpu_set_pc(CPU(cpu), boot_info.bootstrap_pc);
+    cpu_set_pc(cs, boot_info.bootstrap_pc);
     if (boot_info.machine_cpu_reset) {
         boot_info.machine_cpu_reset(cpu);
     }

Regards,
Andreas

--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)

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). There's 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.

changed since v2:
drop error argument
misc commit messages tweaks

changed since v1:
Remove thumb changes

Peter Crosthwaite (4):
cpu: Add wrapper to the set-pc() hook
gdbstub: Use cpu_set_pc() helper
arm: boot: Use cpu_set_pc()
microblaze: boot: Use cpu_set_pc()

 dtc                  |  2 +-
 gdbstub.c            |  5 +----
 hw/arm/boot.c        | 19 +++++++------------
 hw/microblaze/boot.c |  2 +-
 include/qom/cpu.h    | 17 +++++++++++++++++
 5 files changed, 27 insertions(+), 18 deletions(-)