[Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[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);
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);

Re: [Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746, AG Nürnberg

Re: [Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[Paul Brook]

> 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

Re: [Qemu-devel] [PATCH] ARM GIC and CPU state saving/loading fix

[Dmitry Koshelev]

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.