From: Denis V. Lunev <[email protected]> ahci_map_clb_address() already clears cur_cmd, so every consumer of it has to cope with there being no current command. ahci_pio_transfer(), ahci_commit_buf() and ahci_populate_sglist() all dereference it unconditionally instead.
Give the three of them a NULL check. Declaring the data transferred anyway is not enough: ide_transfer_start() goes on to call the end transfer function, and for a multi-sector write that is ide_sector_write(), which commits an io_buffer the guest never refilled. Clearing PxCMD.ST during a WRITE SECTOR(S) of two sectors therefore writes the first sector's contents over the second, at a sector the guest chose. Let pio_transfer report that nothing was transferred and halt there, so no callback acts on a buffer that was never filled. Only the AHCI HBA implements the callback, so the signature change is local to it. Cc: John Snow <[email protected]> Cc: Philippe Mathieu-Daudé <[email protected]> Signed-off-by: Denis V. Lunev <[email protected]> --- hw/ide/ahci.c | 47 +++++++++++++++++++++++++++++++--------- hw/ide/core.c | 11 +++++++++- hw/ide/trace-events | 2 ++ include/hw/ide/ide-dma.h | 3 ++- 4 files changed, 51 insertions(+), 12 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 49f3047e6f..995b40efd5 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -906,12 +906,12 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl) static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, AHCICmdHdr *cmd, int64_t limit, uint64_t offset) { - uint16_t opts = le16_to_cpu(cmd->opts); - uint16_t prdtl = le16_to_cpu(cmd->prdtl); - uint64_t cfis_addr = le64_to_cpu(cmd->tbl_addr); - uint64_t prdt_addr = cfis_addr + 0x80; - dma_addr_t prdt_len = (prdtl * sizeof(AHCI_SG)); - dma_addr_t real_prdt_len = prdt_len; + uint16_t opts; + uint16_t prdtl; + uint64_t cfis_addr; + uint64_t prdt_addr; + dma_addr_t prdt_len; + dma_addr_t real_prdt_len; uint8_t *prdt; int i; int r = 0; @@ -923,6 +923,18 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, trace_ahci_populate_sglist(ad->hba, ad->port_no); + if (!cmd) { + trace_ahci_populate_sglist_no_cmd(ad->hba, ad->port_no); + return -1; + } + + opts = le16_to_cpu(cmd->opts); + prdtl = le16_to_cpu(cmd->prdtl); + cfis_addr = le64_to_cpu(cmd->tbl_addr); + prdt_addr = cfis_addr + 0x80; + prdt_len = (prdtl * sizeof(AHCI_SG)); + real_prdt_len = prdt_len; + if (!prdtl) { trace_ahci_populate_sglist_no_prdtl(ad->hba, ad->port_no, opts); return -1; @@ -1371,18 +1383,27 @@ out: } /* Transfer PIO data between RAM and device */ -static void ahci_pio_transfer(const IDEDMA *dma) +static bool ahci_pio_transfer(const IDEDMA *dma) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; uint32_t size = (uint32_t)(s->data_end - s->data_ptr); /* write == ram -> device */ - uint16_t opts = le16_to_cpu(ad->cur_cmd->opts); - int is_write = opts & AHCI_CMD_WRITE; - int is_atapi = opts & AHCI_CMD_ATAPI; + uint16_t opts; + int is_write; + int is_atapi; int has_sglist = 0; bool pio_fis_i; + if (ad->cur_cmd == NULL) { + trace_ahci_pio_transfer_no_cmd(ad->hba, ad->port_no); + return false; + } + + opts = le16_to_cpu(ad->cur_cmd->opts); + is_write = opts & AHCI_CMD_WRITE; + is_atapi = opts & AHCI_CMD_ATAPI; + /* The PIO Setup FIS is received prior to transfer, but the interrupt * is only triggered after data is received. * @@ -1430,6 +1451,8 @@ out: if (pio_fis_i) { ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_PSS); } + + return true; } static void ahci_start_dma(const IDEDMA *dma, IDEState *s, @@ -1492,6 +1515,10 @@ static void ahci_commit_buf(const IDEDMA *dma, uint32_t tx_bytes) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); + if (ad->cur_cmd == NULL) { + return; + } + tx_bytes += le32_to_cpu(ad->cur_cmd->status); ad->cur_cmd->status = cpu_to_le32(tx_bytes); } diff --git a/hw/ide/core.c b/hw/ide/core.c index 0dca2b5c52..06c18dbf09 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -80,6 +80,7 @@ static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval) } static void ide_dummy_transfer_stop(IDEState *s); +static void ide_transfer_halt(IDEState *s); const MemoryRegionPortio ide_portio_list[] = { { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write }, @@ -568,7 +569,15 @@ bool ide_transfer_start_norecurse(IDEState *s, uint8_t *buf, int size, s->end_transfer_func = end_transfer_func; return false; } - s->bus->dma->ops->pio_transfer(s->bus->dma); + if (!s->bus->dma->ops->pio_transfer(s->bus->dma)) { + /* + * No data reached the buffer, so the caller must not act on it. A + * write would otherwise commit whatever the previous phase left + * there to the next sector. + */ + ide_transfer_halt(s); + return false; + } return true; } diff --git a/hw/ide/trace-events b/hw/ide/trace-events index 57042cafdd..f1472f5852 100644 --- a/hw/ide/trace-events +++ b/hw/ide/trace-events @@ -85,6 +85,7 @@ ahci_reset_port(void *s, int port) "ahci(%p)[%d]: reset port" ahci_unmap_fis_address_null(void *s, int port) "ahci(%p)[%d]: Attempt to unmap NULL FIS address" ahci_unmap_clb_address_null(void *s, int port) "ahci(%p)[%d]: Attempt to unmap NULL CLB address" ahci_populate_sglist(void *s, int port) "ahci(%p)[%d]" +ahci_populate_sglist_no_cmd(void *s, int port) "ahci(%p)[%d]: no command header" ahci_populate_sglist_no_prdtl(void *s, int port, uint16_t opts) "ahci(%p)[%d]: no sg list given by guest: 0x%04x" ahci_populate_sglist_no_map(void *s, int port) "ahci(%p)[%d]: DMA mapping failed" ahci_populate_sglist_short_map(void *s, int port) "ahci(%p)[%d]: mapped less than expected" @@ -109,6 +110,7 @@ handle_cmd_badfis(void *s, int port) "ahci(%p)[%d]: guest provided an invalid cm handle_cmd_badmap(void *s, int port, uint64_t len) "ahci(%p)[%d]: dma_memory_map failed, 0x%02"PRIx64" != 0x80" handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x" ahci_pio_transfer(void *s, int port, const char *rw, uint32_t size, const char *tgt, const char *sgl) "ahci(%p)[%d]: %sing %d bytes on %s w/%s sglist" +ahci_pio_transfer_no_cmd(void *s, int port) "ahci(%p)[%d]: PIO transfer without a command header" ahci_start_dma(void *s, int port) "ahci(%p)[%d]: start dma" ahci_dma_prepare_buf(void *s, int port, int32_t io_buffer_size, int32_t limit) "ahci(%p)[%d]: prepare buf limit=%"PRId32" prepared=%"PRId32 ahci_dma_prepare_buf_fail(void *s, int port) "ahci(%p)[%d]: sglist population failed" diff --git a/include/hw/ide/ide-dma.h b/include/hw/ide/ide-dma.h index 296010a4e0..34154b7cbc 100644 --- a/include/hw/ide/ide-dma.h +++ b/include/hw/ide/ide-dma.h @@ -10,6 +10,7 @@ typedef struct IDEDMA IDEDMA; typedef void DMAStartFunc(const IDEDMA *, IDEState *, BlockCompletionFunc *); typedef void DMAVoidFunc(const IDEDMA *); +typedef bool DMABoolFunc(const IDEDMA *); typedef int DMAIntFunc(const IDEDMA *, bool); typedef int32_t DMAInt32Func(const IDEDMA *, int32_t len); typedef void DMAu32Func(const IDEDMA *, uint32_t); @@ -17,7 +18,7 @@ typedef void DMAStopFunc(const IDEDMA *, bool); struct IDEDMAOps { DMAStartFunc *start_dma; - DMAVoidFunc *pio_transfer; + DMABoolFunc *pio_transfer; DMAInt32Func *prepare_buf; DMAu32Func *commit_buf; DMAIntFunc *rw_buf; -- 2.53.0
