|
| From: | John Snow |
| Subject: | Re: [Qemu-devel] [RFC 02/10] AHCI: Update byte count after DMA completion |
| Date: | Mon, 15 Sep 2014 16:07:31 -0400 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.7.0 |
On 09/13/2014 09:21 AM, Paolo Bonzini wrote:
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.
OK. I tried to clarify below with what I was trying to do, please let me know if you want me to take it in a different direction.
Signed-off-by: John Snow <address@hidden> --- 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).
No reason other than I wasn't completely sure of how unref operated. Was just being very conservative in my PoC. I wanted to be able to be lazy and call sglist_destroy with or without "has_sglist" elsewhere in the DMA code, so the bool just got kicked elsewhere.
I will delete it.
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.
In my mind, I was trying to create an analog to "prepare_buf," where prepare_buf is responsible for instantiating the sglist, and "commit_buf" is responsible for tearing it down.
I felt that since the preparation callback instantiated it, it would make sense to keep the teardown in the same layer.
+ qemu_sglist_destroy(&s->sg); + + return s->sg.size != 0;Also, there's no reason to keep the return value (which is always false).
Yes, not strictly meaningful. Just a holdover from hunting down the short PRDT issues. I will just get rid of the return value.
+} + 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.
As above, the ahci layer is already messing around with &s->sg during preparation, so I don't see why I'd try to abstract this away from parts of that file.
Unless I'm misunderstanding you.
- /* 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.
As long as I add the callback to the PCI BMDMA layer, you're right. If we follow along with my "create" and "destroy" analogy above, we can just do the dispatch here and let PCI or AHCI do the teardown without having a fallback.
+ 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.
Sure, I can add that small cleanup.
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
| [Prev in Thread] | Current Thread | [Next in Thread] |