[Qemu-devel] [RFC 05/10] 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.

This patch also adds an is_ncq() helper to
avoid repetition for checking this property.

Signed-off-by: John Snow <address@hidden>
---
 hw/ide/ahci.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++----
 hw/ide/ahci.h | 38 ++++++++++++++++++++++++++++----------
 2 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 42a77c4..4a1af3b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -822,6 +822,21 @@ static void ncq_cb(void *opaque, int ret)
     ncq_tfs->used = 0;
 }
 
+static int is_ncq(uint8_t ata_cmd)
+{
+    /* Based on SATA 3.2 section 13.6.3.2 */
+    switch (ata_cmd) {
+    case READ_FPDMA_QUEUED:
+    case WRITE_FPDMA_QUEUED:
+    case NCQ_NON_DATA:
+    case RECEIVE_FPDMA_QUEUED:
+    case SEND_FPDMA_QUEUED:
+        return 1;
+    default:
+        return 0;
+    }
+}
+
 static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
                                 int slot)
 {
@@ -845,6 +860,22 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
                    ((uint64_t)ncq_fis->lba1 << 8) |
                    (uint64_t)ncq_fis->lba0;
 
+#ifdef DEBUG_AHCI
+    /* Sanity-check the NCQ packet */
+    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
+
     /* 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) |
@@ -856,6 +887,12 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
             s->dev[port].port.ifs[0].nb_sectors - 1);
 
     ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist, 0);
+    if (ncq_tfs->sglist.size != ncq_tfs->sector_count * 512) {
+        DPRINTF(port, "NCQ header hint (%u) doesn't match DMA buffer size"
+                " (%u)\n", ncq_tfs->sector_count * 512, ncq_tfs->sglist.size);
+        qemu_sglist_destroy(&ncq_tfs->sglist);
+        return;
+    }
     ncq_tfs->tag = tag;
 
     switch(ncq_fis->command) {
@@ -887,9 +924,15 @@ static void process_ncq_command(AHCIState *s, int port, 
uint8_t *cmd_fis,
                                             ncq_cb, ncq_tfs);
             break;
         default:
-            DPRINTF(port, "error: tried to process non-NCQ command as NCQ\n");
+            if (is_ncq(cmd_fis[2])) {
+                DPRINTF(port,
+                        "error: unsupported NCQ command (0x%02x) received\n",
+                        cmd_fis[2]);
+            } else {
+                DPRINTF(port,
+                        "error: tried to process non-NCQ command as NCQ\n");
+            }
             qemu_sglist_destroy(&ncq_tfs->sglist);
-            break;
     }
 }
 
@@ -981,8 +1024,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
     if (cmd_fis[1] == SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER) {
 
         /* Check for NCQ command */
-        if ((cmd_fis[2] == READ_FPDMA_QUEUED) ||
-            (cmd_fis[2] == WRITE_FPDMA_QUEUED)) {
+        if (is_ncq(cmd_fis[2])) {
             process_ncq_command(s, port, cmd_fis, slot);
             goto out;
         }
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 31f32d3..8745848 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -188,6 +188,12 @@
 
 #define READ_FPDMA_QUEUED                  0x60
 #define WRITE_FPDMA_QUEUED                 0x61
+#define NCQ_NON_DATA                       0x63
+#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
@@ -307,27 +313,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;
 
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
-- 
1.9.3