Message ID | CAKUHwZpxH2vUBb9Y_R0uPCMvPhcpNB=yJJFdmJFWfmtbG85_Nw@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Fri, Oct 14, 2011 at 05:25:29PM +0400, Dmitry Koshelev wrote: > Fixes two trivial indices errors. > > Signed-off-by: Dmitry Koshelev <karaghiozis@gmail.com> > --- > hw/arm_gic.c | 12 ++++++------ > target-arm/machine.c | 4 ++-- > 2 files changed, 8 insertions(+), 8 deletions(-) Not obvious to me what the implications are. CCed Peter Maydell so it can go through his ARM tree. Stefan
Am 14.10.2011 15:25, schrieb Dmitry Koshelev: > Fixes two trivial indices errors. > > Signed-off-by: Dmitry Koshelev <karaghiozis@gmail.com> > --- > hw/arm_gic.c | 12 ++++++------ > target-arm/machine.c | 4 ++-- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/hw/arm_gic.c b/hw/arm_gic.c > index 8286a28..ba05131 100644 > --- a/hw/arm_gic.c > +++ b/hw/arm_gic.c > @@ -662,9 +662,6 @@ static void gic_save(QEMUFile *f, void *opaque) > qemu_put_be32(f, s->enabled); > for (i = 0; i < NUM_CPU(s); i++) { > qemu_put_be32(f, s->cpu_enabled[i]); > -#ifndef NVIC > - qemu_put_be32(f, s->irq_target[i]); > -#endif > for (j = 0; j < 32; j++) > qemu_put_be32(f, s->priority1[j][i]); > for (j = 0; j < GIC_NIRQ; j++) > @@ -678,6 +675,9 @@ static void gic_save(QEMUFile *f, void *opaque) > qemu_put_be32(f, s->priority2[i]); > } > for (i = 0; i < GIC_NIRQ; i++) { > +#ifndef NVIC > + qemu_put_be32(f, s->irq_target[i]); > +#endif > qemu_put_byte(f, s->irq_state[i].enabled); > qemu_put_byte(f, s->irq_state[i].pending); > qemu_put_byte(f, s->irq_state[i].active); > @@ -699,9 +699,6 @@ static int gic_load(QEMUFile *f, void *opaque, int > version_id) > s->enabled = qemu_get_be32(f); > for (i = 0; i < NUM_CPU(s); i++) { > s->cpu_enabled[i] = qemu_get_be32(f); > -#ifndef NVIC > - s->irq_target[i] = qemu_get_be32(f); > -#endif > for (j = 0; j < 32; j++) > s->priority1[j][i] = qemu_get_be32(f); > for (j = 0; j < GIC_NIRQ; j++) > @@ -715,6 +712,9 @@ static int gic_load(QEMUFile *f, void *opaque, int > version_id) > s->priority2[i] = qemu_get_be32(f); > } > for (i = 0; i < GIC_NIRQ; i++) { > +#ifndef NVIC > + s->irq_target[i] = qemu_get_be32(f); > +#endif > s->irq_state[i].enabled = qemu_get_byte(f); > s->irq_state[i].pending = qemu_get_byte(f); > s->irq_state[i].active = qemu_get_byte(f); This part: Reviewed-by: Andreas Färber <afaerber@suse.de> The definition is int irq_target[GIC_NIRQ] and not [NCPU] as others. The following part however is totally unrelated and should be put in a separate patch. Both are non-trivial, please cc Peter Maydell instead. > diff --git a/target-arm/machine.c b/target-arm/machine.c > index 3925d3a..1b1b3ec 100644 > --- a/target-arm/machine.c > +++ b/target-arm/machine.c > @@ -53,7 +53,7 @@ void cpu_save(QEMUFile *f, void *opaque) > qemu_put_be32(f, env->features); > > if (arm_feature(env, ARM_FEATURE_VFP)) { > - for (i = 0; i < 16; i++) { > + for (i = 16; i < 32; i++) { > CPU_DoubleU u; > u.d = env->vfp.regs[i]; > qemu_put_be32(f, u.l.upper); > @@ -175,7 +175,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) > env->vfp.vec_stride = qemu_get_be32(f); > > if (arm_feature(env, ARM_FEATURE_VFP3)) { > - for (i = 0; i < 16; i++) { > + for (i = 16; i < 32; i++) { > CPU_DoubleU u; > u.l.upper = qemu_get_be32(f); > u.l.lower = qemu_get_be32(f); > This does not look fully right either way... In addition, it touches the storage format so any change there may require a version bump. Andreas
> Fixes two trivial indices errors. No. You're doing two much in a single patch. While both happen to be bug in the save/restore code involving arrays, these are not two instances of the same bug. The justification for each change is completely different. Even if each change was obviously correct, I believe putting them together into a single commit makes the result non-trivial. The fact your patch introduces a bug strongly suggests it shouldn't have been considered trivial to start with. > @@ -53,7 +53,7 @@ void cpu_save(QEMUFile *f, void *opaque) > if (arm_feature(env, ARM_FEATURE_VFP)) { > - for (i = 0; i < 16; i++) { > + for (i = 16; i < 32; i++) { > CPU_DoubleU u; > u.d = env->vfp.regs[i]; I'm pretty sure this is wrong. Paul
On Fri, Oct 14, 2011 at 7:04 PM, Paul Brook <paul@codesourcery.com> wrote: >> Fixes two trivial indices errors. > > No. > > You're doing two much in a single patch. While both happen to be bug in the > save/restore code involving arrays, these are not two instances of the same > bug. The justification for each change is completely different. Sure. So should i resubmit it as 2 patches ? > > Even if each change was obviously correct, I believe putting them together > into a single commit makes the result non-trivial. The fact your patch > introduces a bug strongly suggests it shouldn't have been considered trivial > to start with. > >> @@ -53,7 +53,7 @@ void cpu_save(QEMUFile *f, void *opaque) >> if (arm_feature(env, ARM_FEATURE_VFP)) { >> - for (i = 0; i < 16; i++) { >> + for (i = 16; i < 32; i++) { >> CPU_DoubleU u; >> u.d = env->vfp.regs[i]; > > I'm pretty sure this is wrong. > > Paul > Oops, don't know how it got here lol. Bug is in cpu_load not in cpu_save, of course.
diff --git a/hw/arm_gic.c b/hw/arm_gic.c index 8286a28..ba05131 100644 --- a/hw/arm_gic.c +++ b/hw/arm_gic.c @@ -662,9 +662,6 @@ static void gic_save(QEMUFile *f, void *opaque) qemu_put_be32(f, s->enabled); for (i = 0; i < NUM_CPU(s); i++) { qemu_put_be32(f, s->cpu_enabled[i]); -#ifndef NVIC - qemu_put_be32(f, s->irq_target[i]); -#endif for (j = 0; j < 32; j++) qemu_put_be32(f, s->priority1[j][i]); for (j = 0; j < GIC_NIRQ; j++) @@ -678,6 +675,9 @@ static void gic_save(QEMUFile *f, void *opaque) qemu_put_be32(f, s->priority2[i]); } for (i = 0; i < GIC_NIRQ; i++) { +#ifndef NVIC + qemu_put_be32(f, s->irq_target[i]); +#endif qemu_put_byte(f, s->irq_state[i].enabled); qemu_put_byte(f, s->irq_state[i].pending); qemu_put_byte(f, s->irq_state[i].active); @@ -699,9 +699,6 @@ static int gic_load(QEMUFile *f, void *opaque, int version_id) s->enabled = qemu_get_be32(f); for (i = 0; i < NUM_CPU(s); i++) { s->cpu_enabled[i] = qemu_get_be32(f); -#ifndef NVIC - s->irq_target[i] = qemu_get_be32(f); -#endif for (j = 0; j < 32; j++) s->priority1[j][i] = qemu_get_be32(f); for (j = 0; j < GIC_NIRQ; j++) @@ -715,6 +712,9 @@ static int gic_load(QEMUFile *f, void *opaque, int version_id) s->priority2[i] = qemu_get_be32(f); } for (i = 0; i < GIC_NIRQ; i++) { +#ifndef NVIC + s->irq_target[i] = qemu_get_be32(f); +#endif s->irq_state[i].enabled = qemu_get_byte(f); s->irq_state[i].pending = qemu_get_byte(f); s->irq_state[i].active = qemu_get_byte(f); diff --git a/target-arm/machine.c b/target-arm/machine.c index 3925d3a..1b1b3ec 100644 --- a/target-arm/machine.c +++ b/target-arm/machine.c @@ -53,7 +53,7 @@ void cpu_save(QEMUFile *f, void *opaque) qemu_put_be32(f, env->features); if (arm_feature(env, ARM_FEATURE_VFP)) { - for (i = 0; i < 16; i++) { + for (i = 16; i < 32; i++) { CPU_DoubleU u; u.d = env->vfp.regs[i]; qemu_put_be32(f, u.l.upper); @@ -175,7 +175,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id) env->vfp.vec_stride = qemu_get_be32(f); if (arm_feature(env, ARM_FEATURE_VFP3)) { - for (i = 0; i < 16; i++) { + for (i = 16; i < 32; i++) { CPU_DoubleU u; u.l.upper = qemu_get_be32(f); u.l.lower = qemu_get_be32(f);
Fixes two trivial indices errors. Signed-off-by: Dmitry Koshelev <karaghiozis@gmail.com> --- hw/arm_gic.c | 12 ++++++------ target-arm/machine.c | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-)