diff mbox

[qom-cpu-next] ppce500_spin: Replace open-coded CPU loop with qemu_get_cpu()

Message ID 1360947119-26115-1-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber Feb. 15, 2013, 4:51 p.m. UTC 
Potentially env could be NULL whereas cpu would still be valid and
correspond to a previous env.

Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ppce500_spin.c |   15 ++++-----------
 1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)

Comments

Alexander Graf Feb. 15, 2013, 4:54 p.m. UTC | #1 
On 15.02.2013, at 17:51, Andreas Färber wrote:

> Potentially env could be NULL whereas cpu would still be valid and
> correspond to a previous env.
> 
> Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
> code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> hw/ppce500_spin.c |   15 ++++-----------
> 1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
> 
> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
> index 7e90fb9..5bdce52 100644
> --- a/hw/ppce500_spin.c
> +++ b/hw/ppce500_spin.c
> @@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
> {
>     SpinState *s = opaque;
>     int env_idx = addr / sizeof(SpinInfo);
> -    CPUPPCState *env;
> -    CPUState *cpu = NULL;
> +    CPUState *cpu;
>     SpinInfo *curspin = &s->spin[env_idx];
>     uint8_t *curspin_p = (uint8_t*)curspin;
> 
> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        cpu = CPU(ppc_env_get_cpu(env));
> -        if (cpu->cpu_index == env_idx) {
> -            break;
> -        }
> -    }
> -
> +    cpu = qemu_get_cpu(env_idx);
>     if (cpu == NULL) {
>         /* Unknown CPU */
>         return;
> @@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>     if (!(ldq_p(&curspin->addr) & 1)) {
>         /* run CPU */
>         SpinKick kick = {
> -            .cpu = ppc_env_get_cpu(env),
> +            .cpu = POWERPC_CPU(cpu),

Why not just cpu?

Alex

>             .spin = curspin,
>         };
> 
> -        run_on_cpu(CPU(kick.cpu), spin_kick, &kick);
> +        run_on_cpu(cpu, spin_kick, &kick);
>     }
> }
> 
> -- 
> 1.7.10.4
>
Andreas Färber Feb. 15, 2013, 4:58 p.m. UTC | #2 
Am 15.02.2013 17:54, schrieb Alexander Graf:
> 
> On 15.02.2013, at 17:51, Andreas Färber wrote:
> 
>> Potentially env could be NULL whereas cpu would still be valid and
>> correspond to a previous env.
>>
>> Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
>> code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> hw/ppce500_spin.c |   15 ++++-----------
>> 1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
>>
>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>> index 7e90fb9..5bdce52 100644
>> --- a/hw/ppce500_spin.c
>> +++ b/hw/ppce500_spin.c
>> @@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>> {
>>     SpinState *s = opaque;
>>     int env_idx = addr / sizeof(SpinInfo);
>> -    CPUPPCState *env;
>> -    CPUState *cpu = NULL;
>> +    CPUState *cpu;
>>     SpinInfo *curspin = &s->spin[env_idx];
>>     uint8_t *curspin_p = (uint8_t*)curspin;
>>
>> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>> -        cpu = CPU(ppc_env_get_cpu(env));
>> -        if (cpu->cpu_index == env_idx) {
>> -            break;
>> -        }
>> -    }
>> -
>> +    cpu = qemu_get_cpu(env_idx);
>>     if (cpu == NULL) {
>>         /* Unknown CPU */
>>         return;
>> @@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>>     if (!(ldq_p(&curspin->addr) & 1)) {
>>         /* run CPU */
>>         SpinKick kick = {
>> -            .cpu = ppc_env_get_cpu(env),
>> +            .cpu = POWERPC_CPU(cpu),
> 
> Why not just cpu?

PowerPCCPU vs. CPUState type.
Having the specific type in ppc code allows direct access to ->env.

If you see a performance issue, we could also use (PowerPCCPU *)cpu.

Andreas

>>             .spin = curspin,
>>         };
>>
>> -        run_on_cpu(CPU(kick.cpu), spin_kick, &kick);
>> +        run_on_cpu(cpu, spin_kick, &kick);
>>     }
>> }
>>
>> -- 
>> 1.7.10.4
>>
>
Alexander Graf Feb. 15, 2013, 5:04 p.m. UTC | #3 
On 15.02.2013, at 17:58, Andreas Färber wrote:

> Am 15.02.2013 17:54, schrieb Alexander Graf:
>> 
>> On 15.02.2013, at 17:51, Andreas Färber wrote:
>> 
>>> Potentially env could be NULL whereas cpu would still be valid and
>>> correspond to a previous env.
>>> 
>>> Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
>>> code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
>>> 
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> hw/ppce500_spin.c |   15 ++++-----------
>>> 1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
>>> 
>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>> index 7e90fb9..5bdce52 100644
>>> --- a/hw/ppce500_spin.c
>>> +++ b/hw/ppce500_spin.c
>>> @@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>>> {
>>>    SpinState *s = opaque;
>>>    int env_idx = addr / sizeof(SpinInfo);
>>> -    CPUPPCState *env;
>>> -    CPUState *cpu = NULL;
>>> +    CPUState *cpu;
>>>    SpinInfo *curspin = &s->spin[env_idx];
>>>    uint8_t *curspin_p = (uint8_t*)curspin;
>>> 
>>> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>> -        cpu = CPU(ppc_env_get_cpu(env));
>>> -        if (cpu->cpu_index == env_idx) {
>>> -            break;
>>> -        }
>>> -    }
>>> -
>>> +    cpu = qemu_get_cpu(env_idx);
>>>    if (cpu == NULL) {
>>>        /* Unknown CPU */
>>>        return;
>>> @@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>>>    if (!(ldq_p(&curspin->addr) & 1)) {
>>>        /* run CPU */
>>>        SpinKick kick = {
>>> -            .cpu = ppc_env_get_cpu(env),
>>> +            .cpu = POWERPC_CPU(cpu),
>> 
>> Why not just cpu?
> 
> PowerPCCPU vs. CPUState type.
> Having the specific type in ppc code allows direct access to ->env.
> 
> If you see a performance issue, we could also use (PowerPCCPU *)cpu.

There are no performance issues in the spin code :). By definition. This thing could be written in bash and still be fast enough.

I was mostly wondering whether ppc_env_get_cpu(env) returns a PowerPCCPU *. But apparently it does. Ok then :)


Acked-by: Alexander Graf <agraf@suse.de>


Alex

> 
> Andreas
> 
>>>            .spin = curspin,
>>>        };
>>> 
>>> -        run_on_cpu(CPU(kick.cpu), spin_kick, &kick);
>>> +        run_on_cpu(cpu, spin_kick, &kick);
>>>    }
>>> }
>>> 
>>> -- 
>>> 1.7.10.4
>>> 
>> 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber Feb. 15, 2013, 5:25 p.m. UTC | #4 
Am 15.02.2013 18:04, schrieb Alexander Graf:
> 
> On 15.02.2013, at 17:58, Andreas Färber wrote:
> 
>> Am 15.02.2013 17:54, schrieb Alexander Graf:
>>>
>>> On 15.02.2013, at 17:51, Andreas Färber wrote:
>>>
>>>> Potentially env could be NULL whereas cpu would still be valid and
>>>> correspond to a previous env.
>>>>
>>>> Wrapping this in qemu_get_cpu(), env is no longer needed, so simplify
>>>> code that existed before 55e5c2850293547203874098f7cec148ffd12dfa.
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> hw/ppce500_spin.c |   15 ++++-----------
>>>> 1 Datei geändert, 4 Zeilen hinzugefügt(+), 11 Zeilen entfernt(-)
>>>>
>>>> diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
>>>> index 7e90fb9..5bdce52 100644
>>>> --- a/hw/ppce500_spin.c
>>>> +++ b/hw/ppce500_spin.c
>>>> @@ -123,18 +123,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>>>> {
>>>>    SpinState *s = opaque;
>>>>    int env_idx = addr / sizeof(SpinInfo);
>>>> -    CPUPPCState *env;
>>>> -    CPUState *cpu = NULL;
>>>> +    CPUState *cpu;
>>>>    SpinInfo *curspin = &s->spin[env_idx];
>>>>    uint8_t *curspin_p = (uint8_t*)curspin;
>>>>
>>>> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>>> -        cpu = CPU(ppc_env_get_cpu(env));
>>>> -        if (cpu->cpu_index == env_idx) {
>>>> -            break;
>>>> -        }
>>>> -    }
>>>> -
>>>> +    cpu = qemu_get_cpu(env_idx);
>>>>    if (cpu == NULL) {
>>>>        /* Unknown CPU */
>>>>        return;
>>>> @@ -161,11 +154,11 @@ static void spin_write(void *opaque, hwaddr addr, uint64_t value,
>>>>    if (!(ldq_p(&curspin->addr) & 1)) {
>>>>        /* run CPU */
>>>>        SpinKick kick = {
>>>> -            .cpu = ppc_env_get_cpu(env),
>>>> +            .cpu = POWERPC_CPU(cpu),
>>>
>>> Why not just cpu?
>>
>> PowerPCCPU vs. CPUState type.
>> Having the specific type in ppc code allows direct access to ->env.
>>
>> If you see a performance issue, we could also use (PowerPCCPU *)cpu.
> 
> There are no performance issues in the spin code :). By definition. This thing could be written in bash and still be fast enough.
> 
> I was mostly wondering whether ppc_env_get_cpu(env) returns a PowerPCCPU *. But apparently it does. Ok then :)
> 
> 
> Acked-by: Alexander Graf <agraf@suse.de>

Thanks, applied to qom-cpu-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next

/-F
diff mbox

Patch

diff --git a/hw/ppce500_spin.c b/hw/ppce500_spin.c
index 7e90fb9..5bdce52 100644
--- a/hw/ppce500_spin.c
+++ b/hw/ppce500_spin.c
@@ -123,18 +123,11 @@  static void spin_write(void *opaque, hwaddr addr, uint64_t value,
 {
     SpinState *s = opaque;
     int env_idx = addr / sizeof(SpinInfo);
-    CPUPPCState *env;
-    CPUState *cpu = NULL;
+    CPUState *cpu;
     SpinInfo *curspin = &s->spin[env_idx];
     uint8_t *curspin_p = (uint8_t*)curspin;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        cpu = CPU(ppc_env_get_cpu(env));
-        if (cpu->cpu_index == env_idx) {
-            break;
-        }
-    }
-
+    cpu = qemu_get_cpu(env_idx);
     if (cpu == NULL) {
         /* Unknown CPU */
         return;
@@ -161,11 +154,11 @@  static void spin_write(void *opaque, hwaddr addr, uint64_t value,
     if (!(ldq_p(&curspin->addr) & 1)) {
         /* run CPU */
         SpinKick kick = {
-            .cpu = ppc_env_get_cpu(env),
+            .cpu = POWERPC_CPU(cpu),
             .spin = curspin,
         };
 
-        run_on_cpu(CPU(kick.cpu), spin_kick, &kick);
+        run_on_cpu(cpu, spin_kick, &kick);
     }
 }