From e9ebb2f76778d19227476e34c3d7aa6b8975c1b6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:02 -0400 Subject: [PATCH 01/35] ahci: Do not ignore memory access read size The only guidance the AHCI specification gives on memory access is: "Register accesses shall have a maximum size of 64-bits; 64-bit access must not cross an 8-byte alignment boundary." I interpret this to mean that aligned or unaligned 1, 2 and 4 byte accesses should work, as well as aligned 8 byte accesses. In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads regardless of alignment. Windows 7 can be observed making 1 byte reads to the middle of 32 bit registers to fetch error codes. Introduce a wrapper to support unaligned accesses to AHCI. This wrapper will support aligned 8 byte reads, but will make no effort to support unaligned 8 byte reads, which although they will work on real hardware, are not guaranteed to work and do not appear to be used by either Windows or Linux. Signed-off-by: John Snow Reviewed-by: Eric Blake Message-id: 1434470575-21625-2-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b4b65c100a..0d6a2d8b4c 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -331,8 +331,7 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val) } } -static uint64_t ahci_mem_read(void *opaque, hwaddr addr, - unsigned size) +static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr) { AHCIState *s = opaque; uint32_t val = 0; @@ -368,6 +367,30 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr, } +/** + * AHCI 1.3 section 3 ("HBA Memory Registers") + * Support unaligned 8/16/32 bit reads, and 64 bit aligned reads. + * Caller is responsible for masking unwanted higher order bytes. + */ +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size) +{ + hwaddr aligned = addr & ~0x3; + int ofst = addr - aligned; + uint64_t lo = ahci_mem_read_32(opaque, aligned); + uint64_t hi; + + /* if < 8 byte read does not cross 4 byte boundary */ + if (ofst + size <= 4) { + return lo >> (ofst * 8); + } + g_assert_cmpint(size, >, 1); + + /* If the 64bit read is unaligned, we will produce undefined + * results. AHCI does not support unaligned 64bit reads. */ + hi = ahci_mem_read_32(opaque, aligned + 4); + return (hi << 32 | lo) >> (ofst * 8); +} + static void ahci_mem_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) From 0d3e9d1f598e803a02c9bf43ec0b053e873ca79a Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:02 -0400 Subject: [PATCH 02/35] qtest/ahci: add test_max Test that the FIS delivered after a nondata command has appropriately updated registers, just as we'd expect a data command to do. Signed-off-by: John Snow Message-id: 1434470575-21625-3-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 50 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index ae9415d74c..a7b4df2cd2 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -890,21 +890,23 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize, g_free(rx); } -static void ahci_test_nondata(AHCIQState *ahci, uint8_t ide_cmd) +static uint8_t ahci_test_nondata(AHCIQState *ahci, uint8_t ide_cmd) { - uint8_t px; + uint8_t port; AHCICommand *cmd; /* Sanitize */ - px = ahci_port_select(ahci); - ahci_port_clear(ahci, px); + port = ahci_port_select(ahci); + ahci_port_clear(ahci, port); /* Issue Command */ cmd = ahci_command_create(ide_cmd); - ahci_command_commit(ahci, cmd, px); + ahci_command_commit(ahci, cmd, port); ahci_command_issue(ahci, cmd); ahci_command_verify(ahci, cmd); ahci_command_free(cmd); + + return port; } static void ahci_test_flush(AHCIQState *ahci) @@ -912,6 +914,33 @@ static void ahci_test_flush(AHCIQState *ahci) ahci_test_nondata(ahci, CMD_FLUSH_CACHE); } +static void ahci_test_max(AHCIQState *ahci) +{ + RegD2HFIS *d2h = g_malloc0(0x20); + uint64_t nsect; + uint8_t port; + uint8_t cmd; + uint64_t config_sect = TEST_IMAGE_SECTORS - 1; + + if (config_sect > 0xFFFFFF) { + cmd = CMD_READ_MAX_EXT; + } else { + cmd = CMD_READ_MAX; + } + + port = ahci_test_nondata(ahci, cmd); + memread(ahci->port[port].fb + 0x40, d2h, 0x20); + nsect = (uint64_t)d2h->lba_hi[2] << 40 | + (uint64_t)d2h->lba_hi[1] << 32 | + (uint64_t)d2h->lba_hi[0] << 24 | + (uint64_t)d2h->lba_lo[2] << 16 | + (uint64_t)d2h->lba_lo[1] << 8 | + (uint64_t)d2h->lba_lo[0]; + + g_assert_cmphex(nsect, ==, config_sect); + g_free(d2h); +} + /******************************************************************************/ /* Test Interfaces */ @@ -1334,6 +1363,15 @@ static void test_flush_migrate(void) ahci_shutdown(dst); } +static void test_max(void) +{ + AHCIQState *ahci; + + ahci = ahci_boot_and_enable(NULL); + ahci_test_max(ahci); + ahci_shutdown(ahci); +} + /******************************************************************************/ /* AHCI I/O Test Matrix Definitions */ @@ -1584,6 +1622,8 @@ int main(int argc, char **argv) qtest_add_func("/ahci/io/dma/lba28/retry", test_halted_dma); qtest_add_func("/ahci/migrate/dma/halted", test_migrate_halted_dma); + qtest_add_func("/ahci/max", test_max); + ret = g_test_run(); /* Cleanup */ From 95ea663693fdf4f39976f9aadb004fc77c2058ee Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:02 -0400 Subject: [PATCH 03/35] libqos/ahci: fix memory management bugs There's a handful of trivial bugs in the libqos/ahci functions, squish them together. - Zero cached pointers after freeing them - The Command List Buffer is an array of 32x 32 byte structures, not 32x 8 byte pointers -- it's 1MiB, not 256 bytes. Zero it ALL. - Free the correct command in ahci_pick_cmd. Signed-off-by: John Snow Message-id: 1434470575-21625-4-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 7e17bb691e..08e1c98f7b 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -138,12 +138,14 @@ void ahci_clean_mem(AHCIQState *ahci) for (port = 0; port < 32; ++port) { if (ahci->port[port].fb) { ahci_free(ahci, ahci->port[port].fb); + ahci->port[port].fb = 0; } if (ahci->port[port].clb) { for (slot = 0; slot < 32; slot++) { ahci_destroy_command(ahci, port, slot); } ahci_free(ahci, ahci->port[port].clb); + ahci->port[port].clb = 0; } } } @@ -252,7 +254,7 @@ void ahci_hba_enable(AHCIQState *ahci) /* Allocate Memory for the Command List Buffer & FIS Buffer */ /* PxCLB space ... 0x20 per command, as in 4.2.2 p 36 */ ahci->port[i].clb = ahci_alloc(ahci, num_cmd_slots * 0x20); - qmemset(ahci->port[i].clb, 0x00, 0x100); + qmemset(ahci->port[i].clb, 0x00, num_cmd_slots * 0x20); g_test_message("CLB: 0x%08" PRIx64, ahci->port[i].clb); ahci_px_wreg(ahci, i, AHCI_PX_CLB, ahci->port[i].clb); g_assert_cmphex(ahci->port[i].clb, ==, @@ -549,7 +551,7 @@ unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port) if (reg & (1 << j)) { continue; } - ahci_destroy_command(ahci, port, i); + ahci_destroy_command(ahci, port, j); ahci->port[port].next = (j + 1) % 32; return j; } From d31a3ebc28bf401cc5cce43f36068697d670c3f9 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:02 -0400 Subject: [PATCH 04/35] qtest/ahci: add port_reset test Test that we can survive a couple of cycles of running a basic identify test, some IO, and resetting the HBA. Ensures that we can bring the HBA back to compliant spec during the lifecycle of the VM. Signed-off-by: John Snow Message-id: 1434470575-21625-5-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index a7b4df2cd2..0a0ef2af14 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -1372,6 +1372,29 @@ static void test_max(void) ahci_shutdown(ahci); } +static void test_reset(void) +{ + AHCIQState *ahci; + int i; + + ahci = ahci_boot(NULL); + ahci_test_pci_spec(ahci); + ahci_pci_enable(ahci); + + for (i = 0; i < 2; i++) { + ahci_test_hba_spec(ahci); + ahci_hba_enable(ahci); + ahci_test_identify(ahci); + ahci_test_io_rw_simple(ahci, 4096, 0, + CMD_READ_DMA_EXT, + CMD_WRITE_DMA_EXT); + ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR); + ahci_clean_mem(ahci); + } + + ahci_shutdown(ahci); +} + /******************************************************************************/ /* AHCI I/O Test Matrix Definitions */ @@ -1623,6 +1646,7 @@ int main(int argc, char **argv) qtest_add_func("/ahci/migrate/dma/halted", test_migrate_halted_dma); qtest_add_func("/ahci/max", test_max); + qtest_add_func("/ahci/reset", test_reset); ret = g_test_run(); From 7763ed1506a9ffe74a80332182cc4f229251f998 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 05/35] ahci: Rename NCQFIS structure fields Several fields of the NCQFIS structure are ambiguously named. This patch clarifies the intended (if unsupported) usage of the NCQ fields to aid in creating more meaningful debug messages through the NCQ codepaths. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-2-git-send-email-jsnow@redhat.com --- hw/ide/ahci.h | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 501c002c31..6d167f2736 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -195,6 +195,9 @@ #define RECEIVE_FPDMA_QUEUED 0x65 #define SEND_FPDMA_QUEUED 0x64 +#define NCQ_FIS_FUA_MASK 0x80 +#define NCQ_FIS_RARC_MASK 0x01 + #define RES_FIS_DSFIS 0x00 #define RES_FIS_PSFIS 0x20 #define RES_FIS_RFIS 0x40 @@ -312,27 +315,39 @@ extern const VMStateDescription vmstate_ahci; .offset = vmstate_offset_value(_state, _field, AHCIState), \ } +/** + * NCQFrame is the same as a Register H2D FIS (described in SATA 3.2), + * but some fields have been re-mapped and re-purposed, as seen in + * SATA 3.2 section 13.6.4.1 ("READ FPDMA QUEUED") + * + * cmd_fis[3], feature 7:0, becomes sector count 7:0. + * cmd_fis[7], device 7:0, uses bit 7 as the Force Unit Access bit. + * cmd_fis[11], feature 15:8, becomes sector count 15:8. + * cmd_fis[12], count 7:0, becomes the NCQ TAG (7:3) and RARC bit (0) + * cmd_fis[13], count 15:8, becomes the priority value (7:6) + * bytes 16-19 become an le32 "auxiliary" field. + */ typedef struct NCQFrame { uint8_t fis_type; uint8_t c; uint8_t command; - uint8_t sector_count_low; + uint8_t sector_count_low; /* (feature 7:0) */ uint8_t lba0; uint8_t lba1; uint8_t lba2; - uint8_t fua; + uint8_t fua; /* (device 7:0) */ uint8_t lba3; uint8_t lba4; uint8_t lba5; - uint8_t sector_count_high; - uint8_t tag; - uint8_t reserved5; - uint8_t reserved6; + uint8_t sector_count_high; /* (feature 15:8) */ + uint8_t tag; /* (count 0:7) */ + uint8_t prio; /* (count 15:8) */ + uint8_t icc; uint8_t control; - uint8_t reserved7; - uint8_t reserved8; - uint8_t reserved9; - uint8_t reserved10; + uint8_t aux0; + uint8_t aux1; + uint8_t aux2; + uint8_t aux3; } QEMU_PACKED NCQFrame; typedef struct SDBFIS { From b6fe41fa6dbdf7b92b76cd4848ef442de18e03d3 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 06/35] ahci: use shorter variables Trivial cleanup that I didn't want to tack-on to anything else. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-3-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 0d6a2d8b4c..3a8bff7e12 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -972,9 +972,11 @@ static int is_ncq(uint8_t ata_cmd) static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, int slot) { + AHCIDevice *ad = &s->dev[port]; + IDEState *ide_state = &ad->port.ifs[0]; NCQFrame *ncq_fis = (NCQFrame*)cmd_fis; uint8_t tag = ncq_fis->tag >> 3; - NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[tag]; + NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag]; if (ncq_tfs->used) { /* error - already in use */ @@ -983,7 +985,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, } ncq_tfs->used = 1; - ncq_tfs->drive = &s->dev[port]; + ncq_tfs->drive = ad; ncq_tfs->slot = slot; ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) | ((uint64_t)ncq_fis->lba4 << 32) | @@ -1000,9 +1002,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", " "drive max %"PRId64"\n", ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2, - s->dev[port].port.ifs[0].nb_sectors - 1); + ide_state->nb_sectors - 1); - ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist, 0); + ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); ncq_tfs->tag = tag; switch(ncq_fis->command) { @@ -1014,9 +1016,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, DPRINTF(port, "tag %d aio read %"PRId64"\n", ncq_tfs->tag, ncq_tfs->lba); - dma_acct_start(ncq_tfs->drive->port.ifs[0].blk, &ncq_tfs->acct, + dma_acct_start(ide_state->blk, &ncq_tfs->acct, &ncq_tfs->sglist, BLOCK_ACCT_READ); - ncq_tfs->aiocb = dma_blk_read(ncq_tfs->drive->port.ifs[0].blk, + ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba, ncq_cb, ncq_tfs); break; @@ -1027,9 +1029,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, DPRINTF(port, "tag %d aio write %"PRId64"\n", ncq_tfs->tag, ncq_tfs->lba); - dma_acct_start(ncq_tfs->drive->port.ifs[0].blk, &ncq_tfs->acct, + dma_acct_start(ide_state->blk, &ncq_tfs->acct, &ncq_tfs->sglist, BLOCK_ACCT_WRITE); - ncq_tfs->aiocb = dma_blk_write(ncq_tfs->drive->port.ifs[0].blk, + ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, ncq_tfs->lba, ncq_cb, ncq_tfs); break; From a55c8231d04e3023bc5c3da9290f01e7d6989a94 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 07/35] ahci: add ncq_err helper Set some appropriate error bits for NCQ for us. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-4-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 3a8bff7e12..7b286a2652 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -922,6 +922,15 @@ out: return r; } +static void ncq_err(NCQTransferState *ncq_tfs) +{ + IDEState *ide_state = &ncq_tfs->drive->port.ifs[0]; + + ide_state->error = ABRT_ERR; + ide_state->status = READY_STAT | ERR_STAT; + ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag); +} + static void ncq_cb(void *opaque, int ret) { NCQTransferState *ncq_tfs = (NCQTransferState *)opaque; @@ -934,10 +943,7 @@ static void ncq_cb(void *opaque, int ret) ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag); if (ret < 0) { - /* error */ - ide_state->error = ABRT_ERR; - ide_state->status = READY_STAT | ERR_STAT; - ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag); + ncq_err(ncq_tfs); } else { ide_state->status = READY_STAT | SEEK_STAT; } From 3bcbe4aa803c1a41e5392ecac7b4fc3c99a42f89 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 08/35] ahci: check for ncq prdtl overflow Don't attempt the NCQ transfer if the PRDT we were given is not big enough to perform the entire transfer. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-5-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 7b286a2652..f18d1f9b0b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -983,6 +983,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, NCQFrame *ncq_fis = (NCQFrame*)cmd_fis; uint8_t tag = ncq_fis->tag >> 3; NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag]; + size_t size; if (ncq_tfs->used) { /* error - already in use */ @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ((uint64_t)ncq_fis->lba2 << 16) | ((uint64_t)ncq_fis->lba1 << 8) | (uint64_t)ncq_fis->lba0; + ncq_tfs->tag = tag; - /* Note: We calculate the sector count, but don't currently rely on it. - * The total size of the DMA buffer tells us the transfer size instead. */ ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | ncq_fis->sector_count_low; + ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); + size = ncq_tfs->sector_count * 512; + + if (ncq_tfs->sglist.size < size) { + error_report("ahci: PRDT length for NCQ command (0x%zx) " + "is smaller than the requested size (0x%zx)", + ncq_tfs->sglist.size, size); + qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); + ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW); + return; + } DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", " "drive max %"PRId64"\n", ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2, ide_state->nb_sectors - 1); - ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); - ncq_tfs->tag = tag; - switch(ncq_fis->command) { case READ_FPDMA_QUEUED: DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", " @@ -1051,6 +1060,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, "error: tried to process non-NCQ command as NCQ\n"); } qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); } } From d56f4d6965ebcf8f3c496845c286e3a66496fdff Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 09/35] ahci: separate prdtl from opts There's no real reason to have it bundled together, and this way is a little nicer to follow if you have the AHCI spec pulled up. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-6-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 23 ++++++++++++----------- hw/ide/ahci.h | 3 ++- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index f18d1f9b0b..4d0615b82b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -834,10 +834,11 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int32_t offset) { AHCICmdHdr *cmd = ad->cur_cmd; - uint32_t opts = le32_to_cpu(cmd->opts); - uint64_t prdt_addr = le64_to_cpu(cmd->tbl_addr) + 0x80; - int sglist_alloc_hint = opts >> AHCI_CMD_HDR_PRDT_LEN; - dma_addr_t prdt_len = (sglist_alloc_hint * sizeof(AHCI_SG)); + 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; uint8_t *prdt; int i; @@ -857,7 +858,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, * request for sector sizes up to 32K. */ - if (!sglist_alloc_hint) { + if (!prdtl) { DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts); return -1; } @@ -876,10 +877,10 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, } /* Get entries in the PRDT, init a qemu sglist accordingly */ - if (sglist_alloc_hint > 0) { + if (prdtl > 0) { AHCI_SG *tbl = (AHCI_SG *)prdt; sum = 0; - for (i = 0; i < sglist_alloc_hint; i++) { + for (i = 0; i < prdtl; i++) { /* flags_size is zero-based */ tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); if (offset <= (sum + tbl_entry_size)) { @@ -897,12 +898,12 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, goto out; } - qemu_sglist_init(sglist, qbus->parent, (sglist_alloc_hint - off_idx), + qemu_sglist_init(sglist, qbus->parent, (prdtl - off_idx), ad->hba->as); qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos, prdt_tbl_entry_size(&tbl[off_idx]) - off_pos); - for (i = off_idx + 1; i < sglist_alloc_hint; i++) { + for (i = off_idx + 1; i < prdtl; i++) { /* flags_size is zero-based */ qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr), prdt_tbl_entry_size(&tbl[i])); @@ -1069,7 +1070,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port, { IDEState *ide_state = &s->dev[port].port.ifs[0]; AHCICmdHdr *cmd = s->dev[port].cur_cmd; - uint32_t opts = le32_to_cpu(cmd->opts); + uint16_t opts = le16_to_cpu(cmd->opts); if (cmd_fis[1] & 0x0F) { DPRINTF(port, "Port Multiplier not supported." @@ -1226,7 +1227,7 @@ static void ahci_start_transfer(IDEDMA *dma) IDEState *s = &ad->port.ifs[0]; uint32_t size = (uint32_t)(s->data_end - s->data_ptr); /* write == ram -> device */ - uint32_t opts = le32_to_cpu(ad->cur_cmd->opts); + uint16_t opts = le16_to_cpu(ad->cur_cmd->opts); int is_write = opts & AHCI_CMD_WRITE; int is_atapi = opts & AHCI_CMD_ATAPI; int has_sglist = 0; diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 6d167f2736..b8872a4e4d 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -236,7 +236,8 @@ typedef struct AHCIPortRegs { } AHCIPortRegs; typedef struct AHCICmdHdr { - uint32_t opts; + uint16_t opts; + uint16_t prdtl; uint32_t status; uint64_t tbl_addr; uint32_t reserved[4]; From 5d5f89212f19e3d7d3da1328137ca9e33eead7bf Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 10/35] ahci: add ncq debug checks Most of the time, these bits can be safely ignored. For the purposes of debugging however, it's nice to know that they're not being used. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-7-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 4d0615b82b..b78017a9dd 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1003,6 +1003,25 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, (uint64_t)ncq_fis->lba0; ncq_tfs->tag = tag; + /* Sanity-check the NCQ packet */ + if (tag != slot) { + DPRINTF(port, "Warn: NCQ slot (%d) did not match the given tag (%d)\n", + slot, tag); + } + + if (ncq_fis->aux0 || ncq_fis->aux1 || ncq_fis->aux2 || ncq_fis->aux3) { + DPRINTF(port, "Warn: Attempt to use NCQ auxiliary fields.\n"); + } + if (ncq_fis->prio || ncq_fis->icc) { + DPRINTF(port, "Warn: Unsupported attempt to use PRIO/ICC fields\n"); + } + if (ncq_fis->fua & NCQ_FIS_FUA_MASK) { + DPRINTF(port, "Warn: Unsupported attempt to use Force Unit Access\n"); + } + if (ncq_fis->tag & NCQ_FIS_RARC_MASK) { + DPRINTF(port, "Warn: Unsupported attempt to use Rebuild Assist\n"); + } + ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | ncq_fis->sector_count_low; ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); @@ -1016,6 +1035,10 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_err(ncq_tfs); ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW); return; + } else if (ncq_tfs->sglist.size != size) { + DPRINTF(port, "Warn: PRDTL (0x%zx)" + " does not match requested size (0x%zx)", + ncq_tfs->sglist.size, size); } DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", " From 0437d32ae232af37d3b94064a563eb51d4eedd62 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 11/35] ahci: ncq sector count correction This value should not be size-corrected, 0 sectors does not imply 1 sector(s). This is just debug information, but it's misleading! Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-8-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b78017a9dd..b73a2e4784 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1043,14 +1043,14 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", " "drive max %"PRId64"\n", - ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2, + ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1, ide_state->nb_sectors - 1); switch(ncq_fis->command) { case READ_FPDMA_QUEUED: DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", " "tag %d\n", - ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag); + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); DPRINTF(port, "tag %d aio read %"PRId64"\n", ncq_tfs->tag, ncq_tfs->lba); @@ -1063,7 +1063,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, break; case WRITE_FPDMA_QUEUED: DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n", - ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag); + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); DPRINTF(port, "tag %d aio write %"PRId64"\n", ncq_tfs->tag, ncq_tfs->lba); From 34475239b8f1fff0b715cb20f8b534b9d07a897e Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 12/35] ahci/qtest: Execute IDENTIFY prior to data commands If you try to execute an NCQ command before trying to engage with the device by issuing an IDENTIFY command, the error bits that are part of the signature will fool the test suite into thinking there was a failure. Issue IDENTIFY first on "boot", which will clear the signature out of the registers for us. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-9-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index 0a0ef2af14..ee1dc20a8f 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -228,6 +228,8 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...) { AHCIQState *ahci; va_list ap; + uint16_t buff[256]; + uint8_t port; if (cli) { va_start(ap, cli); @@ -239,6 +241,10 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...) ahci_pci_enable(ahci); ahci_hba_enable(ahci); + /* Initialize test device */ + port = ahci_port_select(ahci); + ahci_port_clear(ahci, port); + ahci_io(ahci, port, CMD_IDENTIFY, &buff, sizeof(buff), 0); return ahci; } From 40d29928caa6db154182f5314f497020f81e640e Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 13/35] libqos/ahci: fix cmd_sanity for ncq NCQ commands should not / do not update the byte count in the command header post command, so this field is meaningless for NCQ tests. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-10-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 46 +++++++++++++++++++++++---------------------- tests/libqos/ahci.h | 3 +-- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 08e1c98f7b..922af8b48e 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -73,6 +73,22 @@ AHCICommandProp ahci_command_properties[] = { { .cmd = CMD_FLUSH_CACHE, .data = false } }; +struct AHCICommand { + /* Test Management Data */ + uint8_t name; + uint8_t port; + uint8_t slot; + uint32_t interrupts; + uint64_t xbytes; + uint32_t prd_size; + uint64_t buffer; + AHCICommandProp *props; + /* Data to be transferred to the guest */ + AHCICommandHeader header; + RegH2DFIS fis; + void *atapi_cmd; +}; + /** * Allocate space in the guest using information in the AHCIQState object. */ @@ -462,13 +478,15 @@ void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port, g_free(pio); } -void ahci_port_check_cmd_sanity(AHCIQState *ahci, uint8_t port, - uint8_t slot, size_t buffsize) +void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd) { - AHCICommandHeader cmd; + AHCICommandHeader cmdh; - ahci_get_command_header(ahci, port, slot, &cmd); - g_assert_cmphex(buffsize, ==, cmd.prdbc); + ahci_get_command_header(ahci, cmd->port, cmd->slot, &cmdh); + /* Physical Region Descriptor Byte Count is not required to work for NCQ. */ + if (!cmd->props->ncq) { + g_assert_cmphex(cmd->xbytes, ==, cmdh.prdbc); + } } /* Get the command in #slot of port #port. */ @@ -612,22 +630,6 @@ void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd, ahci_command_free(cmd); } -struct AHCICommand { - /* Test Management Data */ - uint8_t name; - uint8_t port; - uint8_t slot; - uint32_t interrupts; - uint64_t xbytes; - uint32_t prd_size; - uint64_t buffer; - AHCICommandProp *props; - /* Data to be transferred to the guest */ - AHCICommandHeader header; - RegH2DFIS fis; - void *atapi_cmd; -}; - static AHCICommandProp *ahci_command_find(uint8_t command_name) { int i; @@ -901,7 +903,7 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd) ahci_port_check_error(ahci, port); ahci_port_check_interrupts(ahci, port, cmd->interrupts); ahci_port_check_nonbusy(ahci, port, slot); - ahci_port_check_cmd_sanity(ahci, port, slot, cmd->xbytes); + ahci_port_check_cmd_sanity(ahci, cmd); ahci_port_check_d2h_sanity(ahci, port, slot); if (cmd->props->pio) { ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes); diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h index 779e812400..ca8abee22c 100644 --- a/tests/libqos/ahci.h +++ b/tests/libqos/ahci.h @@ -512,8 +512,7 @@ void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot); void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot); void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot, size_t buffsize); -void ahci_port_check_cmd_sanity(AHCIQState *ahci, uint8_t port, - uint8_t slot, size_t buffsize); +void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd); void ahci_get_command_header(AHCIQState *ahci, uint8_t port, uint8_t slot, AHCICommandHeader *cmd); void ahci_set_command_header(AHCIQState *ahci, uint8_t port, From cb45304108ab733aaf2e4351e77cb6d07ac88fd5 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 14/35] libqos/ahci: add NCQ frame support NCQ frames are generated a little differently than their non-NCQ cousins. Add support for them. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-11-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 44 +++++++++++++++++++++++++++++++++++--------- tests/libqos/ahci.h | 29 ++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 922af8b48e..9a4d510fec 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -695,19 +695,34 @@ static void command_header_init(AHCICommand *cmd) static void command_table_init(AHCICommand *cmd) { RegH2DFIS *fis = &(cmd->fis); + uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE); fis->fis_type = REG_H2D_FIS; fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */ fis->command = cmd->name; - cmd->fis.feature_low = 0x00; - cmd->fis.feature_high = 0x00; - if (cmd->props->lba28 || cmd->props->lba48) { - cmd->fis.device = ATA_DEVICE_LBA; + + if (cmd->props->ncq) { + NCQFIS *ncqfis = (NCQFIS *)fis; + /* NCQ is weird and re-uses FIS frames for unrelated data. + * See SATA 3.2, 13.6.4.1 READ FPDMA QUEUED for an example. */ + ncqfis->sector_low = sect_count & 0xFF; + ncqfis->sector_hi = (sect_count >> 8) & 0xFF; + ncqfis->device = NCQ_DEVICE_MAGIC; + /* Force Unit Access is bit 7 in the device register */ + ncqfis->tag = 0; /* bits 3-7 are the NCQ tag */ + ncqfis->prio = 0; /* bits 6,7 are a prio tag */ + /* RARC bit is bit 0 of TAG field */ + } else { + fis->feature_low = 0x00; + fis->feature_high = 0x00; + if (cmd->props->lba28 || cmd->props->lba48) { + fis->device = ATA_DEVICE_LBA; + } + fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE); } - cmd->fis.count = (cmd->xbytes / AHCI_SECTOR_SIZE); - cmd->fis.icc = 0x00; - cmd->fis.control = 0x00; - memset(cmd->fis.aux, 0x00, ARRAY_SIZE(cmd->fis.aux)); + fis->icc = 0x00; + fis->control = 0x00; + memset(fis->aux, 0x00, ARRAY_SIZE(fis->aux)); } AHCICommand *ahci_command_create(uint8_t command_name) @@ -721,6 +736,7 @@ AHCICommand *ahci_command_create(uint8_t command_name) g_assert(!(props->lba28 && props->lba48)); g_assert(!(props->read && props->write)); g_assert(!props->size || props->data); + g_assert(!props->ncq || (props->ncq && props->lba48)); /* Defaults and book-keeping */ cmd->props = props; @@ -789,6 +805,8 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t buffer) void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes, unsigned prd_size) { + uint16_t sect_count; + /* Each PRD can describe up to 4MiB, and must not be odd. */ g_assert_cmphex(prd_size, <=, 4096 * 1024); g_assert_cmphex(prd_size & 0x01, ==, 0x00); @@ -796,7 +814,15 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes, cmd->prd_size = prd_size; } cmd->xbytes = xbytes; - cmd->fis.count = (cmd->xbytes / AHCI_SECTOR_SIZE); + sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE); + + if (cmd->props->ncq) { + NCQFIS *nfis = (NCQFIS *)&(cmd->fis); + nfis->sector_low = sect_count & 0xFF; + nfis->sector_hi = (sect_count >> 8) & 0xFF; + } else { + cmd->fis.count = sect_count; + } cmd->header.prdtl = size_to_prdtl(cmd->xbytes, cmd->prd_size); } diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h index ca8abee22c..594a1c9781 100644 --- a/tests/libqos/ahci.h +++ b/tests/libqos/ahci.h @@ -291,8 +291,9 @@ enum { #define CMDH_PMP (0xF000) /* ATA device register masks */ -#define ATA_DEVICE_MAGIC 0xA0 +#define ATA_DEVICE_MAGIC 0xA0 /* used in ata1-3 */ #define ATA_DEVICE_LBA 0x40 +#define NCQ_DEVICE_MAGIC 0x40 /* for ncq device registers */ #define ATA_DEVICE_DRIVE 0x10 #define ATA_DEVICE_HEAD 0x0F @@ -396,6 +397,32 @@ typedef struct RegH2DFIS { uint8_t aux[4]; } __attribute__((__packed__)) RegH2DFIS; +/** + * Register host-to-device FIS structure, for NCQ commands. + * Actually just a RegH2DFIS, but with fields repurposed. + * Repurposed fields are annotated below. + */ +typedef struct NCQFIS { + /* DW0 */ + uint8_t fis_type; + uint8_t flags; + uint8_t command; + uint8_t sector_low; /* H2D: Feature 7:0 */ + /* DW1 */ + uint8_t lba_lo[3]; + uint8_t device; + /* DW2 */ + uint8_t lba_hi[3]; + uint8_t sector_hi; /* H2D: Feature 15:8 */ + /* DW3 */ + uint8_t tag; /* H2D: Count 0:7 */ + uint8_t prio; /* H2D: Count 15:8 */ + uint8_t icc; + uint8_t control; + /* DW4 */ + uint8_t aux[4]; +} __attribute__((__packed__)) NCQFIS; + /** * Command List entry structure. * The command list contains between 1-32 of these structures. From 4de484698bdda6c5e093dfbe4368cdb364fdf87f Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:03 -0400 Subject: [PATCH 15/35] libqos/ahci: edit wait to be ncq aware The wait command should check to make sure SACT is clear as well as the Command Issue register. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-12-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 9a4d510fec..da02e2e4c7 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -908,11 +908,15 @@ void ahci_command_wait(AHCIQState *ahci, AHCICommand *cmd) /* We can't rely on STS_BSY until the command has started processing. * Therefore, we also use the Command Issue bit as indication of * a command in-flight. */ - while (BITSET(ahci_px_rreg(ahci, cmd->port, AHCI_PX_TFD), - AHCI_PX_TFD_STS_BSY) || - BITSET(ahci_px_rreg(ahci, cmd->port, AHCI_PX_CI), (1 << cmd->slot))) { + +#define RSET(REG, MASK) (BITSET(ahci_px_rreg(ahci, cmd->port, (REG)), (MASK))) + + while (RSET(AHCI_PX_TFD, AHCI_PX_TFD_STS_BSY) || + RSET(AHCI_PX_CI, 1 << cmd->slot) || + (cmd->props->ncq && RSET(AHCI_PX_SACT, 1 << cmd->slot))) { usleep(50); } + } void ahci_command_issue(AHCIQState *ahci, AHCICommand *cmd) From 359790c2542a8c4da3d4c8fb626ca2675a417d51 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 16/35] libqos/ahci: adjust expected NCQ interrupts NCQ commands will expect the SDBS interrupt, and in the normative case, do not expect to see a D2H Register FIS unless something went wrong. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-13-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index da02e2e4c7..953a32097b 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -745,12 +745,15 @@ AHCICommand *ahci_command_create(uint8_t command_name) cmd->prd_size = 4096; cmd->buffer = 0xabad1dea; - cmd->interrupts = AHCI_PX_IS_DHRS; + if (!cmd->props->ncq) { + cmd->interrupts = AHCI_PX_IS_DHRS; + } /* BUG: We expect the DPS interrupt for data commands */ /* cmd->interrupts |= props->data ? AHCI_PX_IS_DPS : 0; */ /* BUG: We expect the DMA Setup interrupt for DMA commands */ /* cmd->interrupts |= props->dma ? AHCI_PX_IS_DSS : 0; */ cmd->interrupts |= props->pio ? AHCI_PX_IS_PSS : 0; + cmd->interrupts |= props->ncq ? AHCI_PX_IS_SDBS : 0; command_header_init(cmd); command_table_init(cmd); @@ -934,7 +937,9 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd) ahci_port_check_interrupts(ahci, port, cmd->interrupts); ahci_port_check_nonbusy(ahci, port, slot); ahci_port_check_cmd_sanity(ahci, cmd); - ahci_port_check_d2h_sanity(ahci, port, slot); + if (cmd->interrupts & AHCI_PX_IS_DHRS) { + ahci_port_check_d2h_sanity(ahci, port, slot); + } if (cmd->props->pio) { ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes); } From a8973ff50a04f96c3ce5c803c8fd3ec16ed8d6c5 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 17/35] libqos/ahci: set the NCQ tag on command_commit NCQ commands have the concept of a "TAG" that they need to set, but in the AHCI world, it is mandated that the TAG always match the command slot that you executed the NCQ from. See AHCI 9.3.1.1.5.2 "Native Queued Commands". Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-14-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 953a32097b..7cf667affd 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -857,6 +857,11 @@ void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port) cmd->port = port; cmd->slot = ahci_pick_cmd(ahci, port); + if (cmd->props->ncq) { + NCQFIS *nfis = (NCQFIS *)&cmd->fis; + nfis->tag = (cmd->slot << 3) & 0xFC; + } + /* Create a buffer for the command table */ prdtl = size_to_prdtl(cmd->xbytes, cmd->prd_size); table_size = CMD_TBL_SIZ(prdtl); From e38cc93aca5d40a58e65bb0dfa23eaf3290bbf76 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 18/35] libqos/ahci: Force all NCQ commands to be LBA48 NCQ commands are LBA48 by definition. See SATA 3.2 13.6.4.1 "READ FPDMA QUEUED", or SATA 3.2 13.6.5.1 "WRITE FPDMA QUEUED." Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-15-git-send-email-jsnow@redhat.com --- tests/libqos/ahci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 7cf667affd..3d62cb6f83 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -781,7 +781,7 @@ void ahci_command_set_offset(AHCICommand *cmd, uint64_t lba_sect) RegH2DFIS *fis = &(cmd->fis); if (cmd->props->lba28) { g_assert_cmphex(lba_sect, <=, 0xFFFFFFF); - } else if (cmd->props->lba48) { + } else if (cmd->props->lba48 || cmd->props->ncq) { g_assert_cmphex(lba_sect, <=, 0xFFFFFFFFFFFF); } else { /* Can't set offset if we don't know the format. */ From 26ad004585835e7c126bb94fd5161db1c60169f3 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 19/35] qtest/ahci: simple ncq data test Test the NCQ pathways for a simple IO RW test. Also, test that libqos doesn't explode when running NCQ commands :) Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-16-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 13 +++++++++++++ tests/libqos/ahci.c | 46 ++++++++++++++++++++++++--------------------- tests/libqos/ahci.h | 27 ++++++++++++++------------ 3 files changed, 53 insertions(+), 33 deletions(-) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index ee1dc20a8f..0d117fecd8 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -1401,6 +1401,17 @@ static void test_reset(void) ahci_shutdown(ahci); } +static void test_ncq_simple(void) +{ + AHCIQState *ahci; + + ahci = ahci_boot_and_enable(NULL); + ahci_test_io_rw_simple(ahci, 4096, 0, + READ_FPDMA_QUEUED, + WRITE_FPDMA_QUEUED); + ahci_shutdown(ahci); +} + /******************************************************************************/ /* AHCI I/O Test Matrix Definitions */ @@ -1654,6 +1665,8 @@ int main(int argc, char **argv) qtest_add_func("/ahci/max", test_max); qtest_add_func("/ahci/reset", test_reset); + qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple); + ret = g_test_run(); /* Cleanup */ diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c index 3d62cb6f83..33ecd2abfb 100644 --- a/tests/libqos/ahci.c +++ b/tests/libqos/ahci.c @@ -50,27 +50,31 @@ typedef struct AHCICommandProp { } AHCICommandProp; AHCICommandProp ahci_command_properties[] = { - { .cmd = CMD_READ_PIO, .data = true, .pio = true, - .lba28 = true, .read = true }, - { .cmd = CMD_WRITE_PIO, .data = true, .pio = true, - .lba28 = true, .write = true }, - { .cmd = CMD_READ_PIO_EXT, .data = true, .pio = true, - .lba48 = true, .read = true }, - { .cmd = CMD_WRITE_PIO_EXT, .data = true, .pio = true, - .lba48 = true, .write = true }, - { .cmd = CMD_READ_DMA, .data = true, .dma = true, - .lba28 = true, .read = true }, - { .cmd = CMD_WRITE_DMA, .data = true, .dma = true, - .lba28 = true, .write = true }, - { .cmd = CMD_READ_DMA_EXT, .data = true, .dma = true, - .lba48 = true, .read = true }, - { .cmd = CMD_WRITE_DMA_EXT, .data = true, .dma = true, - .lba48 = true, .write = true }, - { .cmd = CMD_IDENTIFY, .data = true, .pio = true, - .size = 512, .read = true }, - { .cmd = CMD_READ_MAX, .lba28 = true }, - { .cmd = CMD_READ_MAX_EXT, .lba48 = true }, - { .cmd = CMD_FLUSH_CACHE, .data = false } + { .cmd = CMD_READ_PIO, .data = true, .pio = true, + .lba28 = true, .read = true }, + { .cmd = CMD_WRITE_PIO, .data = true, .pio = true, + .lba28 = true, .write = true }, + { .cmd = CMD_READ_PIO_EXT, .data = true, .pio = true, + .lba48 = true, .read = true }, + { .cmd = CMD_WRITE_PIO_EXT, .data = true, .pio = true, + .lba48 = true, .write = true }, + { .cmd = CMD_READ_DMA, .data = true, .dma = true, + .lba28 = true, .read = true }, + { .cmd = CMD_WRITE_DMA, .data = true, .dma = true, + .lba28 = true, .write = true }, + { .cmd = CMD_READ_DMA_EXT, .data = true, .dma = true, + .lba48 = true, .read = true }, + { .cmd = CMD_WRITE_DMA_EXT, .data = true, .dma = true, + .lba48 = true, .write = true }, + { .cmd = CMD_IDENTIFY, .data = true, .pio = true, + .size = 512, .read = true }, + { .cmd = READ_FPDMA_QUEUED, .data = true, .dma = true, + .lba48 = true, .read = true, .ncq = true }, + { .cmd = WRITE_FPDMA_QUEUED, .data = true, .dma = true, + .lba48 = true, .write = true, .ncq = true }, + { .cmd = CMD_READ_MAX, .lba28 = true }, + { .cmd = CMD_READ_MAX_EXT, .lba48 = true }, + { .cmd = CMD_FLUSH_CACHE, .data = false } }; struct AHCICommand { diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h index 594a1c9781..a08a9ddac1 100644 --- a/tests/libqos/ahci.h +++ b/tests/libqos/ahci.h @@ -263,20 +263,23 @@ enum { /* ATA Commands */ enum { /* DMA */ - CMD_READ_DMA = 0xC8, - CMD_READ_DMA_EXT = 0x25, - CMD_WRITE_DMA = 0xCA, - CMD_WRITE_DMA_EXT = 0x35, + CMD_READ_DMA = 0xC8, + CMD_READ_DMA_EXT = 0x25, + CMD_WRITE_DMA = 0xCA, + CMD_WRITE_DMA_EXT = 0x35, /* PIO */ - CMD_READ_PIO = 0x20, - CMD_READ_PIO_EXT = 0x24, - CMD_WRITE_PIO = 0x30, - CMD_WRITE_PIO_EXT = 0x34, + CMD_READ_PIO = 0x20, + CMD_READ_PIO_EXT = 0x24, + CMD_WRITE_PIO = 0x30, + CMD_WRITE_PIO_EXT = 0x34, /* Misc */ - CMD_READ_MAX = 0xF8, - CMD_READ_MAX_EXT = 0x27, - CMD_FLUSH_CACHE = 0xE7, - CMD_IDENTIFY = 0xEC + CMD_READ_MAX = 0xF8, + CMD_READ_MAX_EXT = 0x27, + CMD_FLUSH_CACHE = 0xE7, + CMD_IDENTIFY = 0xEC, + /* NCQ */ + READ_FPDMA_QUEUED = 0x60, + WRITE_FPDMA_QUEUED = 0x61, }; /* AHCI Command Header Flags & Masks*/ From 07a1ee7958cc3433706ab0d3a07c42fdd9d98fe6 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 20/35] qtest/ahci: ncq migration test Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435016308-6150-17-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index 0d117fecd8..3f06fd9b1b 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -1146,9 +1146,9 @@ static void test_migrate_sanity(void) } /** - * DMA Migration test: Write a pattern, migrate, then read. + * Simple migration test: Write a pattern, migrate, then read. */ -static void test_migrate_dma(void) +static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write) { AHCIQState *src, *dst; uint8_t px; @@ -1176,9 +1176,9 @@ static void test_migrate_dma(void) } /* Write, migrate, then read. */ - ahci_io(src, px, CMD_WRITE_DMA, tx, bufsize, 0); + ahci_io(src, px, cmd_write, tx, bufsize, 0); ahci_migrate(src, dst, uri); - ahci_io(dst, px, CMD_READ_DMA, rx, bufsize, 0); + ahci_io(dst, px, cmd_read, rx, bufsize, 0); /* Verify pattern */ g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0); @@ -1189,6 +1189,16 @@ static void test_migrate_dma(void) g_free(tx); } +static void test_migrate_dma(void) +{ + ahci_migrate_simple(CMD_READ_DMA, CMD_WRITE_DMA); +} + +static void test_migrate_ncq(void) +{ + ahci_migrate_simple(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED); +} + /** * DMA Error Test * @@ -1666,6 +1676,7 @@ int main(int argc, char **argv) qtest_add_func("/ahci/reset", test_reset); qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple); + qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq); ret = g_test_run(); From a718978ed58abc1ad92567a9c17525136be02a71 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 21/35] ide: add limit to .prepare_buf() prepare_buf should not always grab as many descriptors as it can, sometimes it should self-limit. For example, an NCQ transfer of 1 sector with a PRDT that describes 4GiB of data should not copy 4GiB of data, it should just transfer that first 512 bytes. PIO is not affected, because the dma_buf_rw dma helpers already have a byte limit built-in to them, but DMA/NCQ will exhaust the entire list regardless of requested size. AHCI 1.3 specifies in section 6.1.6 Command List Underflow that NCQ is not required to detect underflow conditions. Non-NCQ pathways signal underflow by writing to the PRDBC field, which will already occur by writing the actual transferred byte count to the PRDBC, signaling the underflow. Our NCQ pathways aren't required to detect underflow, but since our DMA backend uses the size of the PRDT to determine the size of the transer, if our PRDT is bigger than the transaction (the underflow condition) it doesn't cost us anything to detect it and truncate the PRDT. This is a recoverable error and is not signaled to the guest, in either NCQ or normal DMA cases. For BMDMA, the existing pathways should see no guest-visible difference, but any bytes described in the overage will no longer be transferred before indicating to the guest that there was an underflow. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-2-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 27 ++++++++++++++------------- hw/ide/core.c | 8 ++++---- hw/ide/internal.h | 2 +- hw/ide/macio.c | 2 +- hw/ide/pci.c | 21 ++++++++++++++++----- 5 files changed, 36 insertions(+), 24 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b73a2e4784..de1759a24d 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -49,7 +49,7 @@ 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_dma_prepare_buf(IDEDMA *dma, int32_t limit); static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes); static bool ahci_map_clb_address(AHCIDevice *ad); static bool ahci_map_fis_address(AHCIDevice *ad); @@ -827,11 +827,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) static int prdt_tbl_entry_size(const AHCI_SG *tbl) { + /* flags_size is zero-based */ return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1; } static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, - int32_t offset) + int64_t limit, int32_t offset) { AHCICmdHdr *cmd = ad->cur_cmd; uint16_t opts = le16_to_cpu(cmd->opts); @@ -881,9 +882,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, AHCI_SG *tbl = (AHCI_SG *)prdt; sum = 0; for (i = 0; i < prdtl; i++) { - /* flags_size is zero-based */ tbl_entry_size = prdt_tbl_entry_size(&tbl[i]); - if (offset <= (sum + tbl_entry_size)) { + if (offset < (sum + tbl_entry_size)) { off_idx = i; off_pos = offset - sum; break; @@ -901,12 +901,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, qemu_sglist_init(sglist, qbus->parent, (prdtl - off_idx), ad->hba->as); qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos, - prdt_tbl_entry_size(&tbl[off_idx]) - off_pos); + MIN(prdt_tbl_entry_size(&tbl[off_idx]) - off_pos, + limit)); - for (i = off_idx + 1; i < prdtl; i++) { - /* flags_size is zero-based */ + for (i = off_idx + 1; i < prdtl && sglist->size < limit; i++) { qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr), - prdt_tbl_entry_size(&tbl[i])); + MIN(prdt_tbl_entry_size(&tbl[i]), + limit - sglist->size)); if (sglist->size > INT32_MAX) { error_report("AHCI Physical Region Descriptor Table describes " "more than 2 GiB.\n"); @@ -1024,8 +1025,8 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | ncq_fis->sector_count_low; - ahci_populate_sglist(ad, &ncq_tfs->sglist, 0); size = ncq_tfs->sector_count * 512; + ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0); if (ncq_tfs->sglist.size < size) { error_report("ahci: PRDT length for NCQ command (0x%zx) " @@ -1262,7 +1263,7 @@ static void ahci_start_transfer(IDEDMA *dma) goto out; } - if (ahci_dma_prepare_buf(dma, is_write)) { + if (ahci_dma_prepare_buf(dma, size)) { has_sglist = 1; } @@ -1312,12 +1313,12 @@ static void ahci_restart_dma(IDEDMA *dma) * Not currently invoked by PIO R/W chains, * which invoke ahci_populate_sglist via ahci_start_transfer. */ -static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write) +static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit) { AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; - if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset) == -1) { + if (ahci_populate_sglist(ad, &s->sg, limit, s->io_buffer_offset) == -1) { DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n"); return -1; } @@ -1352,7 +1353,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write) uint8_t *p = s->io_buffer + s->io_buffer_index; int l = s->io_buffer_size - s->io_buffer_index; - if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) { + if (ahci_populate_sglist(ad, &s->sg, l, s->io_buffer_offset)) { return 0; } diff --git a/hw/ide/core.c b/hw/ide/core.c index 1efd98af63..be7c350b87 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -716,8 +716,8 @@ static void ide_dma_cb(void *opaque, int ret) sector_num = ide_get_sector(s); if (n > 0) { - assert(s->io_buffer_size == s->sg.size); - dma_buf_commit(s, s->io_buffer_size); + assert(n * 512 == s->sg.size); + dma_buf_commit(s, s->sg.size); sector_num += n; ide_set_sector(s, sector_num); s->nsector -= n; @@ -734,7 +734,7 @@ static void ide_dma_cb(void *opaque, int ret) n = s->nsector; s->io_buffer_index = 0; s->io_buffer_size = n * 512; - if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) { + if (s->bus->dma->ops->prepare_buf(s->bus->dma, s->io_buffer_size) < 512) { /* The PRDs were too short. Reset the Active bit, but don't raise an * interrupt. */ s->status = READY_STAT | SEEK_STAT; @@ -2326,7 +2326,7 @@ static void ide_nop(IDEDMA *dma) { } -static int32_t ide_nop_int32(IDEDMA *dma, int x) +static int32_t ide_nop_int32(IDEDMA *dma, int32_t l) { return 0; } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 965cc55cb8..3736e1bbfe 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -324,7 +324,7 @@ typedef void EndTransferFunc(IDEState *); typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *); typedef void DMAVoidFunc(IDEDMA *); typedef int DMAIntFunc(IDEDMA *, int); -typedef int32_t DMAInt32Func(IDEDMA *, int); +typedef int32_t DMAInt32Func(IDEDMA *, int32_t len); typedef void DMAu32Func(IDEDMA *, uint32_t); typedef void DMAStopFunc(IDEDMA *, bool); typedef void DMARestartFunc(void *, int, RunState); diff --git a/hw/ide/macio.c b/hw/ide/macio.c index dd52d50732..a55a479da6 100644 --- a/hw/ide/macio.c +++ b/hw/ide/macio.c @@ -499,7 +499,7 @@ static int ide_nop_int(IDEDMA *dma, int x) return 0; } -static int32_t ide_nop_int32(IDEDMA *dma, int x) +static int32_t ide_nop_int32(IDEDMA *dma, int32_t l) { return 0; } diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 4afd0cfe8c..d31ff885b7 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -53,10 +53,14 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s, } /** - * Return the number of bytes successfully prepared. - * -1 on error. + * Prepare an sglist based on available PRDs. + * @limit: How many bytes to prepare total. + * + * Returns the number of bytes prepared, -1 on error. + * IDEState.io_buffer_size will contain the number of bytes described + * by the PRDs, whether or not we added them to the sglist. */ -static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write) +static int32_t bmdma_prepare_buf(IDEDMA *dma, int32_t limit) { BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma); IDEState *s = bmdma_active_if(bm); @@ -75,7 +79,7 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write) /* end of table (with a fail safe of one page) */ if (bm->cur_prd_last || (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) { - return s->io_buffer_size; + return s->sg.size; } pci_dma_read(pci_dev, bm->cur_addr, &prd, 8); bm->cur_addr += 8; @@ -90,7 +94,14 @@ static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write) } l = bm->cur_prd_len; if (l > 0) { - qemu_sglist_add(&s->sg, bm->cur_prd_addr, l); + uint64_t sg_len; + + /* Don't add extra bytes to the SGList; consume any remaining + * PRDs from the guest, but ignore them. */ + sg_len = MIN(limit - s->sg.size, bm->cur_prd_len); + if (sg_len) { + qemu_sglist_add(&s->sg, bm->cur_prd_addr, sg_len); + } /* Note: We limit the max transfer to be 2GiB. * This should accommodate the largest ATA transaction From 4614619ee4ad96d2715dc41f9430fb43335c15d2 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 22/35] ahci: stash ncq command For migration and werror=stop/rerror=stop resume purposes, it will be convenient to have the command handy inside of ncq_tfs. Eventually, we'd like to avoid reading from the FIS entirely after the initial read, so this is a byte (hah!) sized step in that direction. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-3-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 3 ++- hw/ide/ahci.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index de1759a24d..9540a64a39 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -996,6 +996,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->used = 1; ncq_tfs->drive = ad; ncq_tfs->slot = slot; + ncq_tfs->cmd = ncq_fis->command; ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) | ((uint64_t)ncq_fis->lba4 << 32) | ((uint64_t)ncq_fis->lba3 << 24) | @@ -1047,7 +1048,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1, ide_state->nb_sectors - 1); - switch(ncq_fis->command) { + switch (ncq_tfs->cmd) { case READ_FPDMA_QUEUED: DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", " "tag %d\n", diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index b8872a4e4d..33607d7fad 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -259,6 +259,7 @@ typedef struct NCQTransferState { uint16_t sector_count; uint64_t lba; uint8_t tag; + uint8_t cmd; int slot; int used; } NCQTransferState; From 922f893e57da24bc80db3e79bea56485d1c111fa Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 23/35] ahci: assert is_ncq for process_ncq We already checked this in the handle_cmd phase, so just change this to an assertion and simplify the error logic. (Also, fix the switch indent, because checkpatch.pl yelled.) ((Sorry for churn.)) Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-4-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 60 ++++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 34 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 9540a64a39..8171f39aeb 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -987,6 +987,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag]; size_t size; + g_assert(is_ncq(ncq_fis->command)); if (ncq_tfs->used) { /* error - already in use */ fprintf(stderr, "%s: tag %d already used\n", __FUNCTION__, tag); @@ -1049,44 +1050,35 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ide_state->nb_sectors - 1); switch (ncq_tfs->cmd) { - case READ_FPDMA_QUEUED: - DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", " - "tag %d\n", - ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); + case READ_FPDMA_QUEUED: + DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n", + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); - DPRINTF(port, "tag %d aio read %"PRId64"\n", - ncq_tfs->tag, ncq_tfs->lba); + DPRINTF(port, "tag %d aio read %"PRId64"\n", + ncq_tfs->tag, ncq_tfs->lba); - dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_READ); - ncq_tfs->aiocb = dma_blk_read(ide_state->blk, - &ncq_tfs->sglist, ncq_tfs->lba, - ncq_cb, ncq_tfs); - break; - case WRITE_FPDMA_QUEUED: - DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n", - ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); + dma_acct_start(ide_state->blk, &ncq_tfs->acct, + &ncq_tfs->sglist, BLOCK_ACCT_READ); + ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, + ncq_tfs->lba, ncq_cb, ncq_tfs); + break; + case WRITE_FPDMA_QUEUED: + DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n", + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); - DPRINTF(port, "tag %d aio write %"PRId64"\n", - ncq_tfs->tag, ncq_tfs->lba); + DPRINTF(port, "tag %d aio write %"PRId64"\n", + ncq_tfs->tag, ncq_tfs->lba); - dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_WRITE); - ncq_tfs->aiocb = dma_blk_write(ide_state->blk, - &ncq_tfs->sglist, ncq_tfs->lba, - ncq_cb, ncq_tfs); - break; - default: - if (is_ncq(cmd_fis[2])) { - DPRINTF(port, - "error: unsupported NCQ command (0x%02x) received\n", - cmd_fis[2]); - } else { - DPRINTF(port, - "error: tried to process non-NCQ command as NCQ\n"); - } - qemu_sglist_destroy(&ncq_tfs->sglist); - ncq_err(ncq_tfs); + dma_acct_start(ide_state->blk, &ncq_tfs->acct, + &ncq_tfs->sglist, BLOCK_ACCT_WRITE); + ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, + ncq_tfs->lba, ncq_cb, ncq_tfs); + break; + default: + DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n", + ncq_tfs->cmd); + qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); } } From 631ddc22cbb401f2777dc8b117196f0988df4eea Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 24/35] ahci: refactor process_ncq_command Split off execute_ncq_command so that we can call it separately later if we desire. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-5-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 73 +++++++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 8171f39aeb..b3a6a91dbb 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -977,6 +977,47 @@ static int is_ncq(uint8_t ata_cmd) } } +static void execute_ncq_command(NCQTransferState *ncq_tfs) +{ + AHCIDevice *ad = ncq_tfs->drive; + IDEState *ide_state = &ad->port.ifs[0]; + int port = ad->port_no; + g_assert(is_ncq(ncq_tfs->cmd)); + + switch (ncq_tfs->cmd) { + case READ_FPDMA_QUEUED: + DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n", + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); + + DPRINTF(port, "tag %d aio read %"PRId64"\n", + ncq_tfs->tag, ncq_tfs->lba); + + dma_acct_start(ide_state->blk, &ncq_tfs->acct, + &ncq_tfs->sglist, BLOCK_ACCT_READ); + ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, + ncq_tfs->lba, ncq_cb, ncq_tfs); + break; + case WRITE_FPDMA_QUEUED: + DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n", + ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); + + DPRINTF(port, "tag %d aio write %"PRId64"\n", + ncq_tfs->tag, ncq_tfs->lba); + + dma_acct_start(ide_state->blk, &ncq_tfs->acct, + &ncq_tfs->sglist, BLOCK_ACCT_WRITE); + ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, + ncq_tfs->lba, ncq_cb, ncq_tfs); + break; + default: + DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n", + ncq_tfs->cmd); + qemu_sglist_destroy(&ncq_tfs->sglist); + ncq_err(ncq_tfs); + } +} + + static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, int slot) { @@ -1049,37 +1090,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1, ide_state->nb_sectors - 1); - switch (ncq_tfs->cmd) { - case READ_FPDMA_QUEUED: - DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", tag %d\n", - ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); - - DPRINTF(port, "tag %d aio read %"PRId64"\n", - ncq_tfs->tag, ncq_tfs->lba); - - dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_READ); - ncq_tfs->aiocb = dma_blk_read(ide_state->blk, &ncq_tfs->sglist, - ncq_tfs->lba, ncq_cb, ncq_tfs); - break; - case WRITE_FPDMA_QUEUED: - DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n", - ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag); - - DPRINTF(port, "tag %d aio write %"PRId64"\n", - ncq_tfs->tag, ncq_tfs->lba); - - dma_acct_start(ide_state->blk, &ncq_tfs->acct, - &ncq_tfs->sglist, BLOCK_ACCT_WRITE); - ncq_tfs->aiocb = dma_blk_write(ide_state->blk, &ncq_tfs->sglist, - ncq_tfs->lba, ncq_cb, ncq_tfs); - break; - default: - DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n", - ncq_tfs->cmd); - qemu_sglist_destroy(&ncq_tfs->sglist); - ncq_err(ncq_tfs); - } + execute_ncq_command(ncq_tfs); } static void handle_reg_h2d_fis(AHCIState *s, int port, From 54f3223730736fca1e6e89bb7f99c4f8432fdabb Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 25/35] ahci: factor ncq_finish out of ncq_cb When we add werror=stop or rerror=stop support to NCQ, we'll want to take a codepath where we don't actually complete the command, so factor that out into a new routine. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-6-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b3a6a91dbb..b0b9b41ed0 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -933,23 +933,11 @@ static void ncq_err(NCQTransferState *ncq_tfs) ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag); } -static void ncq_cb(void *opaque, int ret) +static void ncq_finish(NCQTransferState *ncq_tfs) { - NCQTransferState *ncq_tfs = (NCQTransferState *)opaque; - IDEState *ide_state = &ncq_tfs->drive->port.ifs[0]; - - if (ret == -ECANCELED) { - return; - } /* Clear bit for this tag in SActive */ ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag); - if (ret < 0) { - ncq_err(ncq_tfs); - } else { - ide_state->status = READY_STAT | SEEK_STAT; - } - ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs->drive->port_no, (1 << ncq_tfs->tag)); @@ -962,6 +950,24 @@ static void ncq_cb(void *opaque, int ret) ncq_tfs->used = 0; } +static void ncq_cb(void *opaque, int ret) +{ + NCQTransferState *ncq_tfs = (NCQTransferState *)opaque; + IDEState *ide_state = &ncq_tfs->drive->port.ifs[0]; + + if (ret == -ECANCELED) { + return; + } + + if (ret < 0) { + ncq_err(ncq_tfs); + } else { + ide_state->status = READY_STAT | SEEK_STAT; + } + + ncq_finish(ncq_tfs); +} + static int is_ncq(uint8_t ata_cmd) { /* Based on SATA 3.2 section 13.6.3.2 */ From 7c03a691077e71a08bbca06568cd97f09537458c Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:04 -0400 Subject: [PATCH 26/35] ahci: add rwerror=stop support for ncq Handle NCQ failures for cases where we want to halt the VM on IO errors. Upon a VM state change, retry the halted NCQ commands. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-7-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 36 ++++++++++++++++++++++++++++++++++-- hw/ide/ahci.h | 1 + hw/ide/core.c | 7 +++++++ hw/ide/internal.h | 2 ++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b0b9b41ed0..d996f37b36 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -581,6 +581,7 @@ static void ahci_reset_port(AHCIState *s, int port) /* reset ncq queue */ for (i = 0; i < AHCI_MAX_CMDS; i++) { NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[i]; + ncq_tfs->halt = false; if (!ncq_tfs->used) { continue; } @@ -960,12 +961,23 @@ static void ncq_cb(void *opaque, int ret) } if (ret < 0) { - ncq_err(ncq_tfs); + bool is_read = ncq_tfs->cmd == READ_FPDMA_QUEUED; + BlockErrorAction action = blk_get_error_action(ide_state->blk, + is_read, -ret); + if (action == BLOCK_ERROR_ACTION_STOP) { + ncq_tfs->halt = true; + ide_state->bus->error_status = IDE_RETRY_HBA; + } else if (action == BLOCK_ERROR_ACTION_REPORT) { + ncq_err(ncq_tfs); + } + blk_error_action(ide_state->blk, action, is_read, -ret); } else { ide_state->status = READY_STAT | SEEK_STAT; } - ncq_finish(ncq_tfs); + if (!ncq_tfs->halt) { + ncq_finish(ncq_tfs); + } } static int is_ncq(uint8_t ata_cmd) @@ -988,7 +1000,9 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) AHCIDevice *ad = ncq_tfs->drive; IDEState *ide_state = &ad->port.ifs[0]; int port = ad->port_no; + g_assert(is_ncq(ncq_tfs->cmd)); + ncq_tfs->halt = false; switch (ncq_tfs->cmd) { case READ_FPDMA_QUEUED: @@ -1318,6 +1332,23 @@ static void ahci_restart_dma(IDEDMA *dma) /* Nothing to do, ahci_start_dma already resets s->io_buffer_offset. */ } +/** + * IDE/PIO restarts are handled by the core layer, but NCQ commands + * need an extra kick from the AHCI HBA. + */ +static void ahci_restart(IDEDMA *dma) +{ + AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); + int i; + + for (i = 0; i < AHCI_MAX_CMDS; i++) { + NCQTransferState *ncq_tfs = &ad->ncq_tfs[i]; + if (ncq_tfs->halt) { + execute_ncq_command(ncq_tfs); + } + } +} + /** * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist. * Not currently invoked by PIO R/W chains, @@ -1406,6 +1437,7 @@ static void ahci_irq_set(void *opaque, int n, int level) static const IDEDMAOps ahci_dma_ops = { .start_dma = ahci_start_dma, + .restart = ahci_restart, .restart_dma = ahci_restart_dma, .start_transfer = ahci_start_transfer, .prepare_buf = ahci_dma_prepare_buf, diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 33607d7fad..47a3122270 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -262,6 +262,7 @@ typedef struct NCQTransferState { uint8_t cmd; int slot; int used; + bool halt; } NCQTransferState; struct AHCIDevice { diff --git a/hw/ide/core.c b/hw/ide/core.c index be7c350b87..122e955084 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2371,6 +2371,13 @@ static void ide_restart_bh(void *opaque) * called function can set a new error status. */ bus->error_status = 0; + /* The HBA has generically asked to be kicked on retry */ + if (error_status & IDE_RETRY_HBA) { + if (s->bus->dma->ops->restart) { + s->bus->dma->ops->restart(s->bus->dma); + } + } + if (error_status & IDE_RETRY_DMA) { if (error_status & IDE_RETRY_TRIM) { ide_restart_dma(s, IDE_DMA_TRIM); diff --git a/hw/ide/internal.h b/hw/ide/internal.h index 3736e1bbfe..30fdcbc5fa 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -436,6 +436,7 @@ struct IDEDMAOps { DMAInt32Func *prepare_buf; DMAu32Func *commit_buf; DMAIntFunc *rw_buf; + DMAVoidFunc *restart; DMAVoidFunc *restart_dma; DMAStopFunc *set_inactive; DMAVoidFunc *cmd_done; @@ -499,6 +500,7 @@ struct IDEDevice { #define IDE_RETRY_READ 0x20 #define IDE_RETRY_FLUSH 0x40 #define IDE_RETRY_TRIM 0x80 +#define IDE_RETRY_HBA 0x100 static inline IDEState *idebus_active_if(IDEBus *bus) { From 9364384de0e3b8a5bdea67ba49bee9ea7f1b8f26 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 27/35] ahci: correct types in NCQTransferState Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-8-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 10 +++++----- hw/ide/ahci.h | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index d996f37b36..efd07ac804 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -45,7 +45,7 @@ do { \ } while (0) static void check_cmd(AHCIState *s, int port); -static int handle_cmd(AHCIState *s,int port,int slot); +static int handle_cmd(AHCIState *s, int port, uint8_t 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); @@ -506,7 +506,7 @@ static void ahci_reg_init(AHCIState *s) static void check_cmd(AHCIState *s, int port) { AHCIPortRegs *pr = &s->dev[port].port_regs; - int slot; + uint8_t slot; if ((pr->cmd & PORT_CMD_START) && pr->cmd_issue) { for (slot = 0; (slot < 32) && pr->cmd_issue; slot++) { @@ -1039,7 +1039,7 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, - int slot) + uint8_t slot) { AHCIDevice *ad = &s->dev[port]; IDEState *ide_state = &ad->port.ifs[0]; @@ -1114,7 +1114,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, } static void handle_reg_h2d_fis(AHCIState *s, int port, - int slot, uint8_t *cmd_fis) + uint8_t slot, uint8_t *cmd_fis) { IDEState *ide_state = &s->dev[port].port.ifs[0]; AHCICmdHdr *cmd = s->dev[port].cur_cmd; @@ -1198,7 +1198,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port, ide_exec_cmd(&s->dev[port].port, cmd_fis[2]); } -static int handle_cmd(AHCIState *s, int port, int slot) +static int handle_cmd(AHCIState *s, int port, uint8_t slot) { IDEState *ide_state; uint64_t tbl_addr; diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 47a3122270..c728e3a07d 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -260,8 +260,8 @@ typedef struct NCQTransferState { uint64_t lba; uint8_t tag; uint8_t cmd; - int slot; - int used; + uint8_t slot; + bool used; bool halt; } NCQTransferState; From e08a98357b5811e7933ff077f6da4b85175caf8a Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 28/35] ahci: correct ncq sector count uint16_t isn't enough to hold the real sector count, since a value of zero implies a full 64K sectors, so we need a uint32_t here. We *could* cheat and pretend that this value is 0-based and fit it in a uint16_t, but I'd rather waste 2 bytes instead of a future dev's 10 minutes when they forget to +1/-1 accordingly somewhere. See SATA 3.2, section 13.6.4.1 "READ FPDMA QUEUED". Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-9-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 7 +++++-- hw/ide/ahci.h | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index efd07ac804..1027a60a9b 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1086,8 +1086,11 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, DPRINTF(port, "Warn: Unsupported attempt to use Rebuild Assist\n"); } - ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) | - ncq_fis->sector_count_low; + ncq_tfs->sector_count = ((ncq_fis->sector_count_high << 8) | + ncq_fis->sector_count_low); + if (!ncq_tfs->sector_count) { + ncq_tfs->sector_count = 0x10000; + } size = ncq_tfs->sector_count * 512; ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0); diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index c728e3a07d..9090d3d882 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -256,7 +256,7 @@ typedef struct NCQTransferState { BlockAIOCB *aiocb; QEMUSGList sglist; BlockAcctCookie acct; - uint16_t sector_count; + uint32_t sector_count; uint64_t lba; uint8_t tag; uint8_t cmd; From 7f6cf5ee1236d94fc25830a47438e57aa294d9fe Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 29/35] qtest/ahci: halted NCQ test Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-10-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index 3f06fd9b1b..c30837b3e6 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -1200,13 +1200,13 @@ static void test_migrate_ncq(void) } /** - * DMA Error Test + * Halted IO Error Test * * Simulate an error on first write, Try to write a pattern, * Confirm the VM has stopped, resume the VM, verify command * has completed, then read back the data and verify. */ -static void test_halted_dma(void) +static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write) { AHCIQState *ahci; uint8_t port; @@ -1241,7 +1241,7 @@ static void test_halted_dma(void) memwrite(ptr, tx, bufsize); /* Attempt to write (and fail) */ - cmd = ahci_guest_io_halt(ahci, port, CMD_WRITE_DMA, + cmd = ahci_guest_io_halt(ahci, port, cmd_write, ptr, bufsize, 0); /* Attempt to resume the command */ @@ -1249,7 +1249,7 @@ static void test_halted_dma(void) ahci_free(ahci, ptr); /* Read back and verify */ - ahci_io(ahci, port, CMD_READ_DMA, rx, bufsize, 0); + ahci_io(ahci, port, cmd_read, rx, bufsize, 0); g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0); /* Cleanup and go home */ @@ -1258,6 +1258,16 @@ static void test_halted_dma(void) g_free(tx); } +static void test_halted_dma(void) +{ + ahci_halted_io_test(CMD_READ_DMA, CMD_WRITE_DMA); +} + +static void test_halted_ncq(void) +{ + ahci_halted_io_test(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED); +} + /** * DMA Error Migration Test * @@ -1677,6 +1687,7 @@ int main(int argc, char **argv) qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple); qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq); + qtest_add_func("/ahci/io/ncq/retry", test_halted_ncq); ret = g_test_run(); From c82bd3c893825fc76af3634f5461f5eabd94e9df Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 30/35] ahci: add cmd header to ncq transfer state While the rest of the AHCI device can rely on a single bookmarked pointer for the AHCI Command Header currently being processed, NCQ is asynchronous and may have many commands in flight simultaneously. Add a cmdh pointer to the ncq_tfs object and make the sglist prepare function take an AHCICmdHeader pointer so we can be explicit about where we'd like to build SGlists from. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-11-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 11 ++++++----- hw/ide/ahci.h | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 1027a60a9b..b77512b0a0 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -833,9 +833,8 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl) } static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, - int64_t limit, int32_t offset) + AHCICmdHdr *cmd, int64_t limit, int32_t offset) { - AHCICmdHdr *cmd = ad->cur_cmd; 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); @@ -1058,6 +1057,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->used = 1; ncq_tfs->drive = ad; ncq_tfs->slot = slot; + ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[slot]; ncq_tfs->cmd = ncq_fis->command; ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) | ((uint64_t)ncq_fis->lba4 << 32) | @@ -1092,7 +1092,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, ncq_tfs->sector_count = 0x10000; } size = ncq_tfs->sector_count * 512; - ahci_populate_sglist(ad, &ncq_tfs->sglist, size, 0); + ahci_populate_sglist(ad, &ncq_tfs->sglist, ncq_tfs->cmdh, size, 0); if (ncq_tfs->sglist.size < size) { error_report("ahci: PRDT length for NCQ command (0x%zx) " @@ -1362,7 +1362,8 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit) AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma); IDEState *s = &ad->port.ifs[0]; - if (ahci_populate_sglist(ad, &s->sg, limit, s->io_buffer_offset) == -1) { + if (ahci_populate_sglist(ad, &s->sg, ad->cur_cmd, + limit, s->io_buffer_offset) == -1) { DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n"); return -1; } @@ -1397,7 +1398,7 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write) uint8_t *p = s->io_buffer + s->io_buffer_index; int l = s->io_buffer_size - s->io_buffer_index; - if (ahci_populate_sglist(ad, &s->sg, l, s->io_buffer_offset)) { + if (ahci_populate_sglist(ad, &s->sg, ad->cur_cmd, l, s->io_buffer_offset)) { return 0; } diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 9090d3d882..9f5b4d20b5 100644 --- a/hw/ide/ahci.h +++ b/hw/ide/ahci.h @@ -254,6 +254,7 @@ typedef struct AHCIDevice AHCIDevice; typedef struct NCQTransferState { AHCIDevice *drive; BlockAIOCB *aiocb; + AHCICmdHdr *cmdh; QEMUSGList sglist; BlockAcctCookie acct; uint32_t sector_count; From ee364416c1b5ed1adc779ca7197451a131666236 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 31/35] ahci: add get_cmd_header helper cur_cmd is an internal bookmark that points to the current AHCI Command Header being processed by the AHCI state machine. With NCQ needing to occasionally rely on some of the same AHCI helpers, we cannot use cur_cmd and will need to grab explicit pointers instead. In an attempt to begin relying on the cur_cmd pointer less, add a helper to let us specifically get the pointer to the command header of particular interest. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-12-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index b77512b0a0..13b0157cbe 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1116,11 +1116,20 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, execute_ncq_command(ncq_tfs); } +static AHCICmdHdr *get_cmd_header(AHCIState *s, uint8_t port, uint8_t slot) +{ + if (port >= s->ports || slot >= AHCI_MAX_CMDS) { + return NULL; + } + + return s->dev[port].lst ? &((AHCICmdHdr *)s->dev[port].lst)[slot] : NULL; +} + static void handle_reg_h2d_fis(AHCIState *s, int port, uint8_t slot, uint8_t *cmd_fis) { IDEState *ide_state = &s->dev[port].port.ifs[0]; - AHCICmdHdr *cmd = s->dev[port].cur_cmd; + AHCICmdHdr *cmd = get_cmd_header(s, port, slot); uint16_t opts = le16_to_cpu(cmd->opts); if (cmd_fis[1] & 0x0F) { @@ -1219,7 +1228,7 @@ static int handle_cmd(AHCIState *s, int port, uint8_t slot) DPRINTF(port, "error: lst not given but cmd handled"); return -1; } - cmd = &((AHCICmdHdr *)s->dev[port].lst)[slot]; + cmd = get_cmd_header(s, port, slot); /* remember current slot handle for later */ s->dev[port].cur_cmd = cmd; @@ -1572,7 +1581,7 @@ static int ahci_state_post_load(void *opaque, int version_id) if (ad->busy_slot < 0 || ad->busy_slot >= AHCI_MAX_CMDS) { return -1; } - ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot]; + ad->cur_cmd = get_cmd_header(s, i, ad->busy_slot); } } From 684d50132fdd68f4c2cba9e65b50f9b8ef4c5164 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 32/35] ahci: ncq migration Migrate the NCQ queue. This is solely for the benefit of halted commands, since anything else should have completed and had any relevant status flushed to the HBA registers already. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-13-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 13b0157cbe..a2620f6514 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1521,6 +1521,21 @@ void ahci_reset(AHCIState *s) } } +static const VMStateDescription vmstate_ncq_tfs = { + .name = "ncq state", + .version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(sector_count, NCQTransferState), + VMSTATE_UINT64(lba, NCQTransferState), + VMSTATE_UINT8(tag, NCQTransferState), + VMSTATE_UINT8(cmd, NCQTransferState), + VMSTATE_UINT8(slot, NCQTransferState), + VMSTATE_BOOL(used, NCQTransferState), + VMSTATE_BOOL(halt, NCQTransferState), + VMSTATE_END_OF_LIST() + }, +}; + static const VMStateDescription vmstate_ahci_device = { .name = "ahci port", .version_id = 1, @@ -1546,14 +1561,17 @@ static const VMStateDescription vmstate_ahci_device = { VMSTATE_BOOL(done_atapi_packet, AHCIDevice), VMSTATE_INT32(busy_slot, AHCIDevice), VMSTATE_BOOL(init_d2h_sent, AHCIDevice), + VMSTATE_STRUCT_ARRAY(ncq_tfs, AHCIDevice, AHCI_MAX_CMDS, + 1, vmstate_ncq_tfs, NCQTransferState), VMSTATE_END_OF_LIST() }, }; static int ahci_state_post_load(void *opaque, int version_id) { - int i; + int i, j; struct AHCIDevice *ad; + NCQTransferState *ncq_tfs; AHCIState *s = opaque; for (i = 0; i < s->ports; i++) { @@ -1565,6 +1583,37 @@ static int ahci_state_post_load(void *opaque, int version_id) return -1; } + for (j = 0; j < AHCI_MAX_CMDS; j++) { + ncq_tfs = &ad->ncq_tfs[j]; + ncq_tfs->drive = ad; + + if (ncq_tfs->used != ncq_tfs->halt) { + return -1; + } + if (!ncq_tfs->halt) { + continue; + } + if (!is_ncq(ncq_tfs->cmd)) { + return -1; + } + if (ncq_tfs->slot != ncq_tfs->tag) { + return -1; + } + /* If ncq_tfs->halt is justly set, the engine should be engaged, + * and the command list buffer should be mapped. */ + ncq_tfs->cmdh = get_cmd_header(s, i, ncq_tfs->slot); + if (!ncq_tfs->cmdh) { + return -1; + } + ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist, + ncq_tfs->cmdh, ncq_tfs->sector_count * 512, + 0); + if (ncq_tfs->sector_count != ncq_tfs->sglist.size >> 9) { + return -1; + } + } + + /* * If an error is present, ad->busy_slot will be valid and not -1. * In this case, an operation is waiting to resume and will re-check From dd6282217d8fee36e3854eab2635bec9cc5d5236 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 33/35] ahci: Do not map cmd_fis to generate response The Register D2H FIS should copy the current values of the registers instead of just parroting back the same values the guest sent back to it. In this case, the SECTOR COUNT variables are actually not generally meaningful in terms of standard commands (See ATA8-AC3 Section 9.2 Normal Outputs), so it actually probably doesn't matter what we put in here. Meanwhile, we do need to use the Register update FIS from the NCQ pathways (in error cases), so getting rid of references to cur_cmd here is a win for AHCI concurrency. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-14-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 50 +++++--------------------------------------------- 1 file changed, 5 insertions(+), 45 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index a2620f6514..eadd8b32c6 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -701,35 +701,13 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished) static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) { AHCIPortRegs *pr = &ad->port_regs; - uint8_t *pio_fis, *cmd_fis; - uint64_t tbl_addr; - dma_addr_t cmd_len = 0x80; + uint8_t *pio_fis; IDEState *s = &ad->port.ifs[0]; if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; } - /* map cmd_fis */ - tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr); - cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len, - DMA_DIRECTION_TO_DEVICE); - - if (cmd_fis == NULL) { - DPRINTF(ad->port_no, "dma_memory_map failed in ahci_write_fis_pio"); - ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR); - return; - } - - if (cmd_len != 0x80) { - DPRINTF(ad->port_no, - "dma_memory_map mapped too few bytes in ahci_write_fis_pio"); - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); - ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR); - return; - } - pio_fis = &ad->res_fis[RES_FIS_PSFIS]; pio_fis[0] = SATA_FIS_TYPE_PIO_SETUP; @@ -745,8 +723,8 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) pio_fis[9] = s->hob_lcyl; pio_fis[10] = s->hob_hcyl; pio_fis[11] = 0; - pio_fis[12] = cmd_fis[12]; - pio_fis[13] = cmd_fis[13]; + pio_fis[12] = s->nsector & 0xFF; + pio_fis[13] = (s->nsector >> 8) & 0xFF; pio_fis[14] = 0; pio_fis[15] = s->status; pio_fis[16] = len & 255; @@ -763,9 +741,6 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) } ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS); - - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); } static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) @@ -773,22 +748,12 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) AHCIPortRegs *pr = &ad->port_regs; uint8_t *d2h_fis; int i; - dma_addr_t cmd_len = 0x80; - int cmd_mapped = 0; IDEState *s = &ad->port.ifs[0]; if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; } - if (!cmd_fis) { - /* map cmd_fis */ - uint64_t tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr); - cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len, - DMA_DIRECTION_TO_DEVICE); - cmd_mapped = 1; - } - d2h_fis = &ad->res_fis[RES_FIS_RFIS]; d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H; @@ -804,8 +769,8 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) d2h_fis[9] = s->hob_lcyl; d2h_fis[10] = s->hob_hcyl; d2h_fis[11] = 0; - d2h_fis[12] = cmd_fis[12]; - d2h_fis[13] = cmd_fis[13]; + d2h_fis[12] = s->nsector & 0xFF; + d2h_fis[13] = (s->nsector >> 8) & 0xFF; for (i = 14; i < 20; i++) { d2h_fis[i] = 0; } @@ -819,11 +784,6 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis) } ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS); - - if (cmd_mapped) { - dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len, - DMA_DIRECTION_TO_DEVICE, cmd_len); - } } static int prdt_tbl_entry_size(const AHCI_SG *tbl) From 8146d7dc2756138bd4011e8d882ead929f25f2d0 Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 34/35] qtest/ahci: halted ncq migration test Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-15-git-send-email-jsnow@redhat.com --- tests/ahci-test.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/ahci-test.c b/tests/ahci-test.c index c30837b3e6..87d7691861 100644 --- a/tests/ahci-test.c +++ b/tests/ahci-test.c @@ -1269,13 +1269,13 @@ static void test_halted_ncq(void) } /** - * DMA Error Migration Test + * IO Error Migration Test * * Simulate an error on first write, Try to write a pattern, * Confirm the VM has stopped, migrate, resume the VM, * verify command has completed, then read back the data and verify. */ -static void test_migrate_halted_dma(void) +static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write) { AHCIQState *src, *dst; uint8_t port; @@ -1321,14 +1321,14 @@ static void test_migrate_halted_dma(void) memwrite(ptr, tx, bufsize); /* Write, trigger the VM to stop, migrate, then resume. */ - cmd = ahci_guest_io_halt(src, port, CMD_WRITE_DMA, + cmd = ahci_guest_io_halt(src, port, cmd_write, ptr, bufsize, 0); ahci_migrate(src, dst, uri); ahci_guest_io_resume(dst, cmd); ahci_free(dst, ptr); /* Read back */ - ahci_io(dst, port, CMD_READ_DMA, rx, bufsize, 0); + ahci_io(dst, port, cmd_read, rx, bufsize, 0); /* Verify TX and RX are identical */ g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0); @@ -1340,6 +1340,16 @@ static void test_migrate_halted_dma(void) g_free(tx); } +static void test_migrate_halted_dma(void) +{ + ahci_migrate_halted_io(CMD_READ_DMA, CMD_WRITE_DMA); +} + +static void test_migrate_halted_ncq(void) +{ + ahci_migrate_halted_io(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED); +} + /** * Migration test: Try to flush, migrate, then resume. */ @@ -1688,6 +1698,7 @@ int main(int argc, char **argv) qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple); qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq); qtest_add_func("/ahci/io/ncq/retry", test_halted_ncq); + qtest_add_func("/ahci/migrate/ncq/halted", test_migrate_halted_ncq); ret = g_test_run(); From 7c649ac5b607e2339fb54fc0fc01311ba5eacadd Mon Sep 17 00:00:00 2001 From: John Snow Date: Sat, 4 Jul 2015 02:06:05 -0400 Subject: [PATCH 35/35] ahci: fix sdb fis semantics There are two things to fix here: The first one is subtle: the PxSACT register in the AHCI HBA has different semantics from the field it is shadowing, the ACT field in the Set Device Bits FIS. In the HBA register, PxSACT acts as a bitfield indicating outstanding NCQ commands where a set bit indicates a pending NCQ operation. The FIS field however operates as an RWC register update to PxSACT, where a set bit indicates a *successfully* completed command. Correct the FIS semantics. At the same time, move the "clear finished" action to the SDB FIS generation instead of the register read to mimick how the other shadow registers work, which always just report the last reported value from a FIS, and not the most current values which may not have been reported by a FIS yet. Lastly and more simply, SATA 3.2 section 13.6.4.2 (and later sections) all specify that the Interrupt bit for the SDB FIS should always be set to one for NCQ commands. That's currently the only time we generate this FIS, so set it on all the time. Signed-off-by: John Snow Reviewed-by: Stefan Hajnoczi Message-id: 1435767578-32743-16-git-send-email-jsnow@redhat.com --- hw/ide/ahci.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index eadd8b32c6..bb6a92f7f4 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -106,8 +106,6 @@ static uint32_t ahci_port_read(AHCIState *s, int port, int offset) val = pr->scr_err; break; case PORT_SCR_ACT: - pr->scr_act &= ~s->dev[port].finished; - s->dev[port].finished = 0; val = pr->scr_act; break; case PORT_CMD_ISSUE: @@ -666,14 +664,14 @@ static void ahci_unmap_clb_address(AHCIDevice *ad) ad->lst = NULL; } -static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished) +static void ahci_write_fis_sdb(AHCIState *s, NCQTransferState *ncq_tfs) { - AHCIDevice *ad = &s->dev[port]; + AHCIDevice *ad = ncq_tfs->drive; AHCIPortRegs *pr = &ad->port_regs; IDEState *ide_state; SDBFIS *sdb_fis; - if (!s->dev[port].res_fis || + if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) { return; } @@ -683,19 +681,23 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished) sdb_fis->type = SATA_FIS_TYPE_SDB; /* Interrupt pending & Notification bit */ - sdb_fis->flags = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0); + sdb_fis->flags = 0x40; /* Interrupt bit, always 1 for NCQ */ sdb_fis->status = ide_state->status & 0x77; sdb_fis->error = ide_state->error; /* update SAct field in SDB_FIS */ - s->dev[port].finished |= finished; sdb_fis->payload = cpu_to_le32(ad->finished); /* Update shadow registers (except BSY 0x80 and DRQ 0x08) */ pr->tfdata = (ad->port.ifs[0].error << 8) | (ad->port.ifs[0].status & 0x77) | (pr->tfdata & 0x88); + pr->scr_act &= ~ad->finished; + ad->finished = 0; - ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); + /* Trigger IRQ if interrupt bit is set (which currently, it always is) */ + if (sdb_fis->flags & 0x40) { + ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); + } } static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len) @@ -895,11 +897,14 @@ static void ncq_err(NCQTransferState *ncq_tfs) static void ncq_finish(NCQTransferState *ncq_tfs) { - /* Clear bit for this tag in SActive */ - ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag); + /* If we didn't error out, set our finished bit. Errored commands + * do not get a bit set for the SDB FIS ACT register, nor do they + * clear the outstanding bit in scr_act (PxSACT). */ + if (!(ncq_tfs->drive->port_regs.scr_err & (1 << ncq_tfs->tag))) { + ncq_tfs->drive->finished |= (1 << ncq_tfs->tag); + } - ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs->drive->port_no, - (1 << ncq_tfs->tag)); + ahci_write_fis_sdb(ncq_tfs->drive->hba, ncq_tfs); DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n", ncq_tfs->tag);