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

[Stefan Hajnoczi]

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 <address@hidden>
> ---
>  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 <stdbool.h> 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, opaque_cb);
> +        return dma_len;

Should be return 0 since we haven't completed I/O yet.

Stefan