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

[Qemu-devel] [PATCH qom v3 0/4] 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). 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(-)

-- 
1.9.1

[Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Peter Crosthwaite]

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

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
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(+)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 39f0f19..5fb1f60 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -600,6 +600,23 @@ 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.
+ *
+ * Set the program counter for a CPU. If there is no available implementation
+ * no action occurs.
+ */
+static inline void cpu_set_pc(CPUState *cpu, vaddr addr)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->set_pc) {
+        cc->set_pc(cpu, addr);
+    }
+}
+
+/**
  * cpu_reset_interrupt:
  * @cpu: The CPU to clear the interrupt on.
  * @mask: The interrupt mask to clear.
-- 
1.9.1

[Qemu-devel] [PATCH qom v3 2/4] 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>
---
Changed since v2:
Add () to fn names in commit message
Drop error argument
---
 gdbstub.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index cea2a84..3ccf31e 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);
 }
 
 static CPUState *find_cpu(uint32_t thread_id)
-- 
1.9.1

[Qemu-devel] [PATCH qom v3 3/4] 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.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
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(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1e7fd28..c9d7f7d 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);
 }
 
 static inline bool have_dtb(const struct arm_boot_info *info)
@@ -452,12 +450,13 @@ 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;
+            uint64_t entry = info->entry;
+
+            if (!env->aarch64) {
                 env->thumb = info->entry & 1;
+                entry &= 0xfffffffe;
             }
+            cpu_set_pc(CPU(cpu), 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
@@ -488,11 +487,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);
 
                 if (!have_dtb(info)) {
                     if (old_param) {
-- 
1.9.1

[Qemu-devel] [PATCH qom v3 4/4] 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.

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
changed since v2:
Add () to function names in commit messages
---
 dtc                  | 2 +-
 hw/microblaze/boot.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/dtc b/dtc
index 65cc4d2..bc895d6 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
+Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 4c44317..9f4698a 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);
     if (boot_info.machine_cpu_reset) {
         boot_info.machine_cpu_reset(cpu);
     }
-- 
1.9.1

Re: [Qemu-devel] [PATCH qom v3 4/4] microblaze: boot: Use cpu_set_pc()

[Andreas Färber]

Am 24.06.2015 um 05:19 schrieb 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.
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> changed since v2:
> Add () to function names in commit messages
> ---
>  dtc                  | 2 +-
>  hw/microblaze/boot.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/dtc b/dtc
> index 65cc4d2..bc895d6 160000
> --- a/dtc
> +++ b/dtc
> @@ -1 +1 @@
> -Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
> +Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde

Submodule strikes again. Preparing to queue.

Andreas

> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 4c44317..9f4698a 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);
>      if (boot_info.machine_cpu_reset) {
>          boot_info.machine_cpu_reset(cpu);
>      }
> 


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

Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Andreas Färber]

s/set-pc/set_pc/

Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:
> Add a wrapper around the CPUClass::set_pc() hook.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 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:

--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -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);

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

 /**

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)

Re: [Qemu-devel] [PATCH qom v3 3/4] arm: boot: Use cpu_set_pc()

[Andreas Färber]

Am 24.06.2015 um 05:19 schrieb 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.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 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(-)

Any objection to avoiding repeated casts as follows?

--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -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)
@@ -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)

Re: [Qemu-devel] [PATCH qom v3 3/4] arm: boot: Use cpu_set_pc()

[Peter Crosthwaite]

On Wed, Jun 24, 2015 at 11:22 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 05:19 schrieb 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.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> 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(-)
>
> Any objection to avoiding repeated casts as follows?

No objection from me. Ack to the squash or can be respun.

Regards,
Peter

>
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -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)
> @@ -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)
>

Re: [Qemu-devel] [PATCH qom v3 4/4] microblaze: boot: Use cpu_set_pc()

[Andreas Färber]

Am 24.06.2015 um 20:00 schrieb Andreas Färber:
> Am 24.06.2015 um 05:19 schrieb 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.
>>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> changed since v2:
>> Add () to function names in commit messages
>> ---
>>  dtc                  | 2 +-
>>  hw/microblaze/boot.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/dtc b/dtc
>> index 65cc4d2..bc895d6 160000
>> --- a/dtc
>> +++ b/dtc
>> @@ -1 +1 @@
>> -Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
>> +Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde
> 
> Submodule strikes again. Preparing to queue.

Will squash the following deduplication:

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 9f4698a..3e8820f 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -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)

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

[Andreas Färber]

Am 24.06.2015 um 05:19 schrieb 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). 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()

Thanks, queued with mentioned code modifications on qom-cpu-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next

(This is a purely intermediate staging for the lesser reviewed patches
and does not indicate they will miss my belated 2.4 pull.)

Regards,
Andreas

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

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

Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Peter Maydell]

On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> wrote:
> s/set-pc/set_pc/
>
> Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:
>> Add a wrapper around the CPUClass::set_pc() hook.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> 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:
>
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -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);
>
> -    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...

-- PMM

Re: [Qemu-devel] [PATCH qom v3 3/4] arm: boot: Use cpu_set_pc()

[Peter Maydell]

On 24 June 2015 at 19:22, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 05:19 schrieb 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.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> 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(-)
>
> Any objection to avoiding repeated casts as follows?

Fine with me; you can keep my r-b tag.

-- PMM

Re: [Qemu-devel] [PATCH qom v3 4/4] microblaze: boot: Use cpu_set_pc()

[Peter Crosthwaite]

On Wed, Jun 24, 2015 at 11:29 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 20:00 schrieb Andreas Färber:
>> Am 24.06.2015 um 05:19 schrieb 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.
>>>
>>> Reviewed-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>> changed since v2:
>>> Add () to function names in commit messages
>>> ---
>>>  dtc                  | 2 +-
>>>  hw/microblaze/boot.c | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/dtc b/dtc
>>> index 65cc4d2..bc895d6 160000
>>> --- a/dtc
>>> +++ b/dtc
>>> @@ -1 +1 @@
>>> -Subproject commit 65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf
>>> +Subproject commit bc895d6d09695d05ceb8b52486ffe861d6cfbdde
>>
>> Submodule strikes again. Preparing to queue.
>
> Will squash the following deduplication:
>

ACK.

> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 9f4698a..3e8820f 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -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)
>

Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Andreas Färber]

Am 24.06.2015 um 21:11 schrieb Peter Maydell:
> On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> wrote:
>> s/set-pc/set_pc/
>>
>> Am 24.06.2015 um 05:19 schrieb Peter Crosthwaite:
>>> Add a wrapper around the CPUClass::set_pc() hook.
>>>
>>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> ---
>>> 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:
>>
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -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);
>>
>> -    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...

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.

We don't seem to have a clear line of when or whether to add such
assertions, so I can certainly drop the assertion line again.

Right now we have the qom-test iterating over (nearly) all machines, but
I'm not aware of all CPU models of all targets being covered by QTest
yet that would give us some more certainty.

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)

Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Peter Maydell]

On 25 June 2015 at 12:12, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 21:11 schrieb Peter Maydell:
>> On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> wrote:
>>> +    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.

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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH qom v3 1/4] cpu: Add wrapper to the set-pc() hook

[Andreas Färber]

Am 25.06.2015 um 13:21 schrieb Peter Maydell:
> On 25 June 2015 at 12:12, Andreas Färber <afaerber@suse.de> wrote:
>> Am 24.06.2015 um 21:11 schrieb Peter Maydell:
>>> On 24 June 2015 at 19:09, Andreas Färber <afaerber@suse.de> wrote:
>>>> +    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.
> 
> 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.

Okay, dropping the assertion:

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index e7ee6c6..5db1ea3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -610,7 +610,6 @@ static inline void cpu_set_pc(CPUState *cpu, vaddr addr)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);

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

Cheers,
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)