qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Maciej W. Rozycki
Subject: Re: [Qemu-devel] [PATCH] mips: Correctly save/restore the FP flush-to-zero state
Date: Wed, 12 Nov 2014 18:58:19 +0000
User-agent: Alpine 1.10 (DEB 962 2008-03-14)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]