Re: [Qemu-devel] [PATCH] Replace bdrv_* to bdrv_aio_* functions in pio mode in fdc.c.

[Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>]

On Fri, Mar 23, 2012 at 03:07:20PM +0800, Li Zhi Hui wrote:
> Replace bdrv_* to bdrv_aio_* functions in pio mode in fdc.c.
> 
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  hw/fdc.c |  117 +++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 90 insertions(+), 27 deletions(-)

I have posted detailed comments below.  The high-level point is to look
back at the datasheet because the information needed to implement
asynchronous I/O is only available there.  Since hw/fdc.c is synchronous
today it omits steps in the request lifecycle that are necessary for
asynchronous I/O.  It is impossible to do asynchronous I/O by
refactoring hw/fdc.c, it won't produce the semantics from the datasheet.
You need to study the request lifecycle in the datasheet and use that
information to implement asynchronous I/O in hw/fdc.c.

> diff --git a/hw/fdc.c b/hw/fdc.c
> index a0236b7..5e855fd 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1301,12 +1301,42 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
>      return len;
>  }
> 
> +enum {
> +    FD_DATA_IDLE,
> +    FD_DATA_WORKING,
> +    FD_DATA_FIN,
> +};
> +
> +int data_status[MAX_FD];

This would need to be a FDCtrl field so that each FDrive on each fdc
instance has it's own data_status.  If you leave it global then it's not
possible to use multiple fdc devices at the same time.

> +
> +static void fdctrl_read_pio_cb(void *opaque, int ret)
> +{
> +    FDCtrl *fdctrl = opaque;
> +    FDrive *cur_drv;
> +
> +    if (ret < 0) {
> +        cur_drv = get_cur_drv(fdctrl);
> +        FLOPPY_DPRINTF("error getting sector %d\n",
> +                       fd_sector(cur_drv));
> +        /* Sure, image size is too small... */
> +        memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
> +        data_status[fdctrl->cur_drv] = FD_DATA_IDLE;
> +        goto end;
> +    }
> +    data_status[fdctrl->cur_drv] = FD_DATA_FIN;
> +
> +end:
> +    return;
> +}
> +
>  /* Data register : 0x05 */
>  static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>  {
>      FDrive *cur_drv;
>      uint32_t retval = 0;
>      int pos;
> +    QEMUIOVector qiov;
> +    struct iovec iov;
> 
>      cur_drv = get_cur_drv(fdctrl);
>      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
> @@ -1318,17 +1348,30 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>      if (fdctrl->msr & FD_MSR_NONDMA) {
>          pos %= FD_SECTOR_LEN;
>          if (pos == 0) {
> -            if (fdctrl->data_pos != 0)
> -                if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> -                    FLOPPY_DPRINTF("error seeking to next sector %d\n",
> -                                   fd_sector(cur_drv));
> -                    return 0;
> +            switch (data_status[fdctrl->cur_drv]) {
> +            case FD_DATA_IDLE:
> +                if (fdctrl->data_pos != 0) {
> +                    if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> +                        FLOPPY_DPRINTF("error seeking to next sector %d\n",
> +                                         fd_sector(cur_drv));
> +                        goto end;
> +                    }
>                  }
> -            if (bdrv_read(cur_drv->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
> -                FLOPPY_DPRINTF("error getting sector %d\n",
> -                               fd_sector(cur_drv));
> -                /* Sure, image size is too small... */
> -                memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
> +                iov.iov_base = fdctrl->fifo;
> +                iov.iov_len  = FD_SECTOR_LEN;
> +                qemu_iovec_init_external(&qiov, &iov, 1);
> +                bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
> +                    &qiov, 1, fdctrl_read_pio_cb, fdctrl);
> +                data_status[fdctrl->cur_drv] = FD_DATA_WORKING;
> +                goto end;
> +            case FD_DATA_WORKING:
> +                goto end;
> +            case FD_DATA_FIN:
> +                data_status[fdctrl->cur_drv] = FD_DATA_IDLE;
> +                break;
> +            default:
> +                data_status[fdctrl->cur_drv] = FD_DATA_IDLE;
> +                goto end;
>              }

Does this approach follow the datasheet?  You are returning 0 on each
"goto end" so the guest will think it is reading zeroes from the floppy.

When fdctrl_read_data() gets called we *must* have valid data in the
fifo - or be able to tell the guest to wait using status register bits.

>          }
>      }
> @@ -1347,6 +1390,7 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>      }
>      FLOPPY_DPRINTF("data register: 0x%02x\n", retval);
> 
> +end:
>      return retval;
>  }
> 
> @@ -1759,10 +1803,38 @@ static const struct {
>  /* Associate command to an index in the 'handlers' array */
>  static uint8_t command_to_handler[256];
> 
> +static void fdctrl_write_pio_cb(void *opaque, int ret)
> +{
> +    FDCtrl *fdctrl = opaque;
> +    FDrive *cur_drv;
> +
> +    cur_drv = get_cur_drv(fdctrl);
> +    if (ret < 0) {
> +        FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));
> +        goto end;
> +    }
> +    if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> +        FLOPPY_DPRINTF("error seeking to next sector %d\n",
> +                       fd_sector(cur_drv));
> +        goto end;
> +    }
> +    /* Switch from transfer mode to status mode
> +     * then from status mode to command mode
> +     */
> +    if (fdctrl->data_pos == fdctrl->data_len) {
> +        fdctrl_stop_transfer(fdctrl, FD_SR0_SEEK, 0x00, 0x00);
> +    }
> +
> +end:
> +    return;
> +}
> +
>  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>  {
>      FDrive *cur_drv;
>      int pos;
> +    QEMUIOVector qiov;
> +    struct iovec iov;

bdrv_aio_*() does not copy qiov, it just takes a reference.  This means
qiov/iov's lifetime must extend beyond bdrv_aio_*() completion.  It's
not safe to allocate these variables on the stack since
fdctrl_write_data() will return.  I suggest making qiov/iov FDCtrl
fields because I think a floppy controller can only perform one request
at a time.

> 
>      /* Reset mode */
>      if (!(fdctrl->dor & FD_DOR_nRESET)) {
> @@ -1780,25 +1852,16 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>          pos = fdctrl->data_pos++;
>          pos %= FD_SECTOR_LEN;
>          fdctrl->fifo[pos] = value;
> -        if (pos == FD_SECTOR_LEN - 1 ||
> -            fdctrl->data_pos == fdctrl->data_len) {
> +        if ((pos == FD_SECTOR_LEN - 1) ||
> +            (fdctrl->data_pos == fdctrl->data_len)) {
>              cur_drv = get_cur_drv(fdctrl);
> -            if (bdrv_write(cur_drv->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
> -                FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));
> -                return;
> -            }
> -            if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
> -                FLOPPY_DPRINTF("error seeking to next sector %d\n",
> -                               fd_sector(cur_drv));
> -                return;
> -            }
> +            iov.iov_base = fdctrl->fifo;
> +            iov.iov_len  = FD_SECTOR_LEN;
> +            qemu_iovec_init_external(&qiov, &iov, 1);
> +            bdrv_aio_writev(cur_drv->bs, fd_sector(cur_drv),
> +                &qiov, 1, fdctrl_write_pio_cb, fdctrl);
> +            return;
>          }
> -        /* Switch from transfer mode to status mode
> -         * then from status mode to command mode
> -         */
> -        if (fdctrl->data_pos == fdctrl->data_len)
> -            fdctrl_stop_transfer(fdctrl, FD_SR0_SEEK, 0x00, 0x00);
> -        return;
>      }
>      if (fdctrl->data_pos == 0) {
>          /* Command */

fdctrl_write_data() fills the fifo and submits bdrv_aio_write() requests
but doesn't tell the guest to stop writing more data to the fifo.  The
guest may continue to call fdctrl_write_data() before we've completed
the last sector write.  So the guest can overwrite the fifo and the
write will be corrupted.

Does the floppy controller datasheet describe a status bit which is used
to indicate that the fifo is busy?  Previously we didn't need to use it
because bdrv_write() was "atomic", but now we need to tell the guest to
wait while we write the sector.

Stefan