Patchwork [next,v2,37/74] arm_boot: Pass ARMCPU to do_cpu_reset()

login
register
mail settings
Submitter Andreas Färber
Date May 10, 2012, 12:14 a.m.
Message ID <1336608892-30501-38-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/158100/
State New
Headers show

Comments

Andreas Färber - May 10, 2012, 12:14 a.m.
Allows us to use cpu_reset() in place of cpu_state_reset().

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/arm_boot.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
Peter Maydell - May 10, 2012, 9:41 p.m.
On 10 May 2012 01:14, Andreas Färber <afaerber@suse.de> wrote:
> Allows us to use cpu_reset() in place of cpu_state_reset().
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  hw/arm_boot.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index 7447f5c..eb2d176 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -274,10 +274,11 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>
>  static void do_cpu_reset(void *opaque)
>  {
> -    CPUARMState *env = opaque;
> +    ARMCPU *cpu = opaque;
> +    CPUARMState *env = &cpu->env;
>     const struct arm_boot_info *info = env->boot_info;
>
> -    cpu_state_reset(env);
> +    cpu_reset(CPU(cpu));

(Sadface for QOM method invocation requiring the explicit cast to
base class...)

>     if (info) {
>         if (!info->is_linux) {
>             /* Jump to the entry point.  */
> @@ -302,6 +303,7 @@ static void do_cpu_reset(void *opaque)
>
>  void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
>  {
> +    ARMCPU *cpu;
>     int kernel_size;
>     int initrd_size;
>     int n;

This variable could be kept in the more limited scope inside
the for loop below, right?

> @@ -406,7 +408,8 @@ void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
>     info->is_linux = is_linux;
>
>     for (; env; env = env->next_cpu) {
> +        cpu = arm_env_get_cpu(env);
>         env->boot_info = info;
> -        qemu_register_reset(do_cpu_reset, env);
> +        qemu_register_reset(do_cpu_reset, cpu);
>     }
>  }
> --
> 1.7.7
>
>


-- PMM
Andreas Färber - May 10, 2012, 10:40 p.m.
Am 10.05.2012 23:41, schrieb Peter Maydell:
> On 10 May 2012 01:14, Andreas Färber <afaerber@suse.de> wrote:
>> Allows us to use cpu_reset() in place of cpu_state_reset().
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  hw/arm_boot.c |    9 ++++++---
>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
>> index 7447f5c..eb2d176 100644
>> --- a/hw/arm_boot.c
>> +++ b/hw/arm_boot.c
[...]
>> @@ -302,6 +303,7 @@ static void do_cpu_reset(void *opaque)
>>
>>  void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
>>  {
>> +    ARMCPU *cpu;
>>     int kernel_size;
>>     int initrd_size;
>>     int n;
> 
> This variable could be kept in the more limited scope inside
> the for loop below, right?

It could. The function currently has a CPUARMState *env argument. That
is supposed to change to ARMCPU *cpu in one of the followup series, in
which case the for loop can reuse that parameter. I generally try to
keep cpu and env close together so that it's visible in diff/grep
context, as explained for your cpu_reset_model_id() series.

Would you like it better if I unneededly assigned ARMCPU *cpu =
arm_env_get_cpu(env); to show its relation to env? The next step would
be to pass cpu through to the write_secondary_boot callback.

I have not included the signature changes for arm_load_kernel() or the
callback in this series because it requires comparable changes to more
actively maintained machines such as exynos and highbank and would've
made it even longer. I'd be happy to reprioritize such a series though
if you're willing to review it and handle any arising conflicts. :)

/-F

>> @@ -406,7 +408,8 @@ void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
>>     info->is_linux = is_linux;
>>
>>     for (; env; env = env->next_cpu) {
>> +        cpu = arm_env_get_cpu(env);
>>         env->boot_info = info;
>> -        qemu_register_reset(do_cpu_reset, env);
>> +        qemu_register_reset(do_cpu_reset, cpu);
>>     }
>>  }
>> --
>> 1.7.7
Peter Maydell - May 11, 2012, 7:17 a.m.
On 10 May 2012 23:40, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.05.2012 23:41, schrieb Peter Maydell:
>> On 10 May 2012 01:14, Andreas Färber <afaerber@suse.de> wrote:
>>> Allows us to use cpu_reset() in place of cpu_state_reset().
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  hw/arm_boot.c |    9 ++++++---
>>>  1 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
>>> index 7447f5c..eb2d176 100644
>>> --- a/hw/arm_boot.c
>>> +++ b/hw/arm_boot.c
> [...]
>>> @@ -302,6 +303,7 @@ static void do_cpu_reset(void *opaque)
>>>
>>>  void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
>>>  {
>>> +    ARMCPU *cpu;
>>>     int kernel_size;
>>>     int initrd_size;
>>>     int n;
>>
>> This variable could be kept in the more limited scope inside
>> the for loop below, right?
>
> It could. The function currently has a CPUARMState *env argument. That
> is supposed to change to ARMCPU *cpu in one of the followup series, in
> which case the for loop can reuse that parameter. I generally try to
> keep cpu and env close together so that it's visible in diff/grep
> context, as explained for your cpu_reset_model_id() series.

Ah, I see.

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

-- PMM
Andreas Färber - May 11, 2012, 1:39 p.m.
Am 11.05.2012 09:17, schrieb Peter Maydell:
> On 10 May 2012 23:40, Andreas Färber <afaerber@suse.de> wrote:
>> Am 10.05.2012 23:41, schrieb Peter Maydell:
>>> On 10 May 2012 01:14, Andreas Färber <afaerber@suse.de> wrote:
>>>> Allows us to use cpu_reset() in place of cpu_state_reset().
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>>  hw/arm_boot.c |    9 ++++++---
>>>>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> Acked-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, applied to qom-next:
http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-next

/-F

Patch

diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index 7447f5c..eb2d176 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -274,10 +274,11 @@  static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
 
 static void do_cpu_reset(void *opaque)
 {
-    CPUARMState *env = opaque;
+    ARMCPU *cpu = opaque;
+    CPUARMState *env = &cpu->env;
     const struct arm_boot_info *info = env->boot_info;
 
-    cpu_state_reset(env);
+    cpu_reset(CPU(cpu));
     if (info) {
         if (!info->is_linux) {
             /* Jump to the entry point.  */
@@ -302,6 +303,7 @@  static void do_cpu_reset(void *opaque)
 
 void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
 {
+    ARMCPU *cpu;
     int kernel_size;
     int initrd_size;
     int n;
@@ -406,7 +408,8 @@  void arm_load_kernel(CPUARMState *env, struct arm_boot_info *info)
     info->is_linux = is_linux;
 
     for (; env; env = env->next_cpu) {
+        cpu = arm_env_get_cpu(env);
         env->boot_info = info;
-        qemu_register_reset(do_cpu_reset, env);
+        qemu_register_reset(do_cpu_reset, cpu);
     }
 }