[Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[John Snow]

The original version of the AHCI test base
which is now staged for being merged, processes
the ahci_identify test in a monolithic fashion.

In authoring new tests, it became necessary and
obvious as to how the operation of this device
should be factored out to ease the writing of
new AHCI tests.

This patch set issues the necessary refactorings
to support future test development for AHCI.

This patch set DOES NOT account for any new fixes
and requires no fixes from my "AHCI fixes" RFC
in order to run successfully on 2014-09-18's
origin/master.

This patch set does not alter the operation of the
existing test, or add new tests. It only offers
refactorings for future patch submissions which
depend on them, but are still under consideration.

John Snow (15):
  qtest/ahci: Add AHCIState structure
  qtest/ahci: Add port_select helper
  qtest/ahci: Add port_clear helper
  qtest/ahci: Add command header helpers
  qtest/ahci: Add build cmd table helper
  qtest/ahci: Add link_cmd_slot helper
  qtest/ahci: Add port_check_error helper
  qtest/ahci: Add issue_command helper
  qtest/ahci: Add port_check_interrupts helper
  qtest/ahci: Add port_check_nonbusy helper
  qtest/ahci: Add cmd response sanity check helpers
  qtest/ahci: Enforce zero-leaks for guest mem usage
  qtest/ahci: Add a macro bootup routine
  qtest/ahci: Add human-readable command names
  qtest/ahci: Don't use a magic constant for buffer size

 tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 583 insertions(+), 277 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 01/15] qtest/ahci: Add AHCIState structure

[John Snow]

Introduce a state structure that can be used to
conveniently bookmark key values of the AHCI Device
instead of re-polling it all the time. This structure
also helps ease of cleanup.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 7cc49f0..d845635 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -254,6 +254,21 @@
 
 /*** Structures ***/
 
+typedef struct AHCIPortState {
+    uint64_t fb;
+    uint64_t clb;
+} AHCIPortState;
+
+typedef struct AHCIState {
+    QPCIDevice *dev;
+    void *hba_base;
+    uint64_t barsize;
+    uint32_t fingerprint;
+    uint32_t cap;
+    uint32_t cap2;
+    AHCIPortState port[32];
+} AHCIState;
+
 /**
  * Generic FIS structure.
  */
@@ -343,18 +358,11 @@ typedef struct PRD {
     uint32_t dbc;  /* Data Byte Count (0-indexed) & Interrupt Flag (bit 2^31) */
 } PRD;
 
-typedef struct HBACap {
-    uint32_t cap;
-    uint32_t cap2;
-} HBACap;
-
 /*** Globals ***/
 static QGuestAllocator *guest_malloc;
 static QPCIBus *pcibus;
-static uint64_t barsize;
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
 static bool ahci_pedantic;
-static uint32_t ahci_fingerprint;
 
 /*** Macro Utilities ***/
 #define BITANY(data, mask) (((data) & (mask)) != 0)
@@ -364,8 +372,9 @@ static uint32_t ahci_fingerprint;
 #define ASSERT_BIT_CLEAR(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
 
 /*** IO macros for the AHCI memory registers. ***/
-#define AHCI_READ(OFST) qpci_io_readl(ahci, hba_base + (OFST))
-#define AHCI_WRITE(OFST, VAL) qpci_io_writel(ahci, hba_base + (OFST), (VAL))
+#define AHCI_READ(OFST) qpci_io_readl(ahci->dev, ahci->hba_base + (OFST))
+#define AHCI_WRITE(OFST, VAL) qpci_io_writel(ahci->dev,                 \
+                                             ahci->hba_base + (OFST), (VAL))
 #define AHCI_RREG(regno)      AHCI_READ(4 * (regno))
 #define AHCI_WREG(regno, val) AHCI_WRITE(4 * (regno), (val))
 #define AHCI_SET(regno, mask) AHCI_WREG((regno), AHCI_RREG(regno) | (mask))
@@ -385,17 +394,16 @@ static uint32_t ahci_fingerprint;
 
 
 /*** Function Declarations ***/
-static QPCIDevice *get_ahci_device(void);
-static QPCIDevice *start_ahci_device(QPCIDevice *dev, void **hba_base);
+static QPCIDevice *get_ahci_device(uint32_t *fingerprint);
+static void start_ahci_device(AHCIState *ahci);
 static void free_ahci_device(QPCIDevice *dev);
-static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
-                                HBACap *hcap, uint8_t port);
-static void ahci_test_pci_spec(QPCIDevice *ahci);
-static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
+static void ahci_test_port_spec(AHCIState *ahci, uint8_t port);
+static void ahci_test_pci_spec(AHCIState *ahci);
+static void ahci_test_pci_caps(AHCIState *ahci, uint16_t header,
                                uint8_t offset);
-static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset);
-static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset);
-static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset);
+static void ahci_test_satacap(AHCIState *ahci, uint8_t offset);
+static void ahci_test_msicap(AHCIState *ahci, uint8_t offset);
+static void ahci_test_pmcap(AHCIState *ahci, uint8_t offset);
 
 /*** Utilities ***/
 
@@ -413,9 +421,10 @@ static void string_bswap16(uint16_t *s, size_t bytes)
 /**
  * Locate, verify, and return a handle to the AHCI device.
  */
-static QPCIDevice *get_ahci_device(void)
+static QPCIDevice *get_ahci_device(uint32_t *fingerprint)
 {
     QPCIDevice *ahci;
+    uint32_t ahci_fingerprint;
 
     pcibus = qpci_init_pc();
 
@@ -433,21 +442,19 @@ static QPCIDevice *get_ahci_device(void)
         g_assert_not_reached();
     }
 
+    *fingerprint = ahci_fingerprint;
     return ahci;
 }
 
-static void free_ahci_device(QPCIDevice *ahci)
+static void free_ahci_device(QPCIDevice *dev)
 {
     /* libqos doesn't have a function for this, so free it manually */
-    g_free(ahci);
+    g_free(dev);
 
     if (pcibus) {
         qpci_free_pc(pcibus);
         pcibus = NULL;
     }
-
-    /* Clear our cached barsize information. */
-    barsize = 0;
 }
 
 /*** Test Setup & Teardown ***/
@@ -485,8 +492,12 @@ static void qtest_shutdown(void)
 /**
  * Start a Q35 machine and bookmark a handle to the AHCI device.
  */
-static QPCIDevice *ahci_boot(void)
+static AHCIState *ahci_boot(void)
 {
+    AHCIState *s;
+
+    s = g_malloc0(sizeof(AHCIState));
+
     qtest_boot("-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
                " -M q35 "
                "-device ide-hd,drive=drive0 "
@@ -494,15 +505,18 @@ static QPCIDevice *ahci_boot(void)
                tmp_path, "testdisk", "version");
 
     /* Verify that we have an AHCI device present. */
-    return get_ahci_device();
+    s->dev = get_ahci_device(&s->fingerprint);
+
+    return s;
 }
 
 /**
  * Clean up the PCI device, then terminate the QEMU instance.
  */
-static void ahci_shutdown(QPCIDevice *ahci)
+static void ahci_shutdown(AHCIState *ahci)
 {
-    free_ahci_device(ahci);
+    free_ahci_device(ahci->dev);
+    g_free(ahci);
     qtest_shutdown();
 }
 
@@ -511,21 +525,21 @@ static void ahci_shutdown(QPCIDevice *ahci)
 /**
  * Start the PCI device and sanity-check default operation.
  */
-static void ahci_pci_enable(QPCIDevice *ahci, void **hba_base)
+static void ahci_pci_enable(AHCIState *ahci)
 {
     uint8_t reg;
 
-    start_ahci_device(ahci, hba_base);
+    start_ahci_device(ahci);
 
-    switch (ahci_fingerprint) {
+    switch (ahci->fingerprint) {
     case AHCI_INTEL_ICH9:
         /* ICH9 has a register at PCI 0x92 that
          * acts as a master port enabler mask. */
-        reg = qpci_config_readb(ahci, 0x92);
+        reg = qpci_config_readb(ahci->dev, 0x92);
         reg |= 0x3F;
-        qpci_config_writeb(ahci, 0x92, reg);
+        qpci_config_writeb(ahci->dev, 0x92, reg);
         /* 0...0111111b -- bit significant, ports 0-5 enabled. */
-        ASSERT_BIT_SET(qpci_config_readb(ahci, 0x92), 0x3F);
+        ASSERT_BIT_SET(qpci_config_readb(ahci->dev, 0x92), 0x3F);
         break;
     }
 
@@ -534,15 +548,14 @@ static void ahci_pci_enable(QPCIDevice *ahci, void **hba_base)
 /**
  * Map BAR5/ABAR, and engage the PCI device.
  */
-static QPCIDevice *start_ahci_device(QPCIDevice *ahci, void **hba_base)
+static void start_ahci_device(AHCIState *ahci)
 {
     /* Map AHCI's ABAR (BAR5) */
-    *hba_base = qpci_iomap(ahci, 5, &barsize);
+    ahci->hba_base = qpci_iomap(ahci->dev, 5, &ahci->barsize);
+    g_assert(ahci->hba_base != 0);
 
     /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
-    qpci_device_enable(ahci);
-
-    return ahci;
+    qpci_device_enable(ahci->dev);
 }
 
 /**
@@ -550,7 +563,7 @@ static QPCIDevice *start_ahci_device(QPCIDevice *ahci, void **hba_base)
  * Initialize and start any ports with devices attached.
  * Bring the HBA into the idle state.
  */
-static void ahci_hba_enable(QPCIDevice *ahci, void *hba_base)
+static void ahci_hba_enable(AHCIState *ahci)
 {
     /* Bits of interest in this section:
      * GHC.AE     Global Host Control / AHCI Enable
@@ -561,15 +574,11 @@ static void ahci_hba_enable(QPCIDevice *ahci, void *hba_base)
      * PxCMD.FR   "FIS Receive Running"
      * PxCMD.CR   "Command List Running"
      */
-
-    g_assert(ahci != NULL);
-    g_assert(hba_base != NULL);
-
-    uint32_t reg, ports_impl, clb, fb;
+    uint32_t reg, ports_impl;
     uint16_t i;
     uint8_t num_cmd_slots;
 
-    g_assert(hba_base != 0);
+    g_assert(ahci != NULL);
 
     /* Set GHC.AE to 1 */
     AHCI_SET(AHCI_GHC, AHCI_GHC_AE);
@@ -610,16 +619,16 @@ static void ahci_hba_enable(QPCIDevice *ahci, void *hba_base)
 
         /* Allocate Memory for the Command List Buffer & FIS Buffer */
         /* PxCLB space ... 0x20 per command, as in 4.2.2 p 36 */
-        clb = guest_alloc(guest_malloc, num_cmd_slots * 0x20);
-        g_test_message("CLB: 0x%08x", clb);
-        PX_WREG(i, AHCI_PX_CLB, clb);
-        g_assert_cmphex(clb, ==, PX_RREG(i, AHCI_PX_CLB));
+        ahci->port[i].clb = guest_alloc(guest_malloc, num_cmd_slots * 0x20);
+        g_test_message("CLB: 0x%08lx", ahci->port[i].clb);
+        PX_WREG(i, AHCI_PX_CLB, ahci->port[i].clb);
+        g_assert_cmphex(ahci->port[i].clb, ==, PX_RREG(i, AHCI_PX_CLB));
 
         /* PxFB space ... 0x100, as in 4.2.1 p 35 */
-        fb = guest_alloc(guest_malloc, 0x100);
-        g_test_message("FB: 0x%08x", fb);
-        PX_WREG(i, AHCI_PX_FB, fb);
-        g_assert_cmphex(fb, ==, PX_RREG(i, AHCI_PX_FB));
+        ahci->port[i].fb = guest_alloc(guest_malloc, 0x100);
+        g_test_message("FB: 0x%08lx", ahci->port[i].fb);
+        PX_WREG(i, AHCI_PX_FB, ahci->port[i].fb);
+        g_assert_cmphex(ahci->port[i].fb, ==, PX_RREG(i, AHCI_PX_FB));
 
         /* Clear PxSERR, PxIS, then IS.IPS[x] by writing '1's. */
         PX_WREG(i, AHCI_PX_SERR, 0xFFFFFFFF);
@@ -684,14 +693,14 @@ static void ahci_hba_enable(QPCIDevice *ahci, void *hba_base)
 /**
  * Implementation for test_pci_spec. Ensures PCI configuration space is sane.
  */
-static void ahci_test_pci_spec(QPCIDevice *ahci)
+static void ahci_test_pci_spec(AHCIState *ahci)
 {
     uint8_t datab;
     uint16_t data;
     uint32_t datal;
 
     /* Most of these bits should start cleared until we turn them on. */
-    data = qpci_config_readw(ahci, PCI_COMMAND);
+    data = qpci_config_readw(ahci->dev, PCI_COMMAND);
     ASSERT_BIT_CLEAR(data, PCI_COMMAND_MEMORY);
     ASSERT_BIT_CLEAR(data, PCI_COMMAND_MASTER);
     ASSERT_BIT_CLEAR(data, PCI_COMMAND_SPECIAL);     /* Reserved */
@@ -703,7 +712,7 @@ static void ahci_test_pci_spec(QPCIDevice *ahci)
     ASSERT_BIT_CLEAR(data, PCI_COMMAND_INTX_DISABLE);
     ASSERT_BIT_CLEAR(data, 0xF800);                  /* Reserved */
 
-    data = qpci_config_readw(ahci, PCI_STATUS);
+    data = qpci_config_readw(ahci->dev, PCI_STATUS);
     ASSERT_BIT_CLEAR(data, 0x01 | 0x02 | 0x04);     /* Reserved */
     ASSERT_BIT_CLEAR(data, PCI_STATUS_INTERRUPT);
     ASSERT_BIT_SET(data, PCI_STATUS_CAP_LIST);      /* must be set */
@@ -716,7 +725,7 @@ static void ahci_test_pci_spec(QPCIDevice *ahci)
     ASSERT_BIT_CLEAR(data, PCI_STATUS_DETECTED_PARITY);
 
     /* RID occupies the low byte, CCs occupy the high three. */
-    datal = qpci_config_readl(ahci, PCI_CLASS_REVISION);
+    datal = qpci_config_readl(ahci->dev, PCI_CLASS_REVISION);
     if (ahci_pedantic) {
         /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00,
          * Though in practice this is likely seldom true. */
@@ -739,40 +748,40 @@ static void ahci_test_pci_spec(QPCIDevice *ahci)
         g_assert_not_reached();
     }
 
-    datab = qpci_config_readb(ahci, PCI_CACHE_LINE_SIZE);
+    datab = qpci_config_readb(ahci->dev, PCI_CACHE_LINE_SIZE);
     g_assert_cmphex(datab, ==, 0);
 
-    datab = qpci_config_readb(ahci, PCI_LATENCY_TIMER);
+    datab = qpci_config_readb(ahci->dev, PCI_LATENCY_TIMER);
     g_assert_cmphex(datab, ==, 0);
 
     /* Only the bottom 7 bits must be off. */
-    datab = qpci_config_readb(ahci, PCI_HEADER_TYPE);
+    datab = qpci_config_readb(ahci->dev, PCI_HEADER_TYPE);
     ASSERT_BIT_CLEAR(datab, 0x7F);
 
     /* BIST is optional, but the low 7 bits must always start off regardless. */
-    datab = qpci_config_readb(ahci, PCI_BIST);
+    datab = qpci_config_readb(ahci->dev, PCI_BIST);
     ASSERT_BIT_CLEAR(datab, 0x7F);
 
     /* BARS 0-4 do not have a boot spec, but ABAR/BAR5 must be clean. */
-    datal = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5);
+    datal = qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
     g_assert_cmphex(datal, ==, 0);
 
-    qpci_config_writel(ahci, PCI_BASE_ADDRESS_5, 0xFFFFFFFF);
-    datal = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5);
+    qpci_config_writel(ahci->dev, PCI_BASE_ADDRESS_5, 0xFFFFFFFF);
+    datal = qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
     /* ABAR must be 32-bit, memory mapped, non-prefetchable and
      * must be >= 512 bytes. To that end, bits 0-8 must be off. */
     ASSERT_BIT_CLEAR(datal, 0xFF);
 
     /* Capability list MUST be present, */
-    datal = qpci_config_readl(ahci, PCI_CAPABILITY_LIST);
+    datal = qpci_config_readl(ahci->dev, PCI_CAPABILITY_LIST);
     /* But these bits are reserved. */
     ASSERT_BIT_CLEAR(datal, ~0xFF);
     g_assert_cmphex(datal, !=, 0);
 
     /* Check specification adherence for capability extenstions. */
-    data = qpci_config_readw(ahci, datal);
+    data = qpci_config_readw(ahci->dev, datal);
 
-    switch (ahci_fingerprint) {
+    switch (ahci->fingerprint) {
     case AHCI_INTEL_ICH9:
         /* Intel ICH9 Family Datasheet 14.1.19 p.550 */
         g_assert_cmphex((data & 0xFF), ==, PCI_CAP_ID_MSI);
@@ -785,18 +794,18 @@ static void ahci_test_pci_spec(QPCIDevice *ahci)
     ahci_test_pci_caps(ahci, data, (uint8_t)datal);
 
     /* Reserved. */
-    datal = qpci_config_readl(ahci, PCI_CAPABILITY_LIST + 4);
+    datal = qpci_config_readl(ahci->dev, PCI_CAPABILITY_LIST + 4);
     g_assert_cmphex(datal, ==, 0);
 
     /* IPIN might vary, but ILINE must be off. */
-    datab = qpci_config_readb(ahci, PCI_INTERRUPT_LINE);
+    datab = qpci_config_readb(ahci->dev, PCI_INTERRUPT_LINE);
     g_assert_cmphex(datab, ==, 0);
 }
 
 /**
  * Test PCI capabilities for AHCI specification adherence.
  */
-static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
+static void ahci_test_pci_caps(AHCIState *ahci, uint16_t header,
                                uint8_t offset)
 {
     uint8_t cid = header & 0xFF;
@@ -820,14 +829,14 @@ static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
     }
 
     if (next) {
-        ahci_test_pci_caps(ahci, qpci_config_readw(ahci, next), next);
+        ahci_test_pci_caps(ahci, qpci_config_readw(ahci->dev, next), next);
     }
 }
 
 /**
  * Test SATA PCI capabilitity for AHCI specification adherence.
  */
-static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset)
+static void ahci_test_satacap(AHCIState *ahci, uint8_t offset)
 {
     uint16_t dataw;
     uint32_t datal;
@@ -835,11 +844,11 @@ static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset)
     g_test_message("Verifying SATACAP");
 
     /* Assert that the SATACAP version is 1.0, And reserved bits are empty. */
-    dataw = qpci_config_readw(ahci, offset + 2);
+    dataw = qpci_config_readw(ahci->dev, offset + 2);
     g_assert_cmphex(dataw, ==, 0x10);
 
     /* Grab the SATACR1 register. */
-    datal = qpci_config_readw(ahci, offset + 4);
+    datal = qpci_config_readw(ahci->dev, offset + 4);
 
     switch (datal & 0x0F) {
     case 0x04: /* BAR0 */
@@ -862,30 +871,29 @@ static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset)
 /**
  * Test MSI PCI capability for AHCI specification adherence.
  */
-static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset)
+static void ahci_test_msicap(AHCIState *ahci, uint8_t offset)
 {
     uint16_t dataw;
     uint32_t datal;
 
     g_test_message("Verifying MSICAP");
 
-    dataw = qpci_config_readw(ahci, offset + PCI_MSI_FLAGS);
+    dataw = qpci_config_readw(ahci->dev, offset + PCI_MSI_FLAGS);
     ASSERT_BIT_CLEAR(dataw, PCI_MSI_FLAGS_ENABLE);
     ASSERT_BIT_CLEAR(dataw, PCI_MSI_FLAGS_QSIZE);
     ASSERT_BIT_CLEAR(dataw, PCI_MSI_FLAGS_RESERVED);
 
-    datal = qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_LO);
+    datal = qpci_config_readl(ahci->dev, offset + PCI_MSI_ADDRESS_LO);
     g_assert_cmphex(datal, ==, 0);
-
     if (dataw & PCI_MSI_FLAGS_64BIT) {
         g_test_message("MSICAP is 64bit");
-        datal = qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_HI);
+        datal = qpci_config_readl(ahci->dev, offset + PCI_MSI_ADDRESS_HI);
         g_assert_cmphex(datal, ==, 0);
-        dataw = qpci_config_readw(ahci, offset + PCI_MSI_DATA_64);
+        dataw = qpci_config_readw(ahci->dev, offset + PCI_MSI_DATA_64);
         g_assert_cmphex(dataw, ==, 0);
     } else {
         g_test_message("MSICAP is 32bit");
-        dataw = qpci_config_readw(ahci, offset + PCI_MSI_DATA_32);
+        dataw = qpci_config_readw(ahci->dev, offset + PCI_MSI_DATA_32);
         g_assert_cmphex(dataw, ==, 0);
     }
 }
@@ -893,36 +901,34 @@ static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset)
 /**
  * Test Power Management PCI capability for AHCI specification adherence.
  */
-static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset)
+static void ahci_test_pmcap(AHCIState *ahci, uint8_t offset)
 {
     uint16_t dataw;
 
     g_test_message("Verifying PMCAP");
 
-    dataw = qpci_config_readw(ahci, offset + PCI_PM_PMC);
+    dataw = qpci_config_readw(ahci->dev, offset + PCI_PM_PMC);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CAP_PME_CLOCK);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CAP_RESERVED);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CAP_D1);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CAP_D2);
 
-    dataw = qpci_config_readw(ahci, offset + PCI_PM_CTRL);
+    dataw = qpci_config_readw(ahci->dev, offset + PCI_PM_CTRL);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CTRL_STATE_MASK);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CTRL_RESERVED);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CTRL_DATA_SEL_MASK);
     ASSERT_BIT_CLEAR(dataw, PCI_PM_CTRL_DATA_SCALE_MASK);
 }
 
-static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
+static void ahci_test_hba_spec(AHCIState *ahci)
 {
-    HBACap hcap;
     unsigned i;
-    uint32_t cap, cap2, reg;
+    uint32_t reg;
     uint32_t ports;
     uint8_t nports_impl;
     uint8_t maxports;
 
-    g_assert(ahci != 0);
-    g_assert(hba_base != 0);
+    g_assert(ahci != NULL);
 
     /*
      * Note that the AHCI spec does expect the BIOS to set up a few things:
@@ -945,15 +951,15 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
      */
 
     /* 1 CAP - Capabilities Register */
-    cap = AHCI_RREG(AHCI_CAP);
-    ASSERT_BIT_CLEAR(cap, AHCI_CAP_RESERVED);
+    ahci->cap = AHCI_RREG(AHCI_CAP);
+    ASSERT_BIT_CLEAR(ahci->cap, AHCI_CAP_RESERVED);
 
     /* 2 GHC - Global Host Control */
     reg = AHCI_RREG(AHCI_GHC);
     ASSERT_BIT_CLEAR(reg, AHCI_GHC_HR);
     ASSERT_BIT_CLEAR(reg, AHCI_GHC_IE);
     ASSERT_BIT_CLEAR(reg, AHCI_GHC_MRSM);
-    if (BITSET(cap, AHCI_CAP_SAM)) {
+    if (BITSET(ahci->cap, AHCI_CAP_SAM)) {
         g_test_message("Supports AHCI-Only Mode: GHC_AE is Read-Only.");
         ASSERT_BIT_SET(reg, AHCI_GHC_AE);
     } else {
@@ -971,13 +977,13 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
     g_assert_cmphex(ports, !=, 0);
     /* Ports Implemented must be <= Number of Ports. */
     nports_impl = ctpopl(ports);
-    g_assert_cmpuint(((AHCI_CAP_NP & cap) + 1), >=, nports_impl);
+    g_assert_cmpuint(((AHCI_CAP_NP & ahci->cap) + 1), >=, nports_impl);
 
-    g_assert_cmphex(barsize, >, 0);
     /* Ports must be within the proper range. Given a mapping of SIZE,
      * 256 bytes are used for global HBA control, and the rest is used
      * for ports data, at 0x80 bytes each. */
-    maxports = (barsize - HBA_DATA_REGION_SIZE) / HBA_PORT_DATA_SIZE;
+    g_assert_cmphex(ahci->barsize, >, 0);
+    maxports = (ahci->barsize - HBA_DATA_REGION_SIZE) / HBA_PORT_DATA_SIZE;
     /* e.g, 30 ports for 4K of memory. (4096 - 256) / 128 = 30 */
     g_assert_cmphex((reg >> maxports), ==, 0);
 
@@ -996,7 +1002,7 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
 
     /* 6 Command Completion Coalescing Control: depends on CAP.CCCS. */
     reg = AHCI_RREG(AHCI_CCCCTL);
-    if (BITSET(cap, AHCI_CAP_CCCS)) {
+    if (BITSET(ahci->cap, AHCI_CAP_CCCS)) {
         ASSERT_BIT_CLEAR(reg, AHCI_CCCCTL_EN);
         ASSERT_BIT_CLEAR(reg, AHCI_CCCCTL_RESERVED);
         ASSERT_BIT_SET(reg, AHCI_CCCCTL_CC);
@@ -1012,13 +1018,13 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
 
     /* 8 EM_LOC */
     reg = AHCI_RREG(AHCI_EMLOC);
-    if (BITCLR(cap, AHCI_CAP_EMS)) {
+    if (BITCLR(ahci->cap, AHCI_CAP_EMS)) {
         g_assert_cmphex(reg, ==, 0);
     }
 
     /* 9 EM_CTL */
     reg = AHCI_RREG(AHCI_EMCTL);
-    if (BITSET(cap, AHCI_CAP_EMS)) {
+    if (BITSET(ahci->cap, AHCI_CAP_EMS)) {
         ASSERT_BIT_CLEAR(reg, AHCI_EMCTL_STSMR);
         ASSERT_BIT_CLEAR(reg, AHCI_EMCTL_CTLTM);
         ASSERT_BIT_CLEAR(reg, AHCI_EMCTL_CTLRST);
@@ -1028,8 +1034,8 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
     }
 
     /* 10 CAP2 -- Capabilities Extended */
-    cap2 = AHCI_RREG(AHCI_CAP2);
-    ASSERT_BIT_CLEAR(cap2, AHCI_CAP2_RESERVED);
+    ahci->cap2 = AHCI_RREG(AHCI_CAP2);
+    ASSERT_BIT_CLEAR(ahci->cap2, AHCI_CAP2_RESERVED);
 
     /* 11 BOHC -- Bios/OS Handoff Control */
     reg = AHCI_RREG(AHCI_BOHC);
@@ -1043,7 +1049,7 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
     }
 
     /* 24 -- 39: NVMHCI */
-    if (BITCLR(cap2, AHCI_CAP2_NVMP)) {
+    if (BITCLR(ahci->cap2, AHCI_CAP2_NVMP)) {
         g_test_message("Verifying HBA/NVMHCI area is empty.");
         for (i = AHCI_NVMHCI; i < AHCI_VENDOR; ++i) {
             reg = AHCI_RREG(i);
@@ -1059,12 +1065,10 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
     }
 
     /* 64 -- XX: Port Space */
-    hcap.cap = cap;
-    hcap.cap2 = cap2;
     for (i = 0; ports || (i < maxports); ports >>= 1, ++i) {
         if (BITSET(ports, 0x1)) {
             g_test_message("Testing port %u for spec", i);
-            ahci_test_port_spec(ahci, hba_base, &hcap, i);
+            ahci_test_port_spec(ahci, i);
         } else {
             uint16_t j;
             uint16_t low = AHCI_PORTS + (32 * i);
@@ -1083,8 +1087,7 @@ static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
 /**
  * Test the memory space for one port for specification adherence.
  */
-static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
-                                HBACap *hcap, uint8_t port)
+static void ahci_test_port_spec(AHCIState *ahci, uint8_t port)
 {
     uint32_t reg;
     unsigned i;
@@ -1094,7 +1097,7 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
     ASSERT_BIT_CLEAR(reg, AHCI_PX_CLB_RESERVED);
 
     /* (1) CLBU */
-    if (BITCLR(hcap->cap, AHCI_CAP_S64A)) {
+    if (BITCLR(ahci->cap, AHCI_CAP_S64A)) {
         reg = PX_RREG(port, AHCI_PX_CLBU);
         g_assert_cmphex(reg, ==, 0);
     }
@@ -1104,7 +1107,7 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
     ASSERT_BIT_CLEAR(reg, AHCI_PX_FB_RESERVED);
 
     /* (3) FBU */
-    if (BITCLR(hcap->cap, AHCI_CAP_S64A)) {
+    if (BITCLR(ahci->cap, AHCI_CAP_S64A)) {
         reg = PX_RREG(port, AHCI_PX_FBU);
         g_assert_cmphex(reg, ==, 0);
     }
@@ -1140,7 +1143,7 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
         ASSERT_BIT_CLEAR(reg, AHCI_PX_CMD_MPSS);
     }
     /* If we do not support MPS, MPSS and MPSP must be off. */
-    if (BITCLR(hcap->cap, AHCI_CAP_SMPS)) {
+    if (BITCLR(ahci->cap, AHCI_CAP_SMPS)) {
         ASSERT_BIT_CLEAR(reg, AHCI_PX_CMD_MPSS);
         ASSERT_BIT_CLEAR(reg, AHCI_PX_CMD_MPSP);
     }
@@ -1151,7 +1154,7 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
     /* HPCP and ESP cannot both be active. */
     g_assert_false(BITSET(reg, AHCI_PX_CMD_HPCP | AHCI_PX_CMD_ESP));
     /* If CAP.FBSS is not set, FBSCP must not be set. */
-    if (BITCLR(hcap->cap, AHCI_CAP_FBSS)) {
+    if (BITCLR(ahci->cap, AHCI_CAP_FBSS)) {
         ASSERT_BIT_CLEAR(reg, AHCI_PX_CMD_FBSCP);
     }
 
@@ -1211,7 +1214,7 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
     ASSERT_BIT_CLEAR(reg, AHCI_PX_FBS_DEV);
     ASSERT_BIT_CLEAR(reg, AHCI_PX_FBS_DWE);
     ASSERT_BIT_CLEAR(reg, AHCI_PX_FBS_RESERVED);
-    if (BITSET(hcap->cap, AHCI_CAP_FBSS)) {
+    if (BITSET(ahci->cap, AHCI_CAP_FBSS)) {
         /* if Port-Multiplier FIS-based switching avail, ADO must >= 2 */
         g_assert((reg & AHCI_PX_FBS_ADO) >> ctzl(AHCI_PX_FBS_ADO) >= 2);
     }
@@ -1235,20 +1238,19 @@ static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
  */
-static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
+static void ahci_test_identify(AHCIState *ahci)
 {
     RegD2HFIS *d2h = g_malloc0(0x20);
     RegD2HFIS *pio = g_malloc0(0x20);
     RegH2DFIS fis;
     AHCICommand cmd;
     PRD prd;
-    uint32_t ports, reg, clb, table, fb, data_ptr;
+    uint32_t ports, reg, table, data_ptr;
     uint16_t buff[256];
     unsigned i;
     int rc;
 
     g_assert(ahci != NULL);
-    g_assert(hba_base != NULL);
 
     /* We need to:
      * (1) Create a Command Table Buffer and update the Command List Slot #0
@@ -1289,9 +1291,8 @@ static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
 
     /* Wipe the FIS-Recieve Buffer */
-    fb = PX_RREG(i, AHCI_PX_FB);
-    g_assert_cmphex(fb, !=, 0);
-    qmemset(fb, 0x00, 0x100);
+    g_assert_cmphex(ahci->port[i].fb, !=, 0);
+    qmemset(ahci->port[i].fb, 0x00, 0x100);
 
     /* Create a Command Table buffer. 0x80 is the smallest with a PRDTL of 0. */
     /* We need at least one PRD, so round up to the nearest 0x80 multiple.    */
@@ -1303,13 +1304,9 @@ static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
     data_ptr = guest_alloc(guest_malloc, 512);
     g_assert(data_ptr);
 
-    /* Grab the Command List Buffer pointer */
-    clb = PX_RREG(i, AHCI_PX_CLB);
-    g_assert(clb);
-
     /* Copy the existing Command #0 structure from the CLB into local memory,
      * and build a new command #0. */
-    memread(clb, &cmd, sizeof(cmd));
+    memread(ahci->port[i].clb, &cmd, sizeof(cmd));
     cmd.b1 = 5;    /* reg_h2d_fis is 5 double-words long */
     cmd.b2 = 0x04; /* clear PxTFD.STS.BSY when done */
     cmd.prdtl = cpu_to_le16(1); /* One PRD table entry. */
@@ -1341,7 +1338,7 @@ static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
     memwrite(table + 0x80, &prd, sizeof(prd));
 
     /* Commit Command #0, pointing to the Table, to the Command List Buffer. */
-    memwrite(clb, &cmd, sizeof(cmd));
+    memwrite(ahci->port[i].clb, &cmd, sizeof(cmd));
 
     /* Everything is in place, but we haven't given the go-ahead yet. */
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
@@ -1371,12 +1368,12 @@ static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
     ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR);
 
     /* Investigate CMD #0, assert that we read 512 bytes */
-    memread(clb, &cmd, sizeof(cmd));
+    memread(ahci->port[i].clb, &cmd, sizeof(cmd));
     g_assert_cmphex(512, ==, le32_to_cpu(cmd.prdbc));
 
     /* Investigate FIS responses */
-    memread(fb + 0x20, pio, 0x20);
-    memread(fb + 0x40, d2h, 0x20);
+    memread(ahci->port[i].fb + 0x20, pio, 0x20);
+    memread(ahci->port[i].fb + 0x40, d2h, 0x20);
     g_assert_cmphex(pio->fis_type, ==, 0x5f);
     g_assert_cmphex(d2h->fis_type, ==, 0x34);
     g_assert_cmphex(pio->flags, ==, d2h->flags);
@@ -1420,7 +1417,7 @@ static void ahci_test_identify(QPCIDevice *ahci, void *hba_base)
  */
 static void test_sanity(void)
 {
-    QPCIDevice *ahci;
+    AHCIState *ahci;
     ahci = ahci_boot();
     ahci_shutdown(ahci);
 }
@@ -1431,7 +1428,7 @@ static void test_sanity(void)
  */
 static void test_pci_spec(void)
 {
-    QPCIDevice *ahci;
+    AHCIState *ahci;
     ahci = ahci_boot();
     ahci_test_pci_spec(ahci);
     ahci_shutdown(ahci);
@@ -1443,10 +1440,9 @@ static void test_pci_spec(void)
  */
 static void test_pci_enable(void)
 {
-    QPCIDevice *ahci;
-    void *hba_base;
+    AHCIState *ahci;
     ahci = ahci_boot();
-    ahci_pci_enable(ahci, &hba_base);
+    ahci_pci_enable(ahci);
     ahci_shutdown(ahci);
 }
 
@@ -1456,12 +1452,11 @@ static void test_pci_enable(void)
  */
 static void test_hba_spec(void)
 {
-    QPCIDevice *ahci;
-    void *hba_base;
+    AHCIState *ahci;
 
     ahci = ahci_boot();
-    ahci_pci_enable(ahci, &hba_base);
-    ahci_test_hba_spec(ahci, hba_base);
+    ahci_pci_enable(ahci);
+    ahci_test_hba_spec(ahci);
     ahci_shutdown(ahci);
 }
 
@@ -1471,12 +1466,11 @@ static void test_hba_spec(void)
  */
 static void test_hba_enable(void)
 {
-    QPCIDevice *ahci;
-    void *hba_base;
+    AHCIState *ahci;
 
     ahci = ahci_boot();
-    ahci_pci_enable(ahci, &hba_base);
-    ahci_hba_enable(ahci, hba_base);
+    ahci_pci_enable(ahci);
+    ahci_hba_enable(ahci);
     ahci_shutdown(ahci);
 }
 
@@ -1486,13 +1480,12 @@ static void test_hba_enable(void)
  */
 static void test_identify(void)
 {
-    QPCIDevice *ahci;
-    void *hba_base;
+    AHCIState *ahci;
 
     ahci = ahci_boot();
-    ahci_pci_enable(ahci, &hba_base);
-    ahci_hba_enable(ahci, hba_base);
-    ahci_test_identify(ahci, hba_base);
+    ahci_pci_enable(ahci);
+    ahci_hba_enable(ahci);
+    ahci_test_identify(ahci);
     ahci_shutdown(ahci);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 02/15] qtest/ahci: Add port_select helper

[John Snow]

This helper identifies which port of the AHCI HBA
has a device we may run tests on.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index d845635..5739063 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1235,6 +1235,33 @@ static void ahci_test_port_spec(AHCIState *ahci, uint8_t port)
 }
 
 /**
+ * Pick the first implemented and running port
+ */
+static unsigned port_select(AHCIState *ahci)
+{
+    uint32_t ports, reg;
+    unsigned i;
+
+    ports = AHCI_RREG(AHCI_PI);
+    for (i = 0; i < 32; ports >>= 1, ++i) {
+        if (ports == 0) {
+            i = 32;
+        }
+
+        if (!(ports & 0x01)) {
+            continue;
+        }
+
+        reg = PX_RREG(i, AHCI_PX_CMD);
+        if (BITSET(reg, AHCI_PX_CMD_ST)) {
+            break;
+        }
+    }
+    g_assert(i < 32);
+    return i;
+}
+
+/**
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
  */
@@ -1245,7 +1272,7 @@ static void ahci_test_identify(AHCIState *ahci)
     RegH2DFIS fis;
     AHCICommand cmd;
     PRD prd;
-    uint32_t ports, reg, table, data_ptr;
+    uint32_t reg, table, data_ptr;
     uint16_t buff[256];
     unsigned i;
     int rc;
@@ -1267,22 +1294,7 @@ static void ahci_test_identify(AHCIState *ahci)
      */
 
     /* Pick the first implemented and running port */
-    ports = AHCI_RREG(AHCI_PI);
-    for (i = 0; i < 32; ports >>= 1, ++i) {
-        if (ports == 0) {
-            i = 32;
-        }
-
-        if (!(ports & 0x01)) {
-            continue;
-        }
-
-        reg = PX_RREG(i, AHCI_PX_CMD);
-        if (BITSET(reg, AHCI_PX_CMD_ST)) {
-            break;
-        }
-    }
-    g_assert_cmphex(i, <, 32);
+    i = port_select(ahci);
     g_test_message("Selected port %u for test", i);
 
     /* Clear out this port's interrupts (ignore the init register d2h fis) */
-- 
1.9.3

[Qemu-devel] [PATCH 03/15] qtest/ahci: Add port_clear helper

[John Snow]

Add a helper that assists in clearing out potentially
old error and FIS information from an AHCI port's
data structures. This ensures we always start with
a blank slate for interrupt and FIS receipt information.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 5739063..77e3963 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1262,6 +1262,22 @@ static unsigned port_select(AHCIState *ahci)
 }
 
 /**
+ * Clear a port's interrupts and status information prior to a test.
+ */
+static void port_clear(AHCIState *ahci, uint8_t px)
+{
+    uint32_t reg;
+
+    /* Clear out this port's interrupts (ignore the init register d2h fis) */
+    reg = PX_RREG(px, AHCI_PX_IS);
+    PX_WREG(px, AHCI_PX_IS, reg);
+    g_assert_cmphex(PX_RREG(px, AHCI_PX_IS), ==, 0);
+
+    /* Wipe the FIS-Recieve Buffer */
+    qmemset(ahci->port[px].fb, 0x00, 0x100);
+}
+
+/**
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
  */
@@ -1297,14 +1313,8 @@ static void ahci_test_identify(AHCIState *ahci)
     i = port_select(ahci);
     g_test_message("Selected port %u for test", i);
 
-    /* Clear out this port's interrupts (ignore the init register d2h fis) */
-    reg = PX_RREG(i, AHCI_PX_IS);
-    PX_WREG(i, AHCI_PX_IS, reg);
-    g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
-
-    /* Wipe the FIS-Recieve Buffer */
-    g_assert_cmphex(ahci->port[i].fb, !=, 0);
-    qmemset(ahci->port[i].fb, 0x00, 0x100);
+    /* Clear out the FIS Receive area and any pending interrupts. */
+    port_clear(ahci, i);
 
     /* Create a Command Table buffer. 0x80 is the smallest with a PRDTL of 0. */
     /* We need at least one PRD, so round up to the nearest 0x80 multiple.    */
-- 
1.9.3

[Qemu-devel] [PATCH 04/15] qtest/ahci: Add command header helpers

[John Snow]

Add command_header_set, command_header_get,
command_destroy and cmd_pick to help quickly
manage the command header information in the
AHCI device.

command_header_set and get will store or retrieve
an AHCI command header, respectively.

cmd_pick chooses the first available but
least recently used command slot to allow us
to cycle through the available command slots.

command_destroy obliterates all information contained
within a given slot's command header, and frees its
associated command table, but not its DMA buffer!

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 77e3963..7a62eef 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -257,6 +257,9 @@
 typedef struct AHCIPortState {
     uint64_t fb;
     uint64_t clb;
+    uint64_t ctba[32];
+    uint16_t prdtl[32];
+    uint8_t next; /** Next Command Slot to Use **/
 } AHCIPortState;
 
 typedef struct AHCIState {
@@ -342,8 +345,7 @@ typedef struct AHCICommand {
     uint8_t b2;
     uint16_t prdtl; /* Phys Region Desc. Table Length */
     uint32_t prdbc; /* Phys Region Desc. Byte Count */
-    uint32_t ctba;  /* Command Table Descriptor Base Address */
-    uint32_t ctbau; /*                                    '' Upper */
+    uint64_t ctba;  /* Command Table Descriptor Base Address */
     uint32_t res[4];
 } __attribute__((__packed__)) AHCICommand;
 
@@ -352,11 +354,10 @@ typedef struct AHCICommand {
  * struct ahci_command.
  */
 typedef struct PRD {
-    uint32_t dba;  /* Data Base Address */
-    uint32_t dbau; /* Data Base Address Upper */
+    uint64_t dba;  /* Data Base Address */
     uint32_t res;  /* Reserved */
     uint32_t dbc;  /* Data Byte Count (0-indexed) & Interrupt Flag (bit 2^31) */
-} PRD;
+} __attribute__((__packed__)) PRD;
 
 /*** Globals ***/
 static QGuestAllocator *guest_malloc;
@@ -1277,6 +1278,78 @@ static void port_clear(AHCIState *ahci, uint8_t px)
     qmemset(ahci->port[px].fb, 0x00, 0x100);
 }
 
+/* Get the #cx'th command of port #px. */
+static void get_command_header(AHCIState *ahci, uint8_t px,
+                               uint8_t cx, AHCICommand *cmd)
+{
+    uint64_t ba = ahci->port[px].clb;
+    ba += cx * sizeof(AHCICommand);
+    memread(ba, cmd, sizeof(AHCICommand));
+
+    cmd->prdtl = le16_to_cpu(cmd->prdtl);
+    cmd->prdbc = le32_to_cpu(cmd->prdbc);
+    cmd->ctba = le64_to_cpu(cmd->ctba);
+}
+
+/* Set the #cx'th command of port #px. */
+static void set_command_header(AHCIState *ahci, uint8_t px,
+                               uint8_t cx, AHCICommand *cmd)
+{
+    uint64_t ba = ahci->port[px].clb;
+    ba += cx * sizeof(AHCICommand);
+
+    cmd->prdtl = cpu_to_le16(cmd->prdtl);
+    cmd->prdbc = cpu_to_le32(cmd->prdbc);
+    cmd->ctba = cpu_to_le64(cmd->ctba);
+
+    memwrite(ba, cmd, sizeof(AHCICommand));
+}
+
+static void destroy_command(AHCIState *ahci, uint8_t px, uint8_t cx)
+{
+    AHCICommand cmd;
+
+    /* Obtain the Nth Command Header */
+    get_command_header(ahci, px, cx, &cmd);
+    if (cmd.ctba == 0) {
+        /* No address in it, so just return -- it's empty. */
+        goto tidy;
+    }
+
+    /* Free the Table */
+    guest_free(guest_malloc, cmd.ctba);
+
+ tidy:
+    /* NULL the header. */
+    memset(&cmd, 0x00, sizeof(cmd));
+    set_command_header(ahci, px, cx, &cmd);
+    ahci->port[px].ctba[cx] = 0;
+    ahci->port[px].prdtl[cx] = 0;
+}
+
+static unsigned pick_cmd(AHCIState *ahci, uint8_t px)
+{
+    unsigned i;
+    unsigned j;
+    uint32_t reg;
+
+    reg = PX_RREG(px, AHCI_PX_CI);
+
+    /* Pick the least recently used command slot that's available */
+    for (i = 0; i < 32; ++i) {
+        j = ((ahci->port[px].next + i) % 32);
+        if (reg & (1 << j)) {
+            continue;
+        }
+        destroy_command(ahci, px, i);
+        ahci->port[px].next = (j + 1) % 32;
+        return j;
+    }
+
+    g_test_message("All command slots were busy.");
+    g_assert_not_reached();
+}
+
 /**
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
@@ -1288,10 +1361,12 @@ static void ahci_test_identify(AHCIState *ahci)
     RegH2DFIS fis;
     AHCICommand cmd;
     PRD prd;
-    uint32_t reg, table, data_ptr;
+    uint32_t reg, data_ptr;
     uint16_t buff[256];
     unsigned i;
     int rc;
+    uint8_t cx;
+    uint64_t table_ptr;
 
     g_assert(ahci != NULL);
 
@@ -1318,27 +1393,27 @@ static void ahci_test_identify(AHCIState *ahci)
 
     /* Create a Command Table buffer. 0x80 is the smallest with a PRDTL of 0. */
     /* We need at least one PRD, so round up to the nearest 0x80 multiple.    */
-    table = guest_alloc(guest_malloc, CMD_TBL_SIZ(1));
-    g_assert(table);
-    ASSERT_BIT_CLEAR(table, 0x7F);
+    table_ptr = guest_alloc(guest_malloc, CMD_TBL_SIZ(1));
+    g_assert(table_ptr);
+    ASSERT_BIT_CLEAR(table_ptr, 0x7F);
 
     /* Create a data buffer ... where we will dump the IDENTIFY data to. */
     data_ptr = guest_alloc(guest_malloc, 512);
     g_assert(data_ptr);
 
-    /* Copy the existing Command #0 structure from the CLB into local memory,
-     * and build a new command #0. */
-    memread(ahci->port[i].clb, &cmd, sizeof(cmd));
+    /* pick a command slot (should be 0!) */
+    cx = pick_cmd(ahci, i);
+
+    /* Construct our Command Header (set_command_header handles endianness.) */
+    memset(&cmd, 0x00, sizeof(cmd));
     cmd.b1 = 5;    /* reg_h2d_fis is 5 double-words long */
     cmd.b2 = 0x04; /* clear PxTFD.STS.BSY when done */
-    cmd.prdtl = cpu_to_le16(1); /* One PRD table entry. */
+    cmd.prdtl = 1; /* One PRD table entry. */
     cmd.prdbc = 0;
-    cmd.ctba = cpu_to_le32(table);
-    cmd.ctbau = 0;
+    cmd.ctba = table_ptr;
 
     /* Construct our PRD, noting that DBC is 0-indexed. */
-    prd.dba = cpu_to_le32(data_ptr);
-    prd.dbau = 0;
+    prd.dba = cpu_to_le64(data_ptr);
     prd.res = 0;
     /* 511+1 bytes, request DPS interrupt */
     prd.dbc = cpu_to_le32(511 | 0x80000000);
@@ -1354,19 +1429,20 @@ static void ahci_test_identify(AHCIState *ahci)
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
 
     /* Commit the Command FIS to the Command Table */
-    memwrite(table, &fis, sizeof(fis));
+    memwrite(table_ptr, &fis, sizeof(fis));
 
     /* Commit the PRD entry to the Command Table */
-    memwrite(table + 0x80, &prd, sizeof(prd));
+    memwrite(table_ptr + 0x80, &prd, sizeof(prd));
 
-    /* Commit Command #0, pointing to the Table, to the Command List Buffer. */
-    memwrite(ahci->port[i].clb, &cmd, sizeof(cmd));
+    /* Commit Command #cx, pointing to the Table, to the Command List Buffer. */
+    set_command_header(ahci, i, cx, &cmd);
 
-    /* Everything is in place, but we haven't given the go-ahead yet. */
+    /* Everything is in place, but we haven't given the go-ahead yet,
+     * so we should find that there are no pending interrupts yet. */
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
 
-    /* Issue Command #0 via PxCI */
-    PX_WREG(i, AHCI_PX_CI, (1 << 0));
+    /* Issue Command #cx via PxCI */
+    PX_WREG(i, AHCI_PX_CI, (1 << cx));
     while (BITSET(PX_RREG(i, AHCI_PX_TFD), AHCI_PX_TFD_STS_BSY)) {
         usleep(50);
     }
@@ -1389,9 +1465,9 @@ static void ahci_test_identify(AHCIState *ahci)
     ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_ERR);
     ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR);
 
-    /* Investigate CMD #0, assert that we read 512 bytes */
-    memread(ahci->port[i].clb, &cmd, sizeof(cmd));
-    g_assert_cmphex(512, ==, le32_to_cpu(cmd.prdbc));
+    /* Investigate the CMD, assert that we read 512 bytes */
+    get_command_header(ahci, i, cx, &cmd);
+    g_assert_cmphex(512, ==, cmd.prdbc);
 
     /* Investigate FIS responses */
     memread(ahci->port[i].fb + 0x20, pio, 0x20);
@@ -1408,7 +1484,7 @@ static void ahci_test_identify(AHCIState *ahci)
     /* The PIO Setup FIS contains a "bytes read" field, which is a
      * 16-bit value. The Physical Region Descriptor Byte Count is
      * 32-bit, but for small transfers using one PRD, it should match. */
-    g_assert_cmphex(le16_to_cpu(pio->res4), ==, le32_to_cpu(cmd.prdbc));
+    g_assert_cmphex(le16_to_cpu(pio->res4), ==, cmd.prdbc);
 
     /* Last, but not least: Investigate the IDENTIFY response data. */
     memread(data_ptr, &buff, 512);
-- 
1.9.3

[Qemu-devel] [PATCH 05/15] qtest/ahci: Add build cmd table helper

[John Snow]

build_cmd_table is a helper routine to build
the command table, which consists of the Command
FIS and the Physical Region Descriptor Table.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 7a62eef..f07495f 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1350,6 +1350,67 @@ static unsigned pick_cmd(AHCIState *ahci, uint8_t px)
     g_assert_not_reached();
 }
 
+static inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
+{
+    /* Each PRD can describe up to 4MiB */
+    g_assert_cmphex(bytes_per_prd, <=, 4096 * 1024);
+    g_assert_cmphex(bytes_per_prd & 0x01, ==, 0x00);
+    return (bytes + bytes_per_prd - 1) / bytes_per_prd;
+}
+
+static uint64_t build_cmd_table(AHCIState *ahci, uint8_t px, size_t sz,
+                                uint8_t cmd, uint64_t buffer)
+{
+    PRD prd;
+    RegH2DFIS fis;
+
+    const unsigned bytes_per_prd = 4096;
+    unsigned prdtl = size_to_prdtl(sz, bytes_per_prd);
+    unsigned table_size = CMD_TBL_SIZ(prdtl);
+    uint64_t table_ptr;
+    unsigned remaining, i;
+
+    /* Create a Command Table buffer. 0x80 for the bulk,
+     * plus enough room for `prdtl` entries. */
+    table_ptr = guest_alloc(guest_malloc, table_size);
+    g_assert(table_ptr);
+    /* AHCI 1.3 spec states that it must be 0x80 aligned. */
+    ASSERT_BIT_CLEAR(table_ptr, 0x7F);
+
+    /* Construct our Command FIS */
+    memset(&fis, 0x00, sizeof(fis));
+    fis.fis_type = 0x27; /* Register Host-to-Device FIS */
+    fis.flags = 0x80;    /* Command FIS. ('C bit')  */
+    fis.command = cmd;   /* e.g, 0xEC : Identify */
+    fis.count = cpu_to_le16(sz / 512);
+
+    /* Construct the PRDs. */
+    remaining = sz;
+    for (i = 0; i < prdtl; ++i) {
+        prd.dba = cpu_to_le64(buffer + (bytes_per_prd * i));
+        prd.res = 0;
+        if (remaining > bytes_per_prd) {
+            /* Note that byte count is 0-based. */
+            prd.dbc = cpu_to_le32(bytes_per_prd - 1);
+            remaining -= bytes_per_prd;
+        } else {
+            /* Again, dbc is 0-based. */
+            prd.dbc = cpu_to_le32(remaining - 1);
+            remaining = 0;
+        }
+        prd.dbc |= cpu_to_le32(0x80000000); /* Request DPS Interrupt */
+
+        /* Commit the PRD entry to the Command Table */
+        memwrite(table_ptr + 0x80 + (i * sizeof(PRD)),
+                 &prd, sizeof(PRD));
+    }
+
+    /* Commit the Command FIS to the Command Table */
+    memwrite(table_ptr, &fis, sizeof(fis));
+
+    return table_ptr;
+}
+
 /**
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
@@ -1358,9 +1419,7 @@ static void ahci_test_identify(AHCIState *ahci)
 {
     RegD2HFIS *d2h = g_malloc0(0x20);
     RegD2HFIS *pio = g_malloc0(0x20);
-    RegH2DFIS fis;
     AHCICommand cmd;
-    PRD prd;
     uint32_t reg, data_ptr;
     uint16_t buff[256];
     unsigned i;
@@ -1371,17 +1430,18 @@ static void ahci_test_identify(AHCIState *ahci)
     g_assert(ahci != NULL);
 
     /* We need to:
-     * (1) Create a Command Table Buffer and update the Command List Slot #0
-     *     to point to this buffer.
-     * (2) Construct an FIS host-to-device command structure, and write it to
-     *     the top of the command table buffer.
-     * (3) Create a data buffer for the IDENTIFY response to be sent to
+     * (1) Create a data buffer for the IDENTIFY response to be sent to,
+     * (2) Create a Command Table Buffer
+     * (3) Construct an FIS host-to-device command structure, and write it to
+     *     the top of the Command Table Buffer.
      * (4) Create a Physical Region Descriptor that points to the data buffer,
-     *     and write it to the bottom (offset 0x80) of the command table.
-     * (5) Now, PxCLB points to the command list, command 0 points to
+     *     and write it to the bottom (offset 0x80) of the Command Table.
+     * (5) Obtain a Command List slot, and update this header to point to
+     *     the Command Table we built above.
+     * (6) Now, PxCLB points to the command list, command 0 points to
      *     our table, and our table contains an FIS instruction and a
      *     PRD that points to our rx buffer.
-     * (6) We inform the HBA via PxCI that there is a command ready in slot #0.
+     * (7) We inform the HBA via PxCI that there is a command ready in slot #0.
      */
 
     /* Pick the first implemented and running port */
@@ -1391,16 +1451,13 @@ static void ahci_test_identify(AHCIState *ahci)
     /* Clear out the FIS Receive area and any pending interrupts. */
     port_clear(ahci, i);
 
-    /* Create a Command Table buffer. 0x80 is the smallest with a PRDTL of 0. */
-    /* We need at least one PRD, so round up to the nearest 0x80 multiple.    */
-    table_ptr = guest_alloc(guest_malloc, CMD_TBL_SIZ(1));
-    g_assert(table_ptr);
-    ASSERT_BIT_CLEAR(table_ptr, 0x7F);
-
-    /* Create a data buffer ... where we will dump the IDENTIFY data to. */
+    /* Create a data buffer where we will dump the IDENTIFY data to. */
     data_ptr = guest_alloc(guest_malloc, 512);
     g_assert(data_ptr);
 
+    /* Construct the Command Table (FIS and PRDT) */
+    table_ptr = build_cmd_table(ahci, i, 512, 0xEC, data_ptr);
+
     /* pick a command slot (should be 0!) */
     cx = pick_cmd(ahci, i);
 
@@ -1412,28 +1469,6 @@ static void ahci_test_identify(AHCIState *ahci)
     cmd.prdbc = 0;
     cmd.ctba = table_ptr;
 
-    /* Construct our PRD, noting that DBC is 0-indexed. */
-    prd.dba = cpu_to_le64(data_ptr);
-    prd.res = 0;
-    /* 511+1 bytes, request DPS interrupt */
-    prd.dbc = cpu_to_le32(511 | 0x80000000);
-
-    /* Construct our Command FIS, Based on http://wiki.osdev.org/AHCI */
-    memset(&fis, 0x00, sizeof(fis));
-    fis.fis_type = 0x27; /* Register Host-to-Device FIS */
-    fis.command = 0xEC;  /* IDENTIFY */
-    fis.device = 0;
-    fis.flags = 0x80;    /* Indicate this is a command FIS */
-
-    /* We've committed nothing yet, no interrupts should be posted yet. */
-    g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
-
-    /* Commit the Command FIS to the Command Table */
-    memwrite(table_ptr, &fis, sizeof(fis));
-
-    /* Commit the PRD entry to the Command Table */
-    memwrite(table_ptr + 0x80, &prd, sizeof(prd));
-
     /* Commit Command #cx, pointing to the Table, to the Command List Buffer. */
     set_command_header(ahci, i, cx, &cmd);
 
-- 
1.9.3

[Qemu-devel] [PATCH 06/15] qtest/ahci: Add link_cmd_slot helper

[John Snow]

link_cmd_slot creates the command header, by
setting a pointer to the command table as well as
adjusting other metadata, like DMA direction and
PRDT length.

It effectively links a command, via its table,
to the indicated command slot.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index f07495f..3e65435 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -252,11 +252,26 @@
 #define AHCI_VERSION_1_2         (0x00010200)
 #define AHCI_VERSION_1_3         (0x00010300)
 
+/* AHCI COMMAND HEADER FLAGS */
+
+#define CMDH_CFL        (0x1F)
+#define CMDH_ATAPI      (0x20)
+#define CMDH_READ       (0x00) /* Fake. */
+#define CMDH_WRITE      (0x40)
+#define CMDH_PREFETCH   (0x80)
+#define CMDH_RESET     (0x100)
+#define CMDH_BIST      (0x200)
+#define CMDH_CLR_BSY   (0x400)
+#define CMDH_RES       (0x800)
+#define CMDH_PMP      (0xF000)
+
 /*** Structures ***/
 
 typedef struct AHCIPortState {
     uint64_t fb;
     uint64_t clb;
+    uint64_t _ctba;
+    uint16_t _prdtl;
     uint64_t ctba[32];
     uint16_t prdtl[32];
     uint8_t next; /** Next Command Slot to Use **/
@@ -341,8 +356,7 @@ typedef struct RegH2DFIS {
  * The command list contains between 1-32 of these structures.
  */
 typedef struct AHCICommand {
-    uint8_t b1;
-    uint8_t b2;
+    uint16_t flags; /* Cmd-Fis-Len, PMP#, and flags. */
     uint16_t prdtl; /* Phys Region Desc. Table Length */
     uint32_t prdbc; /* Phys Region Desc. Byte Count */
     uint64_t ctba;  /* Command Table Descriptor Base Address */
@@ -1286,6 +1300,7 @@ static void get_command_header(AHCIState *ahci, uint8_t px,
     ba += cx * sizeof(AHCICommand);
     memread(ba, cmd, sizeof(AHCICommand));
 
+    cmd->flags = le16_to_cpu(cmd->flags);
     cmd->prdtl = le16_to_cpu(cmd->prdtl);
     cmd->prdbc = le32_to_cpu(cmd->prdbc);
     cmd->ctba = le64_to_cpu(cmd->ctba);
@@ -1298,6 +1313,7 @@ static void set_command_header(AHCIState *ahci, uint8_t px,
     uint64_t ba = ahci->port[px].clb;
     ba += cx * sizeof(AHCICommand);
 
+    cmd->flags = cpu_to_le16(cmd->flags);
     cmd->prdtl = cpu_to_le16(cmd->prdtl);
     cmd->prdbc = cpu_to_le32(cmd->prdbc);
     cmd->ctba = cpu_to_le64(cmd->ctba);
@@ -1408,10 +1424,47 @@ static uint64_t build_cmd_table(AHCIState *ahci, uint8_t px, size_t sz,
     /* Commit the Command FIS to the Command Table */
     memwrite(table_ptr, &fis, sizeof(fis));
 
+    /* Bookmark these values. */
+    ahci->port[px]._prdtl = prdtl;
+    ahci->port[px]._ctba = table_ptr;
+
     return table_ptr;
 }
 
 /**
+ * Given a pointer to a Command Table, create a Command Header for it and write
+ * it to a free slot in the Command List buffer.
+ * @return The command slot number we picked.
+ */
+static unsigned link_cmd_slot(AHCIState *ahci, uint8_t px, uint64_t table_ptr,
+                              uint8_t cmdh_flags)
+{
+    AHCICommand cmd = { .flags = 0 };
+    uint8_t cx;
+
+    cx = pick_cmd(ahci, px);
+
+    /* Construct our Command Header */
+    cmd.flags = 5;
+    cmd.flags |= CMDH_CLR_BSY;
+    cmd.flags |= cmdh_flags;
+    cmd.prdtl = (table_ptr == ahci->port[px]._ctba) ? ahci->port[px]._prdtl : 1;
+    cmd.prdbc = 0;
+    cmd.ctba = table_ptr;
+
+    /* Commit Command #cx, pointing to the Table, to the Command List Buffer. */
+    set_command_header(ahci, px, cx, &cmd);
+
+    /* For convenience only. */
+    ahci->port[px].ctba[cx] = table_ptr;
+    ahci->port[px].prdtl[cx] = cmd.prdtl;
+    ahci->port[px]._ctba = 0;
+    ahci->port[px]._prdtl = 0;
+
+    return cx;
+}
+
+/**
  * Utilizing an initialized AHCI HBA, issue an IDENTIFY command to the first
  * device we see, then read and check the response.
  */
@@ -1458,19 +1511,8 @@ static void ahci_test_identify(AHCIState *ahci)
     /* Construct the Command Table (FIS and PRDT) */
     table_ptr = build_cmd_table(ahci, i, 512, 0xEC, data_ptr);
 
-    /* pick a command slot (should be 0!) */
-    cx = pick_cmd(ahci, i);
-
-    /* Construct our Command Header (set_command_header handles endianness.) */
-    memset(&cmd, 0x00, sizeof(cmd));
-    cmd.b1 = 5;    /* reg_h2d_fis is 5 double-words long */
-    cmd.b2 = 0x04; /* clear PxTFD.STS.BSY when done */
-    cmd.prdtl = 1; /* One PRD table entry. */
-    cmd.prdbc = 0;
-    cmd.ctba = table_ptr;
-
-    /* Commit Command #cx, pointing to the Table, to the Command List Buffer. */
-    set_command_header(ahci, i, cx, &cmd);
+    /* Pick a command slot and link it to the command table we've built */
+    cx = link_cmd_slot(ahci, i, table_ptr, CMDH_READ);
 
     /* Everything is in place, but we haven't given the go-ahead yet,
      * so we should find that there are no pending interrupts yet. */
-- 
1.9.3

[Qemu-devel] [PATCH 07/15] qtest/ahci: Add port_check_error helper

[John Snow]

port_check-error checks a given port's error registers
and asserts that everything from the port-level view
is still OK.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 3e65435..fcf5ffb 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1292,6 +1292,28 @@ static void port_clear(AHCIState *ahci, uint8_t px)
     qmemset(ahci->port[px].fb, 0x00, 0x100);
 }
 
+/**
+ * Check a port for errors.
+ */
+static void port_check_error(AHCIState *ahci, uint8_t px)
+{
+    uint32_t reg;
+
+    /* The upper 9 bits of the IS register all indicate errors. */
+    reg = PX_RREG(px, AHCI_PX_IS);
+    reg >>= 23;
+    g_assert_cmphex(reg, ==, 0);
+
+    /* The Sata Error Register should be empty. */
+    reg = PX_RREG(px, AHCI_PX_SERR);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* The TFD also has two error sections. */
+    reg = PX_RREG(px, AHCI_PX_TFD);
+    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_ERR);
+    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR);
+}
+
 /* Get the #cx'th command of port #px. */
 static void get_command_header(AHCIState *ahci, uint8_t px,
                                uint8_t cx, AHCICommand *cmd)
@@ -1523,6 +1545,7 @@ static void ahci_test_identify(AHCIState *ahci)
     while (BITSET(PX_RREG(i, AHCI_PX_TFD), AHCI_PX_TFD_STS_BSY)) {
         usleep(50);
     }
+    port_check_error(ahci, i);
 
     /* Check for expected interrupts */
     reg = PX_RREG(i, AHCI_PX_IS);
@@ -1535,13 +1558,6 @@ static void ahci_test_identify(AHCIState *ahci)
     PX_WREG(i, AHCI_PX_IS, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS | AHCI_PX_IS_DPS);
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
 
-    /* Check for errors. */
-    reg = PX_RREG(i, AHCI_PX_SERR);
-    g_assert_cmphex(reg, ==, 0);
-    reg = PX_RREG(i, AHCI_PX_TFD);
-    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_ERR);
-    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR);
-
     /* Investigate the CMD, assert that we read 512 bytes */
     get_command_header(ahci, i, cx, &cmd);
     g_assert_cmphex(512, ==, cmd.prdbc);
-- 
1.9.3

[Qemu-devel] [PATCH 08/15] qtest/ahci: Add issue_command helper

[John Snow]

issue_command is a small routine that informs the
AHCI device that we have finalized our command and
we'd wish for the device to begin processing it.

issue_command polls the AHCI device waiting for
the device to finish, in order to achieve a
synchronous effect.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index fcf5ffb..4615681 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1343,6 +1343,26 @@ static void set_command_header(AHCIState *ahci, uint8_t px,
     memwrite(ba, cmd, sizeof(AHCICommand));
 }
 
+/**
+ * Issue Command #cx via PxCI and, if ncq is true, PxSACT
+ */
+static void issue_command(AHCIState *ahci, uint8_t px,
+                          uint8_t cx, bool ncq)
+{
+    if (ncq) {
+        PX_WREG(px, AHCI_PX_SACT, (1 << cx));
+    }
+
+    PX_WREG(px, AHCI_PX_CI, (1 << cx));
+    /* 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(PX_RREG(px, AHCI_PX_TFD), AHCI_PX_TFD_STS_BSY) ||
+           BITSET(PX_RREG(px, AHCI_PX_CI), (1 << cx))) {
+        usleep(50);
+    }
+}
+
 static void destroy_command(AHCIState *ahci, uint8_t px, uint8_t cx)
 {
     AHCICommand cmd;
@@ -1541,10 +1561,7 @@ static void ahci_test_identify(AHCIState *ahci)
     g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
 
     /* Issue Command #cx via PxCI */
-    PX_WREG(i, AHCI_PX_CI, (1 << cx));
-    while (BITSET(PX_RREG(i, AHCI_PX_TFD), AHCI_PX_TFD_STS_BSY)) {
-        usleep(50);
-    }
+    issue_command(ahci, i, cx, 0);
     port_check_error(ahci, i);
 
     /* Check for expected interrupts */
-- 
1.9.3

[Qemu-devel] [PATCH 09/15] qtest/ahci: Add port_check_interrupts helper

[John Snow]

A helper that compares a given port's current
interrupts and checks them against a supplied
list of expected interrupt bits, and throws
an error if they do not match.

The helper then resets the requested interrupts
on this port, and asserts that the interrupt
register is now empty.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 4615681..315e6e9 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1314,6 +1314,20 @@ static void port_check_error(AHCIState *ahci, uint8_t px)
     ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_ERR);
 }
 
+static void port_check_interrupts(AHCIState *ahci, uint8_t px,
+                                  uint32_t intr_mask)
+{
+    uint32_t reg;
+
+    /* Check for expected interrupts */
+    reg = PX_RREG(px, AHCI_PX_IS);
+    ASSERT_BIT_SET(reg, intr_mask);
+
+    /* Clear expected interrupts and assert all interrupts now cleared. */
+    PX_WREG(px, AHCI_PX_IS, intr_mask);
+    g_assert_cmphex(PX_RREG(px, AHCI_PX_IS), ==, 0);
+}
+
 /* Get the #cx'th command of port #px. */
 static void get_command_header(AHCIState *ahci, uint8_t px,
                                uint8_t cx, AHCICommand *cmd)
@@ -1562,18 +1576,10 @@ static void ahci_test_identify(AHCIState *ahci)
 
     /* Issue Command #cx via PxCI */
     issue_command(ahci, i, cx, 0);
+    /* Check registers for post-command consistency */
     port_check_error(ahci, i);
-
-    /* Check for expected interrupts */
-    reg = PX_RREG(i, AHCI_PX_IS);
-    ASSERT_BIT_SET(reg, AHCI_PX_IS_DHRS);
-    ASSERT_BIT_SET(reg, AHCI_PX_IS_PSS);
     /* BUG: we expect AHCI_PX_IS_DPS to be set. */
-    ASSERT_BIT_CLEAR(reg, AHCI_PX_IS_DPS);
-
-    /* Clear expected interrupts and assert all interrupts now cleared. */
-    PX_WREG(i, AHCI_PX_IS, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS | AHCI_PX_IS_DPS);
-    g_assert_cmphex(PX_RREG(i, AHCI_PX_IS), ==, 0);
+    port_check_interrupts(ahci, i, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS);
 
     /* Investigate the CMD, assert that we read 512 bytes */
     get_command_header(ahci, i, cx, &cmd);
-- 
1.9.3

[Qemu-devel] [PATCH 10/15] qtest/ahci: Add port_check_nonbusy helper

[John Snow]

A simple helper that asserts a given port is
not busy processing any commands via the
TFD, Command Issue and SACT registers.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 315e6e9..b7bfd86 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1328,6 +1328,24 @@ static void port_check_interrupts(AHCIState *ahci, uint8_t px,
     g_assert_cmphex(PX_RREG(px, AHCI_PX_IS), ==, 0);
 }
 
+static void port_check_nonbusy(AHCIState *ahci, uint8_t px, uint8_t cx)
+{
+    uint32_t reg;
+
+    /* Assert that the command slot is no longer busy (NCQ) */
+    reg = PX_RREG(px, AHCI_PX_SACT);
+    ASSERT_BIT_CLEAR(reg, (1 << cx));
+
+    /* Non-NCQ */
+    reg = PX_RREG(px, AHCI_PX_CI);
+    ASSERT_BIT_CLEAR(reg, (1 << cx));
+
+    /* And assert that we are generally not busy. */
+    reg = PX_RREG(px, AHCI_PX_TFD);
+    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_BSY);
+    ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_DRQ);
+}
+
 /* Get the #cx'th command of port #px. */
 static void get_command_header(AHCIState *ahci, uint8_t px,
                                uint8_t cx, AHCICommand *cmd)
@@ -1580,6 +1598,7 @@ static void ahci_test_identify(AHCIState *ahci)
     port_check_error(ahci, i);
     /* BUG: we expect AHCI_PX_IS_DPS to be set. */
     port_check_interrupts(ahci, i, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS);
+    port_check_nonbusy(ahci, i, cx);
 
     /* Investigate the CMD, assert that we read 512 bytes */
     get_command_header(ahci, i, cx, &cmd);
-- 
1.9.3

[Qemu-devel] [PATCH 11/15] qtest/ahci: Add cmd response sanity check helpers

[John Snow]

Adds a few helpers to help sanity-check the response of the
AHCI device after a command.

d2h_check_sanity inspects the D2H Register FIS,
pio_check_sanity inspects the PIO Setup FIS,
and cmd_check_sanity inspects the command header.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index b7bfd86..dcc0748 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -306,25 +306,44 @@ typedef struct RegD2HFIS {
     uint8_t status;
     uint8_t error;
     /* DW1 */
-    uint8_t lba_low;
-    uint8_t lba_mid;
-    uint8_t lba_high;
+    uint8_t lba_lo[3];
     uint8_t device;
     /* DW2 */
-    uint8_t lba3;
-    uint8_t lba4;
-    uint8_t lba5;
-    uint8_t res1;
+    uint8_t lba_hi[3];
+    uint8_t res0;
     /* DW3 */
     uint16_t count;
-    uint8_t res2;
-    uint8_t res3;
+    uint16_t res1;
     /* DW4 */
-    uint16_t res4;
-    uint16_t res5;
+    uint32_t res2;
 } __attribute__((__packed__)) RegD2HFIS;
 
 /**
+ * Register device-to-host FIS structure;
+ * PIO Setup variety.
+ */
+typedef struct PIOSetupFIS {
+    /* DW0 */
+    uint8_t fis_type;
+    uint8_t flags;
+    uint8_t status;
+    uint8_t error;
+    /* DW1 */
+    uint8_t lba_lo[3];
+    uint8_t device;
+    /* DW2 */
+    uint8_t lba_hi[3];
+    uint8_t res0;
+    /* DW3 */
+    uint16_t count;
+    uint8_t res1;
+    uint8_t e_status;
+    /* DW4 */
+    uint16_t tx_count;
+    uint16_t res2;
+} __attribute__((__packed__)) PIOSetupFIS;
+
+/**
  * Register host-to-device FIS structure.
  */
 typedef struct RegH2DFIS {
@@ -334,14 +353,10 @@ typedef struct RegH2DFIS {
     uint8_t command;
     uint8_t feature_low;
     /* DW1 */
-    uint8_t lba_low;
-    uint8_t lba_mid;
-    uint8_t lba_high;
+    uint8_t lba_lo[3];
     uint8_t device;
     /* DW2 */
-    uint8_t lba3;
-    uint8_t lba4;
-    uint8_t lba5;
+    uint8_t lba_hi[3];
     uint8_t feature_high;
     /* DW3 */
     uint16_t count;
@@ -419,6 +434,8 @@ static void ahci_test_pci_caps(AHCIState *ahci, uint16_t header,
 static void ahci_test_satacap(AHCIState *ahci, uint8_t offset);
 static void ahci_test_msicap(AHCIState *ahci, uint8_t offset);
 static void ahci_test_pmcap(AHCIState *ahci, uint8_t offset);
+static void get_command_header(AHCIState *ahci, uint8_t px,
+                               uint8_t cx, AHCICommand *cmd);
 
 /*** Utilities ***/
 
@@ -1346,6 +1363,48 @@ static void port_check_nonbusy(AHCIState *ahci, uint8_t px, uint8_t cx)
     ASSERT_BIT_CLEAR(reg, AHCI_PX_TFD_STS_DRQ);
 }
 
+static void port_check_d2h_sanity(AHCIState *ahci, uint8_t px, uint8_t cx)
+{
+    RegD2HFIS *d2h = g_malloc0(0x20);
+    uint32_t reg;
+
+    memread(ahci->port[px].fb + 0x40, d2h, 0x20);
+    g_assert_cmphex(d2h->fis_type, ==, 0x34);
+
+    reg = PX_RREG(px, AHCI_PX_TFD);
+    g_assert_cmphex((reg & AHCI_PX_TFD_ERR), ==, d2h->error);
+    g_assert_cmphex((reg & AHCI_PX_TFD_STS), ==, d2h->status);
+
+    g_free(d2h);
+}
+
+static void port_check_pio_sanity(AHCIState *ahci, uint8_t px,
+                                  uint8_t cx, size_t buffsize)
+{
+    PIOSetupFIS *pio = g_malloc0(0x20);
+    uint32_t reg;
+
+    memread(ahci->port[px].fb + 0x20, pio, 0x20);
+    g_assert_cmphex(pio->fis_type, ==, 0x5f);
+
+    reg = PX_RREG(px, AHCI_PX_TFD);
+    g_assert_cmphex((reg & AHCI_PX_TFD_ERR), ==, pio->error);
+    g_assert_cmphex((reg & AHCI_PX_TFD_STS), ==, pio->status);
+
+    g_assert_cmphex(le16_to_cpu(pio->tx_count), ==, buffsize);
+
+    g_free(pio);
+}
+
+static void port_check_cmd_sanity(AHCIState *ahci, uint8_t px,
+                                  uint8_t cx, size_t buffsize)
+{
+    AHCICommand cmd;
+
+    get_command_header(ahci, px, cx, &cmd);
+    g_assert_cmphex(buffsize, ==, cmd.prdbc);
+}
+
 /* Get the #cx'th command of port #px. */
 static void get_command_header(AHCIState *ahci, uint8_t px,
                                uint8_t cx, AHCICommand *cmd)
@@ -1544,10 +1603,7 @@ static unsigned link_cmd_slot(AHCIState *ahci, uint8_t px, uint64_t table_ptr,
  */
 static void ahci_test_identify(AHCIState *ahci)
 {
-    RegD2HFIS *d2h = g_malloc0(0x20);
-    RegD2HFIS *pio = g_malloc0(0x20);
-    AHCICommand cmd;
-    uint32_t reg, data_ptr;
+    uint32_t data_ptr;
     uint16_t buff[256];
     unsigned i;
     int rc;
@@ -1599,27 +1655,11 @@ static void ahci_test_identify(AHCIState *ahci)
     /* BUG: we expect AHCI_PX_IS_DPS to be set. */
     port_check_interrupts(ahci, i, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS);
     port_check_nonbusy(ahci, i, cx);
-
     /* Investigate the CMD, assert that we read 512 bytes */
-    get_command_header(ahci, i, cx, &cmd);
-    g_assert_cmphex(512, ==, cmd.prdbc);
-
+    port_check_cmd_sanity(ahci, i, cx, 512);
     /* Investigate FIS responses */
-    memread(ahci->port[i].fb + 0x20, pio, 0x20);
-    memread(ahci->port[i].fb + 0x40, d2h, 0x20);
-    g_assert_cmphex(pio->fis_type, ==, 0x5f);
-    g_assert_cmphex(d2h->fis_type, ==, 0x34);
-    g_assert_cmphex(pio->flags, ==, d2h->flags);
-    g_assert_cmphex(pio->status, ==, d2h->status);
-    g_assert_cmphex(pio->error, ==, d2h->error);
-
-    reg = PX_RREG(i, AHCI_PX_TFD);
-    g_assert_cmphex((reg & AHCI_PX_TFD_ERR), ==, pio->error);
-    g_assert_cmphex((reg & AHCI_PX_TFD_STS), ==, pio->status);
-    /* The PIO Setup FIS contains a "bytes read" field, which is a
-     * 16-bit value. The Physical Region Descriptor Byte Count is
-     * 32-bit, but for small transfers using one PRD, it should match. */
-    g_assert_cmphex(le16_to_cpu(pio->res4), ==, cmd.prdbc);
+    port_check_d2h_sanity(ahci, i, cx);
+    port_check_pio_sanity(ahci, i, cx, 512);
 
     /* Last, but not least: Investigate the IDENTIFY response data. */
     memread(data_ptr, &buff, 512);
@@ -1636,9 +1676,6 @@ static void ahci_test_identify(AHCIState *ahci)
     string_bswap16(&buff[23], 8);
     rc = memcmp(&buff[23], "version ", 8);
     g_assert_cmphex(rc, ==, 0);
-
-    g_free(d2h);
-    g_free(pio);
 }
 
 /******************************************************************************/
-- 
1.9.3

[Qemu-devel] [PATCH 12/15] qtest/ahci: Enforce zero-leaks for guest mem usage

[John Snow]

The purpose of this patch is to aid in debugging the new
malloc-pc interface for guest memory, by enabling the
enforcing of no guest memory blocks in use at exit time.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index dcc0748..3f92618 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -436,6 +436,7 @@ static void ahci_test_msicap(AHCIState *ahci, uint8_t offset);
 static void ahci_test_pmcap(AHCIState *ahci, uint8_t offset);
 static void get_command_header(AHCIState *ahci, uint8_t px,
                                uint8_t cx, AHCICommand *cmd);
+static void destroy_command(AHCIState *ahci, uint8_t px, uint8_t cx);
 
 /*** Utilities ***/
 
@@ -506,7 +507,9 @@ static void qtest_boot(const char *cmdline_fmt, ...)
 
     qtest_start(cmdline);
     qtest_irq_intercept_in(global_qtest, "ioapic");
-    guest_malloc = pc_alloc_init();
+    /* Enforce 0 leaks. */
+    guest_malloc = pc_alloc_init_flags(PC_ALLOC_LEAK_ASSERT |
+                                       PC_ALLOC_PARANOID);
 
     g_free(cmdline);
 }
@@ -516,8 +519,10 @@ static void qtest_boot(const char *cmdline_fmt, ...)
  */
 static void qtest_shutdown(void)
 {
-    g_free(guest_malloc);
-    guest_malloc = NULL;
+    if (guest_malloc) {
+        pc_alloc_uninit(guest_malloc);
+        guest_malloc = NULL;
+    }
     qtest_end();
 }
 
@@ -547,6 +552,20 @@ static AHCIState *ahci_boot(void)
  */
 static void ahci_shutdown(AHCIState *ahci)
 {
+    uint8_t px, cx;
+
+    for (px = 0; px < 32; ++px) {
+        if (ahci->port[px].fb) {
+            guest_free(guest_malloc, ahci->port[px].fb);
+        }
+        if (ahci->port[px].clb) {
+            for (cx = 0; cx < 32; cx++) {
+                destroy_command(ahci, px, cx);
+            }
+            guest_free(guest_malloc, ahci->port[px].clb);
+        }
+    }
+
     free_ahci_device(ahci->dev);
     g_free(ahci);
     qtest_shutdown();
@@ -1676,6 +1695,8 @@ static void ahci_test_identify(AHCIState *ahci)
     string_bswap16(&buff[23], 8);
     rc = memcmp(&buff[23], "version ", 8);
     g_assert_cmphex(rc, ==, 0);
+
+    guest_free(guest_malloc, data_ptr);
 }
 
 /******************************************************************************/
-- 
1.9.3

[Qemu-devel] [PATCH 13/15] qtest/ahci: Add a macro bootup routine

[John Snow]

Add a routine that can be used to engage the AHCI
device at a not-granular level so that bringing up
the functionality of the HBA is easy in future tests
that are not concerned with testing the bring-up process.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 3f92618..63fc309 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -739,6 +739,21 @@ static void ahci_hba_enable(AHCIState *ahci)
      * and clear the initial interrupts might be good. */
 }
 
+/**
+ * Boot and fully enable the HBA device.
+ * @see ahci_boot, ahci_pci_enable and ahci_hba_enable.
+ */
+static AHCIState *ahci_macro_bootup(void)
+{
+    AHCIState *ahci;
+    ahci = ahci_boot();
+
+    ahci_pci_enable(ahci);
+    ahci_hba_enable(ahci);
+
+    return ahci;
+}
+
 /*** Specification Adherence Tests ***/
 
 /**
@@ -1773,9 +1788,7 @@ static void test_identify(void)
 {
     AHCIState *ahci;
 
-    ahci = ahci_boot();
-    ahci_pci_enable(ahci);
-    ahci_hba_enable(ahci);
+    ahci = ahci_macro_bootup();
     ahci_test_identify(ahci);
     ahci_shutdown(ahci);
 }
-- 
1.9.3

[Qemu-devel] [PATCH 14/15] qtest/ahci: Add human-readable command names

[John Snow]

Instead of using direct hex values, use SATA command mnemonics.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 63fc309..e3091b9 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -265,6 +265,14 @@
 #define CMDH_RES       (0x800)
 #define CMDH_PMP      (0xF000)
 
+/* ATA Commands */
+enum {
+    CMD_READ_DMA    = 0xC8,
+    CMD_WRITE_DMA   = 0xCA,
+    CMD_FLUSH_CACHE = 0xE7,
+    CMD_IDENTIFY    = 0xEC
+};
+
 /*** Structures ***/
 
 typedef struct AHCIPortState {
@@ -1673,7 +1681,7 @@ static void ahci_test_identify(AHCIState *ahci)
     g_assert(data_ptr);
 
     /* Construct the Command Table (FIS and PRDT) */
-    table_ptr = build_cmd_table(ahci, i, 512, 0xEC, data_ptr);
+    table_ptr = build_cmd_table(ahci, i, 512, CMD_IDENTIFY, data_ptr);
 
     /* Pick a command slot and link it to the command table we've built */
     cx = link_cmd_slot(ahci, i, table_ptr, CMDH_READ);
-- 
1.9.3

[Qemu-devel] [PATCH 15/15] qtest/ahci: Don't use a magic constant for buffer size

[John Snow]

Fix to correct the use of "512" as a magic constant and define
it as the buffer size for the operation instead.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index e3091b9..9d16d0c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1651,6 +1651,7 @@ static void ahci_test_identify(AHCIState *ahci)
     int rc;
     uint8_t cx;
     uint64_t table_ptr;
+    const size_t buffsize = 512;
 
     g_assert(ahci != NULL);
 
@@ -1677,11 +1678,11 @@ static void ahci_test_identify(AHCIState *ahci)
     port_clear(ahci, i);
 
     /* Create a data buffer where we will dump the IDENTIFY data to. */
-    data_ptr = guest_alloc(guest_malloc, 512);
+    data_ptr = guest_alloc(guest_malloc, buffsize);
     g_assert(data_ptr);
 
     /* Construct the Command Table (FIS and PRDT) */
-    table_ptr = build_cmd_table(ahci, i, 512, CMD_IDENTIFY, data_ptr);
+    table_ptr = build_cmd_table(ahci, i, buffsize, CMD_IDENTIFY, data_ptr);
 
     /* Pick a command slot and link it to the command table we've built */
     cx = link_cmd_slot(ahci, i, table_ptr, CMDH_READ);
@@ -1698,13 +1699,13 @@ static void ahci_test_identify(AHCIState *ahci)
     port_check_interrupts(ahci, i, AHCI_PX_IS_DHRS | AHCI_PX_IS_PSS);
     port_check_nonbusy(ahci, i, cx);
     /* Investigate the CMD, assert that we read 512 bytes */
-    port_check_cmd_sanity(ahci, i, cx, 512);
+    port_check_cmd_sanity(ahci, i, cx, buffsize);
     /* Investigate FIS responses */
     port_check_d2h_sanity(ahci, i, cx);
-    port_check_pio_sanity(ahci, i, cx, 512);
+    port_check_pio_sanity(ahci, i, cx, buffsize);
 
     /* Last, but not least: Investigate the IDENTIFY response data. */
-    memread(data_ptr, &buff, 512);
+    memread(data_ptr, &buff, buffsize);
 
     /* Check serial number/version in the buffer */
     /* NB: IDENTIFY strings are packed in 16bit little endian chunks.
-- 
1.9.3

Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The original version of the AHCI test base
> which is now staged for being merged, processes
> the ahci_identify test in a monolithic fashion.
>
> In authoring new tests, it became necessary and
> obvious as to how the operation of this device
> should be factored out to ease the writing of
> new AHCI tests.
>
> This patch set issues the necessary refactorings
> to support future test development for AHCI.
>
> This patch set DOES NOT account for any new fixes
> and requires no fixes from my "AHCI fixes" RFC
> in order to run successfully on 2014-09-18's
> origin/master.
>
> This patch set does not alter the operation of the
> existing test, or add new tests. It only offers
> refactorings for future patch submissions which
> depend on them, but are still under consideration.
[...]
>  tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 583 insertions(+), 277 deletions(-)

Ignorant question: why should we commit the "monolithic" test only to
refactor it extensively right away?

Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[Stefan Hajnoczi]

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

On Fri, Sep 19, 2014 at 12:53:22PM +0200, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
> > The original version of the AHCI test base
> > which is now staged for being merged, processes
> > the ahci_identify test in a monolithic fashion.
> >
> > In authoring new tests, it became necessary and
> > obvious as to how the operation of this device
> > should be factored out to ease the writing of
> > new AHCI tests.
> >
> > This patch set issues the necessary refactorings
> > to support future test development for AHCI.
> >
> > This patch set DOES NOT account for any new fixes
> > and requires no fixes from my "AHCI fixes" RFC
> > in order to run successfully on 2014-09-18's
> > origin/master.
> >
> > This patch set does not alter the operation of the
> > existing test, or add new tests. It only offers
> > refactorings for future patch submissions which
> > depend on them, but are still under consideration.
> [...]
> >  tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 583 insertions(+), 277 deletions(-)
> 
> Ignorant question: why should we commit the "monolithic" test only to
> refactor it extensively right away?

The patches merged in the block tree have been fully reviewed and
tested.  It took a long time to reach that state.

I don't want to go back to square one and have to re-review it all.
Refactoring is mechanical and therefore easy to review.

The earliest we can merge this new series is next week.  Let's not try
to make it perfect if that means building up a monster series over many
weeks.  Let's merge incrementally and keep moving.

(I do believe that clean commit history is important and patches should
be polished, but in this case work spans too long to keep redoing it
all.)

Stefan

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

Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[John Snow]

On 09/19/2014 06:53 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> The original version of the AHCI test base
>> which is now staged for being merged, processes
>> the ahci_identify test in a monolithic fashion.
>>
>> In authoring new tests, it became necessary and
>> obvious as to how the operation of this device
>> should be factored out to ease the writing of
>> new AHCI tests.
>>
>> This patch set issues the necessary refactorings
>> to support future test development for AHCI.
>>
>> This patch set DOES NOT account for any new fixes
>> and requires no fixes from my "AHCI fixes" RFC
>> in order to run successfully on 2014-09-18's
>> origin/master.
>>
>> This patch set does not alter the operation of the
>> existing test, or add new tests. It only offers
>> refactorings for future patch submissions which
>> depend on them, but are still under consideration.
> [...]
>>   tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 583 insertions(+), 277 deletions(-)
>
> Ignorant question: why should we commit the "monolithic" test only to
> refactor it extensively right away?
>

Newbie problems, basically.

I submitted the first AHCI test series 2014-07-07. The tests were staged 
last week. A lot happened on my downstream copy in that time, but I 
opted to keep the series "the same scope" from v1 through v4, because 
otherwise the series would have just kept growing ... I submitted v4

I do apologize at how absurd it is, on your end, to see an introduction 
followed immediately by a refactor. It seemed like the idea at the time 
was to get a basis that people agreed upon, then modify as needed.

Sorry for the confusion.

-- 
—js

Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[John Snow]

PING for this refactor series, which will allow me to check in more AHCI 
tests. To assist review, I'll annotate this cover letter a little bit 
with some additional information.

The hardest/ugliest patch to review is the first one, but it is 
primarily just variable name/location changes and nothing substantial.

The subsequent patches are all fairly small in scope and mostly 
mechanical refactors, but do include some enhanced functionality beyond 
the scope of what the identify test requires, but will be useful later 
for additional tests.

The primary purpose of this series is to break up the monolithic 
identify test into smaller parts that allow it to be easily recycled for 
testing other ATA commands in the future.

(And by "in the future" I mean "I have a lot of test cases on my hard 
drive that I'd like to submit, but they use these re-factored helper 
commands.")

On 09/18/2014 07:43 PM, John Snow wrote:
> The original version of the AHCI test base
> which is now staged for being merged, processes
> the ahci_identify test in a monolithic fashion.
>
> In authoring new tests, it became necessary and
> obvious as to how the operation of this device
> should be factored out to ease the writing of
> new AHCI tests.
>
> This patch set issues the necessary refactorings
> to support future test development for AHCI.
>
> This patch set DOES NOT account for any new fixes
> and requires no fixes from my "AHCI fixes" RFC
> in order to run successfully on 2014-09-18's
> origin/master.
>
> This patch set does not alter the operation of the
> existing test, or add new tests. It only offers
> refactorings for future patch submissions which
> depend on them, but are still under consideration.
>
> John Snow (15):
>    qtest/ahci: Add AHCIState structure

This patch adds a state structure that the test uses to track various 
fields and information within the AHCI device during the run of the 
test, so we don't have to keep re-querying and re-verifying the sanity 
of various pointer fields stored in guest memory.

Capabilities, pointers, and other metadata are now stored within this 
AHCIState object.

Many function declarations are changed from accepting a QPCIDevice 
and/or a pointer to the HBA configuration space to just accepting a 
pointer to the new state object. Many macros are changed to operate on 
this object.

It's mostly just a lot of churn without much functional difference, but 
it keeps the code in future tests a little nicer, and the object is 
managed in a functional way in order to prevent state differences from 
impacting the results of subsequent test runs.

>    qtest/ahci: Add port_select helper

Factor out the loop that finds the first used port.

>    qtest/ahci: Add port_clear helper

Factor out the code that resets a port's state back to a known value.

>    qtest/ahci: Add command header helpers

-Compress 32bit upper/lower fields into single 64bit address fields.
-Add get_command_header, set_command_header and destroy_command as 
helpers to help retrieve the command information stored within the 
command list buffer (CLB)
-Add pick_cmd which picks a command slot (0-31) to use for a new command 
being built. pick_cmd attempts to cycle through all of the command slots 
instead of re-using a previous slot, to mimic real AHCI behavior.

>    qtest/ahci: Add build cmd table helper

build_cmd_table is added as a helper that builds the actual command that 
is pointed to by the command header structure -- the CTBA (Command Table 
Base Address.) Much of the logic of building the command table is ripped 
out of the identify test case and isolated within this helper.

>    qtest/ahci: Add link_cmd_slot helper

Generate a Command Header for use in the CLB from a given Table pointer, 
then commit the new header to the CLB. This command "links" the command 
list and the command table. The chosen command slot is returned.

>    qtest/ahci: Add port_check_error helper

Common error flags in the registers and FIS are checked in a separate 
function that can now be recycled.

>    qtest/ahci: Add issue_command helper

The code responsible for actually issuing a command once it is in-place 
is factored out into its own function.

>    qtest/ahci: Add port_check_interrupts helper

This helper is a shorthand for confirming that the interrupt status of a 
specific port is what we expect it to be; then clears the interrupt.

>    qtest/ahci: Add port_check_nonbusy helper

Simply asserts that the state of the port is consistent with what we 
expect a port who is ready to receive another command would be. This is 
an addition instead of a refactor.

>    qtest/ahci: Add cmd response sanity check helpers

- Fields in the FIS structure are renamed to be a little clearer.
- A PIO Setup FIS structure is added.
- Port_Check_D2H_Sanity inspects a D2H Register FIS from the device and 
makes sure it looks appropriate.
- Port_Check_PIO_Sanity does the same for a PIO Setup FIS.
- Port_Check_CMD_Sanity primarily checks the byte count response that is 
stored within the command header to ensure it matches what we think it 
should be (the full size of the transfer.)


All of the patches below this point are even smaller "niceness" fixes 
that don't really refactor anything at all:


>    qtest/ahci: Enforce zero-leaks for guest mem usage

Utilize extra flags to the PC Alloc layer in libqos to add an assertion 
if we leak any memory. This is mostly for debugging niceness to prove 
that we are not dropping any guest memory in the various linked AHCI 
structures. It also served as a POC for the new linked-list malloc 
implementation in libqos.

>    qtest/ahci: Add a macro bootup routine

For future use: Now that we have tests that establish the subcomponents 
of an AHCI bootup are sane, add a command that will all-at-once bring 
the AHCI device up to functional status for us. This may be useful for 
other tests in the future, as well, that would like to run various tests 
while using the AHCI controller.

>    qtest/ahci: Add human-readable command names

Mostly for future use: add some ATA command mnemonic mappings instead of 
using the raw hex codes.

>    qtest/ahci: Don't use a magic constant for buffer size

Small niceness: Get rid of the magic constant '512' for the IDENTIFY 
buffer size test routine.

>
>   tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 583 insertions(+), 277 deletions(-)
>

Re: [Qemu-devel] [PATCH 00/15] AHCI test helper refactors

[John Snow]

On 09/18/2014 07:43 PM, John Snow wrote:
> The original version of the AHCI test base
> which is now staged for being merged, processes
> the ahci_identify test in a monolithic fashion.
>
> In authoring new tests, it became necessary and
> obvious as to how the operation of this device
> should be factored out to ease the writing of
> new AHCI tests.
>
> This patch set issues the necessary refactorings
> to support future test development for AHCI.
>
> This patch set DOES NOT account for any new fixes
> and requires no fixes from my "AHCI fixes" RFC
> in order to run successfully on 2014-09-18's
> origin/master.
>
> This patch set does not alter the operation of the
> existing test, or add new tests. It only offers
> refactorings for future patch submissions which
> depend on them, but are still under consideration.
>
> John Snow (15):
>    qtest/ahci: Add AHCIState structure
>    qtest/ahci: Add port_select helper
>    qtest/ahci: Add port_clear helper
>    qtest/ahci: Add command header helpers
>    qtest/ahci: Add build cmd table helper
>    qtest/ahci: Add link_cmd_slot helper
>    qtest/ahci: Add port_check_error helper
>    qtest/ahci: Add issue_command helper
>    qtest/ahci: Add port_check_interrupts helper
>    qtest/ahci: Add port_check_nonbusy helper
>    qtest/ahci: Add cmd response sanity check helpers
>    qtest/ahci: Enforce zero-leaks for guest mem usage
>    qtest/ahci: Add a macro bootup routine
>    qtest/ahci: Add human-readable command names
>    qtest/ahci: Don't use a magic constant for buffer size
>
>   tests/ahci-test.c | 860 ++++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 583 insertions(+), 277 deletions(-)
>

NACKing my own patchset here because I am preparing a V2 and it is not 
worth anyone's time to review this version at this point in time.

--js