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

[John Snow <jsnow@redhat.com>]

On 05/19/2015 04:44 PM, Peter Maydell wrote:
> On 19 May 2015 at 16:35, Kevin Wolf <kwolf@redhat.com> 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 <kwolf@redhat.com>
>> ---
>>  hw/block/fdc.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 8c41434..4d4868e 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -495,6 +495,30 @@ 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.
>> + */
>> +typedef enum FDCtrlPhase {
>> +    FD_PHASE_COMMAND,
>> +    FD_PHASE_EXECUTION,
>> +    FD_PHASE_RESULT,
>> +} FDCtrlPhase;
>> +
>>  #define FD_MULTI_TRACK(state) ((state) & FD_STATE_MULTI)
>>  #define FD_FORMAT_CMD(state) ((state) & FD_STATE_FORMAT)
>>
>> @@ -504,6 +528,7 @@ struct FDCtrl {
>>      /* Controller state */
>>      QEMUTimer *result_timer;
>>      int dma_chann;
>> +    FDCtrlPhase phase;
> 
> This is new state -- don't we need to handle it on migration?
> 

Tch, good point.

> In general I'm not a huge fan of caching derived state in
> device models...
> 
> -- PMM
> 

Hmm, I think this is not purely derived state because the flags are not
necessarily sufficient for regenerating that state.

It does make the code read an awful lot nicer, too. Especially if it
brings it closer to the reference materials to encourage future patches.

I think it's worth the stream format change.

--js