[Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Maciej W. Rozycki]

Fix the FP state save/restore operations by saving the `flush_to_zero' 
rather than the `float_detect_tininess' setting.  There is no provision 
for the latter in MIPS hardware, whereas the former is controlled by the 
CP1.FCSR.FS bit.  As a result all the older saved state images are 
invalid as they do not restore the FP state corresponding to the 
CP1.FCSR.FS bit and may execute differently when resumed compared to the 
case where no save/restore operations have ever been made.  Therefore 
reject any such older images too and do not allow them to be loaded.

According to the architecture manual[1][2]:

"Flush to Zero (Flush Subnormals).
[...]
    Encoding                     Meaning
         0        Input subnormal values and tiny non-
                  zero results are not altered.  Unimple-
                  mented Operation Exception may be
                  signaled as needed.
         1        When FS is one, subnormal results are
                  flushed to zero.  The Unimplemented
                  Operation Exception is NOT signalled
                  for this reason.
                     Every tiny non-zero result is
                  replaced with zero of the same sign.
                     Prior to Release 5 it is implementa-
                  tion dependent whether subnormal
                  operand values are flushed to zero
                  before the operation is carried out.
                     As of Release 5 every input subnor-
                  mal value is replaced with zero of the
                  same sign."

References:

[1] "MIPS Architecture For Programmers, Volume I-A: Introduction to the 
    MIPS32 Architecture", MIPS Technologies, Inc., Document Number: 
    MD00082, Revision 5.03, Sept. 9, 2013, Table 5.7 "FCSR Register 
    Field Descriptions", p. 81.

[2] "MIPS Architecture For Programmers, Volume I-A: Introduction to the 
    MIPS64 Architecture", Imagination Technologies, Inc., Document 
    Number: MD00083, Revision 6.00, March 31, 2014, Table 6.7 "FCSR 
    Register Field Descriptions", p. 93.

Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com>
---
 Hopefully obvious.  Please apply.

  Maciej
qemu-mips-fp_status.diff
Index: qemu-git-trunk/target-mips/cpu.h
===================================================================
--- qemu-git-trunk.orig/target-mips/cpu.h	2014-11-12 07:38:02.077674697 +0000
+++ qemu-git-trunk/target-mips/cpu.h	2014-11-12 07:38:07.577674675 +0000
@@ -615,7 +615,7 @@ void mips_cpu_list (FILE *f, fprintf_fun
 extern void cpu_wrdsp(uint32_t rs, uint32_t mask_num, CPUMIPSState *env);
 extern uint32_t cpu_rddsp(uint32_t mask_num, CPUMIPSState *env);
 
-#define CPU_SAVE_VERSION 5
+#define CPU_SAVE_VERSION 6
 
 /* MMU modes definitions. We carefully match the indices with our
    hflags layout. */
Index: qemu-git-trunk/target-mips/machine.c
===================================================================
--- qemu-git-trunk.orig/target-mips/machine.c	2014-11-12 05:38:50.309021048 +0000
+++ qemu-git-trunk/target-mips/machine.c	2014-11-12 07:38:07.577674675 +0000
@@ -34,7 +34,7 @@ static void save_fpu(QEMUFile *f, CPUMIP
 
     for(i = 0; i < 32; i++)
         qemu_put_be64s(f, &fpu->fpr[i].d);
-    qemu_put_s8s(f, &fpu->fp_status.float_detect_tininess);
+    qemu_put_s8s(f, &fpu->fp_status.flush_to_zero);
     qemu_put_s8s(f, &fpu->fp_status.float_rounding_mode);
     qemu_put_s8s(f, &fpu->fp_status.float_exception_flags);
     qemu_put_be32s(f, &fpu->fcr0);
@@ -162,7 +162,7 @@ void cpu_save(QEMUFile *f, void *opaque)
         save_fpu(f, &env->fpus[i]);
 }
 
-static void load_tc(QEMUFile *f, TCState *tc, int version_id)
+static void load_tc(QEMUFile *f, TCState *tc)
 {
     int i;
 
@@ -184,9 +184,7 @@ static void load_tc(QEMUFile *f, TCState
     qemu_get_betls(f, &tc->CP0_TCSchedule);
     qemu_get_betls(f, &tc->CP0_TCScheFBack);
     qemu_get_sbe32s(f, &tc->CP0_Debug_tcstatus);
-    if (version_id >= 4) {
-        qemu_get_betls(f, &tc->CP0_UserLocal);
-    }
+    qemu_get_betls(f, &tc->CP0_UserLocal);
 }
 
 static void load_fpu(QEMUFile *f, CPUMIPSFPUContext *fpu)
@@ -195,7 +193,7 @@ static void load_fpu(QEMUFile *f, CPUMIP
 
     for(i = 0; i < 32; i++)
         qemu_get_be64s(f, &fpu->fpr[i].d);
-    qemu_get_s8s(f, &fpu->fp_status.float_detect_tininess);
+    qemu_get_s8s(f, &fpu->fp_status.flush_to_zero);
     qemu_get_s8s(f, &fpu->fp_status.float_rounding_mode);
     qemu_get_s8s(f, &fpu->fp_status.float_exception_flags);
     qemu_get_be32s(f, &fpu->fcr0);
@@ -208,12 +206,12 @@ int cpu_load(QEMUFile *f, void *opaque, 
     MIPSCPU *cpu = mips_env_get_cpu(env);
     int i;
 
-    if (version_id < 3) {
+    if (version_id != CPU_SAVE_VERSION) {
         return -EINVAL;
     }
 
     /* Load active TC */
-    load_tc(f, &env->active_tc, version_id);
+    load_tc(f, &env->active_tc);
 
     /* Load active FPU */
     load_fpu(f, &env->active_fpu);
@@ -328,7 +326,7 @@ int cpu_load(QEMUFile *f, void *opaque, 
 
     /* Load inactive TC state */
     for (i = 0; i < MIPS_SHADOW_SET_MAX; i++) {
-        load_tc(f, &env->tcs[i], version_id);
+        load_tc(f, &env->tcs[i]);
     }
     for (i = 0; i < MIPS_FPU_MAX; i++)
         load_fpu(f, &env->fpus[i]);

Re: [Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Peter Maydell]

On 12 November 2014 16:07, Maciej W. Rozycki <macro@codesourcery.com> wrote:
> Fix the FP state save/restore operations by saving the `flush_to_zero'
> rather than the `float_detect_tininess' setting.  There is no provision
> for the latter in MIPS hardware, whereas the former is controlled by the
> CP1.FCSR.FS bit.  As a result all the older saved state images are
> invalid as they do not restore the FP state corresponding to the
> CP1.FCSR.FS bit and may execute differently when resumed compared to the
> case where no save/restore operations have ever been made.  Therefore
> reject any such older images too and do not allow them to be loaded.

> @@ -208,12 +206,12 @@ int cpu_load(QEMUFile *f, void *opaque,
>      MIPSCPU *cpu = mips_env_get_cpu(env);
>      int i;
>
> -    if (version_id < 3) {
> +    if (version_id != CPU_SAVE_VERSION) {
>          return -EINVAL;
>      }

Shouldn't this read "if (version_id < 6)" ?
Otherwise next time somebody bumps the CPU_SAVE_VERSION it
will give another migration compatibility break without that
being very obvious.

As a longer-term cleanup I would highly recommend converting
the MIPS machine.c to use VMState structs to define its
migration (this is likely to imply another compatibility
break, so if you have any plans for supporting cross-QEMU-version
migration in future you should definitely do the conversion
before that point). The commit where we did this for ARM was
3cc1d20823 which gives you an idea of how the conversion works.
MIPS, CRIS and SPARC are the only three remaining guest CPUs
still using the old-style by-hand save/restore, so it would
be good to finally complete that transition.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Maciej W. Rozycki]

On Wed, 12 Nov 2014, Peter Maydell wrote:

> > @@ -208,12 +206,12 @@ int cpu_load(QEMUFile *f, void *opaque,
> >      MIPSCPU *cpu = mips_env_get_cpu(env);
> >      int i;
> >
> > -    if (version_id < 3) {
> > +    if (version_id != CPU_SAVE_VERSION) {
> >          return -EINVAL;
> >      }
> 
> Shouldn't this read "if (version_id < 6)" ?
> Otherwise next time somebody bumps the CPU_SAVE_VERSION it
> will give another migration compatibility break without that
> being very obvious.

 I gave it a thought before making this change and concluded it would be 
the lesser evil (plus loudly manifesting and easily correctable) if 
someone accidentally makes QEMU refuse to load older images where in 
fact no compatibility issue exists, than if the reverse is the case, 
that is older incompatible images are accepted where they should not 
(causing a silent misinterpretation of data), simply because someone 
missed the need to change the condition in addition to bumping up 
CPU_SAVE_VERSION.  WDYT?

 Besides if we go with your suggestion after all, then I think it should 
be:

    if (version_id < CPU_SAVE_OLDEST_SUPPORTED_VERSION) {

or suchlike as a hardcoded constant somewhere in the middle of code is 
too easy to miss.  Then the two constants can be maintained in a single 
place.

> As a longer-term cleanup I would highly recommend converting
> the MIPS machine.c to use VMState structs to define its
> migration (this is likely to imply another compatibility
> break, so if you have any plans for supporting cross-QEMU-version
> migration in future you should definitely do the conversion
> before that point). The commit where we did this for ARM was
> 3cc1d20823 which gives you an idea of how the conversion works.
> MIPS, CRIS and SPARC are the only three remaining guest CPUs
> still using the old-style by-hand save/restore, so it would
> be good to finally complete that transition.

 Thanks for the suggestion, it certainly looks like the right direction 
to me.  I need to offload the oustanding stuff first though.

  Maciej

Re: [Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Leon Alrae]

On 12/11/2014 18:58, Maciej W. Rozycki wrote:
> On Wed, 12 Nov 2014, Peter Maydell wrote:
> 
>>> @@ -208,12 +206,12 @@ int cpu_load(QEMUFile *f, void *opaque,
>>>      MIPSCPU *cpu = mips_env_get_cpu(env);
>>>      int i;
>>>
>>> -    if (version_id < 3) {
>>> +    if (version_id != CPU_SAVE_VERSION) {
>>>          return -EINVAL;
>>>      }
>>
>> Shouldn't this read "if (version_id < 6)" ?
>> Otherwise next time somebody bumps the CPU_SAVE_VERSION it
>> will give another migration compatibility break without that
>> being very obvious.
> 
>  I gave it a thought before making this change and concluded it would be 
> the lesser evil (plus loudly manifesting and easily correctable) if 
> someone accidentally makes QEMU refuse to load older images where in 
> fact no compatibility issue exists, than if the reverse is the case, 
> that is older incompatible images are accepted where they should not 
> (causing a silent misinterpretation of data), simply because someone 
> missed the need to change the condition in addition to bumping up 
> CPU_SAVE_VERSION.  WDYT?

Finally I got round to reviewing v2 of this patch. Above sounds
reasonable for me and I haven't seen any objections.
Hopefully we will convert it into VMState during 2.3 development (Peter,
thanks for pointing at the commit, it will certainly help).

Maciej, the only question I have is why you removed only some of "if
(version_id >= X)" lines in machine.c?

Regards,
Leon

Re: [Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state

[Maciej W. Rozycki]

On Fri, 5 Dec 2014, Leon Alrae wrote:

> >  I gave it a thought before making this change and concluded it would be 
> > the lesser evil (plus loudly manifesting and easily correctable) if 
> > someone accidentally makes QEMU refuse to load older images where in 
> > fact no compatibility issue exists, than if the reverse is the case, 
> > that is older incompatible images are accepted where they should not 
> > (causing a silent misinterpretation of data), simply because someone 
> > missed the need to change the condition in addition to bumping up 
> > CPU_SAVE_VERSION.  WDYT?
> 
> Finally I got round to reviewing v2 of this patch. Above sounds
> reasonable for me and I haven't seen any objections.
> Hopefully we will convert it into VMState during 2.3 development (Peter,
> thanks for pointing at the commit, it will certainly help).
> 
> Maciej, the only question I have is why you removed only some of "if
> (version_id >= X)" lines in machine.c?

 Argh, because I missed them.  I regenerated the original change and 
didn't think of checking if additional conditionals haven't been added 
upstream since the inception of the original change.  Sorry about that.  
I'll review all the relevant patches, see what has to be updated and 
resend.  Thanks for pointing this out.

 I'll give a priority to the 2008-NaN change though whose final updated 
version has just passed regression-testing (13+hrs per run).  It 
actually updates this save/restore logic here again and introduces the 
CPU_SAVE_VERSION_OLDEST_SUPPORTED macro I proposed here, to support 
older legacy-NaN images that are compatible with the 2008-NaN update.

  Maciej