diff mbox

[4/8] fdc: Use phase in fdctrl_write_data()

Message ID 1432049762-2184-5-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf May 19, 2015, 3:35 p.m. UTC 
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 <kwolf@redhat.com>
---
 hw/block/fdc.c | 67 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 28 deletions(-)

Comments

John Snow May 19, 2015, 8:39 p.m. UTC | #1 
On 05/19/2015 11:35 AM, Kevin Wolf 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 <kwolf@redhat.com>
> ---
>  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

[snip]

Reviewed-by: John Snow <jsnow@redhat.com>
Peter Maydell May 19, 2015, 8:52 p.m. UTC | #2 
On 19 May 2015 at 16:35, Kevin Wolf <kwolf@redhat.com> 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 <kwolf@redhat.com>
> ---
>  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
diff mbox

Patch

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);
         /* FIFO data write */
         pos = fdctrl->data_pos++;
         pos %= FD_SECTOR_LEN;
@@ -2014,12 +2016,12 @@  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
                 < 0) {
                 FLOPPY_DPRINTF("error writing sector %d\n",
                                fd_sector(cur_drv));
-                return;
+                break;
             }
             if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
                 FLOPPY_DPRINTF("error seeking to next sector %d\n",
                                fd_sector(cur_drv));
-                return;
+                break;
             }
         }
         /* Switch from transfer mode to status mode
@@ -2027,33 +2029,42 @@  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
          */
         if (fdctrl->data_pos == fdctrl->data_len)
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-        return;
-    }
-    if (fdctrl->data_pos == 0) {
-        /* Command */
-        pos = command_to_handler[value & 0xff];
-        FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
-        fdctrl->data_len = handlers[pos].parameters + 1;
-        fdctrl->msr |= FD_MSR_CMDBUSY;
-    }
+        break;
 
-    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
-    pos = fdctrl->data_pos++;
-    pos %= FD_SECTOR_LEN;
-    fdctrl->fifo[pos] = value;
-    if (fdctrl->data_pos == fdctrl->data_len) {
-        /* We now have all parameters
-         * and will be able to treat the command
-         */
-        fdctrl->phase = FD_PHASE_EXECUTION;
-        if (fdctrl->data_state & FD_STATE_FORMAT) {
-            fdctrl_format_sector(fdctrl);
-            return;
+    case FD_PHASE_COMMAND:
+        assert(!(fdctrl->msr & FD_MSR_NONDMA));
+
+        if (fdctrl->data_pos == 0) {
+            /* Command */
+            pos = command_to_handler[value & 0xff];
+            FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
+            fdctrl->data_len = handlers[pos].parameters + 1;
+            fdctrl->msr |= FD_MSR_CMDBUSY;
+        }
+
+        FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
+        pos = fdctrl->data_pos++;
+        pos %= FD_SECTOR_LEN;
+        fdctrl->fifo[pos] = value;
+        if (fdctrl->data_pos == fdctrl->data_len) {
+            /* We now have all parameters
+             * and will be able to treat the command
+             */
+            fdctrl->phase = FD_PHASE_EXECUTION;
+            if (fdctrl->data_state & FD_STATE_FORMAT) {
+                fdctrl_format_sector(fdctrl);
+                break;
+            }
+
+            pos = command_to_handler[fdctrl->fifo[0] & 0xff];
+            FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name);
+            (*handlers[pos].handler)(fdctrl, handlers[pos].direction);
         }
+        break;
 
-        pos = command_to_handler[fdctrl->fifo[0] & 0xff];
-        FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name);
-        (*handlers[pos].handler)(fdctrl, handlers[pos].direction);
+    case FD_PHASE_RESULT:
+    default:
+        abort();
     }
 }