Re: [Qemu-block] [Qemu-devel] [PATCH 3/8] fdc: Introduce fdctrl->phase

[Kevin Wolf]

Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:
> On 21 May 2015 at 10:42, Kevin Wolf <address@hidden> wrote:
> > Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
> >> On 20 May 2015 at 12:55, John Snow <address@hidden> wrote:
> >> > So even if /currently/ we can reconstitute it from the register values,
> >> > we may eventually be unable to.
> >> >
> >> > post_load will work for now, but I fear the case (in ten years) when
> >> > someone else cleans up FDC code but fails to realize that the phase is
> >> > not explicitly migrated.
> >>
> >> I assumed we would only do the post-load thing if the
> >> source for migration doesn't migrate phase (however we
> >> phrase that in a vmstate struct).
> >
> > I think if we extend the VMState, then we have two options. I'm not
> > exactly sure how they work in details, but I'll try an educated guess -
> > Juan, please correct me if I'm wrong:
> >
> > 1. We increase version_id and add the new field at the end. This breaks
> >    backwards migration; on forward migration the new field would be
> >    initialised with 0 and a post_load handler could check the old
> >    version_id to calculate the phase from register bits.
> >
> > 2. We add a subsection for the phase, and declare one phase to be the
> >    default (most likely the command phase) for which a subsection is not
> >    sent. In this case, the destination can't distinguish between a
> >    missing subsection because the source was running an old qemu or
> >    because it is the default phase. Unclear whether post_load should
> >    recalculate the phase or not.
> 
> 2b add a subsection, send the subsection always in new qemu,
> if receiving from an old qemu then calculate the phase from
> the register fields in post-load
> 
> That's like 1 but just using a subsection rather than a new field.
> (I have a vague recollection that distros doing backports prefer
> subsections over increment-version-id-and-add-field).

Hm, okay. The post_load handler doesn't have the version id then to
check. If a subsection isn't present, are its fields initialised as 0?
Or what would be the right way to check whether the subsection was
there?

Any pointers to device that do something similar?

> > If the above is correct, I'm afraid that the third option - which
> > doesn't address John's (valid) concerns - would be the most reasonable:
> >
> > 3. Don't add any VMState fields now and just do the post_load handler.
> >    If we ever extend fdc in a way that makes it impossible to
> >    reconstruct the phase from other migrated state, we add a subsection
> >    that is only sent in cases where it differs from the reconstructed
> >    value.
> 
> I'm definitely against this one. State should always be
> sent in migration, and not doing that once we've added
> it to the device struct is a bug.

On second thought, I could actually add that subsection now and write
the .needed callback such that it checks whether the reconstructed phase
matches the actual one. This condition would always evaluate as false
today, but if we ever change things so that the phase doesn't match what
current qemu would think it is, you automatically get the subsection and
things Just Work (TM).

This also wouldn't break backwards migration for now, and only break it
in future cases if it would really result in a broken device on the
destination.

Kevin