[Qemu-devel] [PATCH 0/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Qemu-devel] [PATCH 0/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Li Zhi Hui]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>

Before v5 version, Although use bdrv_aio_* to replace bdrv_* functions， But In the process
of processing data, only call the bdrv_dio_* one time, it will not really simulate floppy disk's
reading and writing.
In v6 version each read and write all you need to call bdrv_aio_*.

All these patches were done under paolo's advice and guidance, Thank him very much!
 

 hw/dma.c     |   80 +++++++++-----
 hw/fdc.c     |  333 ++++++++++++++++++++++++++++++++++++++++------------------
 hw/isa.h     |    2 +
 hw/sun4m.c   |    3 +
 hw/sun4u.c   |    3 +
 trace-events |    8 ++
 6 files changed, 296 insertions(+), 133 deletions(-)

-- 
1.7.4.1

[Qemu-devel] [PATCH 1/3 v6] add function DMA_set_return and DMA_set_channel_async in dma.c

[Li Zhi Hui]

Prepare for introducing asynchronous I/O in floppy disks by adding
support for asynchronous DMA operations.

We won't need idle bottom halves (or bottom halves in general) anymore.
QEMU works just fine without them with synchronous I/O (though the floppy
drive will stall the guest and be "impolite"), so remove all that code
for that now for simplicity.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
 hw/dma.c   |   80 +++++++++++++++++++++++++++++++++++++----------------------
 hw/fdc.c   |    1 +
 hw/isa.h   |    2 +
 hw/sun4m.c |    3 ++
 hw/sun4u.c |    3 ++
 5 files changed, 59 insertions(+), 30 deletions(-)

diff --git a/hw/dma.c b/hw/dma.c
index 0a9322d..dd5c57d 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -45,6 +45,8 @@ struct dma_regs {
     uint8_t eop;
     DMA_transfer_handler transfer_handler;
     void *opaque;
+    bool channel_running;
+    bool channel_is_asynchronous;
 };
 
 #define ADDR 0
@@ -138,6 +140,8 @@ static inline void init_chan (struct dma_cont *d, int ichan)
     r = d->regs + ichan;
     r->now[ADDR] = r->base[ADDR] << d->dshift;
     r->now[COUNT] = 0;
+    r->channel_running = false;
+    r->channel_is_asynchronous = false;
 }
 
 static inline int getff (struct dma_cont *d)
@@ -327,7 +331,7 @@ void DMA_release_DREQ (int nchan)
     DMA_run();
 }
 
-static void channel_run (int ncont, int ichan)
+static void channel_do_transfer(int ncont, int ichan)
 {
     int n;
     struct dma_regs *r = &dma_controllers[ncont].regs[ichan];
@@ -351,46 +355,64 @@ static void channel_run (int ncont, int ichan)
     ldebug ("dma_pos %d size %d\n", n, (r->base[COUNT] + 1) << ncont);
 }
 
-static QEMUBH *dma_bh;
+static void channel_run(int ncont, int nchan)
+{
+    struct dma_cont *d = &dma_controllers[ncont];
+    struct dma_regs *r = &d->regs[nchan];
+
+    int mask = 1 << nchan;
+    while ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
+        assert(!r->channel_running);
+        r->channel_running = true;
+        channel_do_transfer(ncont, nchan);
+        if (r->channel_is_asynchronous) {
+            break;
+        }
+        r->channel_running = false;
+    }
+}
 
 static void DMA_run (void)
 {
-    struct dma_cont *d;
     int icont, ichan;
-    int rearm = 0;
-    static int running = 0;
-
-    if (running) {
-        rearm = 1;
-        goto out;
-    } else {
-        running = 1;
-    }
-
-    d = dma_controllers;
 
-    for (icont = 0; icont < 2; icont++, d++) {
+    for (icont = 0; icont < 2; icont++) {
         for (ichan = 0; ichan < 4; ichan++) {
-            int mask;
-
-            mask = 1 << ichan;
-
-            if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4)))) {
-                channel_run (icont, ichan);
-                rearm = 1;
+            struct dma_regs *r = &dma_controllers[icont].regs[ichan];
+            if (!r->channel_running) {
+                channel_run(icont, ichan);
             }
         }
     }
+}
 
-    running = 0;
-out:
-    if (rearm)
-        qemu_bh_schedule_idle(dma_bh);
+void DMA_set_channel_async(int nchan, bool val)
+{
+    int icont, ichan;
+    struct dma_regs *r;
+
+    icont = nchan > 3;
+    ichan = nchan & 3;
+    r = &dma_controllers[icont].regs[ichan];
+    r->channel_is_asynchronous = val;
 }
 
-static void DMA_run_bh(void *unused)
+void DMA_set_return(int nret, int nchan)
 {
-    DMA_run();
+    struct dma_regs *r;
+    struct dma_cont *d;
+    int icont, ichan;
+
+    icont = nchan > 3;
+    ichan = nchan & 3;
+    d = dma_controllers;
+    r = &d[icont].regs[ichan];
+    r->now[COUNT] = nret;
+    assert(r->channel_is_asynchronous);
+    assert(r->channel_running);
+    r->channel_running = false;
+    r->channel_is_asynchronous = false;
+    channel_run(icont, ichan);
 }
 
 void DMA_register_channel (int nchan,
@@ -560,6 +582,4 @@ void DMA_init(int high_page_enable, qemu_irq *cpu_request_exit)
               high_page_enable ? 0x488 : -1, cpu_request_exit);
     vmstate_register (NULL, 0, &vmstate_dma, &dma_controllers[0]);
     vmstate_register (NULL, 1, &vmstate_dma, &dma_controllers[1]);
-
-    dma_bh = qemu_bh_new(DMA_run_bh, NULL);
 }
diff --git a/hw/fdc.c b/hw/fdc.c
index cb4cd25..5684a05 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1189,6 +1189,7 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
     fdctrl = opaque;
     if (fdctrl->msr & FD_MSR_RQM) {
         FLOPPY_DPRINTF("Not in DMA transfer mode !\n");
+        DMA_release_DREQ(fdctrl->dma_chann);
         return 0;
     }
     cur_drv = get_cur_drv(fdctrl);
diff --git a/hw/isa.h b/hw/isa.h
index f7bc4b5..4f19c74 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -97,4 +97,6 @@ void DMA_init(int high_page_enable, qemu_irq *cpu_request_exit);
 void DMA_register_channel (int nchan,
                            DMA_transfer_handler transfer_handler,
                            void *opaque);
+void DMA_set_channel_async(int nchan, bool val);
+void DMA_set_return(int nret, int nchan);
 #endif
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 34088ad..f3b8027 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -147,6 +147,9 @@ int DMA_write_memory (int nchan, void *buf, int pos, int size)
 {
     return 0;
 }
+
+void DMA_set_channel_async(int nchan, bool val) {}
+void DMA_set_return(int nret, int nchan) {}
 void DMA_hold_DREQ (int nchan) {}
 void DMA_release_DREQ (int nchan) {}
 void DMA_schedule(int nchan) {}
diff --git a/hw/sun4u.c b/hw/sun4u.c
index fe33138..abf3426 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -110,6 +110,9 @@ int DMA_write_memory (int nchan, void *buf, int pos, int size)
 {
     return 0;
 }
+
+void DMA_set_channel_async(int nchan, bool val) {}
+void DMA_set_return(int nret, int nchan) {}
 void DMA_hold_DREQ (int nchan) {}
 void DMA_release_DREQ (int nchan) {}
 void DMA_schedule(int nchan) {}
-- 
1.7.4.1

[Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Li Zhi Hui]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
 hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 210 insertions(+), 103 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 5684a05..29ab29d 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -404,6 +404,12 @@ struct FDCtrl {
     uint8_t status0;
     uint8_t status1;
     uint8_t status2;
+    /* DMA state */
+    uint32_t dma_pos;
+    uint32_t dma_len;
+    uint32_t dma_left;
+    uint32_t dma_status0;
+    uint32_t dma_status2;
     /* Command FIFO */
     uint8_t *fifo;
     int32_t fifo_size;
@@ -429,6 +435,8 @@ struct FDCtrl {
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
+    QEMUIOVector qiov;
+    struct iovec iov;
 };
 
 typedef struct FDCtrlSysBus {
@@ -1031,6 +1039,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
+
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
                    status0, status1, status2,
                    status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl));
@@ -1043,6 +1052,8 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     fdctrl->fifo[6] = FD_SECTOR_SC;
     fdctrl->data_dir = FD_DIR_READ;
     if (!(fdctrl->msr & FD_MSR_NONDMA)) {
+        fdctrl->dma_pos = 0;
+        fdctrl->dma_len = 0;
         DMA_release_DREQ(fdctrl->dma_chann);
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
@@ -1150,7 +1161,6 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
              * recall us...
              */
             DMA_hold_DREQ(fdctrl->dma_chann);
-            DMA_schedule(fdctrl->dma_chann);
             return;
         } else {
             FLOPPY_ERROR("dma_mode=%d direction=%d\n", dma_mode, direction);
@@ -1160,7 +1170,8 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     fdctrl->msr |= FD_MSR_NONDMA;
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
-    /* IO based transfer: calculate len */
+
+    /* TODO: use AIO here and raise IRQ on the callback.  */
     fdctrl_raise_irq(fdctrl, 0x00);
 
     return;
@@ -1177,125 +1188,217 @@ static void fdctrl_start_transfer_del(FDCtrl *fdctrl, int direction)
     fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
 }
 
+static int fdctrl_end_DMA(FDCtrl *fdctrl, int ret)
+{
+    if (ret < 0) {
+        /* Sure, image size is too small... */
+        fdctrl->dma_pos = fdctrl->dma_len;
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
+        return 0;
+    } else {
+        if (FD_DID_SEEK(fdctrl->data_state)) {
+            fdctrl->dma_status0 |= FD_SR0_SEEK;
+        }
+        /* If the conditions for scan are met, or the end of the
+           track is reached, terminate the command.  */
+        if ((fdctrl->data_dir == FD_DIR_SCANE ||
+             fdctrl->data_dir == FD_DIR_SCANL ||
+             fdctrl->data_dir == FD_DIR_SCANH) &&
+            (!(fdctrl->dma_status2 & FD_SR2_SNS) ||
+             FD_DID_SEEK(fdctrl->data_state))) {
+            fdctrl->dma_pos = fdctrl->dma_len;
+        }
+
+        if (fdctrl->dma_pos == fdctrl->dma_len) {
+            fdctrl_stop_transfer(fdctrl, fdctrl->dma_status0, 0x00,
+                                 fdctrl->dma_status2);
+        }
+    }
+
+    return fdctrl->dma_pos;
+}
+
+static int fdctrl_read_DMA(FDCtrl *fdctrl, int ret)
+{
+    FDrive *cur_drv = get_cur_drv(fdctrl);
+    int nchan = fdctrl->dma_chann;
+    int len;
+
+    if (ret < 0) {
+        fdctrl->dma_status2 = 0x00;
+        return fdctrl_end_DMA(fdctrl, ret);
+    }
+
+    len = MIN(fdctrl->dma_len - fdctrl->dma_pos,
+              FD_SECTOR_LEN - fdctrl->data_pos);
+    if (fdctrl->data_dir == FD_DIR_READ) {
+        /* READ commands */
+        DMA_write_memory(nchan,
+            &fdctrl->fifo[fdctrl->data_pos], fdctrl->dma_pos, len);
+        fdctrl->dma_status2 = 0x00;
+    } else {
+        /* SCAN commands, probably utterly broken... */
+        uint8_t tmpbuf[FD_SECTOR_LEN];
+        int ret;
+        DMA_read_memory(nchan, tmpbuf, fdctrl->dma_pos, len);
+        ret = memcmp(tmpbuf, &fdctrl->fifo[fdctrl->data_pos], len);
+
+        if (0 == ret) {
+            fdctrl->dma_status2 = FD_SR2_SEH;
+        }
+        if ((ret > 0 && fdctrl->data_dir != FD_DIR_SCANH) ||
+            (ret < 0 && fdctrl->data_dir != FD_DIR_SCANL)) {
+            fdctrl->dma_status2 = FD_SR2_SNS;
+        }
+        if ((ret < 0 && fdctrl->data_dir == FD_DIR_SCANL) ||
+            (ret > 0 && fdctrl->data_dir == FD_DIR_SCANH)) {
+            fdctrl->dma_status2 = 0x00;
+        }
+    }
+    fdctrl->data_pos += len;
+    fdctrl->dma_pos += len;
+    if (fdctrl->data_pos == FD_SECTOR_LEN) {
+        /* Seek to next sector */
+        fdctrl->data_pos = 0;
+        if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
+            if (fdctrl->data_dir == FD_DIR_SCANE ||
+                fdctrl->data_dir == FD_DIR_SCANL ||
+                fdctrl->data_dir == FD_DIR_SCANH) {
+                fdctrl->dma_status2 = FD_SR2_SEH;
+            }
+            if (FD_DID_SEEK(fdctrl->data_state)) {
+                fdctrl->dma_status0 |= FD_SR0_SEEK;
+            }
+            fdctrl->dma_len = fdctrl->dma_pos;
+        }
+    }
+    return fdctrl_end_DMA(fdctrl, 0);
+}
+
+static void fdctrl_read_DMA_cb(void *opaque, int ret)
+{
+    FDCtrl *fdctrl = opaque;
+    int len = fdctrl_read_DMA(opaque, ret);
+    DMA_set_return(len, fdctrl->dma_chann);
+}
+
+static int fdctrl_write_DMA(FDCtrl *fdctrl, int ret)
+{
+    FDrive *cur_drv = get_cur_drv(fdctrl);
+    int len;
+
+    if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
+        fdctrl->dma_len = fdctrl->dma_pos;
+    }
+
+    len = fdctrl->dma_left;
+    fdctrl->dma_left = FD_SECTOR_LEN;
+    fdctrl->dma_status2 = 0x00;
+    if (ret < 0) {
+        FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
+               fd_sector(cur_drv));
+    }
+
+    return fdctrl_end_DMA(fdctrl, ret);
+}
+
+static void fdctrl_write_DMA_cb(void *opaque, int ret)
+{
+    FDCtrl *fdctrl = opaque;
+    int len = fdctrl_write_DMA(opaque, ret);
+    DMA_set_return(len, fdctrl->dma_chann);
+}
+
 /* handlers for DMA transfers */
 static int fdctrl_transfer_handler (void *opaque, int nchan,
                                     int dma_pos, int dma_len)
 {
-    FDCtrl *fdctrl;
-    FDrive *cur_drv;
-    int len, start_pos, rel_pos;
-    uint8_t status0 = 0x00, status1 = 0x00, status2 = 0x00;
+    FDCtrl *fdctrl = opaque;
+    FDrive *cur_drv = get_cur_drv(fdctrl);
+    int cur_sector = fd_sector(cur_drv);
+    int len;
+
+    assert(fdctrl->dma_chann == nchan);
 
-    fdctrl = opaque;
     if (fdctrl->msr & FD_MSR_RQM) {
         FLOPPY_DPRINTF("Not in DMA transfer mode !\n");
         DMA_release_DREQ(fdctrl->dma_chann);
         return 0;
     }
-    cur_drv = get_cur_drv(fdctrl);
-    if (fdctrl->data_dir == FD_DIR_SCANE || fdctrl->data_dir == FD_DIR_SCANL ||
-        fdctrl->data_dir == FD_DIR_SCANH)
-        status2 = FD_SR2_SNS;
-    if (dma_len > fdctrl->data_len)
+
+    if (dma_len > fdctrl->data_len) {
         dma_len = fdctrl->data_len;
+    }
     if (cur_drv->bs == NULL) {
-        if (fdctrl->data_dir == FD_DIR_WRITE)
+        if (fdctrl->data_dir == FD_DIR_WRITE) {
             fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
-        else
+        } else {
             fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
-        len = 0;
-        goto transfer_error;
-    }
-    rel_pos = fdctrl->data_pos % FD_SECTOR_LEN;
-    for (start_pos = fdctrl->data_pos; fdctrl->data_pos < dma_len;) {
-        len = dma_len - fdctrl->data_pos;
-        if (len + rel_pos > FD_SECTOR_LEN)
-            len = FD_SECTOR_LEN - rel_pos;
-        FLOPPY_DPRINTF("copy %d bytes (%d %d %d) %d pos %d %02x "
-                       "(%d-0x%08x 0x%08x)\n", len, dma_len, fdctrl->data_pos,
-                       fdctrl->data_len, GET_CUR_DRV(fdctrl), cur_drv->head,
-                       cur_drv->track, cur_drv->sect, fd_sector(cur_drv),
-                       fd_sector(cur_drv) * FD_SECTOR_LEN);
-        if (fdctrl->data_dir != FD_DIR_WRITE ||
-            len < FD_SECTOR_LEN || rel_pos != 0) {
-            /* READ & SCAN commands and realign to a sector for WRITE */
-            if (bdrv_read(cur_drv->bs, fd_sector(cur_drv),
-                          fdctrl->fifo, 1) < 0) {
-                FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
-                               fd_sector(cur_drv));
-                /* Sure, image size is too small... */
-                memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
-            }
         }
-        switch (fdctrl->data_dir) {
-        case FD_DIR_READ:
-            /* READ commands */
-            DMA_write_memory (nchan, fdctrl->fifo + rel_pos,
-                              fdctrl->data_pos, len);
-            break;
-        case FD_DIR_WRITE:
-            /* WRITE commands */
-            if (cur_drv->ro) {
-                /* Handle readonly medium early, no need to do DMA, touch the
-                 * LED or attempt any writes. A real floppy doesn't attempt
-                 * to write to readonly media either. */
-                fdctrl_stop_transfer(fdctrl,
-                                     FD_SR0_ABNTERM | FD_SR0_SEEK, FD_SR1_NW,
-                                     0x00);
-                goto transfer_error;
-            }
+    }
 
-            DMA_read_memory (nchan, fdctrl->fifo + rel_pos,
-                             fdctrl->data_pos, len);
-            if (bdrv_write(cur_drv->bs, fd_sector(cur_drv),
-                           fdctrl->fifo, 1) < 0) {
-                FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));
-                fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
-                goto transfer_error;
-            }
-            break;
-        default:
-            /* SCAN commands */
-            {
-                uint8_t tmpbuf[FD_SECTOR_LEN];
-                int ret;
-                DMA_read_memory (nchan, tmpbuf, fdctrl->data_pos, len);
-                ret = memcmp(tmpbuf, fdctrl->fifo + rel_pos, len);
-                if (ret == 0) {
-                    status2 = FD_SR2_SEH;
-                    goto end_transfer;
-                }
-                if ((ret < 0 && fdctrl->data_dir == FD_DIR_SCANL) ||
-                    (ret > 0 && fdctrl->data_dir == FD_DIR_SCANH)) {
-                    status2 = 0x00;
-                    goto end_transfer;
-                }
-            }
-            break;
-        }
-        fdctrl->data_pos += len;
-        rel_pos = fdctrl->data_pos % FD_SECTOR_LEN;
-        if (rel_pos == 0) {
-            /* Seek to next sector */
-            if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv))
-                break;
-        }
+    if (fdctrl->data_dir == FD_DIR_WRITE && cur_drv->ro) {
+        /* Handle readonly medium early, no need to do DMA, touch the
+         * LED or attempt any writes. A real floppy doesn't attempt
+         * to write to readonly media either.
+         */
+        fdctrl_stop_transfer(fdctrl,
+                             FD_SR0_ABNTERM | FD_SR0_SEEK, FD_SR1_NW,
+                             0x00);
+        return dma_len;
+     }
+
+    /* Start a new DMA operation?  */
+    assert(fdctrl->dma_pos == dma_pos);
+    if (dma_pos == 0) {
+        fdctrl->dma_status0 = 0x00;
+        fdctrl->dma_status2 = 0x00;
+        fdctrl->dma_len = dma_len;
+        fdctrl->dma_pos = dma_pos;
+        fdctrl->dma_left = FD_SECTOR_LEN;
+    }
+
+    assert(fdctrl->dma_len == dma_len);
+    assert(fdctrl->dma_pos < fdctrl->dma_len);
+
+    /* Need to read a partial buffer? */
+    if (fdctrl->data_pos > 0 && fdctrl->data_dir != FD_DIR_WRITE) {
+        return fdctrl_read_DMA(fdctrl, 0);
+    }
+
+    if (fdctrl->data_dir != FD_DIR_WRITE) {
+        DMA_set_channel_async(fdctrl->dma_chann, true);
+        bdrv_aio_readv(cur_drv->bs, cur_sector,
+            &fdctrl->qiov, 1, fdctrl_read_DMA_cb, fdctrl);
+        return 0;
     }
- end_transfer:
-    len = fdctrl->data_pos - start_pos;
-    FLOPPY_DPRINTF("end transfer %d %d %d\n",
-                   fdctrl->data_pos, len, fdctrl->data_len);
-    if (fdctrl->data_dir == FD_DIR_SCANE ||
-        fdctrl->data_dir == FD_DIR_SCANL ||
-        fdctrl->data_dir == FD_DIR_SCANH)
-        status2 = FD_SR2_SEH;
-    if (FD_DID_SEEK(fdctrl->data_state))
-        status0 |= FD_SR0_SEEK;
-    fdctrl->data_len -= len;
-    fdctrl_stop_transfer(fdctrl, status0, status1, status2);
- transfer_error:
 
-    return len;
+    /* Read up to a sector into the (repurposed) FIFO buffer and write
+     * if we complete the sector.  */
+    assert(dma_pos < dma_len || fdctrl->data_pos < FD_SECTOR_LEN);
+    len = MIN(dma_len - dma_pos, FD_SECTOR_LEN - fdctrl->data_pos);
+    FLOPPY_DPRINTF("copy %d bytes (%d %d %d) %d pos %d %02x "
+                   "(%d-0x%08x 0x%08x)\n", len, dma_len, fdctrl->data_pos,
+                   fdctrl->data_len, GET_CUR_DRV(fdctrl), cur_drv->head,
+                   cur_drv->track, cur_drv->sect, cur_sector,
+                   cur_sector * FD_SECTOR_LEN);
+
+    /* WRITE commands */
+    DMA_read_memory(nchan, &fdctrl->fifo[fdctrl->data_pos],
+                    fdctrl->dma_pos, len);
+    fdctrl->data_pos += len;
+    fdctrl->dma_pos += len;
+
+    if (fdctrl->data_pos < FD_SECTOR_LEN) {
+        fdctrl->dma_left -= len;
+        return fdctrl->dma_pos + len;
+    }
+
+    fdctrl->data_pos = 0;
+    DMA_set_channel_async(fdctrl->dma_chann, true);
+    bdrv_aio_writev(cur_drv->bs, cur_sector,
+        &fdctrl->qiov, 1, fdctrl_write_DMA_cb, fdctrl);
+    return 0;
 }
 
 /* Data register : 0x05 */
@@ -1946,6 +2049,10 @@ static int fdctrl_init_common(FDCtrl *fdctrl)
     FLOPPY_DPRINTF("init controller\n");
     fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
     fdctrl->fifo_size = 512;
+    fdctrl->iov.iov_base = fdctrl->fifo;
+    fdctrl->iov.iov_len  = FD_SECTOR_LEN;
+    qemu_iovec_init_external(&fdctrl->qiov, &fdctrl->iov, 1);
+
     fdctrl->result_timer = qemu_new_timer_ns(vm_clock,
                                           fdctrl_result_timer, fdctrl);
 
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Paolo Bonzini]

Il 15/05/2012 11:17, Li Zhi Hui ha scritto:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
> ---
>  hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 210 insertions(+), 103 deletions(-)

To reviewers, this is obviously missing migration support. :)

It is also missing a good commit message.  It should say that the
support in the existing code for scan commands has a lot of "weirdness",
for example:

-                if ((ret < 0 && fdctrl->data_dir == FD_DIR_SCANL) ||
-                    (ret > 0 && fdctrl->data_dir == FD_DIR_SCANH)) {
-                    status2 = 0x00;
-                    goto end_transfer;

...

- end_transfer:
-    len = fdctrl->data_pos - start_pos;
-    FLOPPY_DPRINTF("end transfer %d %d %d\n",
-                   fdctrl->data_pos, len, fdctrl->data_len);
-    if (fdctrl->data_dir == FD_DIR_SCANE ||
-        fdctrl->data_dir == FD_DIR_SCANL ||
-        fdctrl->data_dir == FD_DIR_SCANH)
-        status2 = FD_SR2_SEH;

which blindly overwrites status2.  Hence the new code was not written
based on it.  However, the new code is untested as far as I know.

> +            if (fdctrl->data_dir == FD_DIR_SCANE ||
> +                fdctrl->data_dir == FD_DIR_SCANL ||
> +                fdctrl->data_dir == FD_DIR_SCANH) {
> +                fdctrl->dma_status2 = FD_SR2_SEH;
> +            }

This should be FD_SR2_SNS (Spec for the FDC is at
http://www.cepece.info/amstrad/docs/i8272/8272sp.htm: "If the conditions
for scan are not met between the the starting sector (as specified by R)
and the last sector on the cylinder (EOT), then the FDC sets the SN
(Scan Not Satisfied) flag of Status Register 2 to a 1 (high), and
terminates the Scan Command").

I'm not sure whether scan commands should be kept at all though.  Hervé,
do you know anything that uses them?

Paolo

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Kevin Wolf]

Am 15.05.2012 11:27, schrieb Paolo Bonzini:
> which blindly overwrites status2.  Hence the new code was not written
> based on it.  However, the new code is untested as far as I know.

In the thread of an earlier version of this series, I said that a qtest
for floppy is required. This only confirms it.

Kevin

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Paolo Bonzini]

Il 15/05/2012 11:33, Kevin Wolf ha scritto:
>> > which blindly overwrites status2.  Hence the new code was not written
>> > based on it.  However, the new code is untested as far as I know.
> In the thread of an earlier version of this series, I said that a qtest
> for floppy is required. This only confirms it.

The problem with writing a qtest is that the spec is incredibly complex
and obscure.  It's probably even better to rip out code that cannot be
tested properly, so you don't have to test it at all...

(Mostly tongue-in-cheek of course.  A qtest for basic read/write in PIO
and DMA modes is indeed a very good idea).

Paolo

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Zhi Hui Li]

On 2012年05月15日 17:38, Paolo Bonzini wrote:
> Il 15/05/2012 11:33, Kevin Wolf ha scritto:
>>>> which blindly overwrites status2.  Hence the new code was not written
>>>> based on it.  However, the new code is untested as far as I know.
>> In the thread of an earlier version of this series, I said that a qtest
>> for floppy is required. This only confirms it.
>
> The problem with writing a qtest is that the spec is incredibly complex
> and obscure.  It's probably even better to rip out code that cannot be
> tested properly, so you don't have to test it at all...
>
> (Mostly tongue-in-cheek of course.  A qtest for basic read/write in PIO
> and DMA modes is indeed a very good idea).
>
> Paolo
>
>

Yes , I think maybe Paolo is right.

Because the spec is incredibly complex and obscure and I am newer.
To write the whole code's qtest beyond my ability. I am afraid I can't 
finish it. so I want only do a qtest about basic read/write in PIO
and DMA modes. I don't know whether it is OK.


(I don't know whether we can use qtest to replace the real test, 
especially on PIO mode 's test.)

Thank you very much.

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Paolo Bonzini]

Il 16/05/2012 10:23, Zhi Hui Li ha scritto:
> 
> Yes , I think maybe Paolo is right.
> 
> Because the spec is incredibly complex and obscure and I am newer.
> To write the whole code's qtest beyond my ability. I am afraid I can't
> finish it. so I want only do a qtest about basic read/write in PIO
> and DMA modes. I don't know whether it is OK.

That's a start, go ahead.

Paolo

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Kevin Wolf]

Am 16.05.2012 10:23, schrieb Zhi Hui Li:
> On 2012年05月15日 17:38, Paolo Bonzini wrote:
>> Il 15/05/2012 11:33, Kevin Wolf ha scritto:
>>>>> which blindly overwrites status2.  Hence the new code was not written
>>>>> based on it.  However, the new code is untested as far as I know.
>>> In the thread of an earlier version of this series, I said that a qtest
>>> for floppy is required. This only confirms it.
>>
>> The problem with writing a qtest is that the spec is incredibly complex
>> and obscure.  It's probably even better to rip out code that cannot be
>> tested properly, so you don't have to test it at all...
>>
>> (Mostly tongue-in-cheek of course.  A qtest for basic read/write in PIO
>> and DMA modes is indeed a very good idea).
>>
>> Paolo
>>
>>
> 
> Yes , I think maybe Paolo is right.
> 
> Because the spec is incredibly complex and obscure and I am newer.
> To write the whole code's qtest beyond my ability. I am afraid I can't 
> finish it. so I want only do a qtest about basic read/write in PIO
> and DMA modes. I don't know whether it is OK.

Don't worry, any test is better than no test. We should try to add a
qtest for basic operation to fdc-test.c. More detailed tests can be
added later, or maybe we find good additions during review.

I know that the floppy controller spec is hard to read. Writing test
cases basically means translating it into clearer requirements.

> (I don't know whether we can use qtest to replace the real test, 
> especially on PIO mode 's test.)

In theory yes, qtest can do everything if you have a complete set of
test cases to cover the whole spec. In practice it will just help to
find regressions earlier (floppy isn't tested very often manually).

Kevin

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Hervé Poussineau]

Hi,

Paolo Bonzini a écrit :
> Il 15/05/2012 11:17, Li Zhi Hui ha scritto:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
>> ---
>>  hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
>>  1 files changed, 210 insertions(+), 103 deletions(-)
> 
> To reviewers, this is obviously missing migration support. :)
> 
> It is also missing a good commit message.  It should say that the
> support in the existing code for scan commands has a lot of "weirdness",
> for example:
> 
> -                if ((ret < 0 && fdctrl->data_dir == FD_DIR_SCANL) ||
> -                    (ret > 0 && fdctrl->data_dir == FD_DIR_SCANH)) {
> -                    status2 = 0x00;
> -                    goto end_transfer;
> 
> ...
> 
> - end_transfer:
> -    len = fdctrl->data_pos - start_pos;
> -    FLOPPY_DPRINTF("end transfer %d %d %d\n",
> -                   fdctrl->data_pos, len, fdctrl->data_len);
> -    if (fdctrl->data_dir == FD_DIR_SCANE ||
> -        fdctrl->data_dir == FD_DIR_SCANL ||
> -        fdctrl->data_dir == FD_DIR_SCANH)
> -        status2 = FD_SR2_SEH;
> 
> which blindly overwrites status2.  Hence the new code was not written
> based on it.  However, the new code is untested as far as I know.
> 
>> +            if (fdctrl->data_dir == FD_DIR_SCANE ||
>> +                fdctrl->data_dir == FD_DIR_SCANL ||
>> +                fdctrl->data_dir == FD_DIR_SCANH) {
>> +                fdctrl->dma_status2 = FD_SR2_SEH;
>> +            }
> 
> This should be FD_SR2_SNS (Spec for the FDC is at
> http://www.cepece.info/amstrad/docs/i8272/8272sp.htm: "If the conditions
> for scan are not met between the the starting sector (as specified by R)
> and the last sector on the cylinder (EOT), then the FDC sets the SN
> (Scan Not Satisfied) flag of Status Register 2 to a 1 (high), and
> terminates the Scan Command").
> 
> I'm not sure whether scan commands should be kept at all though.  Hervé,
> do you know anything that uses them?

No, I don't know anything which uses it.
I will ack patches which remove the scan command implementations and 
call the unimplemented command handler instead (like for 
format_and_write command).

Regards,

Hervé

Re: [Qemu-devel] [PATCH 2/3 v6] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c

[Zhi Hui Li]

On 2012年05月15日 17:27, Paolo Bonzini wrote:
> Il 15/05/2012 11:17, Li Zhi Hui ha scritto:
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>> Signed-off-by: Li Zhi Hui<zhihuili@linux.vnet.ibm.com>
>> ---
>>   hw/fdc.c |  313 +++++++++++++++++++++++++++++++++++++++++--------------------
>>   1 files changed, 210 insertions(+), 103 deletions(-)
>
> To reviewers, this is obviously missing migration support. :)
>

Yes, you are right.
I am sorry that I am a newer, I don't know more about migration, so I 
want to send some simple code first .
> It is also missing a good commit message.  It should say that the
> support in the existing code for scan commands has a lot of "weirdness",
> for example:
>
> -                if ((ret<  0&&  fdctrl->data_dir == FD_DIR_SCANL) ||
> -                    (ret>  0&&  fdctrl->data_dir == FD_DIR_SCANH)) {
> -                    status2 = 0x00;
> -                    goto end_transfer;
>
> ...
>
> - end_transfer:
> -    len = fdctrl->data_pos - start_pos;
> -    FLOPPY_DPRINTF("end transfer %d %d %d\n",
> -                   fdctrl->data_pos, len, fdctrl->data_len);
> -    if (fdctrl->data_dir == FD_DIR_SCANE ||
> -        fdctrl->data_dir == FD_DIR_SCANL ||
> -        fdctrl->data_dir == FD_DIR_SCANH)
> -        status2 = FD_SR2_SEH;
>
> which blindly overwrites status2.  Hence the new code was not written
> based on it.  However, the new code is untested as far as I know.
>
Ok, I will add the commit message.
>> +            if (fdctrl->data_dir == FD_DIR_SCANE ||
>> +                fdctrl->data_dir == FD_DIR_SCANL ||
>> +                fdctrl->data_dir == FD_DIR_SCANH) {
>> +                fdctrl->dma_status2 = FD_SR2_SEH;
>> +            }
>
> This should be FD_SR2_SNS (Spec for the FDC is at
> http://www.cepece.info/amstrad/docs/i8272/8272sp.htm: "If the conditions
> for scan are not met between the the starting sector (as specified by R)
> and the last sector on the cylinder (EOT), then the FDC sets the SN
> (Scan Not Satisfied) flag of Status Register 2 to a 1 (high), and
> terminates the Scan Command").
>
ok.

Thank you very much for your feedback. :)

[Qemu-devel] [PATCH 3/3 v6] fdc.c: add tracing

[Li Zhi Hui]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
 hw/fdc.c     |   19 +++++++++++++++++++
 trace-events |    8 ++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 29ab29d..7ab8a1f 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -36,6 +36,7 @@
 #include "qdev-addr.h"
 #include "blockdev.h"
 #include "sysemu.h"
+#include "trace.h"
 
 /********************************************************/
 /* debug Floppy devices */
@@ -1039,6 +1040,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
+    trace_fdctrl_stop_transfer(cur_drv, status0, status1, status2);
 
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
                    status0, status1, status2,
@@ -1077,6 +1079,11 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
                    GET_CUR_DRV(fdctrl), kh, kt, ks,
                    fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
                                   NUM_SIDES(cur_drv)));
+
+    trace_fdctrl_start_transfer(cur_drv, direction, kh, kt, ks,
+                        fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
+                        NUM_SIDES(cur_drv)));
+
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
         /* sect too big */
@@ -1190,6 +1197,10 @@ static void fdctrl_start_transfer_del(FDCtrl *fdctrl, int direction)
 
 static int fdctrl_end_DMA(FDCtrl *fdctrl, int ret)
 {
+    trace_fdctrl_end_DMA(get_cur_drv(fdctrl), ret,
+                         fd_sector(get_cur_drv(fdctrl)), fdctrl->dma_pos,
+                         fdctrl->dma_status2);
+
     if (ret < 0) {
         /* Sure, image size is too small... */
         fdctrl->dma_pos = fdctrl->dma_len;
@@ -1224,6 +1235,10 @@ static int fdctrl_read_DMA(FDCtrl *fdctrl, int ret)
     int nchan = fdctrl->dma_chann;
     int len;
 
+    trace_fdctrl_read_DMA(cur_drv, ret, fdctrl->data_pos,
+                          fd_sector(cur_drv), fdctrl->dma_pos,
+                          fdctrl->dma_status2);
+
     if (ret < 0) {
         fdctrl->dma_status2 = 0x00;
         return fdctrl_end_DMA(fdctrl, ret);
@@ -1287,6 +1302,8 @@ static int fdctrl_write_DMA(FDCtrl *fdctrl, int ret)
     FDrive *cur_drv = get_cur_drv(fdctrl);
     int len;
 
+    trace_fdctrl_write_DMA(cur_drv, ret, fd_sector(cur_drv), fdctrl->dma_pos);
+
     if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
         fdctrl->dma_len = fdctrl->dma_pos;
     }
@@ -1319,6 +1336,8 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
     int len;
 
     assert(fdctrl->dma_chann == nchan);
+    trace_fdctrl_transfer_handler(cur_drv, dma_pos, dma_len,
+                    fdctrl->dma_pos, fdctrl->dma_len);
 
     if (fdctrl->msr & FD_MSR_RQM) {
         FLOPPY_DPRINTF("Not in DMA transfer mode !\n");
diff --git a/trace-events b/trace-events
index 87cb96c..f540302 100644
--- a/trace-events
+++ b/trace-events
@@ -805,3 +805,11 @@ qxl_render_blit_guest_primary_initialized(void) ""
 qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
 qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
 qxl_render_update_area_done(void *cookie) "%p"
+
+# hw/fdc.c
+fdctrl_stop_transfer(void *cur_drv, int status0, int status1, int status2) "drive=%p status=%02x/%02x/%02x"
+fdctrl_start_transfer(void *cur_drv, int direction, int kh, int kt, int ks, int sector) "drive=%p dir=%d h%d/t%d/s%d (sector %d)"
+fdctrl_transfer_handler(void *cur_drv, int dma_pos, int dma_len, int fdctrl_dma_pos, int fdctrl_dma_len) "drive=%p pos=%d/%d (expected %d/%d)"
+fdctrl_end_DMA(void *cur_drv, int ret, int sector, int pos, int status2) "drive=%p ret=%d sector=%d pos=%d status2=%02x"
+fdctrl_read_DMA(void *cur_drv, int ret, int data_pos, int sector, int pos, int status2) "drive=%p ret=%d data_pos=%d sector=%d pos=%d status2=%02x"
+fdctrl_write_DMA(void *cur_drv, int ret, int sector, int pos) "drive=%p ret=%d sector=%d pos=%d"
-- 
1.7.4.1