Patchwork [qom-cpu-next] e500: Replace open-coded loop with qemu_get_cpu()

login
register
mail settings
Submitter Andreas Färber
Date Feb. 15, 2013, 2:25 p.m.
Message ID <1360938355-22799-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/220750/
State New
Headers show

Comments

Andreas Färber - Feb. 15, 2013, 2:25 p.m.
Since we still need env for ppc-specific fields, obtain it via the new
env_ptr fields to avoid "cpu" name conflicts between CPUState and
PowerPCCPU for now.

This fixes a potential issue with env being NULL at the end of the loop
but cpu still being a valid pointer corresponding to a previous env.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Alex, this is a rebased and slimmed-down version of yesterday's paste.
 Can you ack please? Thanks, /-F

 hw/ppc/e500.c |   11 +++--------
 1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
Alexander Graf - Feb. 15, 2013, 2:29 p.m.
On 15.02.2013, at 15:25, Andreas Färber wrote:

> Since we still need env for ppc-specific fields, obtain it via the new
> env_ptr fields to avoid "cpu" name conflicts between CPUState and
> PowerPCCPU for now.
> 
> This fixes a potential issue with env being NULL at the end of the loop
> but cpu still being a valid pointer corresponding to a previous env.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Alex, this is a rebased and slimmed-down version of yesterday's paste.
> Can you ack please? Thanks, /-F

In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem?


Alex

> 
> hw/ppc/e500.c |   11 +++--------
> 1 Datei geändert, 3 Zeilen hinzugefügt(+), 8 Zeilen entfernt(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index b7474c0..451682c 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -240,20 +240,15 @@ static int ppce500_load_device_tree(CPUPPCState *env,
>     /* We need to generate the cpu nodes in reverse order, so Linux can pick
>        the first node as boot node and be happy */
>     for (i = smp_cpus - 1; i >= 0; i--) {
> -        CPUState *cpu = NULL;
> +        CPUState *cpu;
>         char cpu_name[128];
>         uint64_t cpu_release_addr = MPC8544_SPIN_BASE + (i * 0x20);
> 
> -        for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -            cpu = ENV_GET_CPU(env);
> -            if (cpu->cpu_index == i) {
> -                break;
> -            }
> -        }
> -
> +        cpu = qemu_get_cpu(i);
>         if (cpu == NULL) {
>             continue;
>         }
> +        env = cpu->env_ptr;
> 
>         snprintf(cpu_name, sizeof(cpu_name), "/cpus/PowerPC,8544@%x",
>                  cpu->cpu_index);
> -- 
> 1.7.10.4
>
Andreas Färber - Feb. 15, 2013, 2:35 p.m.
Am 15.02.2013 15:29, schrieb Alexander Graf:
> 
> On 15.02.2013, at 15:25, Andreas Färber wrote:
> 
>> Since we still need env for ppc-specific fields, obtain it via the new
>> env_ptr fields to avoid "cpu" name conflicts between CPUState and
>> PowerPCCPU for now.
>>
>> This fixes a potential issue with env being NULL at the end of the loop
>> but cpu still being a valid pointer corresponding to a previous env.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> Alex, this is a rebased and slimmed-down version of yesterday's paste.
>> Can you ack please? Thanks, /-F
> 
> In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem?

Not yet. Not all callers are using this pattern though, so solutions
would differ and be per-case patches.

At some point I obviously intend to change first_cpu and ->next_cpu to
CPUState and have been wondering whether we can use QTAILQ macros or
anything else more "standardized", with better iteration support.

Andreas
Alexander Graf - Feb. 15, 2013, 4:45 p.m.
On 15.02.2013, at 15:35, Andreas Färber wrote:

> Am 15.02.2013 15:29, schrieb Alexander Graf:
>> 
>> On 15.02.2013, at 15:25, Andreas Färber wrote:
>> 
>>> Since we still need env for ppc-specific fields, obtain it via the new
>>> env_ptr fields to avoid "cpu" name conflicts between CPUState and
>>> PowerPCCPU for now.
>>> 
>>> This fixes a potential issue with env being NULL at the end of the loop
>>> but cpu still being a valid pointer corresponding to a previous env.
>>> 
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> Alex, this is a rebased and slimmed-down version of yesterday's paste.
>>> Can you ack please? Thanks, /-F
>> 
>> In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem?
> 
> Not yet. Not all callers are using this pattern though, so solutions
> would differ and be per-case patches.

This specific patch is:

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

However, please make sure to fix the other places too :)


Alex

> 
> At some point I obviously intend to change first_cpu and ->next_cpu to
> CPUState and have been wondering whether we can use QTAILQ macros or
> anything else more "standardized", with better iteration support.
> 
> Andreas
> 
> -- 
> 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:24 p.m.
Am 15.02.2013 17:45, schrieb Alexander Graf:
> 
> On 15.02.2013, at 15:35, Andreas Färber wrote:
> 
>> Am 15.02.2013 15:29, schrieb Alexander Graf:
>>>
>>> On 15.02.2013, at 15:25, Andreas Färber wrote:
>>>
>>>> Since we still need env for ppc-specific fields, obtain it via the new
>>>> env_ptr fields to avoid "cpu" name conflicts between CPUState and
>>>> PowerPCCPU for now.
>>>>
>>>> This fixes a potential issue with env being NULL at the end of the loop
>>>> but cpu still being a valid pointer corresponding to a previous env.
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> Alex, this is a rebased and slimmed-down version of yesterday's paste.
>>>> Can you ack please? Thanks, /-F
>>>
>>> In your QOM conversion you also converted other places like this. Have you verified that these don't suffer from the same problem?
>>
>> Not yet. Not all callers are using this pattern though, so solutions
>> would differ and be per-case patches.
> 
> This specific patch is:
> 
> Acked-by: Alexander Graf <agraf@suse.de>

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

> However, please make sure to fix the other places too :)

In whole there's two e500 patches fixing a potential bug, plus
non-functional cleanups for sPAPR, monitor and QMP code.

Andreas

Patch

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b7474c0..451682c 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -240,20 +240,15 @@  static int ppce500_load_device_tree(CPUPPCState *env,
     /* We need to generate the cpu nodes in reverse order, so Linux can pick
        the first node as boot node and be happy */
     for (i = smp_cpus - 1; i >= 0; i--) {
-        CPUState *cpu = NULL;
+        CPUState *cpu;
         char cpu_name[128];
         uint64_t cpu_release_addr = MPC8544_SPIN_BASE + (i * 0x20);
 
-        for (env = first_cpu; env != NULL; env = env->next_cpu) {
-            cpu = ENV_GET_CPU(env);
-            if (cpu->cpu_index == i) {
-                break;
-            }
-        }
-
+        cpu = qemu_get_cpu(i);
         if (cpu == NULL) {
             continue;
         }
+        env = cpu->env_ptr;
 
         snprintf(cpu_name, sizeof(cpu_name), "/cpus/PowerPC,8544@%x",
                  cpu->cpu_index);