From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53291) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SB1tU-0006PO-1N for qemu-devel@nongnu.org; Fri, 23 Mar 2012 06:38:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SB1tN-0004QA-6z for qemu-devel@nongnu.org; Fri, 23 Mar 2012 06:38:51 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:45146) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SB1tM-0004PR-S5 for qemu-devel@nongnu.org; Fri, 23 Mar 2012 06:38:45 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 23 Mar 2012 10:38:40 -0000 Received: from d06av01.portsmouth.uk.ibm.com (d06av01.portsmouth.uk.ibm.com [9.149.37.212]) by d06nrmr1806.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q2NAcHK62678802 for ; Fri, 23 Mar 2012 10:38:17 GMT Received: from d06av01.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av01.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q2NAcHrC024881 for ; Fri, 23 Mar 2012 04:38:17 -0600 Date: Fri, 23 Mar 2012 08:47:00 +0000 From: Stefan Hajnoczi Message-ID: <20120323084700.GB21619@stefanha-thinkpad.localdomain> References: <1332486440-12512-1-git-send-email-zhihuili@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1332486440-12512-1-git-send-email-zhihuili@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH] Replace bdrv_* to bdrv_aio_* functions in pio mode in fdc.c. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Li Zhi Hui Cc: kwolf@redhat.com, pingfank@linux.vnet.ibm.com, qemu-devel@nongnu.org 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 > --- > 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