[Qemu-devel] [PATCH 0/5] qom-cpu: Wrap set_pc hook and use in bootloaders

[Qemu-devel] [PATCH 0/5] qom-cpu: Wrap set_pc hook and use in bootloaders

[Peter Crosthwaite]

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

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] qom: cpu: Add wrapper to the set-pc hook

[Peter Crosthwaite]

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 <crosthwaite.peter@gmail.com>
---
 include/qom/cpu.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7db310e..97d4edf 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -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.
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] gdbstub: Use cpu_set_pc helper

[Peter Crosthwaite]

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

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
 gdbstub.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 75563db..ceb60ac 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -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)
-- 
1.9.1

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

[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 <crosthwaite.peter@gmail.com>
---
---
 target-arm/cpu.c   | 2 +-
 target-arm/cpu64.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 6181f28..5bb08a6 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -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)
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index bf7dd68..1e26a48 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -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;
     }
 }
 
-- 
1.9.1

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

[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 <crosthwaite.peter@gmail.com>
---
 hw/arm/boot.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d036624..324ba6d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -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) {
-- 
1.9.1

[Qemu-devel] [PATCH 5/5] microblaze: boot: Use cpu_set_pc

[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 <crosthwaite.peter@gmail.com>
---
 hw/microblaze/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 4c44317..ec68479 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -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);
     }
-- 
1.9.1

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

[Peter Maydell]

On 15 June 2015 at 04:48, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> 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 <crosthwaite.peter@gmail.com>
> ---
> ---
>  target-arm/cpu.c   | 2 +-
>  target-arm/cpu64.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 6181f28..5bb08a6 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -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

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

[Peter Maydell]

On 15 June 2015 at 04:48, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> 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 <crosthwaite.peter@gmail.com>
> ---
>  hw/arm/boot.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d036624..324ba6d 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -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);

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

-- PMM

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

[Peter Crosthwaite]

On Mon, Jun 15, 2015 at 12:36 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 15 June 2015 at 04:48, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> 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 <crosthwaite.peter@gmail.com>
>> ---
>> ---
>>  target-arm/cpu.c   | 2 +-
>>  target-arm/cpu64.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 6181f28..5bb08a6 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -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)
>

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

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