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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * John Snow (address@hidden) wrote:
>> 
>> 
>> On 05/21/2015 09:19 AM, Kevin Wolf wrote:
>> > The floppy controller spec describes three different controller phases,
>> > which are currently not explicitly modelled in our emulation. Instead,
>> > each phase is represented by a combination of flags in registers.
>> > 
>> > This patch makes explicit in which phase the controller currently is.
>> > 
>> > Signed-off-by: Kevin Wolf <address@hidden>
>> > ---
>> >  hw/block/fdc.c | 89 
>> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 89 insertions(+)
>> > 
>> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> > index 8c41434..f5bcf0b 100644
>> > --- a/hw/block/fdc.c
>> > +++ b/hw/block/fdc.c
>> > @@ -495,6 +495,33 @@ enum {
>> >      FD_DIR_DSKCHG   = 0x80,
>> >  };
>> >  
>> > +/*
>> > + * See chapter 5.0 "Controller phases" of the spec:
>> > + *
>> > + * Command phase:
>> > + * The host writes a command and its parameters into the FIFO. The command
>> > + * phase is completed when all parameters for the command have been 
>> > supplied,
>> > + * and execution phase is entered.
>> > + *
>> > + * Execution phase:
>> > + * Data transfers, either DMA or non-DMA. For non-DMA transfers, the FIFO
>> > + * contains the payload now, otherwise it's unused. When all bytes of the
>> > + * required data have been transferred, the state is switched to either 
>> > result
>> > + * phase (if the command produces status bytes) or directly back into the
>> > + * command phase for the next command.
>> > + *
>> > + * Result phase:
>> > + * The host reads out the FIFO, which contains one or more result bytes 
>> > now.
>> > + */
>> > +enum {
>> > +    /* Only for migration: reconstruct phase from registers like qemu 2.3 
>> > */
>> > +    FD_PHASE_RECONSTRUCT    = 0,
>> > +
>> > +    FD_PHASE_COMMAND        = 1,
>> > +    FD_PHASE_EXECUTION      = 2,
>> > +    FD_PHASE_RESULT         = 3,
>> > +};
>> > +
>> >  #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
>> >  #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>> >  
>> > @@ -504,6 +531,7 @@ struct FDCtrl {
>> >      /* Controller state */
>> >      QEMUTimer *result_timer;
>> >      int dma_chann;
>> > +    uint8_t phase;
>> >      /* Controller's identification */
>> >      uint8_t version;
>> >      /* HW */
>> > @@ -744,6 +772,28 @@ static const VMStateDescription vmstate_fdrive = {
>> >      }
>> >  };
>> >  
>> > +/*
>> > + * Reconstructs the phase from register values according to the logic 
>> > that was
>> > + * implemented in qemu 2.3. This is the default value that is used if the 
>> > phase
>> > + * subsection is not present on migration.
>> > + *
>> > + * Don't change this function to reflect newer qemu versions, it is part 
>> > of
>> > + * the migration ABI.
>> > + */
>> > +static int reconstruct_phase(FDCtrl *fdctrl)
>> > +{
>> > +    if (fdctrl->msr & FD_MSR_NONDMA) {
>> > +        return FD_PHASE_EXECUTION;
>> > +    } else if ((fdctrl->msr & FD_MSR_RQM) == 0) {
>> > +        /* qemu 2.3 disabled RQM only during DMA transfers */
>> > +        return FD_PHASE_EXECUTION;
>> > +    } else if (fdctrl->msr & FD_MSR_DIO) {
>> > +        return FD_PHASE_RESULT;
>> > +    } else {
>> > +        return FD_PHASE_COMMAND;
>> > +    }
>> > +}
>> > +
>> >  static void fdc_pre_save(void *opaque)
>> >  {
>> >      FDCtrl *s = opaque;
>> > @@ -751,12 +801,24 @@ static void fdc_pre_save(void *opaque)
>> >      s->dor_vmstate = s->dor | GET_CUR_DRV(s);
>> >  }
>> >  
>> > +static int fdc_pre_load(void *opaque)
>> > +{
>> > +    FDCtrl *s = opaque;
>> > +    s->phase = FD_PHASE_RECONSTRUCT;
>> > +    return 0;
>> > +}
>> > +
>> >  static int fdc_post_load(void *opaque, int version_id)
>> >  {
>> >      FDCtrl *s = opaque;
>> >  
>> >      SET_CUR_DRV(s, s->dor_vmstate & FD_DOR_SELMASK);
>> >      s->dor = s->dor_vmstate & ~FD_DOR_SELMASK;
>> > +
>> > +    if (s->phase == FD_PHASE_RECONSTRUCT) {
>> > +        s->phase = reconstruct_phase(s);
>> > +    }
>> > +
>> >      return 0;
>> >  }
>> >  
>> > @@ -794,11 +856,29 @@ static const VMStateDescription 
>> > vmstate_fdc_result_timer = {
>> >      }
>> >  };
>> >  
>> > +static bool fdc_phase_needed(void *opaque)
>> > +{
>> > +    FDCtrl *fdctrl = opaque;
>> > +
>> > +    return reconstruct_phase(fdctrl) != fdctrl->phase;
>> > +}
>
> OK, that is one way of doing it which should work, as long
> as most of the time that matches.
>
> My preference for subsections is normally to make them just
> conditional on machine type, that way backwards migration just
> works.  However, if the result of the backwards migration would
> be data corruption (which if I understand what you saying it could
> in this case) then it's arguably better to fail migration in those
> cases.

This isn't a matter of preference.

Subsections were designed to solves a problem we only have because we're
not perfect: device model bugs, namely forgetting to migrate a bit of
state, or failing to model it in the first place.

Subsections tied to machine types solve an entirely different problem:
the old version of the device is just fine, including migration, but we
now want a new and improved variant, which needs some more state
migrated.  Putting that piece of state into a subsection tied to the new
machine type avoids code duplication.

But what do you do for device model bugs serious enough to need fixing
even for existing machine types, when the fix needs additional state
migrated?  Subsections tied to machine types are *not* a solution there.
That's why this isn't about preference!  It's about having a bug to fix
or not.

You seem rather reluctant to use subsections for their intended purpose.
I don't understand; their correctness argument is really simple.

Fundamentally, we're picking a pair of state serialization and
deserialization functions (in the mathematical sense).

The obvious encoding function for a state consisting of pieces is to
encode all the pieces one by one.  Call this e(), and its decoding
function d().

Here's an alternative pair e'() and d'():

* Pick a piece of the state.

* Pick one of its values.  Let's call it "the prevalent value" for
  reasons that will become apparent shortly.

* Encode just like e() does, except omit this piece of state if and only
  if it has its prevalent value.

* In the decoding function, default this piece of state to its prevalent
  value.

The encodings are equivalent: d(e(x)) = d'(e'(x)) for all x.

At this point, you might want to tell me not to waste your time with
trivial math.  If so, good!  The triviality is exactly my point.

Straightforwardly translated to code, e() and d() put everything in the
section.  Drawback: add/remove/modify of state kills migration to and
from older versions.

e'() and d'() put a piece of state in a subsection that is only present
when the piece doesn't have its prevalent value.

Because the encodings are equivalent, forward migration transfers
exactly the same state regardless of which of them we choose.

So why not choose one that makes backward migration work exactly when
it's safe?

* Put all pieces of state the old version doesn't understand in
  subsections.

* Identify prevalent state.

* Double-check the old version behaves safely in prevalent state.

* In prevalent state, no subsections are sent, and migration succeeds.

* In any other state, migration fails.

Naturally, this is useful only when the prevalent state is actually
prevalent.

> You might like to add a comment to that effect.
>
> Would I be correct in thinking that all our common OSs
> end up not sending this section while running and not
> accessing the floppy?

Yes, the prevalent state better be prevalent.