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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> 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

Doesn't this break new -> old migration?

>> 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.

This is exactly how subsections are meant to be used: weakest "needed"
predicate that's still sufficient for reconstructing correct state on
the destination.

Yes, "state should always be sent", but we're free to encode the state
however we like.  The "if we can reconstruct phase from section, done,
else send it in a subsection" is a faithful, if ugly encoding of the
state.

Tradeoff here: ugly encoding and good backward compatibility vs. natural
encoding and bad backward compatibility.  We usually prefer the former.