* [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c
@ 2012-03-19 9:17 Li Zhi Hui
2012-03-20 13:56 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: Li Zhi Hui @ 2012-03-19 9:17 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, zhihuili, pingfank, stefanha
Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
Signed-off-by: Li Zhi Hui <zhihuili@linux.vnet.ibm.com>
---
hw/fdc.c | 265 ++++++++++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 196 insertions(+), 69 deletions(-)
diff --git a/hw/fdc.c b/hw/fdc.c
index a0236b7..11d2a5c 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -443,6 +443,13 @@ typedef struct FDCtrlISABus {
int32_t bootindexB;
} FDCtrlISABus;
+typedef struct FDC_opaque {
+ FDCtrl *fdctrl;
+ QEMUIOVector *qiov;
+ int nchan;
+ int dma_len;
+} FDC_opaque;
+
static uint32_t fdctrl_read (void *opaque, uint32_t reg)
{
FDCtrl *fdctrl = opaque;
@@ -1181,6 +1188,119 @@ static void fdctrl_start_transfer_del(FDCtrl *fdctrl, int direction)
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
}
+static void fdctrl_read_DMA_cb(void *opaque, int ret)
+{
+ FDC_opaque *opaque_cb = opaque;
+ FDCtrl *fdctrl = opaque_cb->fdctrl;
+ QEMUIOVector *qiov = opaque_cb->qiov;
+ int nchan = opaque_cb->nchan;
+ int dma_len = opaque_cb->dma_len;
+ FDrive *cur_drv;
+ int len, start_pos, rel_pos;
+ uint8_t status0 = 0x00, status1 = 0x00, status2 = 0x00;
+
+ if (ret < 0) {
+ FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
+ fd_sector(cur_drv));
+ /* Sure, image size is too small... */
+ goto error;
+ }
+
+ cur_drv = get_cur_drv(fdctrl);
+ 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;
+ }
+
+ if (fdctrl->data_dir == FD_DIR_READ) {
+ /* READ commands */
+ DMA_write_memory(nchan,
+ (char *)qiov->iov->iov_base + fdctrl->data_pos + rel_pos,
+ fdctrl->data_pos, len);
+ } else {
+ /* 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;
+ }
+ }
+ 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;
+ }
+ }
+ }
+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);
+
+error:
+ qemu_iovec_destroy(qiov);
+ g_free(opaque_cb);
+ return;
+}
+
+static void fdctrl_write_DMA_cb(void *opaque, int ret)
+{
+ FDC_opaque *opaque_cb = opaque;
+ FDCtrl *fdctrl = opaque_cb->fdctrl;
+ QEMUIOVector *qiov = opaque_cb->qiov;
+ int dma_len = opaque_cb->dma_len;
+ uint8_t status0 = 0x00, status1 = 0x00, status2 = 0x00;
+
+ if (ret < 0) {
+ FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
+ fd_sector(cur_drv));
+ /* Sure, image size is too small... */
+ goto error;
+ }
+
+ FLOPPY_DPRINTF("end transfer %d %d %d\n",
+ fdctrl->data_pos, dma_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 -= dma_len;
+ fdctrl_stop_transfer(fdctrl, status0, status1, status2);
+
+error:
+ qemu_iovec_destroy(qiov);
+ g_free(opaque_cb);
+ return;
+}
+
/* handlers for DMA transfers */
static int fdctrl_transfer_handler (void *opaque, int nchan,
int dma_pos, int dma_len)
@@ -1189,6 +1309,12 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
FDrive *cur_drv;
int len, start_pos, rel_pos;
uint8_t status0 = 0x00, status1 = 0x00, status2 = 0x00;
+ int fdc_sector_num = 0;
+ uint8_t *pfdc_string = NULL;
+ FDC_opaque *opaque_cb;
+ QEMUIOVector *qiov;
+ bool write_flag = FALSE;
+ int write_sector = 0;
fdctrl = opaque;
if (fdctrl->msr & FD_MSR_RQM) {
@@ -1196,107 +1322,108 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
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)
+ 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)
- fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
- else
+ if (fdctrl->data_dir == FD_DIR_WRITE) {
+ fdctrl_stop_transfer(fdctrl,
+ FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
+ } else {
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
+ }
len = 0;
goto transfer_error;
}
+
+ if ((fdctrl->data_dir != FD_DIR_WRITE) && (fdctrl->data_pos < dma_len)) {
+ fdc_sector_num = (dma_len + FD_SECTOR_LEN - 1) / FD_SECTOR_LEN;
+ opaque_cb = g_malloc0(sizeof(FDC_opaque));
+ qiov = g_malloc0(sizeof(QEMUIOVector));
+ pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
+
+ qemu_iovec_init(qiov, 1);
+ qiov->iov->iov_base = pfdc_string;
+ qiov->iov->iov_len = fdc_sector_num * FD_SECTOR_LEN;
+ qiov->niov = 1;
+ qiov->size = fdc_sector_num * FD_SECTOR_LEN;
+ opaque_cb->fdctrl = fdctrl;
+ opaque_cb->qiov = qiov;
+ opaque_cb->nchan = nchan;
+ opaque_cb->dma_len = dma_len;
+ bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
+ qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb);
+ return dma_len;
+ }
+
+ if ((fdctrl->data_dir == FD_DIR_WRITE) && (fdctrl->data_pos < dma_len)) {
+ fdc_sector_num = (dma_len + FD_SECTOR_LEN - 1) / FD_SECTOR_LEN;
+ pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
+ write_flag = TRUE;
+ write_sector = fd_sector(cur_drv);
+ }
+
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)
+ 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,
+ /* WRITE commands */
+ DMA_read_memory(nchan, pfdc_string + fdctrl->data_pos + 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))
+ if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) {
break;
+ }
}
}
- end_transfer:
+
+ if (TRUE == write_flag) {
+ opaque_cb = g_malloc0(sizeof(FDC_opaque));
+ qiov = g_malloc0(sizeof(QEMUIOVector));
+ qemu_iovec_init(qiov, 1);
+ qiov->iov->iov_base = pfdc_string;
+ qiov->iov->iov_len = fdc_sector_num * FD_SECTOR_LEN;
+ qiov->niov = 1;
+ qiov->size = fdc_sector_num * FD_SECTOR_LEN;
+ opaque_cb->fdctrl = fdctrl;
+ opaque_cb->qiov = qiov;
+ opaque_cb->nchan = nchan;
+ opaque_cb->dma_len = dma_len;
+ bdrv_aio_writev(cur_drv->bs, write_sector,
+ qiov, fdc_sector_num, fdctrl_write_DMA_cb, opaque_cb);
+ return dma_len;
+ }
+
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)
+ 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))
+ }
+ if (FD_DID_SEEK(fdctrl->data_state)) {
status0 |= FD_SR0_SEEK;
+ }
fdctrl->data_len -= len;
fdctrl_stop_transfer(fdctrl, status0, status1, status2);
- transfer_error:
+transfer_error:
return len;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c
2012-03-19 9:17 [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c Li Zhi Hui
@ 2012-03-20 13:56 ` Stefan Hajnoczi
2012-03-22 7:06 ` liu ping fan
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Hajnoczi @ 2012-03-20 13:56 UTC (permalink / raw)
To: Li Zhi Hui; +Cc: kwolf, pingfank, qemu-devel
On Mon, Mar 19, 2012 at 05:17:15PM +0800, Li Zhi Hui wrote:
> @@ -1196,107 +1322,108 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
> 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)
> + 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)
> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
> - else
> + if (fdctrl->data_dir == FD_DIR_WRITE) {
> + fdctrl_stop_transfer(fdctrl,
> + FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
> + } else {
> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
> + }
> len = 0;
> goto transfer_error;
> }
> +
> + if ((fdctrl->data_dir != FD_DIR_WRITE) && (fdctrl->data_pos < dma_len)) {
> + fdc_sector_num = (dma_len + FD_SECTOR_LEN - 1) / FD_SECTOR_LEN;
> + opaque_cb = g_malloc0(sizeof(FDC_opaque));
I think we can only have 1 I/O pending at a time. Therefore it's
probably not necessary to create a separate struct, we can just pass the
FDrive/FDCtrl.
> + qiov = g_malloc0(sizeof(QEMUIOVector));
This is leaked. I think it can be a field in opaque_cb, there's no need
to allocate this separately on the heap.
> + pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
Looks like this buffer is leaked. A block I/O buffer should be
allocated with qemu_blockalign() instead of g_malloc0() so that memory
alignment requirements for O_DIRECT image files are met.
Would it be possible to use the fifo[] buffer instead of allocating a
new buffer?
> +
> + qemu_iovec_init(qiov, 1);
> + qiov->iov->iov_base = pfdc_string;
> + qiov->iov->iov_len = fdc_sector_num * FD_SECTOR_LEN;
> + qiov->niov = 1;
> + qiov->size = fdc_sector_num * FD_SECTOR_LEN;
The easiest way to do this is:
iov.iov_base = fifo;
iov.iov_len = fdc_sector_num * FD_SECTOR_LEN;
qemu_iovec_init_external(qiov, iov, 1);
We shouldn't duplicate the qiov->size calculation - that's already
provided by qemu_iovec_init_external() or qemu_iovec_add().
> + opaque_cb->fdctrl = fdctrl;
> + opaque_cb->qiov = qiov;
> + opaque_cb->nchan = nchan;
> + opaque_cb->dma_len = dma_len;
> + bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
> + qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb);
> + return dma_len;
We are returning dma_len but the I/O has not yet happened. The guest
could see that the DMA controller register has updated before we've
actually transferred data. This seems risky.
Have you checked what hw/dma.c does after we return? The DMA operation
has not completed yet so I wonder if it will call
fdctrl_transfer_handler() again from DMA_run()?
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c
2012-03-20 13:56 ` Stefan Hajnoczi
@ 2012-03-22 7:06 ` liu ping fan
2012-03-22 10:15 ` Stefan Hajnoczi
0 siblings, 1 reply; 4+ messages in thread
From: liu ping fan @ 2012-03-22 7:06 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pingfank, Li Zhi Hui, qemu-devel
On Tue, Mar 20, 2012 at 9:56 PM, Stefan Hajnoczi
<stefanha@linux.vnet.ibm.com> wrote:
> On Mon, Mar 19, 2012 at 05:17:15PM +0800, Li Zhi Hui wrote:
>> @@ -1196,107 +1322,108 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
>> 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)
>> + 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)
>> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
>> - else
>> + if (fdctrl->data_dir == FD_DIR_WRITE) {
>> + fdctrl_stop_transfer(fdctrl,
>> + FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
>> + } else {
>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
>> + }
>> len = 0;
>> goto transfer_error;
>> }
>> +
>> + if ((fdctrl->data_dir != FD_DIR_WRITE) && (fdctrl->data_pos < dma_len)) {
>> + fdc_sector_num = (dma_len + FD_SECTOR_LEN - 1) / FD_SECTOR_LEN;
>> + opaque_cb = g_malloc0(sizeof(FDC_opaque));
>
> I think we can only have 1 I/O pending at a time. Therefore it's
> probably not necessary to create a separate struct, we can just pass the
> FDrive/FDCtrl.
>
>> + qiov = g_malloc0(sizeof(QEMUIOVector));
>
> This is leaked. I think it can be a field in opaque_cb, there's no need
> to allocate this separately on the heap.
>
>> + pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
>
> Looks like this buffer is leaked. A block I/O buffer should be
> allocated with qemu_blockalign() instead of g_malloc0() so that memory
> alignment requirements for O_DIRECT image files are met.
>
> Would it be possible to use the fifo[] buffer instead of allocating a
> new buffer?
>
>> +
>> + qemu_iovec_init(qiov, 1);
>> + qiov->iov->iov_base = pfdc_string;
>> + qiov->iov->iov_len = fdc_sector_num * FD_SECTOR_LEN;
>> + qiov->niov = 1;
>> + qiov->size = fdc_sector_num * FD_SECTOR_LEN;
>
> The easiest way to do this is:
>
> iov.iov_base = fifo;
> iov.iov_len = fdc_sector_num * FD_SECTOR_LEN;
> qemu_iovec_init_external(qiov, iov, 1);
>
> We shouldn't duplicate the qiov->size calculation - that's already
> provided by qemu_iovec_init_external() or qemu_iovec_add().
>
>> + opaque_cb->fdctrl = fdctrl;
>> + opaque_cb->qiov = qiov;
>> + opaque_cb->nchan = nchan;
>> + opaque_cb->dma_len = dma_len;
>> + bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
>> + qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb);
>> + return dma_len;
>
> We are returning dma_len but the I/O has not yet happened. The guest
> could see that the DMA controller register has updated before we've
> actually transferred data. This seems risky.
>
I think that fdctrl_stop_transfer() update the FDCtrl, so until then,
the guest driver will see the update.
> Have you checked what hw/dma.c does after we return? The DMA operation
> has not completed yet so I wonder if it will call
> fdctrl_transfer_handler() again from DMA_run()?
>
> Stefan
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c
2012-03-22 7:06 ` liu ping fan
@ 2012-03-22 10:15 ` Stefan Hajnoczi
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2012-03-22 10:15 UTC (permalink / raw)
To: liu ping fan; +Cc: kwolf, Li Zhi Hui, pingfank, Stefan Hajnoczi, qemu-devel
On Thu, Mar 22, 2012 at 7:06 AM, liu ping fan <qemulist@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 9:56 PM, Stefan Hajnoczi
> <stefanha@linux.vnet.ibm.com> wrote:
>> On Mon, Mar 19, 2012 at 05:17:15PM +0800, Li Zhi Hui wrote:
>>> @@ -1196,107 +1322,108 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
>>> 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)
>>> + 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)
>>> - fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
>>> - else
>>> + if (fdctrl->data_dir == FD_DIR_WRITE) {
>>> + fdctrl_stop_transfer(fdctrl,
>>> + FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
>>> + } else {
>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, 0x00, 0x00);
>>> + }
>>> len = 0;
>>> goto transfer_error;
>>> }
>>> +
>>> + if ((fdctrl->data_dir != FD_DIR_WRITE) && (fdctrl->data_pos < dma_len)) {
>>> + fdc_sector_num = (dma_len + FD_SECTOR_LEN - 1) / FD_SECTOR_LEN;
>>> + opaque_cb = g_malloc0(sizeof(FDC_opaque));
>>
>> I think we can only have 1 I/O pending at a time. Therefore it's
>> probably not necessary to create a separate struct, we can just pass the
>> FDrive/FDCtrl.
>>
>>> + qiov = g_malloc0(sizeof(QEMUIOVector));
>>
>> This is leaked. I think it can be a field in opaque_cb, there's no need
>> to allocate this separately on the heap.
>>
>>> + pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
>>
>> Looks like this buffer is leaked. A block I/O buffer should be
>> allocated with qemu_blockalign() instead of g_malloc0() so that memory
>> alignment requirements for O_DIRECT image files are met.
>>
>> Would it be possible to use the fifo[] buffer instead of allocating a
>> new buffer?
>>
>>> +
>>> + qemu_iovec_init(qiov, 1);
>>> + qiov->iov->iov_base = pfdc_string;
>>> + qiov->iov->iov_len = fdc_sector_num * FD_SECTOR_LEN;
>>> + qiov->niov = 1;
>>> + qiov->size = fdc_sector_num * FD_SECTOR_LEN;
>>
>> The easiest way to do this is:
>>
>> iov.iov_base = fifo;
>> iov.iov_len = fdc_sector_num * FD_SECTOR_LEN;
>> qemu_iovec_init_external(qiov, iov, 1);
>>
>> We shouldn't duplicate the qiov->size calculation - that's already
>> provided by qemu_iovec_init_external() or qemu_iovec_add().
>>
>>> + opaque_cb->fdctrl = fdctrl;
>>> + opaque_cb->qiov = qiov;
>>> + opaque_cb->nchan = nchan;
>>> + opaque_cb->dma_len = dma_len;
>>> + bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
>>> + qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb);
>>> + return dma_len;
>>
>> We are returning dma_len but the I/O has not yet happened. The guest
>> could see that the DMA controller register has updated before we've
>> actually transferred data. This seems risky.
>>
> I think that fdctrl_stop_transfer() update the FDCtrl, so until then,
> the guest driver will see the update.
You're referring to the fdc device state but I meant the ISA DMA
controller registers. hw/dma.c:read_chan() will expose the
incremented value to the guest before we've transferred any data. I
think we shouldn't increment the DMA transfer byte count before it has
completed.
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-03-22 10:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-19 9:17 [Qemu-devel] [PATCH] use bdrv_aio functions in fdc.c Li Zhi Hui
2012-03-20 13:56 ` Stefan Hajnoczi
2012-03-22 7:06 ` liu ping fan
2012-03-22 10:15 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).