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 <js...@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
>
