[Qemu-devel] [PATCH 00/16] ahci: ncq cleanup, part 2

[Qemu-devel] [PATCH 00/16] ahci: ncq cleanup, part 2

[John Snow]

requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
          1435016308-6150-1-git-send-email-jsnow@redhat.com
          [PATCH v2 0/4] ahci: misc fixes/tests for 2.4
          [PATCH v2 00/16] ahci: ncq cleanup, part 1

This chunk gets NCQ migration and and resume support working.

There's still some left to do, particularly around error handling
and FIS semantics, but this should get us most of the way there.

There is one RFC bit in this patch, inside of Patch #1, concerning
how to handle truncating PRDTs that are too large -- it looks like
we have attempted to address it in the past, and I accidentally
bumped up against it now. By actually trying to consume the PRDs
but ignoring any extra space they describe, I would break ide-test.

I'll post logs later, but judging by the test text itself, we don't
seem to know what the right behavior is.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-ncq-s2
https://github.com/jnsnow/qemu/tree/ahci-ncq-s2

This version is tagged ahci-ncq-s2-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s2-v1

John Snow (16):
  ide: add limit to .prepare_buf()
  ahci: stash ncq command
  ahci: assert is_ncq for process_ncq
  ahci: refactor process_ncq_command
  ahci: factor ncq_finish out of ncq_cb
  ahci: record ncq failures
  ahci: kick NCQ queue
  ahci: correct types in NCQTransferState
  ahci: correct ncq sector count
  qtest/ahci: halted NCQ test
  ahci: add cmd header to ncq transfer state
  ahci: ncq migration
  ahci: add get_cmd_header helper
  ahci: Do not map cmd_fis to generate response
  qtest/ahci: halted ncq migration test
  ahci: fix sdb fis semantics

 hw/ide/ahci.c     | 330 ++++++++++++++++++++++++++++++++----------------------
 hw/ide/ahci.h     |   9 +-
 hw/ide/core.c     |  12 +-
 hw/ide/internal.h |   4 +-
 hw/ide/macio.c    |   2 +-
 hw/ide/pci.c      |   5 +-
 tests/ahci-test.c |  38 +++++--
 7 files changed, 252 insertions(+), 148 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 01/16] ide: add limit to .prepare_buf()

[John Snow]

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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 27 ++++++++++++++-------------
 hw/ide/core.c     |  5 +++--
 hw/ide/internal.h |  2 +-
 hw/ide/macio.c    |  2 +-
 hw/ide/pci.c      |  5 ++++-
 5 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 95d228f..f873ab1 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, int64_t limit, int is_write);
 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, is_write)) {
         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, int64_t limit, int is_write)
 {
     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 1efd98a..6eefb30 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -734,7 +734,8 @@ 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,
+                                      ide_cmd_is_read(s)) < 512) {
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
         s->status = READY_STAT | SEEK_STAT;
@@ -2326,7 +2327,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, int64_t l, int x)
 {
     return 0;
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 965cc55..7a4a86d 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 *, int64_t len, int is_write);
 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 dd52d50..1bd1580 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, int64_t l, int x)
 {
     return 0;
 }
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4afd0cf..a295baa 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -55,8 +55,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
 /**
  * Return the number of bytes successfully prepared.
  * -1 on error.
+ * BUG?: Does not currently heed the 'limit' parameter because
+ *       it is not clear what the correct behavior here is,
+ *       see tests/ide-test.c
  */
-static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
+static int32_t bmdma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
     IDEState *s = bmdma_active_if(bm);
-- 
2.1.0

[Qemu-devel] [PATCH 02/16] ahci: stash ncq command

[John Snow]

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 <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 f873ab1..ce99e81 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 b8872a4..33607d7 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;
-- 
2.1.0

[Qemu-devel] [PATCH 03/16] ahci: assert is_ncq for process_ncq

[John Snow]

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 <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 ce99e81..2d70104 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);
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH 04/16] ahci: refactor process_ncq_command

[John Snow]

Split off execute_ncq_command so that we can call
it separately later if we desire.

Signed-off-by: John Snow <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 2d70104..4f8cceb 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,
-- 
2.1.0

[Qemu-devel] [PATCH 05/16] ahci: factor ncq_finish out of ncq_cb

[John Snow]

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 <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 4f8cceb..71b5085 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 */
-- 
2.1.0

[Qemu-devel] [PATCH 06/16] ahci: record ncq failures

[John Snow]

Handle NCQ failures for cases where we want to halt the VM on IO errors.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 17 +++++++++++++++--
 hw/ide/ahci.h     |  1 +
 hw/ide/internal.h |  1 +
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 71b5085..a838317 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -959,13 +959,25 @@ static void ncq_cb(void *opaque, int ret)
         return;
     }
 
+    ncq_tfs->halt = false;
     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)
@@ -1042,6 +1054,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     }
 
     ncq_tfs->used = 1;
+    ncq_tfs->halt = false;
     ncq_tfs->drive = ad;
     ncq_tfs->slot = slot;
     ncq_tfs->cmd = ncq_fis->command;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 33607d7..47a3122 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/internal.h b/hw/ide/internal.h
index 7a4a86d..5abee19 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -499,6 +499,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)
 {
-- 
2.1.0

[Qemu-devel] [PATCH 07/16] ahci: kick NCQ queue

[John Snow]

Upon a VM state change, retry the halted NCQ commands.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 18 ++++++++++++++++++
 hw/ide/core.c     |  7 +++++++
 hw/ide/internal.h |  1 +
 3 files changed, 26 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a838317..6233059 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1332,6 +1332,23 @@ static void ahci_restart_dma(IDEDMA *dma)
 }
 
 /**
+ * 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,
  * which invoke ahci_populate_sglist via ahci_start_transfer.
@@ -1419,6 +1436,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/core.c b/hw/ide/core.c
index 6eefb30..8c271cc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2372,6 +2372,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 5abee19..adb968c 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;
-- 
2.1.0

[Qemu-devel] [PATCH 08/16] ahci: correct types in NCQTransferState

[John Snow]

Signed-off-by: John Snow <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 6233059..7fcc6a2 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++) {
@@ -1037,7 +1037,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];
@@ -1113,7 +1113,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;
@@ -1197,7 +1197,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 47a3122..c728e3a 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;
 
-- 
2.1.0

[Qemu-devel] [PATCH 09/16] ahci: correct ncq sector count

[John Snow]

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 <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 7fcc6a2..043b959 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1085,8 +1085,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 c728e3a..9090d3d 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;
-- 
2.1.0

[Qemu-devel] [PATCH 10/16] qtest/ahci: halted NCQ test

[John Snow]

Signed-off-by: John Snow <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 3f06fd9..c30837b 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();
 
-- 
2.1.0

[Qemu-devel] [PATCH 11/16] ahci: add cmd header to ncq transfer state

[John Snow]

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 <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 043b959..4cf792b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -832,9 +832,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);
@@ -1057,6 +1056,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     ncq_tfs->halt = false;
     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) |
@@ -1091,7 +1091,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) "
@@ -1361,7 +1361,8 @@ static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int64_t limit, int is_write)
     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;
     }
@@ -1396,7 +1397,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 9090d3d..9f5b4d2 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;
-- 
2.1.0

[Qemu-devel] [PATCH 12/16] ahci: ncq migration

[John Snow]

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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4cf792b..a29cf49 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1511,6 +1511,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,
@@ -1536,14 +1551,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++) {
@@ -1555,6 +1573,35 @@ 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->slot > AHCI_MAX_CMDS) {
+                return -1;
+            }
+            ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];
+            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
-- 
2.1.0

[Qemu-devel] [PATCH 13/16] ahci: add get_cmd_header helper

[John Snow]

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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a29cf49..eeb48fb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1115,11 +1115,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) {
@@ -1218,7 +1227,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;
 
@@ -1592,7 +1601,7 @@ static int ahci_state_post_load(void *opaque, int version_id)
             if (ncq_tfs->slot > AHCI_MAX_CMDS) {
                 return -1;
             }
-            ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];
+            ncq_tfs->cmdh = get_cmd_header(s, i, ncq_tfs->slot);
             ahci_populate_sglist(ncq_tfs->drive, &ncq_tfs->sglist,
                                  ncq_tfs->cmdh, ncq_tfs->sector_count * 512,
                                  0);
@@ -1618,7 +1627,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);
         }
     }
 
-- 
2.1.0

[Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[John Snow]

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 <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 eeb48fb..d344701 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -700,35 +700,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;
@@ -744,8 +722,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;
@@ -762,9 +740,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)
@@ -772,22 +747,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;
@@ -803,8 +768,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;
     }
@@ -818,11 +783,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)
-- 
2.1.0

[Qemu-devel] [PATCH 15/16] qtest/ahci: halted ncq migration test

[John Snow]

Signed-off-by: John Snow <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 c30837b..87d7691 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();
 
-- 
2.1.0

[Qemu-devel] [PATCH 16/16] ahci: fix sdb fis semantics

[John Snow]

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 <jsnow@redhat.com>
---
 hw/ide/ahci.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d344701..db62a53 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:
@@ -665,14 +663,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;
     }
@@ -682,19 +680,22 @@ 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);
+    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)
@@ -894,11 +895,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);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 01/16] ide: add limit to .prepare_buf()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Mon, Jun 22, 2015 at 08:21:00PM -0400, John Snow wrote:
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 95d228f..f873ab1 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, int64_t limit, int is_write);

Why int64_t?

Types involved here are uint64_t, dma_addr_t, size_t, and int.  Out of
these, uint64_t seems like a good candidate but I'm not sure why it
needs to be signed.

> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 4afd0cf..a295baa 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -55,8 +55,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
>  /**
>   * Return the number of bytes successfully prepared.
>   * -1 on error.
> + * BUG?: Does not currently heed the 'limit' parameter because
> + *       it is not clear what the correct behavior here is,
> + *       see tests/ide-test.c

QEMU implements both short and long PRDT cases for IDE in ide_dma_cb()
and the tests check them.  I saw nothing indicating that existing
behavior might not correspond to real hardware behavior.  Why do you say
the correct behavior is unclear?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/16] ahci: record ncq failures

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
> Handle NCQ failures for cases where we want to halt the VM on IO errors.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c     | 17 +++++++++++++++--
>  hw/ide/ahci.h     |  1 +
>  hw/ide/internal.h |  1 +
>  3 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 71b5085..a838317 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -959,13 +959,25 @@ static void ncq_cb(void *opaque, int ret)
>          return;
>      }
>  
> +    ncq_tfs->halt = false;

Why does halt need to be cleared here?

>      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)
> @@ -1042,6 +1054,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>      }
>  
>      ncq_tfs->used = 1;
> +    ncq_tfs->halt = false;
>      ncq_tfs->drive = ad;
>      ncq_tfs->slot = slot;
>      ncq_tfs->cmd = ncq_fis->command;
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 33607d7..47a3122 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/internal.h b/hw/ide/internal.h
> index 7a4a86d..5abee19 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -499,6 +499,7 @@ struct IDEDevice {
>  #define IDE_RETRY_READ  0x20
>  #define IDE_RETRY_FLUSH 0x40
>  #define IDE_RETRY_TRIM 0x80
> +#define IDE_RETRY_HBA  0x100

Feel free to squash this patch together with the next one.  It is hard
to review in isolation since IDE_RETRY_HBA and ->halt aren't used yet.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/16] ahci: ncq migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]

On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote:
> @@ -1555,6 +1573,35 @@ 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->slot > AHCI_MAX_CMDS) {
> +                return -1;
> +            }
> +            ncq_tfs->cmdh = &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];

Is there a guarantee that ->lst has been mapped?  Maybe pr->cmd &
PORT_CMD_START was 0.

We need to check that the HBA is in a valid state for NCQ processing
before attempting this.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 13/16] ahci: add get_cmd_header helper

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 297 bytes --]

On Mon, Jun 22, 2015 at 08:21:12PM -0400, John Snow wrote:
> +static AHCICmdHdr *get_cmd_header(AHCIState *s, uint8_t port, uint8_t slot)
> +{
> +    if (port > s->ports || slot > AHCI_MAX_CMDS) {

Should these be >= instead of >?  Otherwise 1 element beyond the end of
the array can be accessed.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 542 bytes --]

On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
> @@ -744,8 +722,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;

hw/ide/core.c decreases s->nsector until it reaches 0 and the request
ends.

Will the values reported back to the guest be correct if we use
s->nsector?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 16/16] ahci: fix sdb fis semantics

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote:
> @@ -682,19 +680,22 @@ 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 */
...
> -    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
> +    if (sdb_fis->flags & 0x40) {
> +        ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS);
> +    }

This if statement is always true.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 00/16] ahci: ncq cleanup, part 2

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

On Mon, Jun 22, 2015 at 08:20:59PM -0400, John Snow wrote:
> requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
>           1435016308-6150-1-git-send-email-jsnow@redhat.com
>           [PATCH v2 0/4] ahci: misc fixes/tests for 2.4
>           [PATCH v2 00/16] ahci: ncq cleanup, part 1
> 
> This chunk gets NCQ migration and and resume support working.
> 
> There's still some left to do, particularly around error handling
> and FIS semantics, but this should get us most of the way there.
> 
> There is one RFC bit in this patch, inside of Patch #1, concerning
> how to handle truncating PRDTs that are too large -- it looks like
> we have attempted to address it in the past, and I accidentally
> bumped up against it now. By actually trying to consume the PRDs
> but ignoring any extra space they describe, I would break ide-test.
> 
> I'll post logs later, but judging by the test text itself, we don't
> seem to know what the right behavior is.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-ncq-s2
> https://github.com/jnsnow/qemu/tree/ahci-ncq-s2
> 
> This version is tagged ahci-ncq-s2-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s2-v1
> 
> John Snow (16):
>   ide: add limit to .prepare_buf()
>   ahci: stash ncq command
>   ahci: assert is_ncq for process_ncq
>   ahci: refactor process_ncq_command
>   ahci: factor ncq_finish out of ncq_cb
>   ahci: record ncq failures
>   ahci: kick NCQ queue
>   ahci: correct types in NCQTransferState
>   ahci: correct ncq sector count
>   qtest/ahci: halted NCQ test
>   ahci: add cmd header to ncq transfer state
>   ahci: ncq migration
>   ahci: add get_cmd_header helper
>   ahci: Do not map cmd_fis to generate response
>   qtest/ahci: halted ncq migration test
>   ahci: fix sdb fis semantics
> 
>  hw/ide/ahci.c     | 330 ++++++++++++++++++++++++++++++++----------------------
>  hw/ide/ahci.h     |   9 +-
>  hw/ide/core.c     |  12 +-
>  hw/ide/internal.h |   4 +-
>  hw/ide/macio.c    |   2 +-
>  hw/ide/pci.c      |   5 +-
>  tests/ahci-test.c |  38 +++++--
>  7 files changed, 252 insertions(+), 148 deletions(-)

I posted comments on a few patches.  The rest looks good.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/16] ahci: ncq migration

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 11:48 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote:
>> @@ -1555,6 +1573,35 @@ 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->slot > AHCI_MAX_CMDS) { +
>> return -1; +            } +            ncq_tfs->cmdh =
>> &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];
> 
> Is there a guarantee that ->lst has been mapped?  Maybe pr->cmd & 
> PORT_CMD_START was 0.
> 
> We need to check that the HBA is in a valid state for NCQ
> processing before attempting this.
> 

Slipped my mind, there should be no way to have ncq_tfs->halt set
under normal circumstances except if the engine is started (and lst is
mapped.)

That said, stream corruption is always possible, so I'll add another
validation check.

An "is not null" check here should be sufficient, due to the "if not
halt" continue up above.

Thanks,
- --js
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjYHPAAoJEH3vgQaq/DkOkOsP/2O1bJJWaSAUVGB168GLQKGY
B4ffiV37g4JrrLdhVeRuJiHYZmHT31VwBYqX2ZfxFJWqOBsei3PUm6qN7OjKyUQr
td/oAaT3TKou8qySfbbgXMN1ubodtXU8CHOYykfNXiRqMeuW1CZUvP6IvEqJhZrj
nu5H5l+W1lzsgEt2MCbK6VN96l3LMpYp8EafbZV1tOiMW63U8Tdc2IVJD1UP8krv
U5ngLUegs5FCfHXYMBoIamDXDux52iE06EboP0J+nS21jNadtm/akJ2qq7ndKLVJ
vghtlI+9Teq8bKq8JjY9qQY9tAe3FCIqygquwCOcQPKsi7FxnkB396k3sY9BckFK
uBNMOBo6b0+5S4NnS7KV/L3986StepoStxBsEJj89EOi6AidDJocIs67m3TkOb5T
WGbFQsnzXi+MLJw4Ck583DbwZ92fkdUNKEGlZtbvZ3UxpJ0qNos8hNPenewGlmQO
BrcgTrD0f+Hr9GrLA+ACXgUah1I5giSsAYyjX3REwzrhuMEYMAv9vfBszrdMW/KP
VZiLNl763YcjdK4EMC72R/R7+pf25RyTA8w4yAcc8mvTR+fDIuTtgCrHo8MEhIfS
91UC72zgmVhOWq2JelVR0sPKWGC4dvWLs/r0gfTOlthjzx5twnbSG2/XhrWPuJmd
MCTuNL5CiuRCdjfq9hLB
=XQh5
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
>> @@ -744,8 +722,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;
> 
> hw/ide/core.c decreases s->nsector until it reaches 0 and the
> request ends.
> 
> Will the values reported back to the guest be correct if we use 
> s->nsector?
> 

See the commit message for justification of this one. Ultimately, it
doesn't matter what gets put in here (for data transfer commands) --
but getting RID of the cmd_fis mapping is a strong positive.

The field is supposed to contain the contents of the sector count
register at the conclusion of the command. The spec says that for data
transfer commands, this value is useless/uninteresting (it can be
zero-filled, or filled with garbage, etc)

However, for some special purpose commands, the sector count register
is required to hold a value that has some special meaning. I don't
think we currently support any of those, but for the future -- this
part of the FIS generation code will work correctly regardless.

So this patch is mainly about getting rid of the cmd_fis mapping and
putting something nominally correct in its place.

- --js
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjYxgAAoJEH3vgQaq/DkOYiMQAIUetgDGGd8QvntbT147pifE
f8vLqV1TDZL2cmorplUT8K5BFPEOiSFn3GIa2K4sVmfZy9L7r/Q8SUn/WtV0AeLD
/OkU7l25JIz62cSKpZZpfJcKsw9iBjRoeltxM1AfiS+GpUIMRxxORUEsgswEJQzn
CPLkOOE+ME1Bh9qq6/kYj0+gPLwC2UjoW2xny1A5pizwdf4VlJl9jyM5DWMdtryW
Qcd/djxK2CAXsvpMYNQnnjF7JDp+nkGaXct+hTQEUZRWbUv02+KOt9StBfBh+Dq1
njYq6pgUfvvbjOzccHTzgeaUlCCI6mP+ng0ksjG2HW9LI8QyV9whtPB/Rc2ORfhz
R1zTw5JW6fsAVnbfkxBC7OFJBZB4WfmJWEdIPUmrmLgpxaBDi9jOK+bqYXeTTpXv
bgdFJN2BAWXW1F4UCSjDaVUAL1FYG5vjxw+MbbWGcdHTFPT6z3OZVxe1AST7SS3j
xuqIVMaxBeskQgUJiwOBFSFkTMTpWPIfRLY1MLk9yqXuwQpg7NtQncbho3wCj1cC
nxpjDjWRAB3a2odjCocA9RN6uBNeKq2WbmFzU6bODgjOHRFUPfP/s3qz43PPQGxa
+VQZaQahOnvVIGWnerXMyl+LT/0tLhsBUZDeRoNbR3MMMprw3yxozP1BM1bQ4bWx
brUOjknDPIwWfjfIpUID
=SaS/
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 16/16] ahci: fix sdb fis semantics

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote:
>> @@ -682,19 +680,22 @@ 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 */
> ...
>> -    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); +    if
>> (sdb_fis->flags & 0x40) { +        ahci_trigger_irq(s, ad,
>> PORT_IRQ_SDB_FIS); +    }
> 
> This if statement is always true.
> 

Yes. I did that on purpose. Maybe that was too weird.

The idea was to emphasize that the IRQ is definitely only triggered
*IF* the interrupt bit is set. It just so happens that we currently
always set it.

The generation and trigger mechanics are supposed to be separate, but
we currently have no use case for them being separate (because we are
an omniscient HBA-HDD amalgamation), so they're mushed together here.

This is kind of like a documentation "NB."

I can replace it with:

/* Trigger IRQ if interrupt bit is set, which currently it always is: */
ahci_trigger_irq(...);

or just add that comment to the existing structure, etc.

(Or if you really prefer, just the naked IRQ trigger, but I feel
that's kind of misleading.)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjY2XAAoJEH3vgQaq/DkOl60P/j7dpKYPHobr+R8YP2ujWsc0
SinxemsCsgdvUXfTK5ma1yYYccHc0y5f7Ug617U4+/Tr7hK/gMhpQS0sLh+aTh1k
DZTo30kqvXsDXIkw6dUjqBJRAPg0bm3RSB+YjqJxOH8o6nyntOmSzaJrxU6Rd6Oj
AJFNtXPmlk3RLiU6uRD/EK7K+HaoRmvcvga6aGfNX4YJgM3+NUUUqiK6Urjok0sR
qRvoYEkqvsJqujMAyHxlZuzW50PVcvEcWMyBXYaO2bvQWG/e/1SueQ9fLO8emdiF
HQEyeCts4fN5VsZqZBdp+68PUqcRm6BtDodh5pPdPOe8OUc3l9Hu4tkTFqbe+iXM
Em6+G9umMo0jPIepztYr56uXwYdLK0VGc2JQ91CqE0OXEmL+qh0+8xTg0e4Ix4Ff
z7M9g90Lvuq9yLRAGtwjBKO+NDdgGXszc40W64yEtChqgt2GNOfLUIUbx45tVU4O
vmddjNVyKE432AcMDC1+Je4uIKHrXwQGHO6ZdncrCvCeAKaDhZF77gpuy7fvga9v
/I51zZVuxCNXEGPSuLYCE7ynA0vNBzQ1zgcEcxTM/2/odFBmgACY2nMSlgFy2b5x
qpTQsTyIis8/mi6sdaX/l3FZ1i/pXbuoUzmrTxOfOmcjFJjS7n5Qb7Yj6aX64sHn
/vESog1pKwxvzwyPmejk
=OYom
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 01/16] ide: add limit to .prepare_buf()

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 10:32 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:00PM -0400, John Snow wrote:
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 95d228f..f873ab1
>> 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, int64_t limit, int is_write);
> 
> Why int64_t?
> 
> Types involved here are uint64_t, dma_addr_t, size_t, and int.  Out
> of these, uint64_t seems like a good candidate but I'm not sure why
> it needs to be signed.
> 

Thinking about it, I could have used int32_t. We currently have an
implicit limit of int32_t for the number of bytes we can prepare,
constrained by the return size and I believe a migration concern
(number of bytes is stored in the IDEState somewhere)

I think I chose int64_t wrongly to allow values bigger than our
implicit limit to be passed to mean effectively "unlimited."

Thinking about it, int32_t and uint32_t can both accomplish the same
thing. (INT32_MAX or UINT32_MAX)

>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 4afd0cf..a295baa
>> 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -55,8 +55,11 @@
>> static void bmdma_start_dma(IDEDMA *dma, IDEState *s, /** *
>> Return the number of bytes successfully prepared. * -1 on error. 
>> + * BUG?: Does not currently heed the 'limit' parameter because +
>> *       it is not clear what the correct behavior here is, + *
>> see tests/ide-test.c
> 
> QEMU implements both short and long PRDT cases for IDE in
> ide_dma_cb() and the tests check them.  I saw nothing indicating
> that existing behavior might not correspond to real hardware
> behavior.  Why do you say the correct behavior is unclear?
> 

Look at ide-test:

"No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't
be able to access it anyway because the Bus Master bit in the PCI
command register isn't set. This is complete nonsense, but it used to
be pretty good at confusing and occasionally crashing qemu."

"Not entirely clear what the expected result is, but this is what we
get in practice. At least we want to be aware of any changes."

Changing the PRDT underflow behavior here (Where the data to be
transferred is less than the size of the PRDT/SGlist) changes how
these tests operate and it was not clear to me what the correct
behavior should be.

In my mind, the "common sense" behavior is to just truncate the list,
do the transfer, and pretend like everything's fine. However, the long
PRDT test in ide-test seems to rely on the fact that the command will
still be running by the time we get to cancel it, which won't be true
if I add any explicit checking.

If I just "consume" the extra PRDs but don't update the sglist, the
command will actually probably finish before we get to cancel it,
which changes the test.

If I throw an error of any kind, that changes the test too.

I really wasn't sure what the right thing to do for BMDMA was, so I
left it alone.

(Looking at the short test now, I'm not actually sure how that works.
If the PRD is too short we halt the transfer but don't signal an
error? We just pretend like it never happened? That seems wrong, too.
69c38b8fcec823da05f98f6ea98ec2b0013d64e2  -- I trust Kevin's judgment
though, so I really have no idea what the "right" thing to do here is.)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjZcZAAoJEH3vgQaq/DkO6EsP/2h7E3JEz9uhKNjFJ/llebkN
I1StH5XEEr5Yg3Py1ANwRkr3vPtqi2ylPLsEG1fdPmZR7gmPuH8SZu4I4A7mYTwp
IeSv5FEhVEvxWCQp/CYeuK9o5viZdMSGGfKeGh5c/SiloFfh4N4OaBTP58vausp5
HohL73PCPzLTPtiO8BCDh6H9xvVnTg3GbhV9JkKq/dUdcmiuDHR0iQWUFS7r4p3H
ysr4kZz98E1iVz36xLlt1vrgivIBfELQ1ZoNsxOo614oRaYlSanHjq+8AWSzuCZq
8RYZgc26n16b64dsjMeqxdKzv7PndCMUdkvnn3oZJg292XwpVrFZYNhz7B/r6/eq
f/cE6P2eALiYZTVL9WgZTP+1rRtB/EOReC8TmYQ+uBGh6uq+qxB1WiN49gOQpsMa
I4zqjheiMy75jdaasf3TL+RLTLZnqOuf9zEtS62rVTKnbBWevEN4DOyOwKpR2kYw
Tal8ufnP8I1texjQ0xMrBJMHxhs1DVwKzd0qkP36zLpNr7iQbsasLIXXYtHQWdUf
oWZ5nfFLHQHw/Voxprew450mW9DzPVmRamUMBjBT/fOSOc9UvYcIYOv0cCC+szes
M0O1CgUVEn1kpYi3Pi5ZxdKydJPk4ntLQpnMMN70KauDHuw0IRQp/SJAyRBJyWhD
W2geowq0caC/rtdulVvk
=R8kD
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 06/16] ahci: record ncq failures

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
>> Handle NCQ failures for cases where we want to halt the VM on IO
>> errors.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c
>> | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 + hw/ide/internal.h
>> |  1 + 3 files changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 71b5085..a838317
>> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -959,13 +959,25
>> @@ static void ncq_cb(void *opaque, int ret) return; }
>> 
>> +    ncq_tfs->halt = false;
> 
> Why does halt need to be cleared here?
> 

Might make more sense to clear it just on the beginning of every
command, in execute().

There's no strong reason here other than "If there's an error and it
should be set, it'll get reset again pretty soon." It's just a default
state.

I could move it from process to execute.

>> 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) @@ -1042,6 +1054,7 @@ static
>> void process_ncq_command(AHCIState *s, int port, uint8_t
>> *cmd_fis, }
>> 
>> ncq_tfs->used = 1; +    ncq_tfs->halt = false; ncq_tfs->drive =
>> ad; ncq_tfs->slot = slot; ncq_tfs->cmd = ncq_fis->command; diff
>> --git a/hw/ide/ahci.h b/hw/ide/ahci.h index 33607d7..47a3122
>> 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/internal.h
>> b/hw/ide/internal.h index 7a4a86d..5abee19 100644 ---
>> a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -499,6 +499,7 @@
>> struct IDEDevice { #define IDE_RETRY_READ  0x20 #define
>> IDE_RETRY_FLUSH 0x40 #define IDE_RETRY_TRIM 0x80 +#define
>> IDE_RETRY_HBA  0x100
> 
> Feel free to squash this patch together with the next one.  It is
> hard to review in isolation since IDE_RETRY_HBA and ->halt aren't
> used yet.
> 

Will do.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjZmbAAoJEH3vgQaq/DkOPUAP/26368m3XEWpWLPePLnBOvS+
tFuUnYyrWyWdq9p9lNBPcz+v+//CAdISQ8bRZX4OS+f3W+uVB04yJc+N5igiFJCe
9f8+lnLnVO6nmNPzeKPhfigBPF8fc5tnmk4P9bSYUcEmTkdPb+HD8tbNh4l9euWl
0+uqGUhn7Gj1G8aZSZ7UNv+6Ru7SBrygnn/GlMrqrVcbTSW2uj6JmpT1xvdk1iU8
mh+j76JIBYRn2dZf4ZIHJW51x5Zijvsi04Rc8EfRDqiGZ/7HK9EQwwoZ5vUImo/d
vuSufuxISAGc5ECeGpb7fFyGbfl7Y2R+l+qipUsYZ704wTGdnkBMAst0E/NK5y8U
IHCv/5IR9lLp1qaAL0DSmkuaz6xeiBktKmy0lRT1Yq38ZK2RMCzgRPHTbsPGfF/Z
XhRkz5lgqJAdzJJNlvUDNI0jAepiREsBP4Oqi7FMaKq7ix3haSCH0k21JIJbAls5
yRNP9hUNh0Q30E/09h4/ZYRhoQTbvzZS2tLJ8zG1lB0dnIK0uML6SjO2mKbsWpN6
hPIYku04BtqDtPlRcfsOQwJ04bHHKh46PSEWaC0xCsvMVhGzWhs4FqMCxsKOvSMC
rAEYzgEWaJdZUMC+qp4RS8aX2yJP/nmHivEEb2vI6196jsXdqrdmYLBNHcL+Q6kY
bDrMjWdP5CbDUu7E4pAY
=kgU+
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 13/16] ahci: add get_cmd_header helper

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/26/2015 11:51 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:21:12PM -0400, John Snow wrote:
>> +static AHCICmdHdr *get_cmd_header(AHCIState *s, uint8_t port,
>> uint8_t slot) +{ +    if (port > s->ports || slot >
>> AHCI_MAX_CMDS) {
> 
> Should these be >= instead of >?  Otherwise 1 element beyond the
> end of the array can be accessed.
> 

Sigh.

Yes, yes it should. Thank you for saving me from myself.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVjZrRAAoJEH3vgQaq/DkOjcMP/0CqxJAWcgsnFH5Opwz7iNbm
OyvLakvzUmU958qN3nzG2vMME3WwyVF3bxVJkoT3v1Pc6Tm9e+hq+R1oDzD0TD6X
tfjQTc5JZcViKSD74Huo85gKoVp3Tp4idOaaVFn36TmP9gk5i6g/C0Nf7EzY12Er
4op6mLvBhiU6NH72UwP4HHij0KZCMCVYYH4qhAWsil/QIugAgELlM9TFkVI49lHs
WN3IE+5gQijd99Z5VJaC9wrmtOdZWHblfTeWMvVQfMfbg5dJ2Aiya1tHlw6QrPzb
7nGPHTwyGLmNsvUsNqMHrWGaL9e4iEwXGYNRnIT16L8QPzGqg7v4D3lvJcGv5LrM
OFPje2Fk9w8OGdgRjrQhQcJXfU+IPkvlwnW7G3NO2Ts0GK5cHClRafZVjvAhMuXs
5M89BTdi7LBGJ7EWq8ByP5mxPj+hiR3hsSG41OPFg3e0VpwtgFJ6umaZde1tMrHp
IwnOZcvXUoTOAna3Y4E/fSwK1jKczTXIvkl4+HnJB40ekyoSkyl6qXV1FlMRiMnq
pAeJ5jgBovuV2gx8pLYU3ipR0LJTjzz7N39FkAJJyekWWjNVrZ4Ue5IGW/pXKVkN
SjeSrZTtxSmyO8BPya7/xsqjiqo3v7m2jSBLTVEsUtkFEaet4xlAH2OQZBvlTbuI
n5Hc61NkB6qMU5k6We31
=Sfdd
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH 00/16] ahci: ncq cleanup, part 2

[John Snow]

On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 08:20:59PM -0400, John Snow wrote:
>> requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com 
>> 1435016308-6150-1-git-send-email-jsnow@redhat.com [PATCH v2 0/4]
>> ahci: misc fixes/tests for 2.4 [PATCH v2 00/16] ahci: ncq
>> cleanup, part 1
>> 
>> This chunk gets NCQ migration and and resume support working.
>> 
>> There's still some left to do, particularly around error
>> handling and FIS semantics, but this should get us most of the
>> way there.
>> 
>> There is one RFC bit in this patch, inside of Patch #1,
>> concerning how to handle truncating PRDTs that are too large --
>> it looks like we have attempted to address it in the past, and I
>> accidentally bumped up against it now. By actually trying to
>> consume the PRDs but ignoring any extra space they describe, I
>> would break ide-test.
>> 
>> I'll post logs later, but judging by the test text itself, we
>> don't seem to know what the right behavior is.
>> 
>> ________________________________________________________________________________
>>
>>
>> 
For convenience, this branch is available at:
>> https://github.com/jnsnow/qemu.git branch ahci-ncq-s2 
>> https://github.com/jnsnow/qemu/tree/ahci-ncq-s2
>> 
>> This version is tagged ahci-ncq-s2-v1: 
>> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s2-v1
>> 
>> John Snow (16): ide: add limit to .prepare_buf() ahci: stash ncq
>> command ahci: assert is_ncq for process_ncq ahci: refactor
>> process_ncq_command ahci: factor ncq_finish out of ncq_cb ahci:
>> record ncq failures ahci: kick NCQ queue ahci: correct types in
>> NCQTransferState ahci: correct ncq sector count qtest/ahci:
>> halted NCQ test ahci: add cmd header to ncq transfer state ahci:
>> ncq migration ahci: add get_cmd_header helper ahci: Do not map
>> cmd_fis to generate response qtest/ahci: halted ncq migration
>> test ahci: fix sdb fis semantics
>> 
>> hw/ide/ahci.c     | 330
>> ++++++++++++++++++++++++++++++++---------------------- 
>> hw/ide/ahci.h     |   9 +- hw/ide/core.c     |  12 +- 
>> hw/ide/internal.h |   4 +- hw/ide/macio.c    |   2 +- 
>> hw/ide/pci.c      |   5 +- tests/ahci-test.c |  38 +++++-- 7
>> files changed, 252 insertions(+), 148 deletions(-)
> 
> I posted comments on a few patches.  The rest looks good.
> 

OK, I'm smooshing 6/7 and 12/13, and correcting issues where
appropriate. Will wait to feedback-feedback before I hit the send button.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 01/16] ide: add limit to .prepare_buf()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]

On Fri, Jun 26, 2015 at 02:16:57PM -0400, John Snow wrote:
> On 06/26/2015 10:32 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:00PM -0400, John Snow wrote:
> >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 4afd0cf..a295baa
> >> 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -55,8 +55,11 @@
> >> static void bmdma_start_dma(IDEDMA *dma, IDEState *s, /** *
> >> Return the number of bytes successfully prepared. * -1 on error. 
> >> + * BUG?: Does not currently heed the 'limit' parameter because +
> >> *       it is not clear what the correct behavior here is, + *
> >> see tests/ide-test.c
> > 
> > QEMU implements both short and long PRDT cases for IDE in
> > ide_dma_cb() and the tests check them.  I saw nothing indicating
> > that existing behavior might not correspond to real hardware
> > behavior.  Why do you say the correct behavior is unclear?
> > 
> 
> Look at ide-test:
> 
> "No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't
> be able to access it anyway because the Bus Master bit in the PCI
> command register isn't set. This is complete nonsense, but it used to
> be pretty good at confusing and occasionally crashing qemu."
> 
> "Not entirely clear what the expected result is, but this is what we
> get in practice. At least we want to be aware of any changes."
> 
> Changing the PRDT underflow behavior here (Where the data to be
> transferred is less than the size of the PRDT/SGlist) changes how
> these tests operate and it was not clear to me what the correct
> behavior should be.

There are two tests that focus specifically on PRDT underflow/overflow
(test_bmdma_short_prdt and test_bmdma_long_prdt).

The test you are looking at is more complicated and that is why the
expected behavior is unclear:
1. Bus master bit is not set so DMA shouldn't be possible
2. PRDT_EOT missing, invalid PRDT
3. PRDT underflow since 512 sectors * 512B < 64 KB * 4096

(Off-topic, I'm not sure why the test case's PRDT is 4096 elements when
the spec implies it can be 8192 elements for the full 64 KB.)

Overflow/underflow behavior is described in "Programming Interface for
Bus Master IDE Controller" Revision 1.0, 3.1 Status Bit Interpretation:

"Interrupt 1, Active 0 - The IDE device generated an interrupt. The
controller exhausted the Physical Region Descriptors. This is the normal
completion case where the size of the physical memory regions was equal
to the IDE device transfer size.

Interrupt 1, Active 1 - The IDE device generated an interrupt. The
controller has not reached the end of the physical memory regions. This
is a valid completion case where the size of the physical memory regions
was larger than the IDE device transfer size.

Interrupt 0, Active 0 - This bit combination signals an error condition.
If the Error bit in the status register is set, then the controller has
some problem transferring data to/from memory.  Specifics of the error
have to be determined using bus-specific information. If the Error bit
is not set, then the PRD's specified a smaller size than the IDE
transfer size."

http://pdos.csail.mit.edu/6.828/2010/readings/hardware/IDE-BusMaster.pdf

> In my mind, the "common sense" behavior is to just truncate the list,
> do the transfer, and pretend like everything's fine. However, the long
> PRDT test in ide-test seems to rely on the fact that the command will
> still be running by the time we get to cancel it, which won't be true
> if I add any explicit checking.
> 
> If I just "consume" the extra PRDs but don't update the sglist, the
> command will actually probably finish before we get to cancel it,
> which changes the test.
> 
> If I throw an error of any kind, that changes the test too.
> 
> I really wasn't sure what the right thing to do for BMDMA was, so I
> left it alone.

Please follow the spec.

test_bmdma_short_prdt and test_bmdma_long_prdt should be unchanged.  The
bus master test is iffy, so I guess it could change if really necessary.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 06/16] ahci: record ncq failures

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On Fri, Jun 26, 2015 at 02:27:39PM -0400, John Snow wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
> >> Handle NCQ failures for cases where we want to halt the VM on IO
> >> errors.
> >> 
> >> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c
> >> | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 + hw/ide/internal.h
> >> |  1 + 3 files changed, 17 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 71b5085..a838317
> >> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -959,13 +959,25
> >> @@ static void ncq_cb(void *opaque, int ret) return; }
> >> 
> >> +    ncq_tfs->halt = false;
> > 
> > Why does halt need to be cleared here?
> > 
> 
> Might make more sense to clear it just on the beginning of every
> command, in execute().
> 
> There's no strong reason here other than "If there's an error and it
> should be set, it'll get reset again pretty soon." It's just a default
> state.
> 
> I could move it from process to execute.

Clearing it in one place only is cleanest.  I was wondering if I missed
a code path where the two clears are really needed.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 06/16] ahci: record ncq failures

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Fri, Jun 26, 2015 at 02:27:39PM -0400, John Snow wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
> >> Handle NCQ failures for cases where we want to halt the VM on IO
> >> errors.
> >> 
> >> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c
> >> | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 + hw/ide/internal.h
> >> |  1 + 3 files changed, 17 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 71b5085..a838317
> >> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -959,13 +959,25
> >> @@ static void ncq_cb(void *opaque, int ret) return; }
> >> 
> >> +    ncq_tfs->halt = false;
> > 
> > Why does halt need to be cleared here?
> > 
> 
> Might make more sense to clear it just on the beginning of every
> command, in execute().
> 
> There's no strong reason here other than "If there's an error and it
> should be set, it'll get reset again pretty soon." It's just a default
> state.
> 
> I could move it from process to execute.

By the way, does ->halt need to be cleared in ahci_reset_port()?

I'm thinking of a scenario where requests have failed and the port is
reset.  We should not try to reissue those commands after reset.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 12/16] ahci: ncq migration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Fri, Jun 26, 2015 at 12:46:07PM -0400, John Snow wrote:
> On 06/26/2015 11:48 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote:
> >> @@ -1555,6 +1573,35 @@ 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->slot > AHCI_MAX_CMDS) { +
> >> return -1; +            } +            ncq_tfs->cmdh =
> >> &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];
> > 
> > Is there a guarantee that ->lst has been mapped?  Maybe pr->cmd & 
> > PORT_CMD_START was 0.
> > 
> > We need to check that the HBA is in a valid state for NCQ
> > processing before attempting this.
> > 
> 
> Slipped my mind, there should be no way to have ncq_tfs->halt set
> under normal circumstances except if the engine is started (and lst is
> mapped.)
> 
> That said, stream corruption is always possible, so I'll add another
> validation check.
> 
> An "is not null" check here should be sufficient, due to the "if not
> halt" continue up above.

Sounds good.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote:
> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
> >> @@ -744,8 +722,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;
> > 
> > hw/ide/core.c decreases s->nsector until it reaches 0 and the
> > request ends.
> > 
> > Will the values reported back to the guest be correct if we use 
> > s->nsector?
> > 
> 
> See the commit message for justification of this one. Ultimately, it
> doesn't matter what gets put in here (for data transfer commands) --
> but getting RID of the cmd_fis mapping is a strong positive.

Getting rid of cmd_fis mapping is good.

Putting s->nsector into the undefined fields makes the code confusing.

It is clearer to zero the bytes with a comment saying the value does not
matter according to the spec.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 16/16] ahci: fix sdb fis semantics

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

On Fri, Jun 26, 2015 at 01:36:23PM -0400, John Snow wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 06/26/2015 12:11 PM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:15PM -0400, John Snow wrote:
> >> @@ -682,19 +680,22 @@ 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 */
> > ...
> >> -    ahci_trigger_irq(s, ad, PORT_IRQ_SDB_FIS); +    if
> >> (sdb_fis->flags & 0x40) { +        ahci_trigger_irq(s, ad,
> >> PORT_IRQ_SDB_FIS); +    }
> > 
> > This if statement is always true.
> > 
> 
> Yes. I did that on purpose. Maybe that was too weird.
> 
> The idea was to emphasize that the IRQ is definitely only triggered
> *IF* the interrupt bit is set. It just so happens that we currently
> always set it.
> 
> The generation and trigger mechanics are supposed to be separate, but
> we currently have no use case for them being separate (because we are
> an omniscient HBA-HDD amalgamation), so they're mushed together here.
> 
> This is kind of like a documentation "NB."
> 
> I can replace it with:
> 
> /* Trigger IRQ if interrupt bit is set, which currently it always is: */
> ahci_trigger_irq(...);

Good idea, that makes it clear.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[John Snow]

On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote:
>> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
>>>> @@ -744,8 +722,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;
>>>
>>> hw/ide/core.c decreases s->nsector until it reaches 0 and the
>>> request ends.
>>>
>>> Will the values reported back to the guest be correct if we use 
>>> s->nsector?
>>>
>>
>> See the commit message for justification of this one. Ultimately, it
>> doesn't matter what gets put in here (for data transfer commands) --
>> but getting RID of the cmd_fis mapping is a strong positive.
> 
> Getting rid of cmd_fis mapping is good.
> 
> Putting s->nsector into the undefined fields makes the code confusing.
> 
> It is clearer to zero the bytes with a comment saying the value does not
> matter according to the spec.
> 

Well, it's not that it doesn't matter /ever/, it's more that for
standard IO routines it doesn't matter. (See the normative output spec
in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it
carries a diagnostic signature.)

What's really the case is that the FIS always dutifully copies out what
the SATA registers are (or should be.)

There are still a handful of commands that, if we choose to support
them, copying the nsector register would be the "correct thing" to do,
so I decided to copy that field here to serve as documentation and
support future command additions.

I would argue that if this field ever does the /wrong thing/, it would
be a fix in the S/ATA layer, and not a change to the FIS generator here.

I am inclined to leave it as-is, since for the current cases, nsector is
going to empty to zero anyway. I believe the behavior presented here is
correct.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 06/16] ahci: record ncq failures

[John Snow]

On 06/29/2015 10:24 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 26, 2015 at 02:27:39PM -0400, John Snow wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
>> 
>> 
>> 
>> On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
>>>> Handle NCQ failures for cases where we want to halt the VM on
>>>> IO errors.
>>>> 
>>>> Signed-off-by: John Snow <jsnow@redhat.com> ---
>>>> hw/ide/ahci.c | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 +
>>>> hw/ide/internal.h |  1 + 3 files changed, 17 insertions(+), 2
>>>> deletions(-)
>>>> 
>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index
>>>> 71b5085..a838317 100644 --- a/hw/ide/ahci.c +++
>>>> b/hw/ide/ahci.c @@ -959,13 +959,25 @@ static void ncq_cb(void
>>>> *opaque, int ret) return; }
>>>> 
>>>> +    ncq_tfs->halt = false;
>>> 
>>> Why does halt need to be cleared here?
>>> 

I remember my thinking now. I didn't want to leave it dangling after a
successful command, so I wanted to clear it on the callback.

It's harmless, but it's weird to have it set afterwards and I wanted
it to be very crystal clear what it meant when it was set. (With this
usage case, 'halt' being set ALWAYS means that there's a command to
retry -- you don't have to test other variables like 'used' to tell if
it's a left over flag.)

I actually want to leave it here, now.

>> 
>> Might make more sense to clear it just on the beginning of every 
>> command, in execute().
>> 
>> There's no strong reason here other than "If there's an error and
>> it should be set, it'll get reset again pretty soon." It's just a
>> default state.
>> 
>> I could move it from process to execute.
> 
> By the way, does ->halt need to be cleared in ahci_reset_port()?
> 
> I'm thinking of a scenario where requests have failed and the port
> is reset.  We should not try to reissue those commands after
> reset.
> 

If I keep it like I have it now (where it clears itself after a
successful command), we can clear it alongside the 'used' flag to
protect against the case where the guest issues a reset almost
simultaneously with an errored read command, where it might be that
the NCQ command halts the VM, then we try to reset immediately after boot.


(Perilously tangential side-note: what's the default error action if
you don't set rerror=stop or werror=stop? the ide code didn't make it
particularly clear to me if it was IGNORE or REPORT.)

Re: [Qemu-devel] [Qemu-block] [PATCH 06/16] ahci: record ncq failures

[John Snow]

On 06/29/2015 11:42 AM, John Snow wrote:
> 
> 
> On 06/29/2015 10:24 AM, Stefan Hajnoczi wrote:
>> On Fri, Jun 26, 2015 at 02:27:39PM -0400, John Snow wrote:
>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
>>>
>>>
>>>
>>> On 06/26/2015 11:35 AM, Stefan Hajnoczi wrote:
>>>> On Mon, Jun 22, 2015 at 08:21:05PM -0400, John Snow wrote:
>>>>> Handle NCQ failures for cases where we want to halt the VM on
>>>>> IO errors.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com> ---
>>>>> hw/ide/ahci.c | 17 +++++++++++++++-- hw/ide/ahci.h     |  1 +
>>>>> hw/ide/internal.h |  1 + 3 files changed, 17 insertions(+), 2
>>>>> deletions(-)
>>>>>
>>>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index
>>>>> 71b5085..a838317 100644 --- a/hw/ide/ahci.c +++
>>>>> b/hw/ide/ahci.c @@ -959,13 +959,25 @@ static void ncq_cb(void
>>>>> *opaque, int ret) return; }
>>>>>
>>>>> +    ncq_tfs->halt = false;
>>>>
>>>> Why does halt need to be cleared here?
>>>>
> 
> I remember my thinking now. I didn't want to leave it dangling after a
> successful command, so I wanted to clear it on the callback.
> 
> It's harmless, but it's weird to have it set afterwards and I wanted
> it to be very crystal clear what it meant when it was set. (With this
> usage case, 'halt' being set ALWAYS means that there's a command to
> retry -- you don't have to test other variables like 'used' to tell if
> it's a left over flag.)
> 
> I actually want to leave it here, now.
> 

I am literally not awake: if I clear it in execute_ncq_command like I
offered, it will get cleared on retry and we won't leave it hanging.

It takes hitting the 'send' button to realize this.

>>>
>>> Might make more sense to clear it just on the beginning of every 
>>> command, in execute().
>>>
>>> There's no strong reason here other than "If there's an error and
>>> it should be set, it'll get reset again pretty soon." It's just a
>>> default state.
>>>
>>> I could move it from process to execute.
>>
>> By the way, does ->halt need to be cleared in ahci_reset_port()?
>>
>> I'm thinking of a scenario where requests have failed and the port
>> is reset.  We should not try to reissue those commands after
>> reset.
>>
> 
> If I keep it like I have it now (where it clears itself after a
> successful command), we can clear it alongside the 'used' flag to
> protect against the case where the guest issues a reset almost
> simultaneously with an errored read command, where it might be that
> the NCQ command halts the VM, then we try to reset immediately after boot.
> 
> 
> (Perilously tangential side-note: what's the default error action if
> you don't set rerror=stop or werror=stop? the ide code didn't make it
> particularly clear to me if it was IGNORE or REPORT.)
> 

Still curious about this bit, though.

Re: [Qemu-devel] [Qemu-block] [PATCH 01/16] ide: add limit to .prepare_buf()

[John Snow]

On 06/29/2015 09:34 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 26, 2015 at 02:16:57PM -0400, John Snow wrote:
>> On 06/26/2015 10:32 AM, Stefan Hajnoczi wrote:
>>> On Mon, Jun 22, 2015 at 08:21:00PM -0400, John Snow wrote:
>>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 4afd0cf..a295baa
>>>> 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -55,8 +55,11 @@
>>>> static void bmdma_start_dma(IDEDMA *dma, IDEState *s, /** *
>>>> Return the number of bytes successfully prepared. * -1 on error. 
>>>> + * BUG?: Does not currently heed the 'limit' parameter because +
>>>> *       it is not clear what the correct behavior here is, + *
>>>> see tests/ide-test.c
>>>
>>> QEMU implements both short and long PRDT cases for IDE in
>>> ide_dma_cb() and the tests check them.  I saw nothing indicating
>>> that existing behavior might not correspond to real hardware
>>> behavior.  Why do you say the correct behavior is unclear?
>>>
>>
>> Look at ide-test:
>>
>> "No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't
>> be able to access it anyway because the Bus Master bit in the PCI
>> command register isn't set. This is complete nonsense, but it used to
>> be pretty good at confusing and occasionally crashing qemu."
>>
>> "Not entirely clear what the expected result is, but this is what we
>> get in practice. At least we want to be aware of any changes."
>>
>> Changing the PRDT underflow behavior here (Where the data to be
>> transferred is less than the size of the PRDT/SGlist) changes how
>> these tests operate and it was not clear to me what the correct
>> behavior should be.
> 
> There are two tests that focus specifically on PRDT underflow/overflow
> (test_bmdma_short_prdt and test_bmdma_long_prdt).
> 
> The test you are looking at is more complicated and that is why the
> expected behavior is unclear:
> 1. Bus master bit is not set so DMA shouldn't be possible
> 2. PRDT_EOT missing, invalid PRDT
> 3. PRDT underflow since 512 sectors * 512B < 64 KB * 4096
> 
> (Off-topic, I'm not sure why the test case's PRDT is 4096 elements when
> the spec implies it can be 8192 elements for the full 64 KB.)
> 
> Overflow/underflow behavior is described in "Programming Interface for
> Bus Master IDE Controller" Revision 1.0, 3.1 Status Bit Interpretation:
> 
> "Interrupt 1, Active 0 - The IDE device generated an interrupt. The
> controller exhausted the Physical Region Descriptors. This is the normal
> completion case where the size of the physical memory regions was equal
> to the IDE device transfer size.
> 
> Interrupt 1, Active 1 - The IDE device generated an interrupt. The
> controller has not reached the end of the physical memory regions. This
> is a valid completion case where the size of the physical memory regions
> was larger than the IDE device transfer size.
> 
> Interrupt 0, Active 0 - This bit combination signals an error condition.
> If the Error bit in the status register is set, then the controller has
> some problem transferring data to/from memory.  Specifics of the error
> have to be determined using bus-specific information. If the Error bit
> is not set, then the PRD's specified a smaller size than the IDE
> transfer size."
> 
> http://pdos.csail.mit.edu/6.828/2010/readings/hardware/IDE-BusMaster.pdf
> 

Thanks for the PDF!

>> In my mind, the "common sense" behavior is to just truncate the list,
>> do the transfer, and pretend like everything's fine. However, the long
>> PRDT test in ide-test seems to rely on the fact that the command will
>> still be running by the time we get to cancel it, which won't be true
>> if I add any explicit checking.
>>
>> If I just "consume" the extra PRDs but don't update the sglist, the
>> command will actually probably finish before we get to cancel it,
>> which changes the test.
>>
>> If I throw an error of any kind, that changes the test too.
>>
>> I really wasn't sure what the right thing to do for BMDMA was, so I
>> left it alone.
> 
> Please follow the spec.
> 

Yeah, I'm not going to make up behavior, but it really wasn't clear to
me at first so I didn't touch it. I also misunderstood the flow of the
test, so just ignore most of that paragraph. (The test does NOT rely on
timings like I thought it did.)

> test_bmdma_short_prdt and test_bmdma_long_prdt should be unchanged.  The
> bus master test is iffy, so I guess it could change if really necessary.
> 

I'm sending an RFC for just this piece. If it looks good I'll squish it
into this patch.

Thanks!

(I was mostly terrified of trying to fix certain things for AHCI which
highlights how we haven't paid attention to certain things in the core
layer, so I was hesitant to fix certain items for fear of opening up
Pandora's Box, but this particular item winds up not being too bad.)

Re: [Qemu-devel] [Qemu-block] [PATCH 06/16] ahci: record ncq failures

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Mon, Jun 29, 2015 at 11:47:32AM -0400, John Snow wrote:
> > (Perilously tangential side-note: what's the default error action if
> > you don't set rerror=stop or werror=stop? the ide code didn't make it
> > particularly clear to me if it was IGNORE or REPORT.)
> > 
> 
> Still curious about this bit, though.

blockdev.c:blockdev_init() is the function that instantiates a -drive:

  on_write_error = BLOCKDEV_ON_ERROR_ENOSPC;
      if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
  ...
  on_read_error = BLOCKDEV_ON_ERROR_REPORT;
      if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

On Mon, Jun 29, 2015 at 11:07:18AM -0400, John Snow wrote:
> On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote:
> > On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote:
> >> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
> >>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
> >>>> @@ -744,8 +722,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;
> >>>
> >>> hw/ide/core.c decreases s->nsector until it reaches 0 and the
> >>> request ends.
> >>>
> >>> Will the values reported back to the guest be correct if we use 
> >>> s->nsector?
> >>>
> >>
> >> See the commit message for justification of this one. Ultimately, it
> >> doesn't matter what gets put in here (for data transfer commands) --
> >> but getting RID of the cmd_fis mapping is a strong positive.
> > 
> > Getting rid of cmd_fis mapping is good.
> > 
> > Putting s->nsector into the undefined fields makes the code confusing.
> > 
> > It is clearer to zero the bytes with a comment saying the value does not
> > matter according to the spec.
> > 
> 
> Well, it's not that it doesn't matter /ever/, it's more that for
> standard IO routines it doesn't matter. (See the normative output spec
> in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it
> carries a diagnostic signature.)
> 
> What's really the case is that the FIS always dutifully copies out what
> the SATA registers are (or should be.)
> 
> There are still a handful of commands that, if we choose to support
> them, copying the nsector register would be the "correct thing" to do,
> so I decided to copy that field here to serve as documentation and
> support future command additions.
> 
> I would argue that if this field ever does the /wrong thing/, it would
> be a fix in the S/ATA layer, and not a change to the FIS generator here.
> 
> I am inclined to leave it as-is, since for the current cases, nsector is
> going to empty to zero anyway. I believe the behavior presented here is
> correct.

I'm trying to understand the guest-visible change in behavior.  Guests
might take different code paths from before.

For example, I think that after this patch the CHECK POWER MODE command
works correctly for the first time with AHCI.  It sets ->nsector to
0xff.

Anyway, I'm happy with assigning s->nsector.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]