Re: [Qemu-block] [Qemu-devel] [PATCH 4/8] fdc: Use phase in fdctrl_write_data()

[Peter Maydell]

On 19 May 2015 at 16:35, Kevin Wolf <address@hidden> wrote:
> Instead of relying on a flag in the MSR to distinguish controller phases,
> use the explicit phase that we store now. Assertions of the right MSR
> flags are added.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  hw/block/fdc.c | 67 
> ++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 28 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 4d4868e..a13e0ce 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2001,8 +2001,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t 
> value)
>          return;
>      }
>      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> -    /* Is it write command time ? */
> -    if (fdctrl->msr & FD_MSR_NONDMA) {
> +
> +    switch (fdctrl->phase) {
> +    case FD_PHASE_EXECUTION:
> +        assert(fdctrl->msr & FD_MSR_NONDMA);

I'm confused now. Is the fdctrl->phase really modelling hidden
(non-guest-visible) state in the floppy controller, or is it
a cache of the state that's surfaced in the msr and other bits?

This assert looks pretty weird either way. If the former,
then we should presumably be behaving however the hardware
behaves when the register bits and the phase get out of sync,
which isn't going to include assert()ing. (And assert()
provides no protection against bad guests because it might
get compiled out). On the other hand, if it's the latter
then it seems too easy for the two to get out of sync due
to code bugs. The usual approach there is either to
(1) have a function that updates everything at once or
(2) don't actually keep the register bits in fdctrl->msr
but just calculate them from fdctrl->phase on register
reads.

thanks
-- PMM