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

[Qemu-devel] [PATCH v2 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.

===
v2:

 - Cleared out #ifdefs to prevent unneeded bitrot
 - Fixed migration test in patch 16
 - Removed the forceful ide_state->error clear from patch 08,
   replacing it instead with a change to the tests to issue
   IDENTIFY prior to attempting any data transfer.

________________________________________________________________________________

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-v2:
https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s1-v2

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/qtest: Execute IDENTIFY prior to data commands
  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       | 102 +++++++++++++++++++++++----------
 hw/ide/ahci.h       |  38 ++++++++----
 tests/ahci-test.c   |  38 ++++++++++--
 tests/libqos/ahci.c | 162 +++++++++++++++++++++++++++++++++-------------------
 tests/libqos/ahci.h |  59 ++++++++++++++-----
 5 files changed, 281 insertions(+), 118 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 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 v2 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 v2 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 v2 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 v2 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 v2 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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

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

[Qemu-devel] [PATCH v2 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 c2e6642..95d228f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1043,14 +1043,14 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
 
     DPRINTF(port, "NCQ transfer LBA from %"PRId64" to %"PRId64", "
             "drive max %"PRId64"\n",
-            ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
+            ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 1,
             ide_state->nb_sectors - 1);
 
     switch(ncq_fis->command) {
         case READ_FPDMA_QUEUED:
             DPRINTF(port, "NCQ reading %d sectors from LBA %"PRId64", "
                     "tag %d\n",
-                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
+                    ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
 
             DPRINTF(port, "tag %d aio read %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
@@ -1063,7 +1063,7 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             break;
         case WRITE_FPDMA_QUEUED:
             DPRINTF(port, "NCQ writing %d sectors to LBA %"PRId64", tag %d\n",
-                    ncq_tfs->sector_count-1, ncq_tfs->lba, ncq_tfs->tag);
+                    ncq_tfs->sector_count, ncq_tfs->lba, ncq_tfs->tag);
 
             DPRINTF(port, "tag %d aio write %"PRId64"\n",
                     ncq_tfs->tag, ncq_tfs->lba);
-- 
2.1.0

[Qemu-devel] [PATCH v2 08/16] ahci/qtest: Execute IDENTIFY prior to data commands

[John Snow]

If you try to execute an NCQ command before trying to engage with the
device by issuing an IDENTIFY command, the error bits that are part of
the signature will fool the test suite into thinking there was a failure.

Issue IDENTIFY first on "boot", which will clear the signature out of
the registers for us.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 0a0ef2a..ee1dc20 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -228,6 +228,8 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...)
 {
     AHCIQState *ahci;
     va_list ap;
+    uint16_t buff[256];
+    uint8_t port;
 
     if (cli) {
         va_start(ap, cli);
@@ -239,6 +241,10 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...)
 
     ahci_pci_enable(ahci);
     ahci_hba_enable(ahci);
+    /* Initialize test device */
+    port = ahci_port_select(ahci);
+    ahci_port_clear(ahci, port);
+    ahci_io(ahci, port, CMD_IDENTIFY, &buff, sizeof(buff), 0);
 
     return ahci;
 }
-- 
2.1.0

[Qemu-devel] [PATCH v2 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 v2 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 v2 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 v2 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 v2 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 v2 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 v2 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 ee1dc20..0d117fe 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1401,6 +1401,17 @@ static void test_reset(void)
     ahci_shutdown(ahci);
 }
 
+static void test_ncq_simple(void)
+{
+    AHCIQState *ahci;
+
+    ahci = ahci_boot_and_enable(NULL);
+    ahci_test_io_rw_simple(ahci, 4096, 0,
+                           READ_FPDMA_QUEUED,
+                           WRITE_FPDMA_QUEUED);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 /* AHCI I/O Test Matrix Definitions                                           */
 
@@ -1654,6 +1665,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/max", test_max);
     qtest_add_func("/ahci/reset", test_reset);
 
+    qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
+
     ret = g_test_run();
 
     /* Cleanup */
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 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 v2 16/16] qtest/ahci: 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 0d117fe..3f06fd9 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1146,9 +1146,9 @@ static void test_migrate_sanity(void)
 }
 
 /**
- * DMA Migration test: Write a pattern, migrate, then read.
+ * Simple migration test: Write a pattern, migrate, then read.
  */
-static void test_migrate_dma(void)
+static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
 {
     AHCIQState *src, *dst;
     uint8_t px;
@@ -1176,9 +1176,9 @@ static void test_migrate_dma(void)
     }
 
     /* Write, migrate, then read. */
-    ahci_io(src, px, CMD_WRITE_DMA, tx, bufsize, 0);
+    ahci_io(src, px, cmd_write, tx, bufsize, 0);
     ahci_migrate(src, dst, uri);
-    ahci_io(dst, px, CMD_READ_DMA, rx, bufsize, 0);
+    ahci_io(dst, px, cmd_read, rx, bufsize, 0);
 
     /* Verify pattern */
     g_assert_cmphex(memcmp(tx, rx, bufsize), ==, 0);
@@ -1189,6 +1189,16 @@ static void test_migrate_dma(void)
     g_free(tx);
 }
 
+static void test_migrate_dma(void)
+{
+    ahci_migrate_simple(CMD_READ_DMA, CMD_WRITE_DMA);
+}
+
+static void test_migrate_ncq(void)
+{
+    ahci_migrate_simple(READ_FPDMA_QUEUED, WRITE_FPDMA_QUEUED);
+}
+
 /**
  * DMA Error Test
  *
@@ -1666,6 +1676,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/reset", test_reset);
 
     qtest_add_func("/ahci/io/ncq/simple", test_ncq_simple);
+    qtest_add_func("/ahci/migrate/ncq/simple", test_migrate_ncq);
 
     ret = g_test_run();
 
-- 
2.1.0

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

[Stefan Hajnoczi]

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

On Mon, Jun 22, 2015 at 07:38:12PM -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.
> 
> ===
> v2:
> 
>  - Cleared out #ifdefs to prevent unneeded bitrot
>  - Fixed migration test in patch 16
>  - Removed the forceful ide_state->error clear from patch 08,
>    replacing it instead with a change to the tests to issue
>    IDENTIFY prior to attempting any data transfer.
> 
> ________________________________________________________________________________
> 
> 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-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s1-v2
> 
> 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/qtest: Execute IDENTIFY prior to data commands
>   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       | 102 +++++++++++++++++++++++----------
>  hw/ide/ahci.h       |  38 ++++++++----
>  tests/ahci-test.c   |  38 ++++++++++--
>  tests/libqos/ahci.c | 162 +++++++++++++++++++++++++++++++++-------------------
>  tests/libqos/ahci.h |  59 ++++++++++++++-----
>  5 files changed, 281 insertions(+), 118 deletions(-)
> 
> -- 
> 2.1.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[John Snow]

On 06/22/2015 07:38 PM, 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.
> 
> ===
> v2:
> 
>  - Cleared out #ifdefs to prevent unneeded bitrot
>  - Fixed migration test in patch 16
>  - Removed the forceful ide_state->error clear from patch 08,
>    replacing it instead with a change to the tests to issue
>    IDENTIFY prior to attempting any data transfer.
> 
> ________________________________________________________________________________
> 
> 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-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-ncq-s1-v2
> 
> 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/qtest: Execute IDENTIFY prior to data commands
>   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       | 102 +++++++++++++++++++++++----------
>  hw/ide/ahci.h       |  38 ++++++++----
>  tests/ahci-test.c   |  38 ++++++++++--
>  tests/libqos/ahci.c | 162 +++++++++++++++++++++++++++++++++-------------------
>  tests/libqos/ahci.h |  59 ++++++++++++++-----
>  5 files changed, 281 insertions(+), 118 deletions(-)
> 

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js