| Message ID | alpine.DEB.1.10.1411121544140.2881@tp.orcam.me.uk |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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]);
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