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

[Qemu-devel] [PATCH v2 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 v1:
Remove thumb changes

Peter Crosthwaite (4):
  qom: 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

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

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/4] 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

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

[Peter Maydell]

On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> 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");
> +    }
> +}

I don't think there are any CPUClass implementations which
don't implement set_pc. The code in cpu-exec.c which does this:
        if (cc->synchronize_from_tb) {
            cc->synchronize_from_tb(cpu, tb);
        } else {
            assert(cc->set_pc);
            cc->set_pc(cpu, tb->pc);
        }
demonstrates that we only need to check the CPUs which implement
synchronize_from_tb (i386, mips, sh4, sparc, tricore) to check
they have a set_pc method too, and they all do. (You can also
just grep for 'cc->set_pc' in target-*/ and confirm they all
have one, as a crosscheck.)

So I think this can simplify down to just calling the class
method unconditionally.

thanks
-- PMM

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

[Andreas Färber]

Hi,

Just "cpu: " please, compare git-log.

Am 16.06.2015 um 07:46 schrieb 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.
> + */
> +

Please drop the white line here for consistency.

> +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.

Otherwise, modulo Peter M.'s comment, looks okay.

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)

[Qemu-devel] [PATCH v2 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>
---
 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

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper

[Andreas Färber]

Am 16.06.2015 um 07:46 schrieb 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);

I believe this argument will probably go away; otherwise this should've
been &error_abort or something instead of NULL.

Regards,
Andreas

>  }
>  
>  static CPUState *find_cpu(uint32_t thread_id)
> 


-- 
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 v2 2/4] gdbstub: Use cpu_set_pc helper

[Peter Crosthwaite]

On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 16.06.2015 um 07:46 schrieb 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);
>
> I believe this argument will probably go away; otherwise this should've
> been &error_abort or something instead of NULL.
>

I'm not sure. As I don't see what is catching the case of a gdb 'c'
packet for a CPU that doesn't implement set_pc. I'd rather preserve
the existing behaviour, and have the qom wrapper do nothing if it is
not implemented.

Regards,
Peter

> Regards,
> Andreas
>
>>  }
>>
>>  static CPUState *find_cpu(uint32_t thread_id)
>>
>
>
> --
> 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 v2 2/4] gdbstub: Use cpu_set_pc helper

[Peter Maydell]

On 24 June 2015 at 03:50, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>> I believe this argument will probably go away; otherwise this should've
>> been &error_abort or something instead of NULL.
>>
>
> I'm not sure. As I don't see what is catching the case of a gdb 'c'
> packet for a CPU that doesn't implement set_pc. I'd rather preserve
> the existing behaviour, and have the qom wrapper do nothing if it is
> not implemented.

Well, this is one reason why every CPU needs to implement set_pc...

-- PMM

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper

[Peter Crosthwaite]

On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 June 2015 at 03:50, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>> I believe this argument will probably go away; otherwise this should've
>>> been &error_abort or something instead of NULL.
>>>
>>
>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>> the existing behaviour, and have the qom wrapper do nothing if it is
>> not implemented.
>
> Well, this is one reason why every CPU needs to implement set_pc...
>

Well. I guess it works for a common case where a continue doesn't
change the PC? If the debugger doesn't change the PC the "c" should
work even without a set_pc call so we don't want to assert on this
valid use case.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH v2 2/4] gdbstub: Use cpu_set_pc helper

[Andreas Färber]

Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite:
> On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 24 June 2015 at 03:50, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>>> I believe this argument will probably go away; otherwise this should've
>>>> been &error_abort or something instead of NULL.
>>>>
>>>
>>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>>> the existing behaviour, and have the qom wrapper do nothing if it is
>>> not implemented.
>>
>> Well, this is one reason why every CPU needs to implement set_pc...
>>
> 
> Well. I guess it works for a common case where a continue doesn't
> change the PC? If the debugger doesn't change the PC the "c" should
> work even without a set_pc call so we don't want to assert on this
> valid use case.

Guys, is there any target that does not implement set_pc today? If so,
which? I'd rather implement it than carry around the iffery and
resulting complications.

I quickly counted 17 target-* in my tree and all 17 seemed to show up in
git-grep. Can you confirm? Didn't check the latest tilegx series.

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 v2 2/4] gdbstub: Use cpu_set_pc helper

[Peter Crosthwaite]

On Wed, Jun 24, 2015 at 10:16 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 24.06.2015 um 19:04 schrieb Peter Crosthwaite:
>> On Wed, Jun 24, 2015 at 3:01 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 24 June 2015 at 03:50, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> On Mon, Jun 22, 2015 at 10:31 AM, Andreas Färber <afaerber@suse.de> wrote:
>>>>> I believe this argument will probably go away; otherwise this should've
>>>>> been &error_abort or something instead of NULL.
>>>>>
>>>>
>>>> I'm not sure. As I don't see what is catching the case of a gdb 'c'
>>>> packet for a CPU that doesn't implement set_pc. I'd rather preserve
>>>> the existing behaviour, and have the qom wrapper do nothing if it is
>>>> not implemented.
>>>
>>> Well, this is one reason why every CPU needs to implement set_pc...
>>>
>>
>> Well. I guess it works for a common case where a continue doesn't
>> change the PC? If the debugger doesn't change the PC the "c" should
>> work even without a set_pc call so we don't want to assert on this
>> valid use case.
>
> Guys, is there any target that does not implement set_pc today? If so,
> which? I'd rather implement it than carry around the iffery and
> resulting complications.
>
> I quickly counted 17 target-* in my tree and all 17 seemed to show up in
> git-grep. Can you confirm? Didn't check the latest tilegx series.
>

There are 17 targets and 18 sets by my count:
$ git grep "set_pc.*=.*set_pc" target-*
target-alpha/cpu.c:    cc->set_pc = alpha_cpu_set_pc;
target-arm/cpu.c:    cc->set_pc = arm_cpu_set_pc;
target-arm/cpu64.c:    cc->set_pc = aarch64_cpu_set_pc;
target-cris/cpu.c:    cc->set_pc = cris_cpu_set_pc;
target-i386/cpu.c:    cc->set_pc = x86_cpu_set_pc;
target-lm32/cpu.c:    cc->set_pc = lm32_cpu_set_pc;
target-m68k/cpu.c:    cc->set_pc = m68k_cpu_set_pc;
target-microblaze/cpu.c:    cc->set_pc = mb_cpu_set_pc;
target-mips/cpu.c:    cc->set_pc = mips_cpu_set_pc;
target-moxie/cpu.c:    cc->set_pc = moxie_cpu_set_pc;
target-openrisc/cpu.c:    cc->set_pc = openrisc_cpu_set_pc;
target-ppc/translate_init.c:    cc->set_pc = ppc_cpu_set_pc;
target-s390x/cpu.c:    cc->set_pc = s390_cpu_set_pc;
target-sh4/cpu.c:    cc->set_pc = superh_cpu_set_pc;
target-sparc/cpu.c:    cc->set_pc = sparc_cpu_set_pc;
target-tricore/cpu.c:    cc->set_pc = tricore_cpu_set_pc;
target-unicore32/cpu.c:    cc->set_pc = uc32_cpu_set_pc;
target-xtensa/cpu.c:    cc->set_pc = xtensa_cpu_set_pc;

ARM is doubled up. As long as no one else is missing a double up where
there is not default set by a single base class we are OK. If there is
a missing double up with a base class implementation, then there will
be no change by your proposal anyway.

Regards,
Peter

> 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 v2 2/4] gdbstub: Use cpu_set_pc helper

[Peter Maydell]

On 24 June 2015 at 18:16, Andreas Färber <afaerber@suse.de> wrote:
> Guys, is there any target that does not implement set_pc today? If so,
> which? I'd rather implement it than carry around the iffery and
> resulting complications.

No, there are none, see my analysis in my review of patch 1 in this set.

-- PMM

[Qemu-devel] [PATCH v2 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.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
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 d036624..a54add6 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,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, &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 +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, &error_abort);
 
                 if (!have_dtb(info)) {
                     if (old_param) {
-- 
1.9.1

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

[Peter Maydell]

On 16 June 2015 at 06:46, 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>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

though you'll want to drop the &error_abort argument to
cpu_set_pc() (see my review on patch 1).

thanks
-- PMM

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

[Andreas Färber]

Am 16.06.2015 um 13:32 schrieb Peter Maydell:
> On 16 June 2015 at 06:46, 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>
>> ---
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

> 
> though you'll want to drop the &error_abort argument to
> cpu_set_pc() (see my review on patch 1).

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)

[Qemu-devel] [PATCH v2 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.

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 v2 4/4] microblaze: boot: Use cpu_set_pc

[Peter Maydell]

On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
> 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);

Well, it sort of removes an instance of reaching into the CPU
env, but there's all those other ones in plain sight just above.
Is there much point in setting SR_PC indirectly if we don't
have a mechanism for setting the other regs indirectly?

-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc

[Peter Crosthwaite]

On Tue, Jun 16, 2015 at 4:33 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 June 2015 at 06:46, Peter Crosthwaite <crosthwaitepeter@gmail.com> wrote:
>> 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);
>
> Well, it sort of removes an instance of reaching into the CPU
> env, but there's all those other ones in plain sight just above.
> Is there much point in setting SR_PC indirectly if we don't
> have a mechanism for setting the other regs indirectly?
>

Yes. Needs more patches :). I'm starting with the easy stuff, and I am
actually more interested in getting rid of the SR_PC macro usage at
this stage. ARMs solution to this is the machine code bootloader. That
would be one way. But what I want to propose was that we add a virtual
method to CPUs that sets these offsets that bootloaders can call. This
brings us closer to one bootloader that can do it all.

Regards,
Peter

> -- PMM
>

Re: [Qemu-devel] [PATCH v2 4/4] microblaze: boot: Use cpu_set_pc

[Andreas Färber]

Am 16.06.2015 um 07:46 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.
> 
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

Reviewed-by: Andreas Färber <afaerber@suse.de>

but please remember to use "cpu_set_pc()" convention here and elsewhere
in the commit messages including subjects.

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)