From 266e77812ce1630f3f3ff02aadccb151aa01e6f6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:38 -0500 Subject: [PATCH 01/11] ide: Prohibit RESET on IDE drives This command is meant for ATAPI devices only, prohibit acknowledging it with a command aborted response when an IDE device is busy. Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-id: 1453225191-11871-2-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 4c46453e270f..88d5fabcf3ab 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1877,9 +1877,13 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val) return; } - /* Only DEVICE RESET is allowed while BSY or/and DRQ are set */ - if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET) - return; + /* Only RESET is allowed while BSY and/or DRQ are set, + * and only to ATAPI devices. */ + if (s->status & (BUSY_STAT|DRQ_STAT)) { + if (val != WIN_DEVICE_RESET || s->drive_kind != IDE_CD) { + return; + } + } if (!ide_cmd_permitted(s, val)) { ide_abort_command(s); From 4590355bb762b6a1bd46968f3b6e85ff319e4141 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:39 -0500 Subject: [PATCH 02/11] ide: code motion Shuffle the reset function upwards. Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-id: 1453225191-11871-3-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 116 +++++++++++++++++++++++++------------------------- 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 88d5fabcf3ab..37d105841008 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -1175,6 +1175,64 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val) } } +static void ide_reset(IDEState *s) +{ +#ifdef DEBUG_IDE + printf("ide: reset\n"); +#endif + + if (s->pio_aiocb) { + blk_aio_cancel(s->pio_aiocb); + s->pio_aiocb = NULL; + } + + if (s->drive_kind == IDE_CFATA) + s->mult_sectors = 0; + else + s->mult_sectors = MAX_MULT_SECTORS; + /* ide regs */ + s->feature = 0; + s->error = 0; + s->nsector = 0; + s->sector = 0; + s->lcyl = 0; + s->hcyl = 0; + + /* lba48 */ + s->hob_feature = 0; + s->hob_sector = 0; + s->hob_nsector = 0; + s->hob_lcyl = 0; + s->hob_hcyl = 0; + + s->select = 0xa0; + s->status = READY_STAT | SEEK_STAT; + + s->lba48 = 0; + + /* ATAPI specific */ + s->sense_key = 0; + s->asc = 0; + s->cdrom_changed = 0; + s->packet_transfer_size = 0; + s->elementary_transfer_size = 0; + s->io_buffer_index = 0; + s->cd_sector_size = 0; + s->atapi_dma = 0; + s->tray_locked = 0; + s->tray_open = 0; + /* ATA DMA state */ + s->io_buffer_size = 0; + s->req_nb_sectors = 0; + + ide_set_signature(s); + /* init the transfer handler so that 0xffff is returned on data + accesses */ + s->end_transfer_func = ide_dummy_transfer_stop; + ide_dummy_transfer_stop(s); + s->media_changed = 0; +} + static bool cmd_nop(IDEState *s, uint8_t cmd) { return true; @@ -2183,64 +2241,6 @@ static void ide_dummy_transfer_stop(IDEState *s) s->io_buffer[3] = 0xff; } -static void ide_reset(IDEState *s) -{ -#ifdef DEBUG_IDE - printf("ide: reset\n"); -#endif - - if (s->pio_aiocb) { - blk_aio_cancel(s->pio_aiocb); - s->pio_aiocb = NULL; - } - - if (s->drive_kind == IDE_CFATA) - s->mult_sectors = 0; - else - s->mult_sectors = MAX_MULT_SECTORS; - /* ide regs */ - s->feature = 0; - s->error = 0; - s->nsector = 0; - s->sector = 0; - s->lcyl = 0; - s->hcyl = 0; - - /* lba48 */ - s->hob_feature = 0; - s->hob_sector = 0; - s->hob_nsector = 0; - s->hob_lcyl = 0; - s->hob_hcyl = 0; - - s->select = 0xa0; - s->status = READY_STAT | SEEK_STAT; - - s->lba48 = 0; - - /* ATAPI specific */ - s->sense_key = 0; - s->asc = 0; - s->cdrom_changed = 0; - s->packet_transfer_size = 0; - s->elementary_transfer_size = 0; - s->io_buffer_index = 0; - s->cd_sector_size = 0; - s->atapi_dma = 0; - s->tray_locked = 0; - s->tray_open = 0; - /* ATA DMA state */ - s->io_buffer_size = 0; - s->req_nb_sectors = 0; - - ide_set_signature(s); - /* init the transfer handler so that 0xffff is returned on data - accesses */ - s->end_transfer_func = ide_dummy_transfer_stop; - ide_dummy_transfer_stop(s); - s->media_changed = 0; -} - void ide_bus_reset(IDEBus *bus) { bus->unit = 0; From 86698a12f7822387720840775b4eabab5f713d7c Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:39 -0500 Subject: [PATCH 03/11] ide: move buffered DMA cancel to core Buffered DMA cancellation was added to ATAPI devices and implemented for the BMDMA HBA. Move the code over to common IDE code and allow it to be used for any HBA. Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-id: 1453225191-11871-4-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ hw/ide/internal.h | 1 + hw/ide/pci.c | 36 +----------------------------------- 3 files changed, 47 insertions(+), 35 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 37d105841008..5cafcf528760 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -609,6 +609,51 @@ BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num, return aioreq; } +/** + * Cancel all pending DMA requests. + * Any buffered DMA requests are instantly canceled, + * but any pending unbuffered DMA requests must be waited on. + */ +void ide_cancel_dma_sync(IDEState *s) +{ + IDEBufferedRequest *req; + + /* First invoke the callbacks of all buffered requests + * and flag those requests as orphaned. Ideally there + * are no unbuffered (Scatter Gather DMA Requests or + * write requests) pending and we can avoid to drain. */ + QLIST_FOREACH(req, &s->buffered_requests, list) { + if (!req->orphaned) { +#ifdef DEBUG_IDE + printf("%s: invoking cb %p of buffered request %p with" + " -ECANCELED\n", __func__, req->original_cb, req); +#endif + req->original_cb(req->original_opaque, -ECANCELED); + } + req->orphaned = true; + } + + /* + * We can't cancel Scatter Gather DMA in the middle of the + * operation or a partial (not full) DMA transfer would reach + * the storage so we wait for completion instead (we beahve + * like if the DMA was completed by the time the guest trying + * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not + * set). + * + * In the future we'll be able to safely cancel the I/O if the + * whole DMA operation will be submitted to disk with a single + * aio operation with preadv/pwritev. + */ + if (s->bus->dma->aiocb) { +#ifdef DEBUG_IDE + printf("%s: draining all remaining requests", __func__); +#endif + blk_drain_all(); + assert(s->bus->dma->aiocb == NULL); + } +} + static void ide_sector_read(IDEState *s); static void ide_sector_read_cb(void *opaque, int ret) diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 2d1e2d2d2fa8..86bde2655184 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -586,6 +586,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk, BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num, QEMUIOVector *iov, int nb_sectors, BlockCompletionFunc *cb, void *opaque); +void ide_cancel_dma_sync(IDEState *s); /* hw/ide/atapi.c */ void ide_atapi_cmd(IDEState *s); diff --git a/hw/ide/pci.c b/hw/ide/pci.c index fa5b63bc3f94..92ffee726434 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -234,41 +234,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) /* Ignore writes to SSBM if it keeps the old value */ if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) { if (!(val & BM_CMD_START)) { - /* First invoke the callbacks of all buffered requests - * and flag those requests as orphaned. Ideally there - * are no unbuffered (Scatter Gather DMA Requests or - * write requests) pending and we can avoid to drain. */ - IDEBufferedRequest *req; - IDEState *s = idebus_active_if(bm->bus); - QLIST_FOREACH(req, &s->buffered_requests, list) { - if (!req->orphaned) { -#ifdef DEBUG_IDE - printf("%s: invoking cb %p of buffered request %p with" - " -ECANCELED\n", __func__, req->original_cb, req); -#endif - req->original_cb(req->original_opaque, -ECANCELED); - } - req->orphaned = true; - } - /* - * We can't cancel Scatter Gather DMA in the middle of the - * operation or a partial (not full) DMA transfer would reach - * the storage so we wait for completion instead (we beahve - * like if the DMA was completed by the time the guest trying - * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not - * set). - * - * In the future we'll be able to safely cancel the I/O if the - * whole DMA operation will be submitted to disk with a single - * aio operation with preadv/pwritev. - */ - if (bm->bus->dma->aiocb) { -#ifdef DEBUG_IDE - printf("%s: draining all remaining requests", __func__); -#endif - blk_drain_all(); - assert(bm->bus->dma->aiocb == NULL); - } + ide_cancel_dma_sync(idebus_active_if(bm->bus)); bm->status &= ~BM_STATUS_DMAING; } else { bm->cur_addr = bm->addr; From 51f7b5b883a2c9cb98ae28f1563b67f4f6d34c90 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:39 -0500 Subject: [PATCH 04/11] ide: replace blk_drain_all by blk_drain Target the drain for just one device. Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-id: 1453225191-11871-5-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 5cafcf528760..40b6cc8c62e4 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -649,7 +649,7 @@ void ide_cancel_dma_sync(IDEState *s) #ifdef DEBUG_IDE printf("%s: draining all remaining requests", __func__); #endif - blk_drain_all(); + blk_drain(s->blk); assert(s->bus->dma->aiocb == NULL); } } From e3044e238302a887cc1a022e358d68b9bdc69573 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:39 -0500 Subject: [PATCH 05/11] ide: Add silent DRQ cancellation Split apart the ide_transfer_stop function into two versions: one that interrupts and one that doesn't. The one that doesn't can be used to halt any PIO transfers that are in the DRQ phase. It will not halt any PIO transfers that are currently in the process of buffering data for the guest to read. Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi [Renamed 'etf' to 'end_transfer_func' --js] Message-id: 1453225191-11871-6-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 40b6cc8c62e4..3c32b392a04e 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -487,13 +487,28 @@ static void ide_cmd_done(IDEState *s) } } -void ide_transfer_stop(IDEState *s) +static void ide_transfer_halt(IDEState *s, + void(*end_transfer_func)(IDEState *), + bool notify) { - s->end_transfer_func = ide_transfer_stop; + s->end_transfer_func = end_transfer_func; s->data_ptr = s->io_buffer; s->data_end = s->io_buffer; s->status &= ~DRQ_STAT; - ide_cmd_done(s); + if (notify) { + ide_cmd_done(s); + } +} + +void ide_transfer_stop(IDEState *s) +{ + ide_transfer_halt(s, ide_transfer_stop, true); +} + +__attribute__((__unused__)) +static void ide_transfer_cancel(IDEState *s) +{ + ide_transfer_halt(s, ide_transfer_cancel, false); } int64_t ide_get_sector(IDEState *s) From f34ae00d6d22671d8583892fa702f21f3049ef72 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:39 -0500 Subject: [PATCH 06/11] ide: fix device_reset to not ignore pending AIO Signed-off-by: John Snow Reported-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi Message-id: 1453225191-11871-7-git-send-email-jsnow@redhat.com --- hw/ide/core.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/hw/ide/core.c b/hw/ide/core.c index 3c32b392a04e..241e840de098 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -505,7 +505,6 @@ void ide_transfer_stop(IDEState *s) ide_transfer_halt(s, ide_transfer_stop, true); } -__attribute__((__unused__)) static void ide_transfer_cancel(IDEState *s) { ide_transfer_halt(s, ide_transfer_cancel, false); @@ -1298,6 +1297,23 @@ static bool cmd_nop(IDEState *s, uint8_t cmd) return true; } +static bool cmd_device_reset(IDEState *s, uint8_t cmd) +{ + /* Halt PIO (in the DRQ phase), then DMA */ + ide_transfer_cancel(s); + ide_cancel_dma_sync(s); + + /* Reset any PIO commands, reset signature, etc */ + ide_reset(s); + + /* RESET: ATA8-ACS3 7.10.4 "Normal Outputs"; + * ATA8-ACS3 Table 184 "Device Signatures for Normal Output" */ + s->status = 0x00; + + /* Do not overwrite status register */ + return false; +} + static bool cmd_data_set_management(IDEState *s, uint8_t cmd) { switch (s->feature) { @@ -1614,15 +1630,6 @@ static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd) return false; } -static bool cmd_device_reset(IDEState *s, uint8_t cmd) -{ - ide_set_signature(s); - s->status = 0x00; /* NOTE: READY is _not_ set */ - s->error = 0x01; - - return false; -} - static bool cmd_packet(IDEState *s, uint8_t cmd) { /* overlapping commands not supported */ From c691320faa6a1749042134716a628e22abb81ed2 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:40 -0500 Subject: [PATCH 07/11] fdc: always compile-check debug prints Coverity noticed that some variables are only used by debug prints, and called them unused. Always compile the print statements. While we're here, print to stderr as well. Bonus: Fix a debug printf I broke in f31937aa8 Signed-off-by: John Snow Reviewed-by: Eric Blake [Touched up commit message. --js] Message-id: 1454971529-14830-1-git-send-email-jsnow@redhat.com --- hw/block/fdc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index a6f22ef20066..9838d21cf5cd 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -41,14 +41,15 @@ /********************************************************/ /* debug Floppy devices */ -//#define DEBUG_FLOPPY -#ifdef DEBUG_FLOPPY +#define DEBUG_FLOPPY 0 + #define FLOPPY_DPRINTF(fmt, ...) \ - do { printf("FLOPPY: " fmt , ## __VA_ARGS__); } while (0) -#else -#define FLOPPY_DPRINTF(fmt, ...) -#endif + do { \ + if (DEBUG_FLOPPY) { \ + fprintf(stderr, "FLOPPY: " fmt , ## __VA_ARGS__); \ + } \ + } while (0) /********************************************************/ /* Floppy drive emulation */ @@ -353,7 +354,7 @@ static int pick_geometry(FDrive *drv) parse = &fd_formats[size_match]; FLOPPY_DPRINTF("User requested floppy drive type '%s', " "but inserted medium appears to be a " - "%d sector '%s' type\n", + "%"PRId64" sector '%s' type\n", FloppyDriveType_lookup[drv->drive], nb_sectors, FloppyDriveType_lookup[parse->drive]); From 99b4cb71069f109b79b27bc629fc0cf0886dbc4b Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:40 -0500 Subject: [PATCH 08/11] ahci: Do not unmap NULL addresses Definitely don't try to unmap a garbage address. Reported-by: Zuozhi fzz Signed-off-by: John Snow Message-id: 1454103689-13042-2-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 7e87b1805ecd..3a95dadbd6df 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -662,6 +662,10 @@ static bool ahci_map_fis_address(AHCIDevice *ad) static void ahci_unmap_fis_address(AHCIDevice *ad) { + if (ad->res_fis == NULL) { + DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n"); + return; + } dma_memory_unmap(ad->hba->as, ad->res_fis, 256, DMA_DIRECTION_FROM_DEVICE, 256); ad->res_fis = NULL; @@ -678,6 +682,10 @@ static bool ahci_map_clb_address(AHCIDevice *ad) static void ahci_unmap_clb_address(AHCIDevice *ad) { + if (ad->lst == NULL) { + DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n"); + return; + } dma_memory_unmap(ad->hba->as, ad->lst, 1024, DMA_DIRECTION_FROM_DEVICE, 1024); ad->lst = NULL; From f32a2f33c2e765addcca7748f9df692e4131a0e2 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:40 -0500 Subject: [PATCH 09/11] ahci: handle LIST_ON and FIS_ON in map helpers Instead of relying on ahci_cond_start_engines to maintain the engine status indicators itself, have the lower-layer CLB and FIS mapper helpers do it themselves. This makes the cond_start routine slightly nicer to read, and makes sure that the status indicators will always be correct. Signed-off-by: John Snow Message-id: 1454103689-13042-3-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 3a95dadbd6df..ff338fe10a1b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -210,9 +210,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) AHCIPortRegs *pr = &ad->port_regs; if (pr->cmd & PORT_CMD_START) { - if (ahci_map_clb_address(ad)) { - pr->cmd |= PORT_CMD_LIST_ON; - } else { + if (!ahci_map_clb_address(ad)) { error_report("AHCI: Failed to start DMA engine: " "bad command list buffer address"); return -1; @@ -220,7 +218,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) } else if (pr->cmd & PORT_CMD_LIST_ON) { if (allow_stop) { ahci_unmap_clb_address(ad); - pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON); } else { error_report("AHCI: DMA engine should be off, " "but appears to still be running"); @@ -229,9 +226,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) } if (pr->cmd & PORT_CMD_FIS_RX) { - if (ahci_map_fis_address(ad)) { - pr->cmd |= PORT_CMD_FIS_ON; - } else { + if (!ahci_map_fis_address(ad)) { error_report("AHCI: Failed to start FIS receive engine: " "bad FIS receive buffer address"); return -1; @@ -239,7 +234,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) } else if (pr->cmd & PORT_CMD_FIS_ON) { if (allow_stop) { ahci_unmap_fis_address(ad); - pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON); } else { error_report("AHCI: FIS receive engine should be off, " "but appears to still be running"); @@ -657,7 +651,13 @@ static bool ahci_map_fis_address(AHCIDevice *ad) AHCIPortRegs *pr = &ad->port_regs; map_page(ad->hba->as, &ad->res_fis, ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256); - return ad->res_fis != NULL; + if (ad->res_fis != NULL) { + pr->cmd |= PORT_CMD_FIS_ON; + return true; + } + + pr->cmd &= ~PORT_CMD_FIS_ON; + return false; } static void ahci_unmap_fis_address(AHCIDevice *ad) @@ -666,6 +666,7 @@ static void ahci_unmap_fis_address(AHCIDevice *ad) DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n"); return; } + ad->port_regs.cmd &= ~PORT_CMD_FIS_ON; dma_memory_unmap(ad->hba->as, ad->res_fis, 256, DMA_DIRECTION_FROM_DEVICE, 256); ad->res_fis = NULL; @@ -677,7 +678,13 @@ static bool ahci_map_clb_address(AHCIDevice *ad) ad->cur_cmd = NULL; map_page(ad->hba->as, &ad->lst, ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024); - return ad->lst != NULL; + if (ad->lst != NULL) { + pr->cmd |= PORT_CMD_LIST_ON; + return true; + } + + pr->cmd &= ~PORT_CMD_LIST_ON; + return false; } static void ahci_unmap_clb_address(AHCIDevice *ad) @@ -686,6 +693,7 @@ static void ahci_unmap_clb_address(AHCIDevice *ad) DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n"); return; } + ad->port_regs.cmd &= ~PORT_CMD_LIST_ON; dma_memory_unmap(ad->hba->as, ad->lst, 1024, DMA_DIRECTION_FROM_DEVICE, 1024); ad->lst = NULL; From f8a6c5f3188b32db5c63e0759d390cb341888497 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:40 -0500 Subject: [PATCH 10/11] ahci: explicitly reject bad engine states on post_load Currently, we let ahci_cond_start_engines reject weird configurations where either the DMA (CLB) or FIS engines are said to be started, but their matching on/off control bit is toggled off. There should be no way to achieve this, since any time you toggle the control bit off, the status bit should always follow synchronously. Preparing for a refactor in cond_start_engines, move the rejection logic straight up into post_load. Signed-off-by: John Snow Message-id: 1454103689-13042-4-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index ff338fe10a1b..9f4a672cfd27 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -218,10 +218,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) } else if (pr->cmd & PORT_CMD_LIST_ON) { if (allow_stop) { ahci_unmap_clb_address(ad); - } else { - error_report("AHCI: DMA engine should be off, " - "but appears to still be running"); - return -1; } } @@ -234,10 +230,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) } else if (pr->cmd & PORT_CMD_FIS_ON) { if (allow_stop) { ahci_unmap_fis_address(ad); - } else { - error_report("AHCI: FIS receive engine should be off, " - "but appears to still be running"); - return -1; } } @@ -1568,10 +1560,23 @@ static int ahci_state_post_load(void *opaque, int version_id) int i, j; struct AHCIDevice *ad; NCQTransferState *ncq_tfs; + AHCIPortRegs *pr; AHCIState *s = opaque; for (i = 0; i < s->ports; i++) { ad = &s->dev[i]; + pr = &ad->port_regs; + + if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) { + error_report("AHCI: DMA engine should be off, but status bit " + "indicates it is still running."); + return -1; + } + if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) { + error_report("AHCI: FIS RX engine should be off, but status bit " + "indicates it is still running."); + return -1; + } /* Only remap the CLB address if appropriate, disallowing a state * transition from 'on' to 'off' it should be consistent here. */ From d590474922d37372c56075adb229c86d3aeae967 Mon Sep 17 00:00:00 2001 From: John Snow Date: Wed, 10 Feb 2016 13:29:40 -0500 Subject: [PATCH 11/11] ahci: prohibit "restarting" the FIS or CLB engines If the FIS or DMA engines are already started, do not allow them to be "restarted." As a side-effect of this change, the migration post-load routine must be modified to cope. If the engines are listed as "on" in the migrated registers, they must be cleared to allow the startup routine to see the transition from "off" to "on". As a second side-effect, the extra argument to ahci_cond_engine_start is removed in favor of consistent behavior. Signed-off-by: John Snow Message-id: 1454103689-13042-5-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9f4a672cfd27..f244bc01c946 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -199,38 +199,38 @@ static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr, * Check the cmd register to see if we should start or stop * the DMA or FIS RX engines. * - * @ad: Device to engage. - * @allow_stop: Allow device to transition from started to stopped? - * 'no' is useful for migration post_load, which does not expect a transition. + * @ad: Device to dis/engage. * * @return 0 on success, -1 on error. */ -static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop) +static int ahci_cond_start_engines(AHCIDevice *ad) { AHCIPortRegs *pr = &ad->port_regs; + bool cmd_start = pr->cmd & PORT_CMD_START; + bool cmd_on = pr->cmd & PORT_CMD_LIST_ON; + bool fis_start = pr->cmd & PORT_CMD_FIS_RX; + bool fis_on = pr->cmd & PORT_CMD_FIS_ON; - if (pr->cmd & PORT_CMD_START) { + if (cmd_start && !cmd_on) { if (!ahci_map_clb_address(ad)) { + pr->cmd &= ~PORT_CMD_START; error_report("AHCI: Failed to start DMA engine: " "bad command list buffer address"); return -1; } - } else if (pr->cmd & PORT_CMD_LIST_ON) { - if (allow_stop) { - ahci_unmap_clb_address(ad); - } + } else if (!cmd_start && cmd_on) { + ahci_unmap_clb_address(ad); } - if (pr->cmd & PORT_CMD_FIS_RX) { + if (fis_start && !fis_on) { if (!ahci_map_fis_address(ad)) { + pr->cmd &= ~PORT_CMD_FIS_RX; error_report("AHCI: Failed to start FIS receive engine: " "bad FIS receive buffer address"); return -1; } - } else if (pr->cmd & PORT_CMD_FIS_ON) { - if (allow_stop) { - ahci_unmap_fis_address(ad); - } + } else if (!fis_start && fis_on) { + ahci_unmap_fis_address(ad); } return 0; @@ -272,8 +272,8 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val) pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) | (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK)); - /* Check FIS RX and CLB engines, allow transition to false: */ - ahci_cond_start_engines(&s->dev[port], true); + /* Check FIS RX and CLB engines */ + ahci_cond_start_engines(&s->dev[port]); /* XXX usually the FIS would be pending on the bus here and issuing deferred until the OS enables FIS receival. @@ -1578,9 +1578,10 @@ static int ahci_state_post_load(void *opaque, int version_id) return -1; } - /* Only remap the CLB address if appropriate, disallowing a state - * transition from 'on' to 'off' it should be consistent here. */ - if (ahci_cond_start_engines(ad, false) != 0) { + /* After a migrate, the DMA/FIS engines are "off" and + * need to be conditionally restarted */ + pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON); + if (ahci_cond_start_engines(ad) != 0) { return -1; }