Re: [Qemu-devel] [PATCH v3] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
On Tue, Apr 3, 2012 at 12:53 PM, Zhi Hui Li wrote: > > I think what you say up is right, I will correct them, thank you very much! > >>> + bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv), >>> +&fdctrl->qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb); >>> + return dma_len; >> >> >> Should be return 0 since we haven't completed I/O yet. >> >> Stefan >> >> > > I think the return value is not used not, so no matter what is returned it > not necessary. Your patch does not change channel_run(): n = r->transfer_handler (r->opaque, ichan + (ncont << 2), r->now[COUNT], (r->base[COUNT] + 1) << ncont); r->now[COUNT] = n; So the return value will be written to COUNT and the guest will be able to see it (until we call DMA_set_return() later). Stefan
Re: [Qemu-devel] [PATCH v3] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
I think what you say up is right, I will correct them, thank you very much! +bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv), +&fdctrl->qiov, fdc_sector_num, fdctrl_read_DMA_cb, opaque_cb); +return dma_len; Should be return 0 since we haven't completed I/O yet. Stefan I think the return value is not used not, so no matter what is returned it not necessary. if set the dma_len = 0, when call void DMA_set_return(int nret) function will return the real value; :)
Re: [Qemu-devel] [PATCH v3] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
On Sat, Mar 31, 2012 at 09:19:42PM +0800, Li Zhi Hui wrote:
> Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
>
> Signed-off-by: Li Zhi Hui
> ---
> hw/dma.c | 36 +
> hw/fdc.c | 260
> +-
> hw/isa.h |1 +
> 3 files changed, 212 insertions(+), 85 deletions(-)
>
> diff --git a/hw/dma.c b/hw/dma.c
> index 0a9322d..8f4275b 100644
> --- a/hw/dma.c
> +++ b/hw/dma.c
> @@ -357,15 +357,6 @@ 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;
>
> @@ -377,15 +368,9 @@ static void DMA_run (void)
>
> if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4
> {
> channel_run (icont, ichan);
> -rearm = 1;
> }
> }
> }
> -
> -running = 0;
> -out:
> -if (rearm)
> -qemu_bh_schedule_idle(dma_bh);
> }
The 'running' variable is used to protect against reentrancy. Think
about this situation: DMA_run() -> device_foo_channel_handler(icont,
ichan) -> DMA_run().
The 'running' variable ensures that we schedule the BH and return.
Since you removed it we would run more channel handlers inside the
nested DMA_run() call.
Why did you remove 'running'?
>
> static void DMA_run_bh(void *unused)
> @@ -460,6 +445,27 @@ void DMA_schedule(int nchan)
> qemu_irq_pulse(*d->cpu_request_exit);
> }
>
> +void DMA_set_return(int nret)
> +{
> +struct dma_regs *r;
> +struct dma_cont *d;
> +int icont, ichan;
> +
> +d = dma_controllers;
> +for (icont = 0; icont < 2; icont++, d++) {
> +for (ichan = 0; ichan < 4; ichan++) {
> +int mask;
> +
> +mask = 1 << ichan;
> +
> +if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4
> {
> +r = &d[icont].regs[ichan];
> +r->now[COUNT] = nret;
> +}
> +}
> +}
> +}
I think multiple channels can be active at the same time. So we need a
int nchan argument to identify the channel, just like DMA_hold_DREQ() or
DMA_release_DREQ().
I suggest making the hw/dma.c changes in a separate patch in this series.
> /* handlers for DMA transfers */
> static int fdctrl_transfer_handler (void *opaque, int nchan,
> int dma_pos, int dma_len)
> @@ -1189,6 +1310,11 @@ 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;
> +bool write_flag = FALSE;
Please use 'true' and 'false', they are in together with the
bool type.
> +int write_sector = 0;
>
> fdctrl = opaque;
> if (fdctrl->msr & FD_MSR_RQM) {
> @@ -1196,107 +1322,101 @@ 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));
> +pfdc_string = g_malloc0(fdc_sector_num * FD_SECTOR_LEN);
bdrv_*() I/O buffers should be allocated with qemu_blockalign() and
freed with qemu_vfree().
> +opaque_cb->fdctrl = fdctrl;
> +opaque_cb->nchan = nchan;
> +opaque_cb->dma_len = dma_len;
> +
> +fdctrl->iov.iov_base = pfdc_string;
> +fdctrl->iov.iov_len = fdc_sector_num * FD_SECTOR_LEN;
> +qemu_iovec_init_external(&fdctrl->qiov, &fdctrl->iov, 1);
> +bdrv_aio_readv(cur_drv->bs, fd_sector(cur_drv),
> +&fdctrl->qiov, fdc_sector_num, fdctrl_read_DMA_cb, o
[Qemu-devel] [PATCH v3] Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
Replace bdrv_* to bdrv_aio_* functions in DMA mode in fdc.c.
Signed-off-by: Li Zhi Hui
---
hw/dma.c | 36 +
hw/fdc.c | 260 +-
hw/isa.h |1 +
3 files changed, 212 insertions(+), 85 deletions(-)
diff --git a/hw/dma.c b/hw/dma.c
index 0a9322d..8f4275b 100644
--- a/hw/dma.c
+++ b/hw/dma.c
@@ -357,15 +357,6 @@ 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;
@@ -377,15 +368,9 @@ static void DMA_run (void)
if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4 {
channel_run (icont, ichan);
-rearm = 1;
}
}
}
-
-running = 0;
-out:
-if (rearm)
-qemu_bh_schedule_idle(dma_bh);
}
static void DMA_run_bh(void *unused)
@@ -460,6 +445,27 @@ void DMA_schedule(int nchan)
qemu_irq_pulse(*d->cpu_request_exit);
}
+void DMA_set_return(int nret)
+{
+struct dma_regs *r;
+struct dma_cont *d;
+int icont, ichan;
+
+d = dma_controllers;
+for (icont = 0; icont < 2; icont++, d++) {
+for (ichan = 0; ichan < 4; ichan++) {
+int mask;
+
+mask = 1 << ichan;
+
+if ((0 == (d->mask & mask)) && (0 != (d->status & (mask << 4 {
+r = &d[icont].regs[ichan];
+r->now[COUNT] = nret;
+}
+}
+}
+}
+
static void dma_reset(void *opaque)
{
struct dma_cont *d = opaque;
diff --git a/hw/fdc.c b/hw/fdc.c
index a0236b7..afb2b65 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -429,6 +429,8 @@ struct FDCtrl {
/* Timers state */
uint8_t timer0;
uint8_t timer1;
+QEMUIOVector qiov;
+struct iovec iov;
};
typedef struct FDCtrlSysBus {
@@ -443,6 +445,12 @@ typedef struct FDCtrlISABus {
int32_t bootindexB;
} FDCtrlISABus;
+typedef struct FDC_opaque {
+FDCtrl *fdctrl;
+int nchan;
+int dma_len;
+} FDC_opaque;
+
static uint32_t fdctrl_read (void *opaque, uint32_t reg)
{
FDCtrl *fdctrl = opaque;
@@ -1154,7 +1162,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);
@@ -1181,6 +1188,120 @@ 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;
+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... */
+fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
+DMA_set_return(0);
+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 *)(fdctrl->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;
+}
+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) ||
+ (
