[Qemu-devel] [PATCH 0/8] fdc: Clean up and fix command processing

[Qemu-devel] [PATCH 0/8] fdc: Clean up and fix command processing

[Kevin Wolf]

This series fixes the real bug that caused CVE-2015-3456, and does some
cleanup in the FIFO access functions to make the command processing more
obvious.

Kevin Wolf (8):
  fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase()
  fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase()
  fdc: Introduce fdctrl->phase
  fdc: Use phase in fdctrl_write_data()
  fdc: Code cleanup in fdctrl_write_data()
  fdc: Disentangle phases in fdctrl_read_data()
  fdc: Fix MSR.RQM flag
  fdc-test: Test state for existing cases more thoroughly

 hw/block/fdc.c   | 235 +++++++++++++++++++++++++++++++++++++------------------
 tests/fdc-test.c |  34 ++++++++
 2 files changed, 192 insertions(+), 77 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase()

[Kevin Wolf]

What all callers of fdctrl_reset_fifo() really want to do is to start
the command phase, where writes to the data port initiate a new command.

The function doesn't only clear the FIFO, but also sets up the state so
that a new command can be received. Rename it to reflect this.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d8a8edd..9f95ace 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -324,7 +324,7 @@ static void fd_revalidate(FDrive *drv)
 /* Intel 82078 floppy disk controller emulation          */
 
 static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
-static void fdctrl_reset_fifo(FDCtrl *fdctrl);
+static void fdctrl_to_command_phase(FDCtrl *fdctrl);
 static int fdctrl_transfer_handler (void *opaque, int nchan,
                                     int dma_pos, int dma_len);
 static void fdctrl_raise_irq(FDCtrl *fdctrl);
@@ -918,7 +918,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
     fdctrl->data_dir = FD_DIR_WRITE;
     for (i = 0; i < MAX_FD; i++)
         fd_recalibrate(&fdctrl->drives[i]);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     if (do_irq) {
         fdctrl->status0 |= FD_SR0_RDYCHG;
         fdctrl_raise_irq(fdctrl);
@@ -1134,8 +1134,8 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl)
     return retval;
 }
 
-/* FIFO state control */
-static void fdctrl_reset_fifo(FDCtrl *fdctrl)
+/* Clear the FIFO and update the state for receiving the next command */
+static void fdctrl_to_command_phase(FDCtrl *fdctrl)
 {
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
@@ -1533,7 +1533,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         if (fdctrl->msr & FD_MSR_NONDMA) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
         } else {
-            fdctrl_reset_fifo(fdctrl);
+            fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }
     }
@@ -1667,7 +1667,7 @@ static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
     fdctrl->config = fdctrl->fifo[11];
     fdctrl->precomp_trk = fdctrl->fifo[12];
     fdctrl->pwrd = fdctrl->fifo[13];
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
@@ -1746,7 +1746,7 @@ static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction)
     else
         fdctrl->dor |= FD_DOR_DMAEN;
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
@@ -1772,7 +1772,7 @@ static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     fd_recalibrate(cur_drv);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* Raise Interrupt */
     fdctrl->status0 |= FD_SR0_SEEK;
     fdctrl_raise_irq(fdctrl);
@@ -1808,7 +1808,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
 
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* The seek command just sends step pulses to the drive and doesn't care if
      * there is a medium inserted of if it's banging the head against the drive.
      */
@@ -1825,7 +1825,7 @@ static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)
     if (fdctrl->fifo[1] & 0x80)
         cur_drv->perpendicular = fdctrl->fifo[1] & 0x7;
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)
@@ -1833,7 +1833,7 @@ static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)
     fdctrl->config = fdctrl->fifo[2];
     fdctrl->precomp_trk =  fdctrl->fifo[3];
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
@@ -1846,7 +1846,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
 {
     /* No result back */
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
 }
 
 static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)
@@ -1864,7 +1864,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[3] = 0;
             fdctrl_set_fifo(fdctrl, 4);
         } else {
-            fdctrl_reset_fifo(fdctrl);
+            fdctrl_to_command_phase(fdctrl);
         }
     } else if (fdctrl->data_len > 7) {
         /* ERROR */
@@ -1887,7 +1887,7 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction)
         fd_seek(cur_drv, cur_drv->head,
                 cur_drv->track + fdctrl->fifo[2], cur_drv->sect, 1);
     }
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* Raise Interrupt */
     fdctrl->status0 |= FD_SR0_SEEK;
     fdctrl_raise_irq(fdctrl);
@@ -1905,7 +1905,7 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
         fd_seek(cur_drv, cur_drv->head,
                 cur_drv->track - fdctrl->fifo[2], cur_drv->sect, 1);
     }
-    fdctrl_reset_fifo(fdctrl);
+    fdctrl_to_command_phase(fdctrl);
     /* Raise Interrupt */
     fdctrl->status0 |= FD_SR0_SEEK;
     fdctrl_raise_irq(fdctrl);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/8] fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase()

[John Snow]

On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> What all callers of fdctrl_reset_fifo() really want to do is to start
> the command phase, where writes to the data port initiate a new command.
> 
> The function doesn't only clear the FIFO, but also sets up the state so
> that a new command can be received. Rename it to reflect this.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index d8a8edd..9f95ace 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -324,7 +324,7 @@ static void fd_revalidate(FDrive *drv)
>  /* Intel 82078 floppy disk controller emulation          */
>  
>  static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
> -static void fdctrl_reset_fifo(FDCtrl *fdctrl);
> +static void fdctrl_to_command_phase(FDCtrl *fdctrl);
>  static int fdctrl_transfer_handler (void *opaque, int nchan,
>                                      int dma_pos, int dma_len);
>  static void fdctrl_raise_irq(FDCtrl *fdctrl);
> @@ -918,7 +918,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
>      fdctrl->data_dir = FD_DIR_WRITE;
>      for (i = 0; i < MAX_FD; i++)
>          fd_recalibrate(&fdctrl->drives[i]);
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>      if (do_irq) {
>          fdctrl->status0 |= FD_SR0_RDYCHG;
>          fdctrl_raise_irq(fdctrl);
> @@ -1134,8 +1134,8 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl)
>      return retval;
>  }
>  
> -/* FIFO state control */
> -static void fdctrl_reset_fifo(FDCtrl *fdctrl)
> +/* Clear the FIFO and update the state for receiving the next command */
> +static void fdctrl_to_command_phase(FDCtrl *fdctrl)
>  {
>      fdctrl->data_dir = FD_DIR_WRITE;
>      fdctrl->data_pos = 0;
> @@ -1533,7 +1533,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>          if (fdctrl->msr & FD_MSR_NONDMA) {
>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
>          } else {
> -            fdctrl_reset_fifo(fdctrl);
> +            fdctrl_to_command_phase(fdctrl);
>              fdctrl_reset_irq(fdctrl);
>          }
>      }
> @@ -1667,7 +1667,7 @@ static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
>      fdctrl->config = fdctrl->fifo[11];
>      fdctrl->precomp_trk = fdctrl->fifo[12];
>      fdctrl->pwrd = fdctrl->fifo[13];
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>  }
>  
>  static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
> @@ -1746,7 +1746,7 @@ static void fdctrl_handle_specify(FDCtrl *fdctrl, int direction)
>      else
>          fdctrl->dor |= FD_DOR_DMAEN;
>      /* No result back */
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>  }
>  
>  static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
> @@ -1772,7 +1772,7 @@ static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
>      SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
>      cur_drv = get_cur_drv(fdctrl);
>      fd_recalibrate(cur_drv);
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>      /* Raise Interrupt */
>      fdctrl->status0 |= FD_SR0_SEEK;
>      fdctrl_raise_irq(fdctrl);
> @@ -1808,7 +1808,7 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>  
>      SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
>      cur_drv = get_cur_drv(fdctrl);
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>      /* The seek command just sends step pulses to the drive and doesn't care if
>       * there is a medium inserted of if it's banging the head against the drive.
>       */
> @@ -1825,7 +1825,7 @@ static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)
>      if (fdctrl->fifo[1] & 0x80)
>          cur_drv->perpendicular = fdctrl->fifo[1] & 0x7;
>      /* No result back */
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>  }
>  
>  static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)
> @@ -1833,7 +1833,7 @@ static void fdctrl_handle_configure(FDCtrl *fdctrl, int direction)
>      fdctrl->config = fdctrl->fifo[2];
>      fdctrl->precomp_trk =  fdctrl->fifo[3];
>      /* No result back */
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>  }
>  
>  static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
> @@ -1846,7 +1846,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
>  static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
>  {
>      /* No result back */
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>  }
>  
>  static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direction)
> @@ -1864,7 +1864,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
>              fdctrl->fifo[3] = 0;
>              fdctrl_set_fifo(fdctrl, 4);
>          } else {
> -            fdctrl_reset_fifo(fdctrl);
> +            fdctrl_to_command_phase(fdctrl);
>          }
>      } else if (fdctrl->data_len > 7) {
>          /* ERROR */
> @@ -1887,7 +1887,7 @@ static void fdctrl_handle_relative_seek_in(FDCtrl *fdctrl, int direction)
>          fd_seek(cur_drv, cur_drv->head,
>                  cur_drv->track + fdctrl->fifo[2], cur_drv->sect, 1);
>      }
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>      /* Raise Interrupt */
>      fdctrl->status0 |= FD_SR0_SEEK;
>      fdctrl_raise_irq(fdctrl);
> @@ -1905,7 +1905,7 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
>          fd_seek(cur_drv, cur_drv->head,
>                  cur_drv->track - fdctrl->fifo[2], cur_drv->sect, 1);
>      }
> -    fdctrl_reset_fifo(fdctrl);
> +    fdctrl_to_command_phase(fdctrl);
>      /* Raise Interrupt */
>      fdctrl->status0 |= FD_SR0_SEEK;
>      fdctrl_raise_irq(fdctrl);
> 

why 'to' instead of 'start' or 'init'? It seems weird to describe it in
a third-party descriptive way instead of with the imperative.

Bike-shedding aside:

Reviewed-by: John Snow <jsnow@redhat.com>

[Qemu-devel] [PATCH 2/8] fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase()

[Kevin Wolf]

What callers really do with this function is to switch from execution
phase (including data transfers) to result phase where the guest can
read out one or more status bytes from the FIFO (the number depends on
the command).

Rename the function accordingly.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9f95ace..8c41434 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1142,8 +1142,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
 }
 
-/* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len)
+/* Update the state to allow the guest to read out the command status.
+ * @fifo_len is the number of result bytes to be read out. */
+static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
@@ -1157,7 +1158,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
     qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n",
                   fdctrl->fifo[0]);
     fdctrl->fifo[0] = FD_SR0_INVCMD;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 /* Seek to next sector
@@ -1238,7 +1239,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
 
-    fdctrl_set_fifo(fdctrl, 7);
+    fdctrl_to_result_phase(fdctrl, 7);
     fdctrl_raise_irq(fdctrl);
 }
 
@@ -1606,7 +1607,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
 {
     fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
     fdctrl->fifo[0] = fdctrl->lock << 4;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
@@ -1631,20 +1632,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
         (cur_drv->perpendicular << 2);
     fdctrl->fifo[8] = fdctrl->config;
     fdctrl->fifo[9] = fdctrl->precomp_trk;
-    fdctrl_set_fifo(fdctrl, 10);
+    fdctrl_to_result_phase(fdctrl, 10);
 }
 
 static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 {
     /* Controller's version */
     fdctrl->fifo[0] = fdctrl->version;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
 {
     fdctrl->fifo[0] = 0x41; /* Stepping 1 */
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
@@ -1697,7 +1698,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
     fdctrl->fifo[12] = fdctrl->pwrd;
     fdctrl->fifo[13] = 0;
     fdctrl->fifo[14] = 0;
-    fdctrl_set_fifo(fdctrl, 15);
+    fdctrl_to_result_phase(fdctrl, 15);
 }
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
@@ -1762,7 +1763,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
         (cur_drv->head << 2) |
         GET_CUR_DRV(fdctrl) |
         0x28;
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
@@ -1788,7 +1789,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
         fdctrl->reset_sensei--;
     } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
         fdctrl->fifo[0] = FD_SR0_INVCMD;
-        fdctrl_set_fifo(fdctrl, 1);
+        fdctrl_to_result_phase(fdctrl, 1);
         return;
     } else {
         fdctrl->fifo[0] =
@@ -1797,7 +1798,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-    fdctrl_set_fifo(fdctrl, 2);
+    fdctrl_to_result_phase(fdctrl, 2);
     fdctrl_reset_irq(fdctrl);
     fdctrl->status0 = FD_SR0_RDYCHG;
 }
@@ -1840,7 +1841,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 {
     fdctrl->pwrd = fdctrl->fifo[1];
     fdctrl->fifo[0] = fdctrl->fifo[1];
-    fdctrl_set_fifo(fdctrl, 1);
+    fdctrl_to_result_phase(fdctrl, 1);
 }
 
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
@@ -1862,7 +1863,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
-            fdctrl_set_fifo(fdctrl, 4);
+            fdctrl_to_result_phase(fdctrl, 4);
         } else {
             fdctrl_to_command_phase(fdctrl);
         }
@@ -1870,7 +1871,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
         /* ERROR */
         fdctrl->fifo[0] = 0x80 |
             (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
-        fdctrl_set_fifo(fdctrl, 1);
+        fdctrl_to_result_phase(fdctrl, 1);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/8] fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase()

[John Snow]

On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> What callers really do with this function is to switch from execution
> phase (including data transfers) to result phase where the guest can
> read out one or more status bytes from the FIFO (the number depends on
> the command).
> 
> Rename the function accordingly.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 9f95ace..8c41434 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1142,8 +1142,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
>      fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
>  }
>  
> -/* Set FIFO status for the host to read */
> -static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len)
> +/* Update the state to allow the guest to read out the command status.
> + * @fifo_len is the number of result bytes to be read out. */
> +static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len)
>  {
>      fdctrl->data_dir = FD_DIR_READ;
>      fdctrl->data_len = fifo_len;
> @@ -1157,7 +1158,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
>      qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n",
>                    fdctrl->fifo[0]);
>      fdctrl->fifo[0] = FD_SR0_INVCMD;
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  /* Seek to next sector
> @@ -1238,7 +1239,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>      fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
>      fdctrl->msr &= ~FD_MSR_NONDMA;
>  
> -    fdctrl_set_fifo(fdctrl, 7);
> +    fdctrl_to_result_phase(fdctrl, 7);
>      fdctrl_raise_irq(fdctrl);
>  }
>  
> @@ -1606,7 +1607,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
>  {
>      fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
>      fdctrl->fifo[0] = fdctrl->lock << 4;
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
> @@ -1631,20 +1632,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
>          (cur_drv->perpendicular << 2);
>      fdctrl->fifo[8] = fdctrl->config;
>      fdctrl->fifo[9] = fdctrl->precomp_trk;
> -    fdctrl_set_fifo(fdctrl, 10);
> +    fdctrl_to_result_phase(fdctrl, 10);
>  }
>  
>  static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
>  {
>      /* Controller's version */
>      fdctrl->fifo[0] = fdctrl->version;
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
>  {
>      fdctrl->fifo[0] = 0x41; /* Stepping 1 */
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
> @@ -1697,7 +1698,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
>      fdctrl->fifo[12] = fdctrl->pwrd;
>      fdctrl->fifo[13] = 0;
>      fdctrl->fifo[14] = 0;
> -    fdctrl_set_fifo(fdctrl, 15);
> +    fdctrl_to_result_phase(fdctrl, 15);
>  }
>  
>  static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
> @@ -1762,7 +1763,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
>          (cur_drv->head << 2) |
>          GET_CUR_DRV(fdctrl) |
>          0x28;
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
> @@ -1788,7 +1789,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
>          fdctrl->reset_sensei--;
>      } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
>          fdctrl->fifo[0] = FD_SR0_INVCMD;
> -        fdctrl_set_fifo(fdctrl, 1);
> +        fdctrl_to_result_phase(fdctrl, 1);
>          return;
>      } else {
>          fdctrl->fifo[0] =
> @@ -1797,7 +1798,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
>      }
>  
>      fdctrl->fifo[1] = cur_drv->track;
> -    fdctrl_set_fifo(fdctrl, 2);
> +    fdctrl_to_result_phase(fdctrl, 2);
>      fdctrl_reset_irq(fdctrl);
>      fdctrl->status0 = FD_SR0_RDYCHG;
>  }
> @@ -1840,7 +1841,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
>  {
>      fdctrl->pwrd = fdctrl->fifo[1];
>      fdctrl->fifo[0] = fdctrl->fifo[1];
> -    fdctrl_set_fifo(fdctrl, 1);
> +    fdctrl_to_result_phase(fdctrl, 1);
>  }
>  
>  static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
> @@ -1862,7 +1863,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
>              fdctrl->fifo[0] = fdctrl->fifo[1];
>              fdctrl->fifo[2] = 0;
>              fdctrl->fifo[3] = 0;
> -            fdctrl_set_fifo(fdctrl, 4);
> +            fdctrl_to_result_phase(fdctrl, 4);
>          } else {
>              fdctrl_to_command_phase(fdctrl);
>          }
> @@ -1870,7 +1871,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
>          /* ERROR */
>          fdctrl->fifo[0] = 0x80 |
>              (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
> -        fdctrl_set_fifo(fdctrl, 1);
> +        fdctrl_to_result_phase(fdctrl, 1);
>      }
>  }
>  
> 

Similar bike-shedding comment here to match patch #1, but that won't
stop this:

Reviewed-by: John Snow <jsnow@redhat.com>

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

[Kevin Wolf]

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;
     /* Controller's identification */
     uint8_t version;
     /* HW */
@@ -1137,6 +1162,7 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl)
 /* Clear the FIFO and update the state for receiving the next command */
 static void fdctrl_to_command_phase(FDCtrl *fdctrl)
 {
+    fdctrl->phase = FD_PHASE_COMMAND;
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
@@ -1146,6 +1172,7 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
  * @fifo_len is the number of result bytes to be read out. */
 static void fdctrl_to_result_phase(FDCtrl *fdctrl, int fifo_len)
 {
+    fdctrl->phase = FD_PHASE_RESULT;
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
     fdctrl->data_pos = 0;
@@ -1912,6 +1939,9 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
     fdctrl_raise_irq(fdctrl);
 }
 
+/*
+ * Handlers for the execution phase of each command
+ */
 static const struct {
     uint8_t value;
     uint8_t mask;
@@ -2015,6 +2045,7 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
         /* 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;
-- 
1.8.3.1

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

[John Snow]

On 05/19/2015 11:35 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 <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

[snip]

Reviewed-by: John Snow <jsnow@redhat.com>

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

[Peter Maydell]

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?

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

-- PMM

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

[John Snow]

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

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

[Peter Maydell]

On 19 May 2015 at 21:52, John Snow <jsnow@redhat.com> wrote:
> Hmm, I think this is not purely derived state because the flags are not
> necessarily sufficient for regenerating that state.

Yeah, if there's genuinely an underlying state machine that's
not completely visible in registers you need to actually model it.
You should probably then model the register bits by calculating
them from the state rather than by changing them as you go along
in parallel with moving the state machine around.

-- PMM

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

[Kevin Wolf]

Am 19.05.2015 um 22:57 hat Peter Maydell geschrieben:
> On 19 May 2015 at 21:52, John Snow <jsnow@redhat.com> wrote:
> > Hmm, I think this is not purely derived state because the flags are not
> > necessarily sufficient for regenerating that state.
> 
> Yeah, if there's genuinely an underlying state machine that's
> not completely visible in registers you need to actually model it.
> You should probably then model the register bits by calculating
> them from the state rather than by changing them as you go along
> in parallel with moving the state machine around.

I think the combination of registers is actually enough to reconstruct
what state we're in, so it is derived (otherwise I would have fixed a
bug that I'm not aware of). Adding logic to derive it in a post-load
handler should be good enough.

The reason why I want to have it explicit anyway is that having the
information spread across multiple registers is non-obvious and
error-prone.

Before this eries, most places only check some bits instead of the full
combination. And they are correct in the sense that some states are
never expected to happen in that specific place - but if it happens
anyway (i.e. there is a bug) without the patches something undefined
happens, whereas with the patches we catch it.

And then there's the readability of the code, too, of course.

Kevin

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

[Peter Maydell]

On 20 May 2015 at 08:54, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 19.05.2015 um 22:57 hat Peter Maydell geschrieben:
>> Yeah, if there's genuinely an underlying state machine that's
>> not completely visible in registers you need to actually model it.
>> You should probably then model the register bits by calculating
>> them from the state rather than by changing them as you go along
>> in parallel with moving the state machine around.
>
> I think the combination of registers is actually enough to reconstruct
> what state we're in, so it is derived (otherwise I would have fixed a
> bug that I'm not aware of). Adding logic to derive it in a post-load
> handler should be good enough.

That handles migration, which is good. But I still think that
storing the same information in two places in the device
state (phase field and the register fields) is error-prone.
If we want to switch to having a phase field we should calculate
the relevant register bits on demand based on the phase, rather
than keeping both copies of the state in sync manually.

thanks
-- PMM

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

[Kevin Wolf]

Am 20.05.2015 um 10:06 hat Peter Maydell geschrieben:
> On 20 May 2015 at 08:54, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 19.05.2015 um 22:57 hat Peter Maydell geschrieben:
> >> Yeah, if there's genuinely an underlying state machine that's
> >> not completely visible in registers you need to actually model it.
> >> You should probably then model the register bits by calculating
> >> them from the state rather than by changing them as you go along
> >> in parallel with moving the state machine around.
> >
> > I think the combination of registers is actually enough to reconstruct
> > what state we're in, so it is derived (otherwise I would have fixed a
> > bug that I'm not aware of). Adding logic to derive it in a post-load
> > handler should be good enough.
> 
> That handles migration, which is good. But I still think that
> storing the same information in two places in the device
> state (phase field and the register fields) is error-prone.

That's actually my point. The registers are accessed everywhere in the
code, whereas phase transitions are in very few well-defined places
(there are exactly four of them, iirc). If they get out of sync, chances
are that the bug is in the register value, not in the phase. When we
know what phase we're in, we can assert the bits and actually catch such
bugs.

> If we want to switch to having a phase field we should calculate
> the relevant register bits on demand based on the phase, rather
> than keeping both copies of the state in sync manually.

That doesn't work, unfortunately. Some register bits imply a specific
phase (assuming correct code), but you can't derive the exact bits just
from the phase.

In fact, I'm not even sure if we would still be able to derive the phase
from registers alone if we ever converted the FDC to use AIO instead of
synchronous I/O.

Kevin

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

[Peter Maydell]

On 20 May 2015 at 09:43, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 20.05.2015 um 10:06 hat Peter Maydell geschrieben:
>> That handles migration, which is good. But I still think that
>> storing the same information in two places in the device
>> state (phase field and the register fields) is error-prone.
>
> That's actually my point. The registers are accessed everywhere in the
> code, whereas phase transitions are in very few well-defined places
> (there are exactly four of them, iirc). If they get out of sync, chances
> are that the bug is in the register value, not in the phase. When we
> know what phase we're in, we can assert the bits and actually catch such
> bugs.
>
>> If we want to switch to having a phase field we should calculate
>> the relevant register bits on demand based on the phase, rather
>> than keeping both copies of the state in sync manually.
>
> That doesn't work, unfortunately. Some register bits imply a specific
> phase (assuming correct code), but you can't derive the exact bits just
> from the phase.

Having now dug out a copy of the 82078 spec, I agree that the state
isn't derivable purely from the register values in the general case.
The controller clearly has a state machine internally but it doesn't
surface that in the register state except indirectly.

-- PMM

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

[John Snow]

On 05/20/2015 05:24 AM, Peter Maydell wrote:
> On 20 May 2015 at 09:43, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 20.05.2015 um 10:06 hat Peter Maydell geschrieben:
>>> That handles migration, which is good. But I still think that
>>> storing the same information in two places in the device
>>> state (phase field and the register fields) is error-prone.
>>
>> That's actually my point. The registers are accessed everywhere in the
>> code, whereas phase transitions are in very few well-defined places
>> (there are exactly four of them, iirc). If they get out of sync, chances
>> are that the bug is in the register value, not in the phase. When we
>> know what phase we're in, we can assert the bits and actually catch such
>> bugs.
>>
>>> If we want to switch to having a phase field we should calculate
>>> the relevant register bits on demand based on the phase, rather
>>> than keeping both copies of the state in sync manually.
>>
>> That doesn't work, unfortunately. Some register bits imply a specific
>> phase (assuming correct code), but you can't derive the exact bits just
>> from the phase.
> 
> Having now dug out a copy of the 82078 spec, I agree that the state
> isn't derivable purely from the register values in the general case.
> The controller clearly has a state machine internally but it doesn't
> surface that in the register state except indirectly.
> 
> -- PMM
> 

So even if /currently/ we can reconstitute it from the register values,
we may eventually be unable to.

post_load will work for now, but I fear the case (in ten years) when
someone else cleans up FDC code but fails to realize that the phase is
not explicitly migrated.

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

[Peter Maydell]

On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
> So even if /currently/ we can reconstitute it from the register values,
> we may eventually be unable to.
>
> post_load will work for now, but I fear the case (in ten years) when
> someone else cleans up FDC code but fails to realize that the phase is
> not explicitly migrated.

I assumed we would only do the post-load thing if the
source for migration doesn't migrate phase (however we
phrase that in a vmstate struct).

-- PMM

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

[Kevin Wolf]

Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
> On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
> > So even if /currently/ we can reconstitute it from the register values,
> > we may eventually be unable to.
> >
> > post_load will work for now, but I fear the case (in ten years) when
> > someone else cleans up FDC code but fails to realize that the phase is
> > not explicitly migrated.
> 
> I assumed we would only do the post-load thing if the
> source for migration doesn't migrate phase (however we
> phrase that in a vmstate struct).

I think if we extend the VMState, then we have two options. I'm not
exactly sure how they work in details, but I'll try an educated guess -
Juan, please correct me if I'm wrong:

1. We increase version_id and add the new field at the end. This breaks
   backwards migration; on forward migration the new field would be
   initialised with 0 and a post_load handler could check the old
   version_id to calculate the phase from register bits.

2. We add a subsection for the phase, and declare one phase to be the
   default (most likely the command phase) for which a subsection is not
   sent. In this case, the destination can't distinguish between a
   missing subsection because the source was running an old qemu or
   because it is the default phase. Unclear whether post_load should
   recalculate the phase or not.

If the above is correct, I'm afraid that the third option - which
doesn't address John's (valid) concerns - would be the most reasonable:

3. Don't add any VMState fields now and just do the post_load handler.
   If we ever extend fdc in a way that makes it impossible to
   reconstruct the phase from other migrated state, we add a subsection
   that is only sent in cases where it differs from the reconstructed
   value.

Kevin

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

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
> > On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
> > > So even if /currently/ we can reconstitute it from the register values,
> > > we may eventually be unable to.
> > >
> > > post_load will work for now, but I fear the case (in ten years) when
> > > someone else cleans up FDC code but fails to realize that the phase is
> > > not explicitly migrated.
> > 
> > I assumed we would only do the post-load thing if the
> > source for migration doesn't migrate phase (however we
> > phrase that in a vmstate struct).
> 
> I think if we extend the VMState, then we have two options. I'm not
> exactly sure how they work in details, but I'll try an educated guess -
> Juan, please correct me if I'm wrong:
> 
> 1. We increase version_id and add the new field at the end. This breaks
>    backwards migration; on forward migration the new field would be
>    initialised with 0 and a post_load handler could check the old
>    version_id to calculate the phase from register bits.
> 
> 2. We add a subsection for the phase, and declare one phase to be the
>    default (most likely the command phase) for which a subsection is not
>    sent. In this case, the destination can't distinguish between a
>    missing subsection because the source was running an old qemu or
>    because it is the default phase. Unclear whether post_load should
>    recalculate the phase or not.

There's another variant;  add a subsection and always send it for new machine
types.

> If the above is correct, I'm afraid that the third option - which
> doesn't address John's (valid) concerns - would be the most reasonable:
> 
> 3. Don't add any VMState fields now and just do the post_load handler.
>    If we ever extend fdc in a way that makes it impossible to
>    reconstruct the phase from other migrated state, we add a subsection
>    that is only sent in cases where it differs from the reconstructed
>    value.

Dave

> 
> Kevin
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Peter Maydell]

On 21 May 2015 at 10:42, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
>> On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
>> > So even if /currently/ we can reconstitute it from the register values,
>> > we may eventually be unable to.
>> >
>> > post_load will work for now, but I fear the case (in ten years) when
>> > someone else cleans up FDC code but fails to realize that the phase is
>> > not explicitly migrated.
>>
>> I assumed we would only do the post-load thing if the
>> source for migration doesn't migrate phase (however we
>> phrase that in a vmstate struct).
>
> I think if we extend the VMState, then we have two options. I'm not
> exactly sure how they work in details, but I'll try an educated guess -
> Juan, please correct me if I'm wrong:
>
> 1. We increase version_id and add the new field at the end. This breaks
>    backwards migration; on forward migration the new field would be
>    initialised with 0 and a post_load handler could check the old
>    version_id to calculate the phase from register bits.
>
> 2. We add a subsection for the phase, and declare one phase to be the
>    default (most likely the command phase) for which a subsection is not
>    sent. In this case, the destination can't distinguish between a
>    missing subsection because the source was running an old qemu or
>    because it is the default phase. Unclear whether post_load should
>    recalculate the phase or not.

2b add a subsection, send the subsection always in new qemu,
if receiving from an old qemu then calculate the phase from
the register fields in post-load

That's like 1 but just using a subsection rather than a new field.
(I have a vague recollection that distros doing backports prefer
subsections over increment-version-id-and-add-field).

> If the above is correct, I'm afraid that the third option - which
> doesn't address John's (valid) concerns - would be the most reasonable:
>
> 3. Don't add any VMState fields now and just do the post_load handler.
>    If we ever extend fdc in a way that makes it impossible to
>    reconstruct the phase from other migrated state, we add a subsection
>    that is only sent in cases where it differs from the reconstructed
>    value.

I'm definitely against this one. State should always be
sent in migration, and not doing that once we've added
it to the device struct is a bug.

-- PMM

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

[Kevin Wolf]

Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:
> On 21 May 2015 at 10:42, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
> >> On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
> >> > So even if /currently/ we can reconstitute it from the register values,
> >> > we may eventually be unable to.
> >> >
> >> > post_load will work for now, but I fear the case (in ten years) when
> >> > someone else cleans up FDC code but fails to realize that the phase is
> >> > not explicitly migrated.
> >>
> >> I assumed we would only do the post-load thing if the
> >> source for migration doesn't migrate phase (however we
> >> phrase that in a vmstate struct).
> >
> > I think if we extend the VMState, then we have two options. I'm not
> > exactly sure how they work in details, but I'll try an educated guess -
> > Juan, please correct me if I'm wrong:
> >
> > 1. We increase version_id and add the new field at the end. This breaks
> >    backwards migration; on forward migration the new field would be
> >    initialised with 0 and a post_load handler could check the old
> >    version_id to calculate the phase from register bits.
> >
> > 2. We add a subsection for the phase, and declare one phase to be the
> >    default (most likely the command phase) for which a subsection is not
> >    sent. In this case, the destination can't distinguish between a
> >    missing subsection because the source was running an old qemu or
> >    because it is the default phase. Unclear whether post_load should
> >    recalculate the phase or not.
> 
> 2b add a subsection, send the subsection always in new qemu,
> if receiving from an old qemu then calculate the phase from
> the register fields in post-load
> 
> That's like 1 but just using a subsection rather than a new field.
> (I have a vague recollection that distros doing backports prefer
> subsections over increment-version-id-and-add-field).

Hm, okay. The post_load handler doesn't have the version id then to
check. If a subsection isn't present, are its fields initialised as 0?
Or what would be the right way to check whether the subsection was
there?

Any pointers to device that do something similar?

> > If the above is correct, I'm afraid that the third option - which
> > doesn't address John's (valid) concerns - would be the most reasonable:
> >
> > 3. Don't add any VMState fields now and just do the post_load handler.
> >    If we ever extend fdc in a way that makes it impossible to
> >    reconstruct the phase from other migrated state, we add a subsection
> >    that is only sent in cases where it differs from the reconstructed
> >    value.
> 
> I'm definitely against this one. State should always be
> sent in migration, and not doing that once we've added
> it to the device struct is a bug.

On second thought, I could actually add that subsection now and write
the .needed callback such that it checks whether the reconstructed phase
matches the actual one. This condition would always evaluate as false
today, but if we ever change things so that the phase doesn't match what
current qemu would think it is, you automatically get the subsection and
things Just Work (TM).

This also wouldn't break backwards migration for now, and only break it
in future cases if it would really result in a broken device on the
destination.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:
>> On 21 May 2015 at 10:42, Kevin Wolf <kwolf@redhat.com> wrote:
>> > Am 20.05.2015 um 14:07 hat Peter Maydell geschrieben:
>> >> On 20 May 2015 at 12:55, John Snow <jsnow@redhat.com> wrote:
>> >> > So even if /currently/ we can reconstitute it from the register values,
>> >> > we may eventually be unable to.
>> >> >
>> >> > post_load will work for now, but I fear the case (in ten years) when
>> >> > someone else cleans up FDC code but fails to realize that the phase is
>> >> > not explicitly migrated.
>> >>
>> >> I assumed we would only do the post-load thing if the
>> >> source for migration doesn't migrate phase (however we
>> >> phrase that in a vmstate struct).
>> >
>> > I think if we extend the VMState, then we have two options. I'm not
>> > exactly sure how they work in details, but I'll try an educated guess -
>> > Juan, please correct me if I'm wrong:
>> >
>> > 1. We increase version_id and add the new field at the end. This breaks
>> >    backwards migration; on forward migration the new field would be
>> >    initialised with 0 and a post_load handler could check the old
>> >    version_id to calculate the phase from register bits.
>> >
>> > 2. We add a subsection for the phase, and declare one phase to be the
>> >    default (most likely the command phase) for which a subsection is not
>> >    sent. In this case, the destination can't distinguish between a
>> >    missing subsection because the source was running an old qemu or
>> >    because it is the default phase. Unclear whether post_load should
>> >    recalculate the phase or not.
>> 
>> 2b add a subsection, send the subsection always in new qemu,
>> if receiving from an old qemu then calculate the phase from
>> the register fields in post-load

Doesn't this break new -> old migration?

>> That's like 1 but just using a subsection rather than a new field.
>> (I have a vague recollection that distros doing backports prefer
>> subsections over increment-version-id-and-add-field).
>
> Hm, okay. The post_load handler doesn't have the version id then to
> check. If a subsection isn't present, are its fields initialised as 0?
> Or what would be the right way to check whether the subsection was
> there?
>
> Any pointers to device that do something similar?
>
>> > If the above is correct, I'm afraid that the third option - which
>> > doesn't address John's (valid) concerns - would be the most reasonable:
>> >
>> > 3. Don't add any VMState fields now and just do the post_load handler.
>> >    If we ever extend fdc in a way that makes it impossible to
>> >    reconstruct the phase from other migrated state, we add a subsection
>> >    that is only sent in cases where it differs from the reconstructed
>> >    value.
>> 
>> I'm definitely against this one. State should always be
>> sent in migration, and not doing that once we've added
>> it to the device struct is a bug.
>
> On second thought, I could actually add that subsection now and write
> the .needed callback such that it checks whether the reconstructed phase
> matches the actual one. This condition would always evaluate as false
> today, but if we ever change things so that the phase doesn't match what
> current qemu would think it is, you automatically get the subsection and
> things Just Work (TM).
>
> This also wouldn't break backwards migration for now, and only break it
> in future cases if it would really result in a broken device on the
> destination.

This is exactly how subsections are meant to be used: weakest "needed"
predicate that's still sufficient for reconstructing correct state on
the destination.

Yes, "state should always be sent", but we're free to encode the state
however we like.  The "if we can reconstruct phase from section, done,
else send it in a subsection" is a faithful, if ugly encoding of the
state.

Tradeoff here: ugly encoding and good backward compatibility vs. natural
encoding and bad backward compatibility.  We usually prefer the former.

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

[Peter Maydell]

On 21 May 2015 at 12:09, Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:
>>> 2b add a subsection, send the subsection always in new qemu,
>>> if receiving from an old qemu then calculate the phase from
>>> the register fields in post-load
>
> Doesn't this break new -> old migration?

Do we care about that? Personally I would have thought it was
too hard to defend and a very narrow use case compared to old->new.

-- PMM

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

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 21 May 2015 at 12:09, Markus Armbruster <armbru@redhat.com> wrote:
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> >> Am 21.05.2015 um 12:11 hat Peter Maydell geschrieben:
> >>> 2b add a subsection, send the subsection always in new qemu,
> >>> if receiving from an old qemu then calculate the phase from
> >>> the register fields in post-load
> >
> > Doesn't this break new -> old migration?
> 
> Do we care about that? Personally I would have thought it was
> too hard to defend and a very narrow use case compared to old->new.

I have to care about it downstream so I'd prefer if it wasn't
broken upstream except where really hard.

Dave

> -- PMM
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Kevin Wolf]

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);
         /* 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();
     }
 }
 
-- 
1.8.3.1

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

[John Snow]

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>

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

[Peter Maydell]

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

[Qemu-devel] [PATCH 5/8] fdc: Code cleanup in fdctrl_write_data()

[Kevin Wolf]

Factor out a few common lines of code, reformat, improve comments.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a13e0ce..cbf7abf 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
 /*
  * Handlers for the execution phase of each command
  */
-static const struct {
+typedef struct FDCtrlCommand {
     uint8_t value;
     uint8_t mask;
     const char* name;
     int parameters;
     void (*handler)(FDCtrl *fdctrl, int direction);
     int direction;
-} handlers[] = {
+} FDCtrlCommand;
+
+static const FDCtrlCommand handlers[] = {
     { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
     { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
     { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
@@ -1986,9 +1988,19 @@ static const struct {
 /* Associate command to an index in the 'handlers' array */
 static uint8_t command_to_handler[256];
 
+static const FDCtrlCommand *get_command(uint8_t cmd)
+{
+    int idx;
+
+    idx = command_to_handler[cmd];
+    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
+    return &handlers[idx];
+}
+
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
 {
     FDrive *cur_drv;
+    const FDCtrlCommand *cmd;
     uint32_t pos;
 
     /* Reset mode */
@@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     }
     fdctrl->dsr &= ~FD_DSR_PWRDOWN;
 
+    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
+    pos = fdctrl->data_pos++;
+    pos %= FD_SECTOR_LEN;
+    fdctrl->fifo[pos] = value;
+
     switch (fdctrl->phase) {
     case FD_PHASE_EXECUTION:
         assert(fdctrl->msr & FD_MSR_NONDMA);
+
         /* FIFO data write */
-        pos = fdctrl->data_pos++;
-        pos %= FD_SECTOR_LEN;
-        fdctrl->fifo[pos] = value;
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);
@@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
                 break;
             }
         }
-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->data_pos == fdctrl->data_len)
+
+        /* Switch to result phase when done with the transfer */
+        if (fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
+        }
         break;
 
     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;
+        if (fdctrl->data_pos == 1) {
+            /* The first byte specifies the command. Now we start reading
+             * as many parameters as this command requires. */
+            cmd = get_command(value);
+            fdctrl->data_len = cmd->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
-             */
+            /* We have all parameters now, execute 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);
+            cmd = get_command(fdctrl->fifo[0]);
+            FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
+            cmd->handler(fdctrl, cmd->direction);
         }
         break;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/8] fdc: Code cleanup in fdctrl_write_data()

[John Snow]

On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> Factor out a few common lines of code, reformat, improve comments.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index a13e0ce..cbf7abf 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
>  /*
>   * Handlers for the execution phase of each command
>   */
> -static const struct {
> +typedef struct FDCtrlCommand {
>      uint8_t value;
>      uint8_t mask;
>      const char* name;
>      int parameters;
>      void (*handler)(FDCtrl *fdctrl, int direction);
>      int direction;
> -} handlers[] = {
> +} FDCtrlCommand;
> +
> +static const FDCtrlCommand handlers[] = {
>      { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
>      { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
>      { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
> @@ -1986,9 +1988,19 @@ static const struct {
>  /* Associate command to an index in the 'handlers' array */
>  static uint8_t command_to_handler[256];
>  
> +static const FDCtrlCommand *get_command(uint8_t cmd)
> +{
> +    int idx;
> +
> +    idx = command_to_handler[cmd];
> +    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
> +    return &handlers[idx];
> +}
> +
>  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>  {
>      FDrive *cur_drv;
> +    const FDCtrlCommand *cmd;
>      uint32_t pos;
>  
>      /* Reset mode */
> @@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>      }
>      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>  
> +    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> +
> +    /* If data_len spans multiple sectors, the current position in the FIFO
> +     * wraps around while fdctrl->data_pos is the real position in the whole
> +     * request. */
> +    pos = fdctrl->data_pos++;
> +    pos %= FD_SECTOR_LEN;
> +    fdctrl->fifo[pos] = value;
> +
>      switch (fdctrl->phase) {
>      case FD_PHASE_EXECUTION:
>          assert(fdctrl->msr & FD_MSR_NONDMA);
> +
>          /* FIFO data write */
> -        pos = fdctrl->data_pos++;
> -        pos %= FD_SECTOR_LEN;
> -        fdctrl->fifo[pos] = value;
>          if (pos == FD_SECTOR_LEN - 1 ||
>              fdctrl->data_pos == fdctrl->data_len) {
>              cur_drv = get_cur_drv(fdctrl);
> @@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>                  break;
>              }
>          }
> -        /* Switch from transfer mode to status mode
> -         * then from status mode to command mode
> -         */
> -        if (fdctrl->data_pos == fdctrl->data_len)
> +
> +        /* Switch to result phase when done with the transfer */
> +        if (fdctrl->data_pos == fdctrl->data_len) {
>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> +        }
>          break;
>  
>      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;
> +        if (fdctrl->data_pos == 1) {

This reads more awkwardly than the previous ifz, but it's not
like I have a better idea. (I just had a momentary pause of "Why 1?")

> +            /* The first byte specifies the command. Now we start reading
> +             * as many parameters as this command requires. */
> +            cmd = get_command(value);
> +            fdctrl->data_len = cmd->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
> -             */
> +            /* We have all parameters now, execute 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);
> +            cmd = get_command(fdctrl->fifo[0]);
> +            FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
> +            cmd->handler(fdctrl, cmd->direction);
>          }
>          break;
>  
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 5/8] fdc: Code cleanup in fdctrl_write_data()

[Kevin Wolf]

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> > Factor out a few common lines of code, reformat, improve comments.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 38 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index a13e0ce..cbf7abf 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl *fdctrl, int direction)
> >  /*
> >   * Handlers for the execution phase of each command
> >   */
> > -static const struct {
> > +typedef struct FDCtrlCommand {
> >      uint8_t value;
> >      uint8_t mask;
> >      const char* name;
> >      int parameters;
> >      void (*handler)(FDCtrl *fdctrl, int direction);
> >      int direction;
> > -} handlers[] = {
> > +} FDCtrlCommand;
> > +
> > +static const FDCtrlCommand handlers[] = {
> >      { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
> >      { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
> >      { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
> > @@ -1986,9 +1988,19 @@ static const struct {
> >  /* Associate command to an index in the 'handlers' array */
> >  static uint8_t command_to_handler[256];
> >  
> > +static const FDCtrlCommand *get_command(uint8_t cmd)
> > +{
> > +    int idx;
> > +
> > +    idx = command_to_handler[cmd];
> > +    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
> > +    return &handlers[idx];
> > +}
> > +
> >  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >  {
> >      FDrive *cur_drv;
> > +    const FDCtrlCommand *cmd;
> >      uint32_t pos;
> >  
> >      /* Reset mode */
> > @@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >      }
> >      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> >  
> > +    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> > +
> > +    /* If data_len spans multiple sectors, the current position in the FIFO
> > +     * wraps around while fdctrl->data_pos is the real position in the whole
> > +     * request. */
> > +    pos = fdctrl->data_pos++;
> > +    pos %= FD_SECTOR_LEN;
> > +    fdctrl->fifo[pos] = value;
> > +
> >      switch (fdctrl->phase) {
> >      case FD_PHASE_EXECUTION:
> >          assert(fdctrl->msr & FD_MSR_NONDMA);
> > +
> >          /* FIFO data write */
> > -        pos = fdctrl->data_pos++;
> > -        pos %= FD_SECTOR_LEN;
> > -        fdctrl->fifo[pos] = value;
> >          if (pos == FD_SECTOR_LEN - 1 ||
> >              fdctrl->data_pos == fdctrl->data_len) {
> >              cur_drv = get_cur_drv(fdctrl);
> > @@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> >                  break;
> >              }
> >          }
> > -        /* Switch from transfer mode to status mode
> > -         * then from status mode to command mode
> > -         */
> > -        if (fdctrl->data_pos == fdctrl->data_len)
> > +
> > +        /* Switch to result phase when done with the transfer */
> > +        if (fdctrl->data_pos == fdctrl->data_len) {
> >              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> > +        }
> >          break;
> >  
> >      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;
> > +        if (fdctrl->data_pos == 1) {
> 
> This reads more awkwardly than the previous ifz, but it's not
> like I have a better idea. (I just had a momentary pause of "Why 1?")

Hm, I think we can assert fdctrl->data_pos < FD_SECTOR_LEN (this is the
command phase and commands only have a few bytes of parameters) and
therefore check for pos == 0 here, if you think that's better.

Kevin

[Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()

[Kevin Wolf]

This commit makes similar improvements as have already been made to the
write function: 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 | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cbf7abf..8d322e0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         FLOPPY_DPRINTF("error: controller not ready for reading\n");
         return 0;
     }
+
+    /* If data_len spans multiple sectors, the current position in the FIFO
+     * wraps around while fdctrl->data_pos is the real position in the whole
+     * request. */
     pos = fdctrl->data_pos;
     pos %= FD_SECTOR_LEN;
-    if (fdctrl->msr & FD_MSR_NONDMA) {
+
+    switch (fdctrl->phase) {
+    case FD_PHASE_EXECUTION:
+        assert(fdctrl->msr & FD_MSR_NONDMA);
         if (pos == 0) {
             if (fdctrl->data_pos != 0)
                 if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
@@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                 memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
             }
         }
-    }
-    retval = fdctrl->fifo[pos];
-    if (++fdctrl->data_pos == fdctrl->data_len) {
-        fdctrl->data_pos = 0;
-        /* Switch from transfer mode to status mode
-         * then from status mode to command mode
-         */
-        if (fdctrl->msr & FD_MSR_NONDMA) {
+
+        if (++fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
-        } else {
+        }
+        break;
+
+    case FD_PHASE_RESULT:
+        assert(!(fdctrl->msr & FD_MSR_NONDMA));
+        if (++fdctrl->data_pos == fdctrl->data_len) {
             fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }
+        break;
+
+    case FD_PHASE_COMMAND:
+    default:
+        abort();
     }
+
+    retval = fdctrl->fifo[pos];
     FLOPPY_DPRINTF("data register: 0x%02x\n", retval);
 
     return retval;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()

[John Snow]

On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> This commit makes similar improvements as have already been made to the
> write function: 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 | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index cbf7abf..8d322e0 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>          FLOPPY_DPRINTF("error: controller not ready for reading\n");
>          return 0;
>      }
> +
> +    /* If data_len spans multiple sectors, the current position in the FIFO
> +     * wraps around while fdctrl->data_pos is the real position in the whole
> +     * request. */
>      pos = fdctrl->data_pos;
>      pos %= FD_SECTOR_LEN;
> -    if (fdctrl->msr & FD_MSR_NONDMA) {
> +
> +    switch (fdctrl->phase) {
> +    case FD_PHASE_EXECUTION:
> +        assert(fdctrl->msr & FD_MSR_NONDMA);
>          if (pos == 0) {
>              if (fdctrl->data_pos != 0)
>                  if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>                  memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
>              }
>          }
> -    }
> -    retval = fdctrl->fifo[pos];
> -    if (++fdctrl->data_pos == fdctrl->data_len) {
> -        fdctrl->data_pos = 0;

I suppose data_pos is now reset by either stop_transfer (via
to_result_phase) or to_command_phase, so this is OK.

> -        /* Switch from transfer mode to status mode
> -         * then from status mode to command mode
> -         */
> -        if (fdctrl->msr & FD_MSR_NONDMA) {
> +
> +        if (++fdctrl->data_pos == fdctrl->data_len) {
>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> -        } else {
> +        }
> +        break;
> +
> +    case FD_PHASE_RESULT:
> +        assert(!(fdctrl->msr & FD_MSR_NONDMA));
> +        if (++fdctrl->data_pos == fdctrl->data_len) {

Not a terribly big fan of moving this pointer independently inside of
each case statement, but I guess the alternative does look a lot worse.
Having things separated by phases is a lot easier to follow.

>              fdctrl_to_command_phase(fdctrl);
>              fdctrl_reset_irq(fdctrl);
>          }
> +        break;
> +
> +    case FD_PHASE_COMMAND:
> +    default:
> +        abort();
>      }
> +
> +    retval = fdctrl->fifo[pos];
>      FLOPPY_DPRINTF("data register: 0x%02x\n", retval);
>  
>      return retval;
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()

[Kevin Wolf]

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> > This commit makes similar improvements as have already been made to the
> > write function: 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 | 33 +++++++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index cbf7abf..8d322e0 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> >          FLOPPY_DPRINTF("error: controller not ready for reading\n");
> >          return 0;
> >      }
> > +
> > +    /* If data_len spans multiple sectors, the current position in the FIFO
> > +     * wraps around while fdctrl->data_pos is the real position in the whole
> > +     * request. */
> >      pos = fdctrl->data_pos;
> >      pos %= FD_SECTOR_LEN;
> > -    if (fdctrl->msr & FD_MSR_NONDMA) {
> > +
> > +    switch (fdctrl->phase) {
> > +    case FD_PHASE_EXECUTION:
> > +        assert(fdctrl->msr & FD_MSR_NONDMA);
> >          if (pos == 0) {
> >              if (fdctrl->data_pos != 0)
> >                  if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> > @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> >                  memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
> >              }
> >          }
> > -    }
> > -    retval = fdctrl->fifo[pos];
> > -    if (++fdctrl->data_pos == fdctrl->data_len) {
> > -        fdctrl->data_pos = 0;
> 
> I suppose data_pos is now reset by either stop_transfer (via
> to_result_phase) or to_command_phase, so this is OK.

Yes, that was redundant code.

> > -        /* Switch from transfer mode to status mode
> > -         * then from status mode to command mode
> > -         */
> > -        if (fdctrl->msr & FD_MSR_NONDMA) {
> > +
> > +        if (++fdctrl->data_pos == fdctrl->data_len) {
> >              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> > -        } else {
> > +        }
> > +        break;
> > +
> > +    case FD_PHASE_RESULT:
> > +        assert(!(fdctrl->msr & FD_MSR_NONDMA));
> > +        if (++fdctrl->data_pos == fdctrl->data_len) {
> 
> Not a terribly big fan of moving this pointer independently inside of
> each case statement, but I guess the alternative does look a lot worse.
> Having things separated by phases is a lot easier to follow.

I'm not too happy about it either, but I couldn't think of anything
better. Having two different switches almost immediately after each
other, with only the if line in between, would look really awkward and
be hard to read. And the old code isn't nice either.

If you have any idea for a better solution, let me know.

Kevin

Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data()

[John Snow]

On 05/20/2015 04:25 AM, Kevin Wolf wrote:
> Am 19.05.2015 um 22:40 hat John Snow geschrieben:
>>
>>
>> On 05/19/2015 11:36 AM, Kevin Wolf wrote:
>>> This commit makes similar improvements as have already been made to the
>>> write function: 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 | 33 +++++++++++++++++++++++----------
>>>  1 file changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index cbf7abf..8d322e0 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>          FLOPPY_DPRINTF("error: controller not ready for reading\n");
>>>          return 0;
>>>      }
>>> +
>>> +    /* If data_len spans multiple sectors, the current position in the FIFO
>>> +     * wraps around while fdctrl->data_pos is the real position in the whole
>>> +     * request. */
>>>      pos = fdctrl->data_pos;
>>>      pos %= FD_SECTOR_LEN;
>>> -    if (fdctrl->msr & FD_MSR_NONDMA) {
>>> +
>>> +    switch (fdctrl->phase) {
>>> +    case FD_PHASE_EXECUTION:
>>> +        assert(fdctrl->msr & FD_MSR_NONDMA);
>>>          if (pos == 0) {
>>>              if (fdctrl->data_pos != 0)
>>>                  if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
>>> @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>                  memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
>>>              }
>>>          }
>>> -    }
>>> -    retval = fdctrl->fifo[pos];
>>> -    if (++fdctrl->data_pos == fdctrl->data_len) {
>>> -        fdctrl->data_pos = 0;
>>
>> I suppose data_pos is now reset by either stop_transfer (via
>> to_result_phase) or to_command_phase, so this is OK.
> 
> Yes, that was redundant code.
> 
>>> -        /* Switch from transfer mode to status mode
>>> -         * then from status mode to command mode
>>> -         */
>>> -        if (fdctrl->msr & FD_MSR_NONDMA) {
>>> +
>>> +        if (++fdctrl->data_pos == fdctrl->data_len) {
>>>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
>>> -        } else {
>>> +        }
>>> +        break;
>>> +
>>> +    case FD_PHASE_RESULT:
>>> +        assert(!(fdctrl->msr & FD_MSR_NONDMA));
>>> +        if (++fdctrl->data_pos == fdctrl->data_len) {
>>
>> Not a terribly big fan of moving this pointer independently inside of
>> each case statement, but I guess the alternative does look a lot worse.
>> Having things separated by phases is a lot easier to follow.
> 
> I'm not too happy about it either, but I couldn't think of anything
> better. Having two different switches almost immediately after each
> other, with only the if line in between, would look really awkward and
> be hard to read. And the old code isn't nice either.
> 
> If you have any idea for a better solution, let me know.
> 
> Kevin
> 

I'm all complaints and no solutions. I believe I gave you my R-b anyway. :)

[Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag

[Kevin Wolf]

The RQM bit in MSR should be set whenever the guest is supposed to
access the FIFO, and it should be cleared in all other cases. This is
important so the guest can't continue writing/reading the FIFO beyond
the length that it's suppossed to access (see CVE-2015-3456).

Commit e9077462 fixed the CVE by adding code that avoids the buffer
overflow; however it doesn't correct the wrong behaviour of the floppy
controller which should already have cleared RQM.

Currently, RQM stays set all the time and during all phases while a
command is being processed. This is error-prone because the command has
to explicitly clear the flag if it doesn't need data (and indeed, the
two buggy commands that are the culprits for the CVE just forgot to do
that).

This patch clears RQM immediately as soon as all bytes that are expected
have been received. If the the FIFO is used in the next phase, the flag
has to be set explicitly there.

This alone should have been enough to fix the CVE, but now we have two
lines of defense - even better.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 8d322e0..c6a046e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
     fdctrl->phase = FD_PHASE_COMMAND;
     fdctrl->data_dir = FD_DIR_WRITE;
     fdctrl->data_pos = 0;
+    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
     fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
+    fdctrl->msr |= FD_MSR_RQM;
 }
 
 /* Update the state to allow the guest to read out the command status.
@@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         }
     }
     FLOPPY_DPRINTF("start non-DMA transfer\n");
-    fdctrl->msr |= FD_MSR_NONDMA;
+    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
@@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
         }
 
         if (++fdctrl->data_pos == fdctrl->data_len) {
+            fdctrl->msr &= ~FD_MSR_RQM;
             fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
         }
         break;
@@ -1567,6 +1570,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
     case FD_PHASE_RESULT:
         assert(!(fdctrl->msr & FD_MSR_NONDMA));
         if (++fdctrl->data_pos == fdctrl->data_len) {
+            fdctrl->msr &= ~FD_MSR_RQM;
             fdctrl_to_command_phase(fdctrl);
             fdctrl_reset_irq(fdctrl);
         }
@@ -2036,6 +2040,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
     pos %= FD_SECTOR_LEN;
     fdctrl->fifo[pos] = value;
 
+    if (fdctrl->data_pos == fdctrl->data_len) {
+        fdctrl->msr &= ~FD_MSR_RQM;
+    }
+
     switch (fdctrl->phase) {
     case FD_PHASE_EXECUTION:
         assert(fdctrl->msr & FD_MSR_NONDMA);
@@ -2071,6 +2079,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
              * as many parameters as this command requires. */
             cmd = get_command(value);
             fdctrl->data_len = cmd->parameters + 1;
+            if (cmd->parameters) {
+                fdctrl->msr |= FD_MSR_RQM;
+            }
             fdctrl->msr |= FD_MSR_CMDBUSY;
         }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag

[John Snow]

On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> The RQM bit in MSR should be set whenever the guest is supposed to
> access the FIFO, and it should be cleared in all other cases. This is
> important so the guest can't continue writing/reading the FIFO beyond
> the length that it's suppossed to access (see CVE-2015-3456).
> 
> Commit e9077462 fixed the CVE by adding code that avoids the buffer
> overflow; however it doesn't correct the wrong behaviour of the floppy
> controller which should already have cleared RQM.
> 
> Currently, RQM stays set all the time and during all phases while a
> command is being processed. This is error-prone because the command has
> to explicitly clear the flag if it doesn't need data (and indeed, the
> two buggy commands that are the culprits for the CVE just forgot to do
> that).
> 
> This patch clears RQM immediately as soon as all bytes that are expected
> have been received. If the the FIFO is used in the next phase, the flag
> has to be set explicitly there.
> 
> This alone should have been enough to fix the CVE, but now we have two
> lines of defense - even better.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 8d322e0..c6a046e 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
>      fdctrl->phase = FD_PHASE_COMMAND;
>      fdctrl->data_dir = FD_DIR_WRITE;
>      fdctrl->data_pos = 0;
> +    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
>      fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
> +    fdctrl->msr |= FD_MSR_RQM;
>  }
>  
>  /* Update the state to allow the guest to read out the command status.
> @@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>          }
>      }
>      FLOPPY_DPRINTF("start non-DMA transfer\n");
> -    fdctrl->msr |= FD_MSR_NONDMA;
> +    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
>      if (direction != FD_DIR_WRITE)
>          fdctrl->msr |= FD_MSR_DIO;
>      /* IO based transfer: calculate len */
> @@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>          }
>  
>          if (++fdctrl->data_pos == fdctrl->data_len) {
> +            fdctrl->msr &= ~FD_MSR_RQM;

Doesn't stop_transfer set this flag back right away?

>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
>          }
>          break;
> @@ -1567,6 +1570,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>      case FD_PHASE_RESULT:
>          assert(!(fdctrl->msr & FD_MSR_NONDMA));
>          if (++fdctrl->data_pos == fdctrl->data_len) {
> +            fdctrl->msr &= ~FD_MSR_RQM;

Same here with to_command_phase.

>              fdctrl_to_command_phase(fdctrl);
>              fdctrl_reset_irq(fdctrl);
>          }
> @@ -2036,6 +2040,10 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>      pos %= FD_SECTOR_LEN;
>      fdctrl->fifo[pos] = value;
>  
> +    if (fdctrl->data_pos == fdctrl->data_len) {
> +        fdctrl->msr &= ~FD_MSR_RQM;
> +    }
> +
>      switch (fdctrl->phase) {
>      case FD_PHASE_EXECUTION:
>          assert(fdctrl->msr & FD_MSR_NONDMA);
> @@ -2071,6 +2079,9 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>               * as many parameters as this command requires. */
>              cmd = get_command(value);
>              fdctrl->data_len = cmd->parameters + 1;
> +            if (cmd->parameters) {
> +                fdctrl->msr |= FD_MSR_RQM;
> +            }
>              fdctrl->msr |= FD_MSR_CMDBUSY;
>          }
>  
> 

Re: [Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag

[Kevin Wolf]

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> > The RQM bit in MSR should be set whenever the guest is supposed to
> > access the FIFO, and it should be cleared in all other cases. This is
> > important so the guest can't continue writing/reading the FIFO beyond
> > the length that it's suppossed to access (see CVE-2015-3456).
> > 
> > Commit e9077462 fixed the CVE by adding code that avoids the buffer
> > overflow; however it doesn't correct the wrong behaviour of the floppy
> > controller which should already have cleared RQM.
> > 
> > Currently, RQM stays set all the time and during all phases while a
> > command is being processed. This is error-prone because the command has
> > to explicitly clear the flag if it doesn't need data (and indeed, the
> > two buggy commands that are the culprits for the CVE just forgot to do
> > that).
> > 
> > This patch clears RQM immediately as soon as all bytes that are expected
> > have been received. If the the FIFO is used in the next phase, the flag
> > has to be set explicitly there.
> > 
> > This alone should have been enough to fix the CVE, but now we have two
> > lines of defense - even better.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/block/fdc.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > index 8d322e0..c6a046e 100644
> > --- a/hw/block/fdc.c
> > +++ b/hw/block/fdc.c
> > @@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
> >      fdctrl->phase = FD_PHASE_COMMAND;
> >      fdctrl->data_dir = FD_DIR_WRITE;
> >      fdctrl->data_pos = 0;
> > +    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
> >      fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
> > +    fdctrl->msr |= FD_MSR_RQM;
> >  }
> >  
> >  /* Update the state to allow the guest to read out the command status.
> > @@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
> >          }
> >      }
> >      FLOPPY_DPRINTF("start non-DMA transfer\n");
> > -    fdctrl->msr |= FD_MSR_NONDMA;
> > +    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
> >      if (direction != FD_DIR_WRITE)
> >          fdctrl->msr |= FD_MSR_DIO;
> >      /* IO based transfer: calculate len */
> > @@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
> >          }
> >  
> >          if (++fdctrl->data_pos == fdctrl->data_len) {
> > +            fdctrl->msr &= ~FD_MSR_RQM;
> 
> Doesn't stop_transfer set this flag back right away?

It does, by switching to the result phase.

I think it's clearer to disable the bit anywhere where the FIFO has
received as many bytes as it's supposed to, even if the next phase is
started immediately and reenables it.

In real hardware, sending a byte causes the FDC to disable RQM, then
process the byte (which means completing command execution for this code
path), then reenable RQM if needed.

Currently our code is completely synchronous, so we could ignore this
detail because the state between clearing and setting RQM isn't
observable by the guest. If we ever introduce something asynchronous in
the path, we will need this though - and modelling real hardware more
precisely has never hurt anyway.

Kevin

Re: [Qemu-devel] [PATCH 7/8] fdc: Fix MSR.RQM flag

[John Snow]

On 05/20/2015 04:14 AM, Kevin Wolf wrote:
> Am 19.05.2015 um 22:40 hat John Snow geschrieben:
>>
>>
>> On 05/19/2015 11:36 AM, Kevin Wolf wrote:
>>> The RQM bit in MSR should be set whenever the guest is supposed to
>>> access the FIFO, and it should be cleared in all other cases. This is
>>> important so the guest can't continue writing/reading the FIFO beyond
>>> the length that it's suppossed to access (see CVE-2015-3456).
>>>
>>> Commit e9077462 fixed the CVE by adding code that avoids the buffer
>>> overflow; however it doesn't correct the wrong behaviour of the floppy
>>> controller which should already have cleared RQM.
>>>
>>> Currently, RQM stays set all the time and during all phases while a
>>> command is being processed. This is error-prone because the command has
>>> to explicitly clear the flag if it doesn't need data (and indeed, the
>>> two buggy commands that are the culprits for the CVE just forgot to do
>>> that).
>>>
>>> This patch clears RQM immediately as soon as all bytes that are expected
>>> have been received. If the the FIFO is used in the next phase, the flag
>>> has to be set explicitly there.
>>>
>>> This alone should have been enough to fix the CVE, but now we have two
>>> lines of defense - even better.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  hw/block/fdc.c | 13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 8d322e0..c6a046e 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -1165,7 +1165,9 @@ static void fdctrl_to_command_phase(FDCtrl *fdctrl)
>>>      fdctrl->phase = FD_PHASE_COMMAND;
>>>      fdctrl->data_dir = FD_DIR_WRITE;
>>>      fdctrl->data_pos = 0;
>>> +    fdctrl->data_len = 1; /* Accept command byte, adjust for params later */
>>>      fdctrl->msr &= ~(FD_MSR_CMDBUSY | FD_MSR_DIO);
>>> +    fdctrl->msr |= FD_MSR_RQM;
>>>  }
>>>  
>>>  /* Update the state to allow the guest to read out the command status.
>>> @@ -1380,7 +1382,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>>>          }
>>>      }
>>>      FLOPPY_DPRINTF("start non-DMA transfer\n");
>>> -    fdctrl->msr |= FD_MSR_NONDMA;
>>> +    fdctrl->msr |= FD_MSR_NONDMA | FD_MSR_RQM;
>>>      if (direction != FD_DIR_WRITE)
>>>          fdctrl->msr |= FD_MSR_DIO;
>>>      /* IO based transfer: calculate len */
>>> @@ -1560,6 +1562,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>>>          }
>>>  
>>>          if (++fdctrl->data_pos == fdctrl->data_len) {
>>> +            fdctrl->msr &= ~FD_MSR_RQM;
>>
>> Doesn't stop_transfer set this flag back right away?
> 
> It does, by switching to the result phase.
> 
> I think it's clearer to disable the bit anywhere where the FIFO has
> received as many bytes as it's supposed to, even if the next phase is
> started immediately and reenables it.
> 
> In real hardware, sending a byte causes the FDC to disable RQM, then
> process the byte (which means completing command execution for this code
> path), then reenable RQM if needed.
> 
> Currently our code is completely synchronous, so we could ignore this
> detail because the state between clearing and setting RQM isn't
> observable by the guest. If we ever introduce something asynchronous in
> the path, we will need this though - and modelling real hardware more
> precisely has never hurt anyway.
> 
> Kevin
> 

OK, just amend the commit message to explain that clearing the bits here
is to accommodate a possible asynchronous refactor, or to be more
explicit, or etc etc etc.

--js

[Qemu-devel] [PATCH 8/8] fdc-test: Test state for existing cases more thoroughly

[Kevin Wolf]

This just adds a few additional checks of the MSR and interrupt pin to
the already existing test cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 3c6c83c..416394f 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -218,6 +218,10 @@ static uint8_t send_read_no_dma_command(int nb_sect, uint8_t expected_st0)
         inb(FLOPPY_BASE + reg_fifo);
     }
 
+    msr = inb(FLOPPY_BASE + reg_msr);
+    assert_bit_set(msr, BUSY | RQM | DIO);
+    g_assert(get_irq(FLOPPY_IRQ));
+
     st0 = floppy_recv();
     if (st0 != expected_st0) {
         ret = 1;
@@ -228,8 +232,15 @@ static uint8_t send_read_no_dma_command(int nb_sect, uint8_t expected_st0)
     floppy_recv();
     floppy_recv();
     floppy_recv();
+    g_assert(get_irq(FLOPPY_IRQ));
     floppy_recv();
 
+    /* Check that we're back in command phase */
+    msr = inb(FLOPPY_BASE + reg_msr);
+    assert_bit_clear(msr, BUSY | DIO);
+    assert_bit_set(msr, RQM);
+    g_assert(!get_irq(FLOPPY_IRQ));
+
     return ret;
 }
 
@@ -403,6 +414,7 @@ static void test_read_id(void)
     uint8_t head = 0;
     uint8_t cyl;
     uint8_t st0;
+    uint8_t msr;
 
     /* Seek to track 0 and check with READ ID */
     send_seek(0);
@@ -411,18 +423,29 @@ static void test_read_id(void)
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(head << 2 | drive);
 
+    msr = inb(FLOPPY_BASE + reg_msr);
+    if (!get_irq(FLOPPY_IRQ)) {
+        assert_bit_set(msr, BUSY);
+        assert_bit_clear(msr, RQM);
+    }
+
     while (!get_irq(FLOPPY_IRQ)) {
         /* qemu involves a timer with READ ID... */
         clock_step(1000000000LL / 50);
     }
 
+    msr = inb(FLOPPY_BASE + reg_msr);
+    assert_bit_set(msr, BUSY | RQM | DIO);
+
     st0 = floppy_recv();
     floppy_recv();
     floppy_recv();
     cyl = floppy_recv();
     head = floppy_recv();
     floppy_recv();
+    g_assert(get_irq(FLOPPY_IRQ));
     floppy_recv();
+    g_assert(!get_irq(FLOPPY_IRQ));
 
     g_assert_cmpint(cyl, ==, 0);
     g_assert_cmpint(head, ==, 0);
@@ -443,18 +466,29 @@ static void test_read_id(void)
     g_assert(!get_irq(FLOPPY_IRQ));
     floppy_send(head << 2 | drive);
 
+    msr = inb(FLOPPY_BASE + reg_msr);
+    if (!get_irq(FLOPPY_IRQ)) {
+        assert_bit_set(msr, BUSY);
+        assert_bit_clear(msr, RQM);
+    }
+
     while (!get_irq(FLOPPY_IRQ)) {
         /* qemu involves a timer with READ ID... */
         clock_step(1000000000LL / 50);
     }
 
+    msr = inb(FLOPPY_BASE + reg_msr);
+    assert_bit_set(msr, BUSY | RQM | DIO);
+
     st0 = floppy_recv();
     floppy_recv();
     floppy_recv();
     cyl = floppy_recv();
     head = floppy_recv();
     floppy_recv();
+    g_assert(get_irq(FLOPPY_IRQ));
     floppy_recv();
+    g_assert(!get_irq(FLOPPY_IRQ));
 
     g_assert_cmpint(cyl, ==, 8);
     g_assert_cmpint(head, ==, 1);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 8/8] fdc-test: Test state for existing cases more thoroughly

[John Snow]

On 05/19/2015 11:36 AM, Kevin Wolf wrote:
> This just adds a few additional checks of the MSR and interrupt pin to
> the already existing test cases.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/fdc-test.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> index 3c6c83c..416394f 100644
> --- a/tests/fdc-test.c
> +++ b/tests/fdc-test.c

[snip]

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 0/8] fdc: Clean up and fix command processing

[John Snow]

On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> This series fixes the real bug that caused CVE-2015-3456, and does some
> cleanup in the FIFO access functions to make the command processing more
> obvious.
> 
> Kevin Wolf (8):
>   fdc: Rename fdctrl_reset_fifo() to fdctrl_to_command_phase()
>   fdc: Rename fdctrl_set_fifo() to fdctrl_to_result_phase()
>   fdc: Introduce fdctrl->phase
>   fdc: Use phase in fdctrl_write_data()
>   fdc: Code cleanup in fdctrl_write_data()
>   fdc: Disentangle phases in fdctrl_read_data()
>   fdc: Fix MSR.RQM flag
>   fdc-test: Test state for existing cases more thoroughly
> 
>  hw/block/fdc.c   | 235 +++++++++++++++++++++++++++++++++++++------------------
>  tests/fdc-test.c |  34 ++++++++
>  2 files changed, 192 insertions(+), 77 deletions(-)
> 

This is just the cover letter, but I might not leave the implication
dangling that the CVE-2015-3456 bug remains to be patched, or that the
vulnerability still exists in the current codebase.

So for posterity: This patch series is a thorough cleanup of the code
that was patched to prevent the CVE-2015-3456 vulnerability.

Thanks!
--js