| Message ID | 1360938355-22799-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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
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);
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(-)