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

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

[John Snow]

requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
          [PATCH v2 0/4] ahci: misc fixes/tests for 2.4

This series adds a couple of tests to exercise the NCQ pathways
and establish a baseline for us.

Most of these patches are fairly short and should be relatively trivial
to review. this series will lay the groundwork for part 2,
which adds the rerror/werror=stop and migration support for that feature
as well.

________________________________________________________________________________

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

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

John Snow (16):
  ahci: Rename NCQFIS structure fields
  ahci: use shorter variables
  ahci: add ncq_err helper
  ahci: check for ncq prdtl overflow
  ahci: separate prdtl from opts
  ahci: add ncq debug checks
  ahci: ncq sector count correction
  ahci: clear error register before NCQ cmd
  libqos/ahci: fix cmd_sanity for ncq
  libqos/ahci: add NCQ frame support
  libqos/ahci: edit wait to be ncq aware
  libqos/ahci: adjust expected NCQ interrupts
  libqos/ahci: set the NCQ tag on command_commit
  libqos/ahci: Force all NCQ commands to be LBA48
  qtest/ahci: simple ncq data test
  qtest/ahci: ncq migration test

 hw/ide/ahci.c       | 104 +++++++++++++++++++++++----------
 hw/ide/ahci.h       |  38 ++++++++----
 tests/ahci-test.c   |  28 ++++++++-
 tests/libqos/ahci.c | 162 +++++++++++++++++++++++++++++++++-------------------
 tests/libqos/ahci.h |  59 ++++++++++++++-----
 5 files changed, 276 insertions(+), 115 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 01/16] ahci: Rename NCQFIS structure fields

[John Snow]

Several fields of the NCQFIS structure are ambiguously named. This patch
clarifies the intended (if unsupported) usage of the NCQ fields to aid
in creating more meaningful debug messages through the NCQ codepaths.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.h | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 501c002..6d167f2 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -195,6 +195,9 @@
 #define RECEIVE_FPDMA_QUEUED               0x65
 #define SEND_FPDMA_QUEUED                  0x64
 
+#define NCQ_FIS_FUA_MASK                   0x80
+#define NCQ_FIS_RARC_MASK                  0x01
+
 #define RES_FIS_DSFIS                      0x00
 #define RES_FIS_PSFIS                      0x20
 #define RES_FIS_RFIS                       0x40
@@ -312,27 +315,39 @@ extern const VMStateDescription vmstate_ahci;
     .offset     = vmstate_offset_value(_state, _field, AHCIState),   \
 }
 
+/**
+ * NCQFrame is the same as a Register H2D FIS (described in SATA 3.2),
+ * but some fields have been re-mapped and re-purposed, as seen in
+ * SATA 3.2 section 13.6.4.1 ("READ FPDMA QUEUED")
+ *
+ * cmd_fis[3], feature 7:0, becomes sector count 7:0.
+ * cmd_fis[7], device 7:0, uses bit 7 as the Force Unit Access bit.
+ * cmd_fis[11], feature 15:8, becomes sector count 15:8.
+ * cmd_fis[12], count 7:0, becomes the NCQ TAG (7:3) and RARC bit (0)
+ * cmd_fis[13], count 15:8, becomes the priority value (7:6)
+ * bytes 16-19 become an le32 "auxiliary" field.
+ */
 typedef struct NCQFrame {
     uint8_t fis_type;
     uint8_t c;
     uint8_t command;
-    uint8_t sector_count_low;
+    uint8_t sector_count_low;  /* (feature 7:0) */
     uint8_t lba0;
     uint8_t lba1;
     uint8_t lba2;
-    uint8_t fua;
+    uint8_t fua;               /* (device 7:0) */
     uint8_t lba3;
     uint8_t lba4;
     uint8_t lba5;
-    uint8_t sector_count_high;
-    uint8_t tag;
-    uint8_t reserved5;
-    uint8_t reserved6;
+    uint8_t sector_count_high; /* (feature 15:8) */
+    uint8_t tag;               /* (count 0:7) */
+    uint8_t prio;              /* (count 15:8) */
+    uint8_t icc;
     uint8_t control;
-    uint8_t reserved7;
-    uint8_t reserved8;
-    uint8_t reserved9;
-    uint8_t reserved10;
+    uint8_t aux0;
+    uint8_t aux1;
+    uint8_t aux2;
+    uint8_t aux3;
 } QEMU_PACKED NCQFrame;
 
 typedef struct SDBFIS {
-- 
2.1.0

[Qemu-devel] [PATCH 02/16] ahci: use shorter variables

[John Snow]

Trivial cleanup that I didn't want to tack-on to anything else.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 26df2ca..14eccb8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -972,9 +972,11 @@ static int is_ncq(uint8_t ata_cmd)
 static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
                                 int slot)
 {
+    AHCIDevice *ad = &s->dev[port];
+    IDEState *ide_state = &ad->port.ifs[0];
     NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
     uint8_t tag = ncq_fis->tag >> 3;
-    NCQTransferState *ncq_tfs = &s->dev[port].ncq_tfs[tag];
+    NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag];
 
     if (ncq_tfs->used) {
         /* error - already in use */
@@ -983,7 +985,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     }
 
     ncq_tfs->used = 1;
-    ncq_tfs->drive = &s->dev[port];
+    ncq_tfs->drive = ad;
     ncq_tfs->slot = slot;
     ncq_tfs->lba = ((uint64_t)ncq_fis->lba5 << 40) |
                    ((uint64_t)ncq_fis->lba4 << 32) |
@@ -1000,9 +1002,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", "
             "drive max %"PRId64"\n",
             ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
-            s->dev[port].port.ifs[0].nb_sectors - 1);
+            ide_state->nb_sectors - 1);
 
-    ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist, 0);
+    ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
     ncq_tfs->tag = tag;
 
     switch(ncq_fis->command) {
@@ -1014,9 +1016,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             DPRINTF(port, "tag %d aio read %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
 
-            dma_acct_start(ncq_tfs->drive->port.ifs[0].blk, &ncq_tfs->acct,
+            dma_acct_start(ide_state->blk, &ncq_tfs->acct,
                            &ncq_tfs->sglist, BLOCK_ACCT_READ);
-            ncq_tfs->aiocb = dma_blk_read(ncq_tfs->drive->port.ifs[0].blk,
+            ncq_tfs->aiocb = dma_blk_read(ide_state->blk,
                                           &ncq_tfs->sglist, ncq_tfs->lba,
                                           ncq_cb, ncq_tfs);
             break;
@@ -1027,9 +1029,9 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             DPRINTF(port, "tag %d aio write %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
 
-            dma_acct_start(ncq_tfs->drive->port.ifs[0].blk, &ncq_tfs->acct,
+            dma_acct_start(ide_state->blk, &ncq_tfs->acct,
                            &ncq_tfs->sglist, BLOCK_ACCT_WRITE);
-            ncq_tfs->aiocb = dma_blk_write(ncq_tfs->drive->port.ifs[0].blk,
+            ncq_tfs->aiocb = dma_blk_write(ide_state->blk,
                                            &ncq_tfs->sglist, ncq_tfs->lba,
                                            ncq_cb, ncq_tfs);
             break;
-- 
2.1.0

[Qemu-devel] [PATCH 03/16] ahci: add ncq_err helper

[John Snow]

Set some appropriate error bits for NCQ for us.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 14eccb8..375aa44 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -922,6 +922,15 @@ out:
     return r;
 }
 
+static void ncq_err(NCQTransferState *ncq_tfs)
+{
+    IDEState *ide_state = &ncq_tfs->drive->port.ifs[0];
+
+    ide_state->error = ABRT_ERR;
+    ide_state->status = READY_STAT | ERR_STAT;
+    ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
+}
+
 static void ncq_cb(void *opaque, int ret)
 {
     NCQTransferState *ncq_tfs = (NCQTransferState *)opaque;
@@ -934,10 +943,7 @@ static void ncq_cb(void *opaque, int ret)
     ncq_tfs->drive->port_regs.scr_act &= ~(1 << ncq_tfs->tag);
 
     if (ret < 0) {
-        /* error */
-        ide_state->error = ABRT_ERR;
-        ide_state->status = READY_STAT | ERR_STAT;
-        ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
+        ncq_err(ncq_tfs);
     } else {
         ide_state->status = READY_STAT | SEEK_STAT;
     }
-- 
2.1.0

[Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[John Snow]

Don't attempt the NCQ transfer if the PRDT we were given is not big
enough to perform the entire transfer.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 375aa44..24c4832 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -983,6 +983,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
     NCQFrame *ncq_fis = (NCQFrame*)cmd_fis;
     uint8_t tag = ncq_fis->tag >> 3;
     NCQTransferState *ncq_tfs = &ad->ncq_tfs[tag];
+    size_t size;
 
     if (ncq_tfs->used) {
         /* error - already in use */
@@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
                    ((uint64_t)ncq_fis->lba2 << 16) |
                    ((uint64_t)ncq_fis->lba1 << 8) |
                    (uint64_t)ncq_fis->lba0;
+    ncq_tfs->tag = tag;
 
-    /* Note: We calculate the sector count, but don't currently rely on it.
-     * The total size of the DMA buffer tells us the transfer size instead. */
     ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
                                 ncq_fis->sector_count_low;
+    ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
+    size = ncq_tfs->sector_count * 512;
+
+    if (ncq_tfs->sglist.size < size) {
+        error_report("ahci: PRDT length for NCQ command (0x%zx) "
+                     "is smaller than the requested size (0x%zx)",
+                     ncq_tfs->sglist.size, size);
+        qemu_sglist_destroy(&ncq_tfs->sglist);
+        ncq_err(ncq_tfs);
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
+        return;
+    }
 
     DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", "
             "drive max %"PRId64"\n",
             ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
             ide_state->nb_sectors - 1);
 
-    ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
-    ncq_tfs->tag = tag;
-
     switch(ncq_fis->command) {
         case READ_FPDMA_QUEUED:
             DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
@@ -1051,6 +1060,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
                         "error: tried to process non-NCQ command as NCQ\n");
             }
             qemu_sglist_destroy(&ncq_tfs->sglist);
+            ncq_err(ncq_tfs);
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH 05/16] ahci: separate prdtl from opts

[John Snow]

There's no real reason to have it bundled together, and this way
is a little nicer to follow if you have the AHCI spec pulled up.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 24c4832..14961a0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -834,10 +834,11 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
                                 int32_t offset)
 {
     AHCICmdHdr *cmd = ad->cur_cmd;
-    uint32_t opts = le32_to_cpu(cmd->opts);
-    uint64_t prdt_addr = le64_to_cpu(cmd->tbl_addr) + 0x80;
-    int sglist_alloc_hint = opts >> AHCI_CMD_HDR_PRDT_LEN;
-    dma_addr_t prdt_len = (sglist_alloc_hint * sizeof(AHCI_SG));
+    uint16_t opts = le16_to_cpu(cmd->opts);
+    uint16_t prdtl = le16_to_cpu(cmd->prdtl);
+    uint64_t cfis_addr = le64_to_cpu(cmd->tbl_addr);
+    uint64_t prdt_addr = cfis_addr + 0x80;
+    dma_addr_t prdt_len = (prdtl * sizeof(AHCI_SG));
     dma_addr_t real_prdt_len = prdt_len;
     uint8_t *prdt;
     int i;
@@ -857,7 +858,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
      * request for sector sizes up to 32K.
      */
 
-    if (!sglist_alloc_hint) {
+    if (!prdtl) {
         DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);
         return -1;
     }
@@ -876,10 +877,10 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
     }
 
     /* Get entries in the PRDT, init a qemu sglist accordingly */
-    if (sglist_alloc_hint > 0) {
+    if (prdtl > 0) {
         AHCI_SG *tbl = (AHCI_SG *)prdt;
         sum = 0;
-        for (i = 0; i < sglist_alloc_hint; i++) {
+        for (i = 0; i < prdtl; i++) {
             /* flags_size is zero-based */
             tbl_entry_size = prdt_tbl_entry_size(&tbl[i]);
             if (offset <= (sum + tbl_entry_size)) {
@@ -897,12 +898,12 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
             goto out;
         }
 
-        qemu_sglist_init(sglist, qbus->parent, (sglist_alloc_hint - off_idx),
+        qemu_sglist_init(sglist, qbus->parent, (prdtl - off_idx),
                          ad->hba->as);
         qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos,
                         prdt_tbl_entry_size(&tbl[off_idx]) - off_pos);
 
-        for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
+        for (i = off_idx + 1; i < prdtl; i++) {
             /* flags_size is zero-based */
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
                             prdt_tbl_entry_size(&tbl[i]));
@@ -1069,7 +1070,7 @@ static void handle_reg_h2d_fis(AHCIState *s, int port,
 {
     IDEState *ide_state = &s->dev[port].port.ifs[0];
     AHCICmdHdr *cmd = s->dev[port].cur_cmd;
-    uint32_t opts = le32_to_cpu(cmd->opts);
+    uint16_t opts = le16_to_cpu(cmd->opts);
 
     if (cmd_fis[1] & 0x0F) {
         DPRINTF(port, "Port Multiplier not supported."
@@ -1226,7 +1227,7 @@ static void ahci_start_transfer(IDEDMA *dma)
     IDEState *s = &ad->port.ifs[0];
     uint32_t size = (uint32_t)(s->data_end - s->data_ptr);
     /* write == ram -> device */
-    uint32_t opts = le32_to_cpu(ad->cur_cmd->opts);
+    uint16_t opts = le16_to_cpu(ad->cur_cmd->opts);
     int is_write = opts & AHCI_CMD_WRITE;
     int is_atapi = opts & AHCI_CMD_ATAPI;
     int has_sglist = 0;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 6d167f2..b8872a4 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -236,7 +236,8 @@ typedef struct AHCIPortRegs {
 } AHCIPortRegs;
 
 typedef struct AHCICmdHdr {
-    uint32_t    opts;
+    uint16_t    opts;
+    uint16_t    prdtl;
     uint32_t    status;
     uint64_t    tbl_addr;
     uint32_t    reserved[4];
-- 
2.1.0

[Qemu-devel] [PATCH 06/16] ahci: add ncq debug checks

[John Snow]

Most of the time, these bits can be safely ignored. For the purposes
of debugging however, it's nice to know that they're not being used.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 14961a0..8fcea18 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1003,6 +1003,27 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
                    (uint64_t)ncq_fis->lba0;
     ncq_tfs->tag = tag;
 
+#ifdef DEBUG_AHCI
+    /* Sanity-check the NCQ packet */
+    if (tag != slot) {
+        DPRINTF(port, "Warn: NCQ slot (%d) did not match the given tag (%d)\n",
+                slot, tag);
+    }
+
+    if (ncq_fis->aux0 || ncq_fis->aux1 || ncq_fis->aux2 || ncq_fis->aux3) {
+        DPRINTF(port, "Warn: Attempt to use NCQ auxiliary fields.\n");
+    }
+    if (ncq_fis->prio || ncq_fis->icc) {
+        DPRINTF(port, "Warn: Unsupported attempt to use PRIO/ICC fields\n");
+    }
+    if (ncq_fis->fua & NCQ_FIS_FUA_MASK) {
+        DPRINTF(port, "Warn: Unsupported attempt to use Force Unit Access\n");
+    }
+    if (ncq_fis->tag & NCQ_FIS_RARC_MASK) {
+        DPRINTF(port, "Warn: Unsupported attempt to use Rebuild Assist\n");
+    }
+#endif
+
     ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
                                 ncq_fis->sector_count_low;
     ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
@@ -1016,6 +1037,10 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
         ncq_err(ncq_tfs);
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW);
         return;
+    } else if (ncq_tfs->sglist.size != size) {
+        DPRINTF(port, "Warn: PRDTL (0x%zx)"
+                " does not match requested size (0x%zx)",
+                ncq_tfs->sglist.size, size);
     }
 
     DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", "
-- 
2.1.0

[Qemu-devel] [PATCH 07/16] ahci: ncq sector count correction

[John Snow]

This value should not be size-corrected, 0 sectors does not imply
1 sector(s). This is just debug information, but it's misleading!

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8fcea18..6bded67 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1045,14 +1045,14 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
 
     DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", "
             "drive max %"PRId64"\n",
-            ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
+            ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1,
             ide_state->nb_sectors - 1);
 
     switch(ncq_fis->command) {
         case READ_FPDMA_QUEUED:
             DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
                     "tag %d\n",
-                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
+                    ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
 
             DPRINTF(port, "tag %d aio read %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
@@ -1065,7 +1065,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             break;
         case WRITE_FPDMA_QUEUED:
             DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
-                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
+                    ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
 
             DPRINTF(port, "tag %d aio write %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
-- 
2.1.0

[Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[John Snow]

The legacy ide command execution layer will clear any errors outstanding
before execution, but the NCQ layer doesn't.
Even on success, this register will remain clogged.

Clear it out before each NCQ command so the guest can tell if the
error code produced after completion is meaningful or not.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6bded67..e63ba9b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1048,6 +1048,8 @@ 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);
 
+    ide_state->error = 0;
+
     switch(ncq_fis->command) {
         case READ_FPDMA_QUEUED:
             DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
-- 
2.1.0

[Qemu-devel] [PATCH 09/16] libqos/ahci: fix cmd_sanity for ncq

[John Snow]

NCQ commands should not / do not update the byte count
in the command header post command, so this field is
meaningless for NCQ tests.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 46 ++++++++++++++++++++++++----------------------
 tests/libqos/ahci.h |  3 +--
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 08e1c98..922af8b 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -73,6 +73,22 @@ AHCICommandProp ahci_command_properties[] = {
     { .cmd = CMD_FLUSH_CACHE,   .data = false }
 };
 
+struct AHCICommand {
+    /* Test Management Data */
+    uint8_t name;
+    uint8_t port;
+    uint8_t slot;
+    uint32_t interrupts;
+    uint64_t xbytes;
+    uint32_t prd_size;
+    uint64_t buffer;
+    AHCICommandProp *props;
+    /* Data to be transferred to the guest */
+    AHCICommandHeader header;
+    RegH2DFIS fis;
+    void *atapi_cmd;
+};
+
 /**
  * Allocate space in the guest using information in the AHCIQState object.
  */
@@ -462,13 +478,15 @@ void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
     g_free(pio);
 }
 
-void ahci_port_check_cmd_sanity(AHCIQState *ahci, uint8_t port,
-                                uint8_t slot, size_t buffsize)
+void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd)
 {
-    AHCICommandHeader cmd;
+    AHCICommandHeader cmdh;
 
-    ahci_get_command_header(ahci, port, slot, &cmd);
-    g_assert_cmphex(buffsize, ==, cmd.prdbc);
+    ahci_get_command_header(ahci, cmd->port, cmd->slot, &cmdh);
+    /* Physical Region Descriptor Byte Count is not required to work for NCQ. */
+    if (!cmd->props->ncq) {
+        g_assert_cmphex(cmd->xbytes, ==, cmdh.prdbc);
+    }
 }
 
 /* Get the command in #slot of port #port. */
@@ -612,22 +630,6 @@ void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
     ahci_command_free(cmd);
 }
 
-struct AHCICommand {
-    /* Test Management Data */
-    uint8_t name;
-    uint8_t port;
-    uint8_t slot;
-    uint32_t interrupts;
-    uint64_t xbytes;
-    uint32_t prd_size;
-    uint64_t buffer;
-    AHCICommandProp *props;
-    /* Data to be transferred to the guest */
-    AHCICommandHeader header;
-    RegH2DFIS fis;
-    void *atapi_cmd;
-};
-
 static AHCICommandProp *ahci_command_find(uint8_t command_name)
 {
     int i;
@@ -901,7 +903,7 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd)
     ahci_port_check_error(ahci, port);
     ahci_port_check_interrupts(ahci, port, cmd->interrupts);
     ahci_port_check_nonbusy(ahci, port, slot);
-    ahci_port_check_cmd_sanity(ahci, port, slot, cmd->xbytes);
+    ahci_port_check_cmd_sanity(ahci, cmd);
     ahci_port_check_d2h_sanity(ahci, port, slot);
     if (cmd->props->pio) {
         ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 779e812..ca8abee 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -512,8 +512,7 @@ void ahci_port_check_nonbusy(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
                                 uint8_t slot, size_t buffsize);
-void ahci_port_check_cmd_sanity(AHCIQState *ahci, uint8_t port,
-                                uint8_t slot, size_t buffsize);
+void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_get_command_header(AHCIQState *ahci, uint8_t port,
                              uint8_t slot, AHCICommandHeader *cmd);
 void ahci_set_command_header(AHCIQState *ahci, uint8_t port,
-- 
2.1.0

[Qemu-devel] [PATCH 10/16] libqos/ahci: add NCQ frame support

[John Snow]

NCQ frames are generated a little differently than
their non-NCQ cousins. Add support for them.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 44 +++++++++++++++++++++++++++++++++++---------
 tests/libqos/ahci.h | 29 ++++++++++++++++++++++++++++-
 2 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 922af8b..9a4d510 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -695,19 +695,34 @@ static void command_header_init(AHCICommand *cmd)
 static void command_table_init(AHCICommand *cmd)
 {
     RegH2DFIS *fis = &(cmd->fis);
+    uint16_t sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
 
     fis->fis_type = REG_H2D_FIS;
     fis->flags = REG_H2D_FIS_CMD; /* "Command" bit */
     fis->command = cmd->name;
-    cmd->fis.feature_low = 0x00;
-    cmd->fis.feature_high = 0x00;
-    if (cmd->props->lba28 || cmd->props->lba48) {
-        cmd->fis.device = ATA_DEVICE_LBA;
+
+    if (cmd->props->ncq) {
+        NCQFIS *ncqfis = (NCQFIS *)fis;
+        /* NCQ is weird and re-uses FIS frames for unrelated data.
+         * See SATA 3.2, 13.6.4.1 READ FPDMA QUEUED for an example. */
+        ncqfis->sector_low = sect_count & 0xFF;
+        ncqfis->sector_hi = (sect_count >> 8) & 0xFF;
+        ncqfis->device = NCQ_DEVICE_MAGIC;
+        /* Force Unit Access is bit 7 in the device register */
+        ncqfis->tag = 0;  /* bits 3-7 are the NCQ tag */
+        ncqfis->prio = 0; /* bits 6,7 are a prio tag */
+        /* RARC bit is bit 0 of TAG field */
+    } else {
+        fis->feature_low = 0x00;
+        fis->feature_high = 0x00;
+        if (cmd->props->lba28 || cmd->props->lba48) {
+            fis->device = ATA_DEVICE_LBA;
+        }
+        fis->count = (cmd->xbytes / AHCI_SECTOR_SIZE);
     }
-    cmd->fis.count = (cmd->xbytes / AHCI_SECTOR_SIZE);
-    cmd->fis.icc = 0x00;
-    cmd->fis.control = 0x00;
-    memset(cmd->fis.aux, 0x00, ARRAY_SIZE(cmd->fis.aux));
+    fis->icc = 0x00;
+    fis->control = 0x00;
+    memset(fis->aux, 0x00, ARRAY_SIZE(fis->aux));
 }
 
 AHCICommand *ahci_command_create(uint8_t command_name)
@@ -721,6 +736,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
     g_assert(!(props->lba28 && props->lba48));
     g_assert(!(props->read && props->write));
     g_assert(!props->size || props->data);
+    g_assert(!props->ncq || (props->ncq && props->lba48));
 
     /* Defaults and book-keeping */
     cmd->props = props;
@@ -789,6 +805,8 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t buffer)
 void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
                             unsigned prd_size)
 {
+    uint16_t sect_count;
+
     /* Each PRD can describe up to 4MiB, and must not be odd. */
     g_assert_cmphex(prd_size, <=, 4096 * 1024);
     g_assert_cmphex(prd_size & 0x01, ==, 0x00);
@@ -796,7 +814,15 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
         cmd->prd_size = prd_size;
     }
     cmd->xbytes = xbytes;
-    cmd->fis.count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+    sect_count = (cmd->xbytes / AHCI_SECTOR_SIZE);
+
+    if (cmd->props->ncq) {
+        NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
+        nfis->sector_low = sect_count & 0xFF;
+        nfis->sector_hi = (sect_count >> 8) & 0xFF;
+    } else {
+        cmd->fis.count = sect_count;
+    }
     cmd->header.prdtl = size_to_prdtl(cmd->xbytes, cmd->prd_size);
 }
 
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index ca8abee..594a1c9 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -291,8 +291,9 @@ enum {
 #define CMDH_PMP      (0xF000)
 
 /* ATA device register masks */
-#define ATA_DEVICE_MAGIC 0xA0
+#define ATA_DEVICE_MAGIC 0xA0 /* used in ata1-3 */
 #define ATA_DEVICE_LBA   0x40
+#define NCQ_DEVICE_MAGIC 0x40 /* for ncq device registers */
 #define ATA_DEVICE_DRIVE 0x10
 #define ATA_DEVICE_HEAD  0x0F
 
@@ -397,6 +398,32 @@ typedef struct RegH2DFIS {
 } __attribute__((__packed__)) RegH2DFIS;
 
 /**
+ * Register host-to-device FIS structure, for NCQ commands.
+ * Actually just a RegH2DFIS, but with fields repurposed.
+ * Repurposed fields are annotated below.
+ */
+typedef struct NCQFIS {
+    /* DW0 */
+    uint8_t fis_type;
+    uint8_t flags;
+    uint8_t command;
+    uint8_t sector_low; /* H2D: Feature 7:0 */
+    /* DW1 */
+    uint8_t lba_lo[3];
+    uint8_t device;
+    /* DW2 */
+    uint8_t lba_hi[3];
+    uint8_t sector_hi; /* H2D: Feature 15:8 */
+    /* DW3 */
+    uint8_t tag;       /* H2D: Count 0:7 */
+    uint8_t prio;      /* H2D: Count 15:8 */
+    uint8_t icc;
+    uint8_t control;
+    /* DW4 */
+    uint8_t aux[4];
+} __attribute__((__packed__)) NCQFIS;
+
+/**
  * Command List entry structure.
  * The command list contains between 1-32 of these structures.
  */
-- 
2.1.0

[Qemu-devel] [PATCH 11/16] libqos/ahci: edit wait to be ncq aware

[John Snow]

The wait command should check to make sure SACT is clear as well
as the Command Issue register.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 9a4d510..da02e2e 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -908,11 +908,15 @@ void ahci_command_wait(AHCIQState *ahci, AHCICommand *cmd)
     /* We can't rely on STS_BSY until the command has started processing.
      * Therefore, we also use the Command Issue bit as indication of
      * a command in-flight. */
-    while (BITSET(ahci_px_rreg(ahci, cmd->port, AHCI_PX_TFD),
-                  AHCI_PX_TFD_STS_BSY) ||
-           BITSET(ahci_px_rreg(ahci, cmd->port, AHCI_PX_CI), (1 << cmd->slot))) {
+
+#define RSET(REG, MASK) (BITSET(ahci_px_rreg(ahci, cmd->port, (REG)), (MASK)))
+
+    while (RSET(AHCI_PX_TFD, AHCI_PX_TFD_STS_BSY) ||
+           RSET(AHCI_PX_CI, 1 << cmd->slot) ||
+           (cmd->props->ncq && RSET(AHCI_PX_SACT, 1 << cmd->slot))) {
         usleep(50);
     }
+
 }
 
 void ahci_command_issue(AHCIQState *ahci, AHCICommand *cmd)
-- 
2.1.0

[Qemu-devel] [PATCH 12/16] libqos/ahci: adjust expected NCQ interrupts

[John Snow]

NCQ commands will expect the SDBS interrupt,
and in the normative case, do not expect to see
a D2H Register FIS unless something went wrong.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index da02e2e..953a320 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -745,12 +745,15 @@ AHCICommand *ahci_command_create(uint8_t command_name)
     cmd->prd_size = 4096;
     cmd->buffer = 0xabad1dea;
 
-    cmd->interrupts = AHCI_PX_IS_DHRS;
+    if (!cmd->props->ncq) {
+        cmd->interrupts = AHCI_PX_IS_DHRS;
+    }
     /* BUG: We expect the DPS interrupt for data commands */
     /* cmd->interrupts |= props->data ? AHCI_PX_IS_DPS : 0; */
     /* BUG: We expect the DMA Setup interrupt for DMA commands */
     /* cmd->interrupts |= props->dma ? AHCI_PX_IS_DSS : 0; */
     cmd->interrupts |= props->pio ? AHCI_PX_IS_PSS : 0;
+    cmd->interrupts |= props->ncq ? AHCI_PX_IS_SDBS : 0;
 
     command_header_init(cmd);
     command_table_init(cmd);
@@ -934,7 +937,9 @@ void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd)
     ahci_port_check_interrupts(ahci, port, cmd->interrupts);
     ahci_port_check_nonbusy(ahci, port, slot);
     ahci_port_check_cmd_sanity(ahci, cmd);
-    ahci_port_check_d2h_sanity(ahci, port, slot);
+    if (cmd->interrupts & AHCI_PX_IS_DHRS) {
+        ahci_port_check_d2h_sanity(ahci, port, slot);
+    }
     if (cmd->props->pio) {
         ahci_port_check_pio_sanity(ahci, port, slot, cmd->xbytes);
     }
-- 
2.1.0

[Qemu-devel] [PATCH 13/16] libqos/ahci: set the NCQ tag on command_commit

[John Snow]

NCQ commands have the concept of a "TAG" that they need to set,
but in the AHCI world, it is mandated that the TAG always match
the command slot that you executed the NCQ from.

See AHCI 9.3.1.1.5.2 "Native Queued Commands".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 953a320..7cf667a 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -857,6 +857,11 @@ void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port)
     cmd->port = port;
     cmd->slot = ahci_pick_cmd(ahci, port);
 
+    if (cmd->props->ncq) {
+        NCQFIS *nfis = (NCQFIS *)&cmd->fis;
+        nfis->tag = (cmd->slot << 3) & 0xFC;
+    }
+
     /* Create a buffer for the command table */
     prdtl = size_to_prdtl(cmd->xbytes, cmd->prd_size);
     table_size = CMD_TBL_SIZ(prdtl);
-- 
2.1.0

[Qemu-devel] [PATCH 14/16] libqos/ahci: Force all NCQ commands to be LBA48

[John Snow]

NCQ commands are LBA48 by definition.

See SATA 3.2 13.6.4.1 "READ FPDMA QUEUED", or
    SATA 3.2 13.6.5.1 "WRITE FPDMA QUEUED."

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 7cf667a..3d62cb6 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -781,7 +781,7 @@ void ahci_command_set_offset(AHCICommand *cmd, uint64_t lba_sect)
     RegH2DFIS *fis = &(cmd->fis);
     if (cmd->props->lba28) {
         g_assert_cmphex(lba_sect, <=, 0xFFFFFFF);
-    } else if (cmd->props->lba48) {
+    } else if (cmd->props->lba48 || cmd->props->ncq) {
         g_assert_cmphex(lba_sect, <=, 0xFFFFFFFFFFFF);
     } else {
         /* Can't set offset if we don't know the format. */
-- 
2.1.0

[Qemu-devel] [PATCH 15/16] qtest/ahci: simple ncq data test

[John Snow]

Test the NCQ pathways for a simple IO RW test.
Also, test that libqos doesn't explode when
running NCQ commands :)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c   | 13 +++++++++++++
 tests/libqos/ahci.c | 46 +++++++++++++++++++++++++---------------------
 tests/libqos/ahci.h | 27 +++++++++++++++------------
 3 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 43da3d1..941e0dd 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1395,6 +1395,17 @@ static void test_reset(void)
     ahci_shutdown(ahci);
 }
 
+static void test_ncq_simple(void)
+{
+    AHCIQState *ahci;
+
+    ahci = ahci_boot_and_enable(NULL);
+    ahci_test_io_rw_simple(ahci, 4096, 0,
+                           READ_FPDMA_QUEUED,
+                           WRITE_FPDMA_QUEUED);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 /* AHCI I/O Test Matrix Definitions                                           */
 
@@ -1653,6 +1664,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/max", test_max);
     qtest_add_func("/ahci/reset", test_reset);
 
+    qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
+
     ret = g_test_run();
 
     /* Cleanup */
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 3d62cb6..33ecd2a 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -50,27 +50,31 @@ typedef struct AHCICommandProp {
 } AHCICommandProp;
 
 AHCICommandProp ahci_command_properties[] = {
-    { .cmd = CMD_READ_PIO,      .data = true,  .pio = true,
-                                .lba28 = true, .read = true },
-    { .cmd = CMD_WRITE_PIO,     .data = true,  .pio = true,
-                                .lba28 = true, .write = true },
-    { .cmd = CMD_READ_PIO_EXT,  .data = true,  .pio = true,
-                                .lba48 = true, .read = true },
-    { .cmd = CMD_WRITE_PIO_EXT, .data = true,  .pio = true,
-                                .lba48 = true, .write = true },
-    { .cmd = CMD_READ_DMA,      .data = true,  .dma = true,
-                                .lba28 = true, .read = true },
-    { .cmd = CMD_WRITE_DMA,     .data = true,  .dma = true,
-                                .lba28 = true, .write = true },
-    { .cmd = CMD_READ_DMA_EXT,  .data = true,  .dma = true,
-                                .lba48 = true, .read = true },
-    { .cmd = CMD_WRITE_DMA_EXT, .data = true,  .dma = true,
-                                .lba48 = true, .write = true },
-    { .cmd = CMD_IDENTIFY,      .data = true,  .pio = true,
-                                .size = 512,   .read = true },
-    { .cmd = CMD_READ_MAX,      .lba28 = true },
-    { .cmd = CMD_READ_MAX_EXT,  .lba48 = true },
-    { .cmd = CMD_FLUSH_CACHE,   .data = false }
+    { .cmd = CMD_READ_PIO,       .data = true,  .pio = true,
+                                 .lba28 = true, .read = true },
+    { .cmd = CMD_WRITE_PIO,      .data = true,  .pio = true,
+                                 .lba28 = true, .write = true },
+    { .cmd = CMD_READ_PIO_EXT,   .data = true,  .pio = true,
+                                 .lba48 = true, .read = true },
+    { .cmd = CMD_WRITE_PIO_EXT,  .data = true,  .pio = true,
+                                 .lba48 = true, .write = true },
+    { .cmd = CMD_READ_DMA,       .data = true,  .dma = true,
+                                 .lba28 = true, .read = true },
+    { .cmd = CMD_WRITE_DMA,      .data = true,  .dma = true,
+                                 .lba28 = true, .write = true },
+    { .cmd = CMD_READ_DMA_EXT,   .data = true,  .dma = true,
+                                 .lba48 = true, .read = true },
+    { .cmd = CMD_WRITE_DMA_EXT,  .data = true,  .dma = true,
+                                 .lba48 = true, .write = true },
+    { .cmd = CMD_IDENTIFY,       .data = true,  .pio = true,
+                                 .size = 512,   .read = true },
+    { .cmd = READ_FPDMA_QUEUED,  .data = true,  .dma = true,
+                                 .lba48 = true, .read = true, .ncq = true },
+    { .cmd = WRITE_FPDMA_QUEUED, .data = true,  .dma = true,
+                                 .lba48 = true, .write = true, .ncq = true },
+    { .cmd = CMD_READ_MAX,       .lba28 = true },
+    { .cmd = CMD_READ_MAX_EXT,   .lba48 = true },
+    { .cmd = CMD_FLUSH_CACHE,    .data = false }
 };
 
 struct AHCICommand {
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 594a1c9..a08a9dd 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -263,20 +263,23 @@ enum {
 /* ATA Commands */
 enum {
     /* DMA */
-    CMD_READ_DMA      = 0xC8,
-    CMD_READ_DMA_EXT  = 0x25,
-    CMD_WRITE_DMA     = 0xCA,
-    CMD_WRITE_DMA_EXT = 0x35,
+    CMD_READ_DMA       = 0xC8,
+    CMD_READ_DMA_EXT   = 0x25,
+    CMD_WRITE_DMA      = 0xCA,
+    CMD_WRITE_DMA_EXT  = 0x35,
     /* PIO */
-    CMD_READ_PIO      = 0x20,
-    CMD_READ_PIO_EXT  = 0x24,
-    CMD_WRITE_PIO     = 0x30,
-    CMD_WRITE_PIO_EXT = 0x34,
+    CMD_READ_PIO       = 0x20,
+    CMD_READ_PIO_EXT   = 0x24,
+    CMD_WRITE_PIO      = 0x30,
+    CMD_WRITE_PIO_EXT  = 0x34,
     /* Misc */
-    CMD_READ_MAX      = 0xF8,
-    CMD_READ_MAX_EXT  = 0x27,
-    CMD_FLUSH_CACHE   = 0xE7,
-    CMD_IDENTIFY      = 0xEC
+    CMD_READ_MAX       = 0xF8,
+    CMD_READ_MAX_EXT   = 0x27,
+    CMD_FLUSH_CACHE    = 0xE7,
+    CMD_IDENTIFY       = 0xEC,
+    /* NCQ */
+    READ_FPDMA_QUEUED  = 0x60,
+    WRITE_FPDMA_QUEUED = 0x61,
 };
 
 /* AHCI Command Header Flags & Masks*/
-- 
2.1.0

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

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 941e0dd..206e6bb 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1140,9 +1140,9 @@ static void test_migrate_sanity(void)
 }
 
 /**
- * DMA Migration test: Write a pattern, migrate, then read.
+ * Simple migration test: Write a pattern, migrate, then read.
  */
-static void test_migrate_dma(void)
+static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
 {
     AHCIQState *src, *dst;
     uint8_t px;
@@ -1183,6 +1183,16 @@ static void test_migrate_dma(void)
     g_free(tx);
 }
 
+static void test_migrate_dma(void)
+{
+    ahci_migrate_simple(CMD_READ_DMA, CMD_WRITE_DMA);
+}
+
+static void test_migrate_ncq(void)
+{
+    ahci_migrate_simple(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED);
+}
+
 /**
  * DMA Error Test
  *
@@ -1665,6 +1675,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/reset", test_reset);
 
     qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
+    qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq);
 
     ret = g_test_run();
 
-- 
2.1.0

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

[John Snow]

On 06/19/2015 09:50 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ahci-test.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index 941e0dd..206e6bb 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -1140,9 +1140,9 @@ static void test_migrate_sanity(void)
>  }
>  
>  /**
> - * DMA Migration test: Write a pattern, migrate, then read.
> + * Simple migration test: Write a pattern, migrate, then read.
>   */
> -static void test_migrate_dma(void)
> +static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
>  {
>      AHCIQState *src, *dst;
>      uint8_t px;
> @@ -1183,6 +1183,16 @@ static void test_migrate_dma(void)
>      g_free(tx);
>  }
>  

This is what I get for staying up so late...

It would help if I actually used the parameters I created to use the
right command.

Fixed offline in a v2, will send out after other comments if any.

> +static void test_migrate_dma(void)
> +{
> +    ahci_migrate_simple(CMD_READ_DMA, CMD_WRITE_DMA);
> +}
> +
> +static void test_migrate_ncq(void)
> +{
> +    ahci_migrate_simple(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED);
> +}
> +
>  /**
>   * DMA Error Test
>   *
> @@ -1665,6 +1675,7 @@ int main(int argc, char **argv)
>      qtest_add_func("/ahci/reset", test_reset);
>  
>      qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
> +    qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq);
>  
>      ret = g_test_run();
>  
> 

Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[Stefan Hajnoczi]

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

On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote:
> @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>                     ((uint64_t)ncq_fis->lba2 << 16) |
>                     ((uint64_t)ncq_fis->lba1 << 8) |
>                     (uint64_t)ncq_fis->lba0;
> +    ncq_tfs->tag = tag;
>  
> -    /* Note: We calculate the sector count, but don't currently rely on it.
> -     * The total size of the DMA buffer tells us the transfer size instead. */
>      ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
>                                  ncq_fis->sector_count_low;
> +    ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
> +    size = ncq_tfs->sector_count * 512;

ncq_tfs->sector_count is used with - 2 and - 1 below.  What is the
semantics of this field and why is it okay to use it without subtracting
here?

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

Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[John Snow]

On 06/22/2015 10:06 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote:
>> @@ -999,20 +1000,28 @@ static void process_ncq_command(AHCIState
>> *s, int port, uint8_t *cmd_fis, ((uint64_t)ncq_fis->lba2 << 16)
>> | ((uint64_t)ncq_fis->lba1 << 8) | (uint64_t)ncq_fis->lba0; +
>> ncq_tfs->tag = tag;
>> 
>> -    /* Note: We calculate the sector count, but don't currently
>> rely on it. -     * The total size of the DMA buffer tells us the
>> transfer size instead. */ ncq_tfs->sector_count =
>> ((uint16_t)ncq_fis->sector_count_high << 8) | 
>> ncq_fis->sector_count_low; +    ahci_populate_sglist(ad,
>> &ncq_tfs->sglist, 0); +    size = ncq_tfs->sector_count * 512;
> 
> ncq_tfs->sector_count is used with - 2 and - 1 below.  What is the 
> semantics of this field and why is it okay to use it without
> subtracting here?
> 

Just a bonkers patch order. Patch #7 fixes the debug prints, which are
wrong.

The field is not "off by one" in the spec, sector_count == 1 means 1
sector.

(sector_count == 0 means 64K sectors, though, like it does in regular
ATA.)

Re: [Qemu-devel] [PATCH 06/16] ahci: add ncq debug checks

[Stefan Hajnoczi]

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

On Fri, Jun 19, 2015 at 09:50:37PM -0400, John Snow wrote:
> @@ -1003,6 +1003,27 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
>                     (uint64_t)ncq_fis->lba0;
>      ncq_tfs->tag = tag;
>  
> +#ifdef DEBUG_AHCI

These sorts of debug ifdefs have a tendency to bitrot since the code
isn't compiled.

There is no need for the ifdef since DPRINTF() is deadcode when DEBUG_AHCI
is 0.  The compiler parses the code, preventing bitrot, but optimizes
the code away when DEBUG_ACHI is 0.

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

Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[Stefan Hajnoczi]

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

On Fri, Jun 19, 2015 at 09:50:35PM -0400, John Snow wrote:
> -    /* Note: We calculate the sector count, but don't currently rely on it.
> -     * The total size of the DMA buffer tells us the transfer size instead. */
>      ncq_tfs->sector_count = ((uint16_t)ncq_fis->sector_count_high << 8) |
>                                  ncq_fis->sector_count_low;
> +    ahci_populate_sglist(ad, &ncq_tfs->sglist, 0);
> +    size = ncq_tfs->sector_count * 512;

I just saw that a later patch addresses the sector_count inconsistency,
so I'm happy with this now.

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

Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[Stefan Hajnoczi]

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

On Fri, Jun 19, 2015 at 09:50:39PM -0400, John Snow wrote:
> The legacy ide command execution layer will clear any errors outstanding
> before execution, but the NCQ layer doesn't.
> Even on success, this register will remain clogged.
> 
> Clear it out before each NCQ command so the guest can tell if the
> error code produced after completion is meaningful or not.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 6bded67..e63ba9b 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1048,6 +1048,8 @@ 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);
>  
> +    ide_state->error = 0;

I'm not sure it makes sense use ide_state at all in NCQ.

ide_state is per-port and NCQ can issue multiple asynchronous commands
per port.  If process_ncq_command() modifies ide_state, it may do that
while other commands are still pending or about to be processed.

This will clobber ide_state->error.

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

Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[John Snow]

On 06/22/2015 10:38 AM, Stefan Hajnoczi wrote:
> On Fri, Jun 19, 2015 at 09:50:39PM -0400, John Snow wrote:
>> The legacy ide command execution layer will clear any errors
>> outstanding before execution, but the NCQ layer doesn't. Even on
>> success, this register will remain clogged.
>> 
>> Clear it out before each NCQ command so the guest can tell if
>> the error code produced after completion is meaningful or not.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 2
>> ++ 1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 6bded67..e63ba9b
>> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1048,6 +1048,8
>> @@ 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);
>> 
>> +    ide_state->error = 0;
> 
> I'm not sure it makes sense use ide_state at all in NCQ.
> 
> ide_state is per-port and NCQ can issue multiple asynchronous
> commands per port.  If process_ncq_command() modifies ide_state, it
> may do that while other commands are still pending or about to be
> processed.
> 
> This will clobber ide_state->error.
> 

Good point. The real problem that we need to fix then is in the core
IDE layer, which tends to set a "default error" and waits for
ide_exec_cmd to clear it before execution.

If we don't clear that bit somehow, we will get failed commands.

I'll have to do a little research to see if it's safe to remove our
startup error, since it might be part of an ATA signature handshake.

Hmm.

Maybe it's not actually a problem because any real OS should probably
have issued ATA IDENTIFY (which will clear ERR) by the time they get
to NCQ, so maybe we can scrap this, and I'll adjust the test accordingly
to issue at least IDENTIFY before attempting NCQ.

Thanks.
--js

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

[Stefan Hajnoczi]

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

On Fri, Jun 19, 2015 at 09:50:31PM -0400, John Snow wrote:
> requires: 1434470575-21625-1-git-send-email-jsnow@redhat.com
>           [PATCH v2 0/4] ahci: misc fixes/tests for 2.4
> 
> This series adds a couple of tests to exercise the NCQ pathways
> and establish a baseline for us.
> 
> Most of these patches are fairly short and should be relatively trivial
> to review. this series will lay the groundwork for part 2,
> which adds the rerror/werror=stop and migration support for that feature
> as well.

Finished reviewing, left comments on some patches.

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

Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[John Snow]

On 06/22/2015 10:43 AM, John Snow wrote:
> 
> 
> On 06/22/2015 10:38 AM, Stefan Hajnoczi wrote:
>> On Fri, Jun 19, 2015 at 09:50:39PM -0400, John Snow wrote:
>>> The legacy ide command execution layer will clear any errors
>>> outstanding before execution, but the NCQ layer doesn't. Even on
>>> success, this register will remain clogged.
>>>
>>> Clear it out before each NCQ command so the guest can tell if
>>> the error code produced after completion is meaningful or not.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c | 2
>>> ++ 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 6bded67..e63ba9b
>>> 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1048,6 +1048,8
>>> @@ 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);
>>>
>>> +    ide_state->error = 0;
>>
>> I'm not sure it makes sense use ide_state at all in NCQ.
>>
>> ide_state is per-port and NCQ can issue multiple asynchronous
>> commands per port.  If process_ncq_command() modifies ide_state, it
>> may do that while other commands are still pending or about to be
>> processed.
>>
>> This will clobber ide_state->error.
>>
> 
> Good point. The real problem that we need to fix then is in the core
> IDE layer, which tends to set a "default error" and waits for
> ide_exec_cmd to clear it before execution.
> 
> If we don't clear that bit somehow, we will get failed commands.
> 
> I'll have to do a little research to see if it's safe to remove our
> startup error, since it might be part of an ATA signature handshake.
> 

It is indeed part of a startup signature and should not be removed.

> Hmm.
> 
> Maybe it's not actually a problem because any real OS should probably
> have issued ATA IDENTIFY (which will clear ERR) by the time they get
> to NCQ, so maybe we can scrap this, and I'll adjust the test accordingly
> to issue at least IDENTIFY before attempting NCQ.

It's not exquisitely clear what the NCQ state machine for SATA should
look like -- I need to figure out what a real device might do if you
attempt to send it an NCQ command before a "hello."

Or I'll just ignore this whole train of thought under the premise that
every OS alive will always send an IDENTIFY packet first, and be on my way.

Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[Stefan Hajnoczi]

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

On Mon, Jun 22, 2015 at 10:08:22AM -0400, John Snow wrote:
> (sector_count == 0 means 64K sectors, though, like it does in regular
> ATA.)

I don't see a case to handle this.

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

Re: [Qemu-devel] [PATCH 08/16] ahci: clear error register before NCQ cmd

[Stefan Hajnoczi]

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

On Mon, Jun 22, 2015 at 05:51:10PM -0400, John Snow wrote:
> Or I'll just ignore this whole train of thought under the premise that
> every OS alive will always send an IDENTIFY packet first, and be on my way.

Yes, it's not worth worrying about at this stage when guest OSes seem to
be happy with QEMU's AHCI emulation.

Stefan

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

Re: [Qemu-devel] [PATCH 04/16] ahci: check for ncq prdtl overflow

[John Snow]

On 06/23/2015 09:46 AM, Stefan Hajnoczi wrote:
> On Mon, Jun 22, 2015 at 10:08:22AM -0400, John Snow wrote:
>> (sector_count == 0 means 64K sectors, though, like it does in regular
>> ATA.)
> 
> I don't see a case to handle this.
> 

I apologize:

Corrected in [PATCH 09/16] ahci: correct ncq sector count, in the "part
2" series.

Sorry for the confusing order, I played a lot of patch-reshuffling and
due to the refactors to get NCQ migrating, sometimes it was literally
just a sin of convenience to leave certain patches after the shuffle.

I tried to leave each patch fixing just one very small issue at a time
so they were easier to digest, instead of batching them together like
"Fix (everything wrong about) sector count." Maybe that wasn't the right
approach, you can let me know.

Thank you,
--John