Re: [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion

[Paolo Bonzini <pbonzini@redhat.com>]

Il 13/09/2014 06:34, John Snow ha scritto:
> Currently, DMA read/write operations neglect to update
> the byte count after a successful transfer like ATAPI
> DMA read or PIO read/write operations do.
> 
> We correct this oversight by adding another callback into
> the IDEDMAOps structure. The commit callback is called
> whenever we are cleaning up a scatter-gather list.
> AHCI can register this callback in order to update post-
> transfer information such as byte count updates.
> 
> We use this callback in AHCI to consolidate where we delete
> the SGlist as generated from the PRDT, as well as update the
> byte count after the transfer is complete.
> 
> The QEMUSGList structure has an init flag added to it in order
> to make qemu_sglist_destroy a nop if it is called when
> there is no sglist, which simplifies cleanup and error paths.
> 
> This patch fixes several AHCI problems, notably Non-NCQ modes
> of operation for Windows 7 as well as Hibernate support for Windows 7.

I think this patch is touching the internals more than it should.
There's obviously some good stuff here, but the abstractions needed to
fix the problem are already there.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  dma-helpers.c        |  5 +++++
>  hw/ide/ahci.c        | 47 +++++++++++++++++++++++++++++++++++++----------
>  hw/ide/core.c        | 14 +++++++++-----
>  hw/ide/internal.h    |  1 +
>  include/sysemu/dma.h |  1 +
>  5 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 499b52b..ba965a3 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -45,6 +45,7 @@ void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
>      qsg->as = as;
>      qsg->dev = dev;
>      object_ref(OBJECT(dev));
> +    qsg->init = true;
>  }
>  
>  void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
> @@ -61,6 +62,10 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
>  
>  void qemu_sglist_destroy(QEMUSGList *qsg)
>  {
> +    if (!qsg->init) {
> +        return;
> +    }
> +
>      object_unref(OBJECT(qsg->dev));
>      g_free(qsg->sg);
>      memset(qsg, 0, sizeof(*qsg));

Why do you need this?  qemu_sglist_destroy is idempotent (neither free
nor unref of NULL cause problems).

> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 0ee713b..d3ece78 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -48,6 +48,9 @@ static int handle_cmd(AHCIState *s,int port,int slot);
>  static void ahci_reset_port(AHCIState *s, int port);
>  static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
>  static void ahci_init_d2h(AHCIDevice *ad);
> +static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write);
> +static int ahci_commit_buf(IDEDMA *dma, int xfer_ok);
> +
>  
>  static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
>  {
> @@ -1084,16 +1087,12 @@ static void ahci_start_transfer(IDEDMA *dma)
>          }
>      }
>  
> -    /* update number of transferred bytes */
> -    ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + size);
> -
>  out:
>      /* declare that we processed everything */
>      s->data_ptr = s->data_end;
>  
> -    if (has_sglist) {
> -        qemu_sglist_destroy(&s->sg);
> -    }
> +    /* Update number of transferred bytes, destroy sglist */
> +    ahci_commit_buf(dma, true);
>  
>      s->end_transfer_func(s);
>  
> @@ -1114,6 +1113,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
>      dma_cb(s, 0);
>  }
>  
> +/**
> + * Called in DMA R/W chains to read the PRDTL, utilizing ahci_populate_sglist.
> + * Not currently invoked by PIO R/W chains,
> + * which invoke ahci_populate_sglist via ahci_start_transfer.
> + */
>  static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> @@ -1126,6 +1130,30 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
>      return s->io_buffer_size != 0;
>  }
>  
> +/**
> + * Destroys the scatter-gather list,
> + * and updates the command header with a bytes-read value.
> + * called explicitly via ahci_dma_rw_buf (ATAPI DMA),
> + * and ahci_start_transfer (PIO R/W),
> + * and called via callback from ide_dma_cb for DMA R/W paths.
> + */
> +static int ahci_commit_buf(IDEDMA *dma, int xfer_ok)
> +{
> +    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> +    IDEState *s = &ad->port.ifs[0];
> +    uint32_t byte_count;
> +
> +    if (xfer_ok) {
> +        byte_count = le32_to_cpu(ad->cur_cmd->status);
> +        byte_count += s->sg.size;
> +        ad->cur_cmd->status = cpu_to_le32(byte_count);
> +    }

If you leave qemu_sglist_destroy in the caller, you do not need to call
ahci_commit_buf in the error cases.  Maybe I'm wrong, but I have a
feeling that it would be much simpler to me if your commit hook is simply

       byte_count += le32_to_cpu(ad->cur_cmd->status);
       ad->cur_cmd->status = cpu_to_le32(byte_count);

where byte_count is the second argument to the hook.  The callers can
pass 0 in the error case, s->sg.size otherwise.

> +    qemu_sglist_destroy(&s->sg);
> +
> +    return s->sg.size != 0;

Also, there's no reason to keep the return value (which is always false).

> +}
> +
>  static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> @@ -1143,11 +1171,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>          dma_buf_write(p, l, &s->sg);
>      }
>  
> -    /* free sglist that was created in ahci_populate_sglist() */
> -    qemu_sglist_destroy(&s->sg);
> +    /* free sglist, update byte count */
> +    ahci_commit_buf(dma, true);

Perhaps you should make dma_buf_commit public (and add the call to the
hook), so that ahci_commit_buf does not have to mess with &s->sg.

> -    /* update number of transferred bytes */
> -    ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
>      s->io_buffer_index += l;
>      s->io_buffer_offset += l;
>  
> @@ -1190,6 +1216,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .start_dma = ahci_start_dma,
>      .start_transfer = ahci_start_transfer,
>      .prepare_buf = ahci_dma_prepare_buf,
> +    .commit_buf = ahci_commit_buf,
>      .rw_buf = ahci_dma_rw_buf,
>      .set_unit = ahci_dma_set_unit,
>      .cmd_done = ahci_cmd_done,
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3d682e2..b2980e9 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -629,9 +629,13 @@ void ide_sector_read(IDEState *s)
>                                    ide_sector_read_cb, s);
>  }
>  
> -static void dma_buf_commit(IDEState *s)
> +static void dma_buf_commit(IDEState *s, int xfer_ok)
>  {
> -    qemu_sglist_destroy(&s->sg);
> +    if (s->bus->dma->ops->commit_buf) {
> +        s->bus->dma->ops->commit_buf(s->bus->dma, xfer_ok);
> +    } else {

The "else" is not necessary, I think.

> +        qemu_sglist_destroy(&s->sg);
> +    }
>  }
>  
>  void ide_set_inactive(IDEState *s, bool more)
> @@ -660,7 +664,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>          s->bus->error_status = op;
>      } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>          if (op & IDE_RETRY_DMA) {
> -            dma_buf_commit(s);
> +            dma_buf_commit(s, false);

For the two error cases, it makes sense to move the call to
dma_buf_commit from the callers to ide_dma_error.

>              ide_dma_error(s);
>          } else {
>              ide_rw_error(s);
> @@ -701,7 +705,7 @@ void ide_dma_cb(void *opaque, int ret)
>  
>      sector_num = ide_get_sector(s);
>      if (n > 0) {
> -        dma_buf_commit(s);
> +        dma_buf_commit(s, true);
>          sector_num += n;
>          ide_set_sector(s, sector_num);
>          s->nsector -= n;
> @@ -732,7 +736,7 @@ void ide_dma_cb(void *opaque, int ret)
>  
>      if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) &&
>          !ide_sect_range_ok(s, sector_num, n)) {
> -        dma_buf_commit(s);
> +        dma_buf_commit(s, false);
>          ide_dma_error(s);
>          return;
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 72d0147..f190e8c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -432,6 +432,7 @@ struct IDEDMAOps {
>      DMAStartFunc *start_dma;
>      DMAVoidFunc *start_transfer;
>      DMAIntFunc *prepare_buf;
> +    DMAIntFunc *commit_buf;
>      DMAIntFunc *rw_buf;
>      DMAIntFunc *set_unit;
>      DMAStopFunc *set_inactive;
> diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
> index 00f21f3..8d2c4d6 100644
> --- a/include/sysemu/dma.h
> +++ b/include/sysemu/dma.h
> @@ -31,6 +31,7 @@ struct QEMUSGList {
>      size_t size;
>      DeviceState *dev;
>      AddressSpace *as;
> +    bool init;
>  };
>  
>  #ifndef CONFIG_USER_ONLY
>