Re: ARM Snapshots Not Backwards-Compatible

[Aaron Lindsay]

On Feb 03 15:10, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> > On 2/3/21 3:58 PM, Aaron Lindsay wrote:
> > > On Feb 03 10:01, Peter Maydell wrote:
> > >>> The third is that meanings of the bits in env->features (as defined by
> > >>> `enum arm_features` in target/arm/cpu.h) has shifted. For example,
> > >>> ARM_FEATURE_PXN, ARM_FEATURE_CRC, ARM_FEATURE_VFP, ARM_FEATURE_VFP3,
> > >>> ARM_FEATURE_VFP4 have all been removed and ARM_FEATURE_V8_1M has been
> > >>> added since 4.1.0. Heck, even I have added a field there in the past.
> > >>> Unfortunately, these additions/removals mean that when env->features is
> > >>> saved on one version and restored on another the bits can mean different
> > >>> things. Notably, the removal of the *VFP features means that a snapshot
> > >>> of a CPU reporting it supports ARM_FEATURE_VFP3 on 4.1.0 thinks it's now
> > >>> ARM_FEATURE_M on 5.2.0!
> > >>
> > >> Ow. I didn't realize the env->features was in the migration state :-(
> > >> There is no reason for it to be, because it's a constant property
> > >> of the CPU. The easy fix is to replace
> > >>        VMSTATE_UINT64(env.features, ARMCPU),
> > >> in target/arm/machine.c with whatever the syntax is for "ignore
> > >> 64 bits of data here". Then we'll ignore whatever is coming in
> > >> from the source, which we don't need, and we'll stop sending it
> > >> out if we're the destination.
> > > 
> > > I'll look into this.
> > 
> > I think this is:
> > 
> >   VMSTATE_UNUSED(sizeof(uint64_t))
> 
> It's interesting that on x86 we've got a longterm request to *add* cpu
> features to the stream to detect screwups caused by using mismatched
> CPUs; so it's not necessarily a bad idea to include it once you realise
> it's there.

Another approach would be to assign integer values to the members of
`enum arm_features` in target/arm/cpu.h instead of letting the compiler
assign them for us. Of course, this only fixes the problem moving
forward and doesn't allow future QEMU versions to gain the ability to
load snapshots taken with previous QEMU versions. But seeing as how
no one else has raised the issue, I'm not sure how many people this
impacts.

I've tested the `VMSTATE_UNUSED(sizeof(uint64_t))` change and it appears
to have the same effect as me manually fixing up env.features (which is
to say: it works).

Maybe we could change it to `VMSTATE_UINT64(env.incoming_features,
ARMCPU),` instead and add a check in `cpu_post_load` that the
incoming_features match the expected ones (along with something matching
in `cpu_pre_save` to populate it)? We could ignore the incoming_features
field entirely in cpu_post_load for old snapshot versions, but check
that they match moving forward (perhaps generating a mask from the
current value of the enum to ignore fields which have been removed)?

I can send a patch for whichever path we settle on.

-Aaron