[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH 4/8] fdc: Use phase in fdctrl_write_data()
Date: Tue, 19 May 2015 21:52:28 +0100

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) {
> +        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

-- PMM

reply via email to

[Prev in Thread] Current Thread [Next in Thread]