[Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[John Snow]

This patch series introduces a number of small fixes and tweaks to
help support an AHCI test suite that in the future I hope to expand
to a fuller regression suite to help guide the development of the
AHCI device support under, in particular, the Q35 machine type in QEMU.

Paolo Bonzini has contributed a number of cleanup and refactoring patches
that support changes to the PIO setup FIS packet construction code, which
is necessary for testing ths specification adherence of the IDENTIFY command,
which issues its data exclusively via PIO mechanisms.

The ahci-test code being checked in represents a minimum of functionality
needed in order to issue and receive commands from the AHCI HBA under the
libqos / qtest environment.

In V2, as detailed below, these tests are not currently expected to pass.
I will post a complementary patch outside of this set that highlights
the exact set of tests that will not pass, which can help verify at least
the portions of these tests that do work correctly.

Assertions that currently fail:
    - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
    - Boot-time values of the PxTFD register, which should not have valid
      data until after a D2H FIS is received, but does in Qemu 2.1
    - Boot-time values of the PxSIG register, which should have a specific
      placeholder signature until the first D2H FIS is received, but is
      currently blank.
    - The "Descriptor Processed" interrupt is expected after the IDENTIFY
      command exhausts the given PRDT, but is not seen.

V2:
"ide-test: add test for werror=stop"
    - changed filename variable name
    - altered the QMP event polling to avoid sleep
    - debug_path file cleanup on exit.
"ide: stop PIO transfer on errors"
    - Modified logic to be unconditional.
"ahci: construct PIO Setup FIS for PIO commands"
    - Added in dma_memory_map success checks and error pathways.
"libqtest: Correct small memory leak."
    - Corrected raw usage of free()
"ahci: Adding basic functionality qtest."
    - Removed HBA type.
    - Corrected cleanup order.
    - Corrected raw usage of free()
    - Removed needless conditional around g_free()
    - Altered ahci_boot to return by value instead of parameter.
"ahci: Add test_pci_spec to ahci-test."
    - Removed all ifdef logic in favor of runtime tuning. (--pedantic)
    - Removed all warnings, all infractions are now hard assertions.
    - Replaced g_assert with g_assert_cmphex in many cases
"ahci: add test_pci_enable to ahci-test."
    - Removed MSI codepaths, as it was incomplete and unused.
"ahci: Add test_hba_spec to ahci-test."
    - Removed unneeded macros
    - Added in an optional bar_size return parameter to qpci_iomap
"ahci: Add test_identify case to ahci-test."
    - Corrected raw usage of free()

For convenience; https://github.com/jnsnow/qemu/tree/ahci-test-v2

John Snow (13):
  q35: Enable the ioapic device to be seen by qtest.
  qtest: Adding qtest_memset and qmemset.
  libqos: Correct memory leak
  libqtest: Correct small memory leak.
  libqos: Fixes a small memory leak.
  libqos: allow qpci_iomap to return BAR mapping size
  qtest/ide: Fix small memory leak
  ahci: Adding basic functionality qtest.
  ahci: Add test_pci_spec to ahci-test.
  ahci: add test_pci_enable to ahci-test.
  ahci: Add test_hba_spec to ahci-test.
  ahci: Add test_hba_enable to ahci-test.
  ahci: Add test_identify case to ahci-test.

Paolo Bonzini (17):
  blkdebug: report errors on flush too
  libqtest: add QTEST_LOG for debugging qtest testcases
  ide-test: add test for werror=stop
  ide: stash aiocb for flushes
  ide: simplify reset callbacks
  ide: simplify set_inactive callbacks
  ide: simplify async_cmd_done callbacks
  ide: simplify start_transfer callbacks
  ide: wrap start_dma callback
  ide: remove wrong setting of BM_STATUS_INT
  ide: fold add_status callback into set_inactive
  ide: move BM_STATUS bits to pci.[ch]
  ide: move retry constants out of BM_STATUS_* namespace
  ahci: remove duplicate PORT_IRQ_* constants
  ide: stop PIO transfer on errors
  ide: make all commands go through cmd_done
  ahci: construct PIO Setup FIS for PIO commands

 block/blkdebug.c          |   20 +
 hw/i386/pc_q35.c          |    2 +-
 hw/ide/ahci.c             |  115 ++--
 hw/ide/ahci.h             |   21 -
 hw/ide/atapi.c            |   11 +-
 hw/ide/core.c             |   94 ++-
 hw/ide/internal.h         |   38 +-
 hw/ide/macio.c            |    9 -
 hw/ide/pci.c              |   45 +-
 hw/ide/pci.h              |    7 +
 tests/Makefile            |    2 +
 tests/ahci-test.c         | 1555 +++++++++++++++++++++++++++++++++++++++++++++
 tests/ide-test.c          |   85 ++-
 tests/libqos/malloc-pc.c  |    3 +
 tests/libqos/pci-pc.c     |   12 +-
 tests/libqos/pci-pc.h     |    1 +
 tests/libqos/pci.c        |   10 +-
 tests/libqos/pci.h        |    4 +-
 tests/libqtest.c          |   20 +-
 tests/libqtest.h          |   24 +
 tests/usb-hcd-ehci-test.c |    2 +-
 21 files changed, 1892 insertions(+), 188 deletions(-)
 create mode 100644 tests/ahci-test.c

-- 
1.9.3

[Qemu-devel] [PATCH v2 01/30] blkdebug: report errors on flush too

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/blkdebug.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index f51407d..1586ed9 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -522,6 +522,25 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
     return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
 }
 
+static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = NULL;
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.sector == -1) {
+            break;
+        }
+    }
+
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, cb, opaque, rule);
+    }
+
+    return bdrv_aio_flush(bs->file, cb, opaque);
+}
+
 
 static void blkdebug_close(BlockDriverState *bs)
 {
@@ -699,6 +718,7 @@ static BlockDriver bdrv_blkdebug = {
 
     .bdrv_aio_readv         = blkdebug_aio_readv,
     .bdrv_aio_writev        = blkdebug_aio_writev,
+    .bdrv_aio_flush         = blkdebug_aio_flush,
 
     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
-- 
1.9.3

[Qemu-devel] [PATCH v2 02/30] libqtest: add QTEST_LOG for debugging qtest testcases

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqtest.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 98e8f4b..4a75cd3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -167,11 +167,12 @@ QTestState *qtest_init(const char *extra_args)
     if (s->qemu_pid == 0) {
         command = g_strdup_printf("exec %s "
                                   "-qtest unix:%s,nowait "
-                                  "-qtest-log /dev/null "
+                                  "-qtest-log %s "
                                   "-qmp unix:%s,nowait "
                                   "-machine accel=qtest "
                                   "-display none "
                                   "%s", qemu_binary, socket_path,
+                                  getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
                                   qmp_socket_path,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
@@ -358,6 +359,7 @@ static void qmp_response(JSONMessageParser *parser, QList *tokens)
 QDict *qtest_qmp_receive(QTestState *s)
 {
     QMPResponseParser qmp;
+    bool log = getenv("QTEST_LOG") != NULL;
 
     qmp.response = NULL;
     json_message_parser_init(&qmp.parser, qmp_response);
@@ -375,6 +377,9 @@ QDict *qtest_qmp_receive(QTestState *s)
             exit(1);
         }
 
+        if (log) {
+            len = write(2, &c, 1);
+        }
         json_message_parser_feed(&qmp.parser, &c, 1);
     }
     json_message_parser_destroy(&qmp.parser);
-- 
1.9.3

[Qemu-devel] [PATCH v2 03/30] ide-test: add test for werror=stop

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a0d97f..151ef30 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -106,6 +106,7 @@ static QPCIBus *pcibus = NULL;
 static QGuestAllocator *guest_malloc;
 
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
+static char debug_path[] = "/tmp/qtest-blkdebug.XXXXXX";
 
 static void ide_test_start(const char *cmdline_fmt, ...)
 {
@@ -489,6 +490,78 @@ static void test_flush(void)
     ide_test_quit();
 }
 
+static void prepare_blkdebug_script(const char *debug_fn, const char *event)
+{
+    FILE *debug_file = fopen(debug_fn, "w");
+    int ret;
+
+    fprintf(debug_file, "[inject-error]\n");
+    fprintf(debug_file, "event = \"%s\"\n", event);
+    fprintf(debug_file, "errno = \"5\"\n");
+    fprintf(debug_file, "state = \"1\"\n");
+    fprintf(debug_file, "immediately = \"off\"\n");
+    fprintf(debug_file, "once = \"on\"\n");
+
+    fprintf(debug_file, "[set-state]\n");
+    fprintf(debug_file, "event = \"%s\"\n", event);
+    fprintf(debug_file, "new_state = \"2\"\n");
+    fflush(debug_file);
+    g_assert(!ferror(debug_file));
+
+    ret = fclose(debug_file);
+    g_assert(ret == 0);
+}
+
+static void test_retry_flush(void)
+{
+    uint8_t data;
+    const char *s;
+    QDict *response;
+
+    prepare_blkdebug_script(debug_path, "flush_to_disk");
+
+    ide_test_start(
+        "-vnc none "
+        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
+        debug_path, tmp_path);
+
+    /* FLUSH CACHE command on device 0*/
+    outb(IDE_BASE + reg_device, 0);
+    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+
+    /* Check status while request is in flight*/
+    data = inb(IDE_BASE + reg_status);
+    assert_bit_set(data, BSY | DRDY);
+    assert_bit_clear(data, DF | ERR | DRQ);
+
+    for (;; response = NULL) {
+        response = qmp_receive();
+        if ((qdict_haskey(response, "event")) &&
+            (strcmp(qdict_get_str(response, "event"), "STOP") == 0)) {
+            QDECREF(response);
+            break;
+        }
+        QDECREF(response);
+    }
+
+    /* Complete the command */
+    s = "{'execute':'cont' }";
+    qmp_discard_response(s);
+
+    /* Check registers */
+    data = inb(IDE_BASE + reg_device);
+    g_assert_cmpint(data & DEV, ==, 0);
+
+    do {
+        data = inb(IDE_BASE + reg_status);
+    } while (data & BSY);
+
+    assert_bit_set(data, DRDY);
+    assert_bit_clear(data, BSY | DF | ERR | DRQ);
+
+    ide_test_quit();
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -501,6 +574,11 @@ int main(int argc, char **argv)
         return 0;
     }
 
+    /* Create temporary blkdebug instructions */
+    fd = mkstemp(debug_path);
+    g_assert(fd >= 0);
+    close(fd);
+
     /* Create a temporary raw image */
     fd = mkstemp(tmp_path);
     g_assert(fd >= 0);
@@ -522,10 +600,13 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/flush", test_flush);
 
+    qtest_add_func("/ide/retry/flush", test_retry_flush);
+
     ret = g_test_run();
 
     /* Cleanup */
     unlink(tmp_path);
+    unlink(debug_path);
 
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 04/30] ide: stash aiocb for flushes

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

This ensures that operations are completed after a reset

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index db191a6..79985f9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -831,6 +831,8 @@ static void ide_flush_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
 
+    s->pio_aiocb = NULL;
+
     if (ret < 0) {
         /* XXX: What sector number to set here? */
         if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
@@ -853,7 +855,7 @@ void ide_flush_cache(IDEState *s)
 
     s->status |= BUSY_STAT;
     bdrv_acct_start(s->bs, &s->acct, 0, BDRV_ACCT_FLUSH);
-    bdrv_aio_flush(s->bs, ide_flush_cb, s);
+    s->pio_aiocb = bdrv_aio_flush(s->bs, ide_flush_cb, s);
 }
 
 static void ide_cfata_metadata_inquiry(IDEState *s)
-- 
1.9.3

[Qemu-devel] [PATCH v2 05/30] ide: simplify reset callbacks

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 6 ------
 hw/ide/core.c     | 5 +++--
 hw/ide/internal.h | 3 ++-
 hw/ide/macio.c    | 1 -
 hw/ide/pci.c      | 4 +---
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 604152a..273712f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1147,11 +1147,6 @@ static void ahci_dma_restart_cb(void *opaque, int running, RunState state)
 {
 }
 
-static int ahci_dma_reset(IDEDMA *dma)
-{
-    return 0;
-}
-
 static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
     .start_transfer = ahci_start_transfer,
@@ -1162,7 +1157,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .set_inactive = ahci_dma_set_inactive,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
-    .reset = ahci_dma_reset,
 };
 
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 79985f9..4183a3a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2088,7 +2088,9 @@ void ide_bus_reset(IDEBus *bus)
     }
 
     /* reset dma provider too */
-    bus->dma->ops->reset(bus->dma);
+    if (bus->dma->ops->reset) {
+        bus->dma->ops->reset(bus->dma);
+    }
 }
 
 static bool ide_cd_is_tray_open(void *opaque)
@@ -2226,7 +2228,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .add_status     = ide_nop_int,
     .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
-    .reset          = ide_nop,
 };
 
 static IDEDMA ide_dma_nop = {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 0567a52..6cca46f 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -320,6 +320,7 @@ typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
 typedef void EndTransferFunc(IDEState *);
 
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
+typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
 typedef void DMARestartFunc(void *, int, RunState);
@@ -435,7 +436,7 @@ struct IDEDMAOps {
     DMAFunc *set_inactive;
     DMAFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
-    DMAFunc *reset;
+    DMAVoidFunc *reset;
 };
 
 struct IDEDMA {
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index c14a1dd..ca39e3f 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -578,7 +578,6 @@ static const IDEDMAOps dbdma_ops = {
     .add_status     = ide_nop_int,
     .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
-    .reset          = ide_nop,
 };
 
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 6257a21..c24496a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -247,7 +247,7 @@ static void bmdma_cancel(BMDMAState *bm)
     }
 }
 
-static int bmdma_reset(IDEDMA *dma)
+static void bmdma_reset(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
@@ -264,8 +264,6 @@ static int bmdma_reset(IDEDMA *dma)
     bm->cur_prd_len = 0;
     bm->sector_num = 0;
     bm->nsector = 0;
-
-    return 0;
 }
 
 static int bmdma_start_transfer(IDEDMA *dma)
-- 
1.9.3

[Qemu-devel] [PATCH v2 06/30] ide: simplify set_inactive callbacks

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 6 ------
 hw/ide/core.c     | 5 +++--
 hw/ide/internal.h | 2 +-
 hw/ide/macio.c    | 1 -
 hw/ide/pci.c      | 4 +---
 5 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 273712f..679357f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1116,11 +1116,6 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int ahci_dma_set_inactive(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1154,7 +1149,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
     .add_status = ahci_dma_add_status,
-    .set_inactive = ahci_dma_set_inactive,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
 };
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 4183a3a..d52a6f6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -595,7 +595,9 @@ static void ide_async_cmd_done(IDEState *s)
 void ide_set_inactive(IDEState *s)
 {
     s->bus->dma->aiocb = NULL;
-    s->bus->dma->ops->set_inactive(s->bus->dma);
+    if (s->bus->dma->ops->set_inactive) {
+        s->bus->dma->ops->set_inactive(s->bus->dma);
+    }
     ide_async_cmd_done(s);
 }
 
@@ -2226,7 +2228,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .add_status     = ide_nop_int,
-    .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 6cca46f..34064cf 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -433,7 +433,7 @@ struct IDEDMAOps {
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
-    DMAFunc *set_inactive;
+    DMAVoidFunc *set_inactive;
     DMAFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index ca39e3f..0e26bac 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -576,7 +576,6 @@ static const IDEDMAOps dbdma_ops = {
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .add_status     = ide_nop_int,
-    .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index c24496a..9dc3cc5 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -160,15 +160,13 @@ static int bmdma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int bmdma_set_inactive(IDEDMA *dma)
+static void bmdma_set_inactive(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
     bm->status &= ~BM_STATUS_DMAING;
     bm->dma_cb = NULL;
     bm->unit = -1;
-
-    return 0;
 }
 
 static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
-- 
1.9.3

[Qemu-devel] [PATCH v2 07/30] ide: simplify async_cmd_done callbacks

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Drop the unused return value.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 679357f..e494503 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1116,7 +1116,7 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int ahci_async_cmd_done(IDEDMA *dma)
+static void ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
@@ -1130,8 +1130,6 @@ static int ahci_async_cmd_done(IDEDMA *dma)
         ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
         qemu_bh_schedule(ad->check_bh);
     }
-
-    return 0;
 }
 
 static void ahci_irq_set(void *opaque, int n, int level)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 34064cf..940dd45 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -434,7 +434,7 @@ struct IDEDMAOps {
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
     DMAVoidFunc *set_inactive;
-    DMAFunc *async_cmd_done;
+    DMAVoidFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
 };
-- 
1.9.3

[Qemu-devel] [PATCH v2 08/30] ide: simplify start_transfer callbacks

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     |  4 +---
 hw/ide/core.c     | 10 +++-------
 hw/ide/internal.h |  3 +--
 hw/ide/macio.c    |  6 ------
 hw/ide/pci.c      |  6 ------
 5 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e494503..adbac3d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -991,7 +991,7 @@ out:
 }
 
 /* DMA dev <-> ram */
-static int ahci_start_transfer(IDEDMA *dma)
+static void ahci_start_transfer(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
@@ -1041,8 +1041,6 @@ out:
         /* done with DMA */
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
     }
-
-    return 0;
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d52a6f6..e5ddecb 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -434,7 +434,9 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     if (!(s->status & ERR_STAT)) {
         s->status |= DRQ_STAT;
     }
-    s->bus->dma->ops->start_transfer(s->bus->dma);
+    if (s->bus->dma->ops->start_transfer) {
+        s->bus->dma->ops->start_transfer(s->bus->dma);
+    }
 }
 
 void ide_transfer_stop(IDEState *s)
@@ -2207,11 +2209,6 @@ static void ide_nop_start(IDEDMA *dma, IDEState *s,
 {
 }
 
-static int ide_nop(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -2223,7 +2220,6 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
 
 static const IDEDMAOps ide_dma_nop_ops = {
     .start_dma      = ide_nop_start,
-    .start_transfer = ide_nop,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 940dd45..64fbf2f 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -321,7 +321,6 @@ typedef void EndTransferFunc(IDEState *);
 
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
-typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
 typedef void DMARestartFunc(void *, int, RunState);
 
@@ -428,7 +427,7 @@ struct IDEState {
 
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
-    DMAFunc *start_transfer;
+    DMAVoidFunc *start_transfer;
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 0e26bac..b7cedb6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -545,11 +545,6 @@ static void macio_ide_reset(DeviceState *dev)
     ide_bus_reset(&d->bus);
 }
 
-static int ide_nop(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -571,7 +566,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
 
 static const IDEDMAOps dbdma_ops = {
     .start_dma      = ide_dbdma_start,
-    .start_transfer = ide_nop,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 9dc3cc5..1ee8c0a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -264,11 +264,6 @@ static void bmdma_reset(IDEDMA *dma)
     bm->nsector = 0;
 }
 
-static int bmdma_start_transfer(IDEDMA *dma)
-{
-    return 0;
-}
-
 static void bmdma_irq(void *opaque, int n, int level)
 {
     BMDMAState *bm = opaque;
@@ -500,7 +495,6 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
 
 static const struct IDEDMAOps bmdma_ops = {
     .start_dma = bmdma_start_dma,
-    .start_transfer = bmdma_start_transfer,
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
-- 
1.9.3

[Qemu-devel] [PATCH v2 09/30] ide: wrap start_dma callback

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Make it optional and prepare for the next patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c    |  6 ++----
 hw/ide/core.c     | 15 ++++++++-------
 hw/ide/internal.h |  1 +
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index f7d2009..d13395e 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -255,8 +255,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
     if (s->atapi_dma) {
         bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
         s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
-        s->bus->dma->ops->start_dma(s->bus->dma, s,
-                                   ide_atapi_cmd_read_dma_cb);
+        ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
     } else {
         s->status = READY_STAT | SEEK_STAT;
         ide_atapi_cmd_reply_end(s);
@@ -375,8 +374,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
 
     /* XXX: check if BUSY_STAT should be set */
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
-    s->bus->dma->ops->start_dma(s->bus->dma, s,
-                               ide_atapi_cmd_read_dma_cb);
+    ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
 }
 
 static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index e5ddecb..aa561ae 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -745,7 +745,14 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
         break;
     }
 
-    s->bus->dma->ops->start_dma(s->bus->dma, s, ide_dma_cb);
+    ide_start_dma(s, ide_dma_cb);
+}
+
+void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb)
+{
+    if (s->bus->dma->ops->start_dma) {
+        s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
+    }
 }
 
 static void ide_sector_write_timer_cb(void *opaque)
@@ -2204,11 +2211,6 @@ static void ide_init1(IDEBus *bus, int unit)
                                            ide_sector_write_timer_cb, s);
 }
 
-static void ide_nop_start(IDEDMA *dma, IDEState *s,
-                          BlockDriverCompletionFunc *cb)
-{
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -2219,7 +2221,6 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
 }
 
 static const IDEDMAOps ide_dma_nop_ops = {
-    .start_dma      = ide_nop_start,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 64fbf2f..2fe1f0a 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -532,6 +532,7 @@ void ide_bus_reset(IDEBus *bus);
 int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
 
+void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb);
 void ide_dma_error(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
-- 
1.9.3

[Qemu-devel] [PATCH v2 10/30] ide: remove wrong setting of BM_STATUS_INT

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Similar to the case removed in commit 69c38b8 (ide/core: Remove explicit
setting of BM_STATUS_INT, 2011-05-19), the only remaining use of
add_status(..., BM_STATUS_INT) is for short PRDs.  The flag should
not be raised in this case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c  | 4 ----
 hw/ide/atapi.c | 1 -
 2 files changed, 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index adbac3d..14677ec 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1107,10 +1107,6 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     DPRINTF(ad->port_no, "set status: %x\n", status);
 
-    if (status & BM_STATUS_INT) {
-        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
-    }
-
     return 0;
 }
 
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index d13395e..46ed3f5 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -355,7 +355,6 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 
 eot:
     bdrv_acct_done(s->bs, &s->acct);
-    s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
     ide_set_inactive(s);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 11/30] ide: fold add_status callback into set_inactive

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

It is now called only after the set_inactive callback.  Put the two together.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     |  9 ---------
 hw/ide/atapi.c    |  2 +-
 hw/ide/core.c     | 12 ++++--------
 hw/ide/internal.h |  6 +++---
 hw/ide/macio.c    |  1 -
 hw/ide/pci.c      | 19 +++++++------------
 6 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 14677ec..0ec5627 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1102,14 +1102,6 @@ static int ahci_dma_set_unit(IDEDMA *dma, int unit)
     return 0;
 }
 
-static int ahci_dma_add_status(IDEDMA *dma, int status)
-{
-    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    DPRINTF(ad->port_no, "set status: %x\n", status);
-
-    return 0;
-}
-
 static void ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1140,7 +1132,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .prepare_buf = ahci_dma_prepare_buf,
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
-    .add_status = ahci_dma_add_status,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
 };
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 46ed3f5..3b419b3 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -355,7 +355,7 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 
 eot:
     bdrv_acct_done(s->bs, &s->acct);
-    ide_set_inactive(s);
+    ide_set_inactive(s, false);
 }
 
 /* start a CD-CDROM read command with DMA */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index aa561ae..24f24ce 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -594,11 +594,11 @@ static void ide_async_cmd_done(IDEState *s)
     }
 }
 
-void ide_set_inactive(IDEState *s)
+void ide_set_inactive(IDEState *s, bool more)
 {
     s->bus->dma->aiocb = NULL;
     if (s->bus->dma->ops->set_inactive) {
-        s->bus->dma->ops->set_inactive(s->bus->dma);
+        s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
     ide_async_cmd_done(s);
 }
@@ -608,7 +608,7 @@ void ide_dma_error(IDEState *s)
     ide_transfer_stop(s);
     s->error = ABRT_ERR;
     s->status = READY_STAT | ERR_STAT;
-    ide_set_inactive(s);
+    ide_set_inactive(s, false);
     ide_set_irq(s->bus);
 }
 
@@ -719,10 +719,7 @@ eot:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
         bdrv_acct_done(s->bs, &s->acct);
     }
-    ide_set_inactive(s);
-    if (stay_active) {
-        s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_DMAING);
-    }
+    ide_set_inactive(s, stay_active);
 }
 
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
@@ -2224,7 +2221,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .add_status     = ide_nop_int,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 2fe1f0a..b35e52c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -322,6 +322,7 @@ typedef void EndTransferFunc(IDEState *);
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
+typedef void DMAStopFunc(IDEDMA *, bool);
 typedef void DMARestartFunc(void *, int, RunState);
 
 struct unreported_events {
@@ -431,8 +432,7 @@ struct IDEDMAOps {
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
-    DMAIntFunc *add_status;
-    DMAVoidFunc *set_inactive;
+    DMAStopFunc *set_inactive;
     DMAVoidFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
@@ -565,7 +565,7 @@ void ide_flush_cache(IDEState *s);
 void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func);
 void ide_transfer_stop(IDEState *s);
-void ide_set_inactive(IDEState *s);
+void ide_set_inactive(IDEState *s, bool more);
 BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index b7cedb6..b0c0d40 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -569,7 +569,6 @@ static const IDEDMAOps dbdma_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .add_status     = ide_nop_int,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 1ee8c0a..73267a4 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -152,21 +152,17 @@ static int bmdma_set_unit(IDEDMA *dma, int unit)
     return 0;
 }
 
-static int bmdma_add_status(IDEDMA *dma, int status)
+static void bmdma_set_inactive(IDEDMA *dma, bool more)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-    bm->status |= status;
 
-    return 0;
-}
-
-static void bmdma_set_inactive(IDEDMA *dma)
-{
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-
-    bm->status &= ~BM_STATUS_DMAING;
     bm->dma_cb = NULL;
     bm->unit = -1;
+    if (more) {
+        bm->status |= BM_STATUS_DMAING;
+    } else {
+        bm->status &= ~BM_STATUS_DMAING;
+    }
 }
 
 static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
@@ -241,7 +237,7 @@ static void bmdma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
         /* cancel DMA request */
-        bmdma_set_inactive(&bm->dma);
+        bmdma_set_inactive(&bm->dma, false);
     }
 }
 
@@ -498,7 +494,6 @@ static const struct IDEDMAOps bmdma_ops = {
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
-    .add_status = bmdma_add_status,
     .set_inactive = bmdma_set_inactive,
     .restart_cb = bmdma_restart_cb,
     .reset = bmdma_reset,
-- 
1.9.3

[Qemu-devel] [PATCH v2 12/30] ide: move BM_STATUS bits to pci.[ch]

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

They are not used by AHCI, and should not be even available there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/internal.h | 11 -----------
 hw/ide/pci.c      |  4 ++++
 hw/ide/pci.h      |  7 +++++++
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b35e52c..f369b0b 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -484,10 +484,6 @@ struct IDEDevice {
     uint64_t wwn;
 };
 
-#define BM_STATUS_DMAING 0x01
-#define BM_STATUS_ERROR  0x02
-#define BM_STATUS_INT    0x04
-
 /* FIXME These are not status register bits */
 #define BM_STATUS_DMA_RETRY  0x08
 #define BM_STATUS_PIO_RETRY  0x10
@@ -495,13 +491,6 @@ struct IDEDevice {
 #define BM_STATUS_RETRY_FLUSH 0x40
 #define BM_STATUS_RETRY_TRIM 0x80
 
-#define BM_MIGRATION_COMPAT_STATUS_BITS \
-        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
-        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
-
-#define BM_CMD_START     0x01
-#define BM_CMD_READ      0x08
-
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
     return bus->ifs + bus->unit;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 73267a4..0e82cab 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -33,6 +33,10 @@
 
 #define BMDMA_PAGE_SIZE 4096
 
+#define BM_MIGRATION_COMPAT_STATUS_BITS \
+        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
+        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
+
 static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
                             BlockDriverCompletionFunc *dma_cb)
 {
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index 2428275..517711f 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -3,6 +3,13 @@
 
 #include <hw/ide/internal.h>
 
+#define BM_STATUS_DMAING 0x01
+#define BM_STATUS_ERROR  0x02
+#define BM_STATUS_INT    0x04
+
+#define BM_CMD_START     0x01
+#define BM_CMD_READ      0x08
+
 typedef struct BMDMAState {
     IDEDMA dma;
     uint8_t cmd;
-- 
1.9.3

[Qemu-devel] [PATCH v2 13/30] ide: move retry constants out of BM_STATUS_* namespace

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c     | 20 ++++++++++----------
 hw/ide/internal.h | 12 ++++++------
 hw/ide/pci.c      | 14 +++++++-------
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 24f24ce..00fe043 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -523,8 +523,8 @@ static void ide_sector_read_cb(void *opaque, int ret)
 
     bdrv_acct_done(s->bs, &s->acct);
     if (ret != 0) {
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY |
-                                BM_STATUS_RETRY_READ)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO |
+                                IDE_RETRY_READ)) {
             return;
         }
     }
@@ -614,14 +614,14 @@ void ide_dma_error(IDEState *s)
 
 static int ide_handle_rw_error(IDEState *s, int error, int op)
 {
-    bool is_read = (op & BM_STATUS_RETRY_READ) != 0;
+    bool is_read = (op & IDE_RETRY_READ) != 0;
     BlockErrorAction action = bdrv_get_error_action(s->bs, is_read, error);
 
     if (action == BLOCK_ERROR_ACTION_STOP) {
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->error_status = op;
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
-        if (op & BM_STATUS_DMA_RETRY) {
+        if (op & IDE_RETRY_DMA) {
             dma_buf_commit(s);
             ide_dma_error(s);
         } else {
@@ -640,12 +640,12 @@ void ide_dma_cb(void *opaque, int ret)
     bool stay_active = false;
 
     if (ret < 0) {
-        int op = BM_STATUS_DMA_RETRY;
+        int op = IDE_RETRY_DMA;
 
         if (s->dma_cmd == IDE_DMA_READ)
-            op |= BM_STATUS_RETRY_READ;
+            op |= IDE_RETRY_READ;
         else if (s->dma_cmd == IDE_DMA_TRIM)
-            op |= BM_STATUS_RETRY_TRIM;
+            op |= IDE_RETRY_TRIM;
 
         if (ide_handle_rw_error(s, -ret, op)) {
             return;
@@ -769,7 +769,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
     s->status &= ~BUSY_STAT;
 
     if (ret != 0) {
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO)) {
             return;
         }
     }
@@ -843,7 +843,7 @@ static void ide_flush_cb(void *opaque, int ret)
 
     if (ret < 0) {
         /* XXX: What sector number to set here? */
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_FLUSH)) {
             return;
         }
     }
@@ -2338,7 +2338,7 @@ static bool ide_drive_pio_state_needed(void *opaque)
     IDEState *s = opaque;
 
     return ((s->status & DRQ_STAT) != 0)
-        || (s->bus->error_status & BM_STATUS_PIO_RETRY);
+        || (s->bus->error_status & IDE_RETRY_PIO);
 }
 
 static bool ide_tray_state_needed(void *opaque)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index f369b0b..b919e96 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -484,12 +484,12 @@ struct IDEDevice {
     uint64_t wwn;
 };
 
-/* FIXME These are not status register bits */
-#define BM_STATUS_DMA_RETRY  0x08
-#define BM_STATUS_PIO_RETRY  0x10
-#define BM_STATUS_RETRY_READ  0x20
-#define BM_STATUS_RETRY_FLUSH 0x40
-#define BM_STATUS_RETRY_TRIM 0x80
+/* These are used for the error_status field of IDEBus */
+#define IDE_RETRY_DMA  0x08
+#define IDE_RETRY_PIO  0x10
+#define IDE_RETRY_READ  0x20
+#define IDE_RETRY_FLUSH 0x40
+#define IDE_RETRY_TRIM 0x80
 
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 0e82cab..2397f35 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -34,8 +34,8 @@
 #define BMDMA_PAGE_SIZE 4096
 
 #define BM_MIGRATION_COMPAT_STATUS_BITS \
-        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
-        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
+        (IDE_RETRY_DMA | IDE_RETRY_PIO | \
+        IDE_RETRY_READ | IDE_RETRY_FLUSH)
 
 static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
                             BlockDriverCompletionFunc *dma_cb)
@@ -198,7 +198,7 @@ static void bmdma_restart_bh(void *opaque)
         return;
     }
 
-    is_read = (bus->error_status & BM_STATUS_RETRY_READ) != 0;
+    is_read = (bus->error_status & IDE_RETRY_READ) != 0;
 
     /* The error status must be cleared before resubmitting the request: The
      * request may fail again, and this case can only be distinguished if the
@@ -206,19 +206,19 @@ static void bmdma_restart_bh(void *opaque)
     error_status = bus->error_status;
     bus->error_status = 0;
 
-    if (error_status & BM_STATUS_DMA_RETRY) {
-        if (error_status & BM_STATUS_RETRY_TRIM) {
+    if (error_status & IDE_RETRY_DMA) {
+        if (error_status & IDE_RETRY_TRIM) {
             bmdma_restart_dma(bm, IDE_DMA_TRIM);
         } else {
             bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
         }
-    } else if (error_status & BM_STATUS_PIO_RETRY) {
+    } else if (error_status & IDE_RETRY_PIO) {
         if (is_read) {
             ide_sector_read(bmdma_active_if(bm));
         } else {
             ide_sector_write(bmdma_active_if(bm));
         }
-    } else if (error_status & BM_STATUS_RETRY_FLUSH) {
+    } else if (error_status & IDE_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
     }
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 14/30] ahci: remove duplicate PORT_IRQ_* constants

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

These are defined twice, just use one set consistently.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c |  6 +++---
 hw/ide/ahci.h | 21 ---------------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0ec5627..e1f27bd 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -584,7 +584,7 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
     s->dev[port].finished |= finished;
     *(uint32_t*)(sdb_fis + 4) = cpu_to_le32(s->dev[port].finished);
 
-    ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_STAT_SDBS);
+    ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_SDB_FIS);
 }
 
 static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
@@ -629,7 +629,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     }
 
     if (d2h_fis[2] & ERR_STAT) {
-        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_TFES);
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
     }
 
     ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
@@ -1039,7 +1039,7 @@ out:
 
     if (!(s->status & DRQ_STAT)) {
         /* done with DMA */
-        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
     }
 }
 
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index f418b30..1543df7 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -132,27 +132,6 @@
 #define PORT_CMD_ICC_PARTIAL      (0x2 << 28) /* Put i/f in partial state */
 #define PORT_CMD_ICC_SLUMBER      (0x6 << 28) /* Put i/f in slumber state */
 
-#define PORT_IRQ_STAT_DHRS        (1 << 0) /* Device to Host Register FIS */
-#define PORT_IRQ_STAT_PSS         (1 << 1) /* PIO Setup FIS */
-#define PORT_IRQ_STAT_DSS         (1 << 2) /* DMA Setup FIS */
-#define PORT_IRQ_STAT_SDBS        (1 << 3) /* Set Device Bits */
-#define PORT_IRQ_STAT_UFS         (1 << 4) /* Unknown FIS */
-#define PORT_IRQ_STAT_DPS         (1 << 5) /* Descriptor Processed */
-#define PORT_IRQ_STAT_PCS         (1 << 6) /* Port Connect Change Status */
-#define PORT_IRQ_STAT_DMPS        (1 << 7) /* Device Mechanical Presence
-                                              Status */
-#define PORT_IRQ_STAT_PRCS        (1 << 22) /* File Ready Status */
-#define PORT_IRQ_STAT_IPMS        (1 << 23) /* Incorrect Port Multiplier
-                                               Status */
-#define PORT_IRQ_STAT_OFS         (1 << 24) /* Overflow Status */
-#define PORT_IRQ_STAT_INFS        (1 << 26) /* Interface Non-Fatal Error
-                                               Status */
-#define PORT_IRQ_STAT_IFS         (1 << 27) /* Interface Fatal Error */
-#define PORT_IRQ_STAT_HBDS        (1 << 28) /* Host Bus Data Error Status */
-#define PORT_IRQ_STAT_HBFS        (1 << 29) /* Host Bus Fatal Error Status */
-#define PORT_IRQ_STAT_TFES        (1 << 30) /* Task File Error Status */
-#define PORT_IRQ_STAT_CPDS        (1U << 31) /* Code Port Detect Status */
-
 /* ap->flags bits */
 #define AHCI_FLAG_NO_NCQ                  (1 << 24)
 #define AHCI_FLAG_IGN_IRQ_IF_ERR          (1 << 25) /* ignore IRQ_IF_ERR */
-- 
1.9.3

[Qemu-devel] [PATCH v2 15/30] ide: stop PIO transfer on errors

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

This will provide a hook for sending the result of the command via the
FIS receive area.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 00fe043..91a17e6 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -420,6 +420,7 @@ BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
 
 static inline void ide_abort_command(IDEState *s)
 {
+    ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
 }
@@ -605,9 +606,7 @@ void ide_set_inactive(IDEState *s, bool more)
 
 void ide_dma_error(IDEState *s)
 {
-    ide_transfer_stop(s);
-    s->error = ABRT_ERR;
-    s->status = READY_STAT | ERR_STAT;
+    ide_abort_command(s);
     ide_set_inactive(s, false);
     ide_set_irq(s->bus);
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 16/30] ide: make all commands go through cmd_done

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

AHCI has code to fill in the D2H FIS trigger the IRQ all over the place.
Centralize this in a single cmd_done callback by generalizing the existing
async_cmd_done callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 16 +++-------------
 hw/ide/atapi.c    |  2 +-
 hw/ide/core.c     | 20 +++++++++++---------
 hw/ide/internal.h |  2 +-
 4 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e1f27bd..b40ec06 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -969,11 +969,6 @@ static int handle_cmd(AHCIState *s, int port, int slot)
 
         /* We're ready to process the command in FIS byte 2. */
         ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
-
-        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
-            READY_STAT) {
-            ahci_write_fis_d2h(&s->dev[port], cmd_fis);
-        }
     }
 
 out:
@@ -1036,11 +1031,6 @@ out:
     }
 
     s->end_transfer_func(s);
-
-    if (!(s->status & DRQ_STAT)) {
-        /* done with DMA */
-        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
-    }
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
@@ -1102,11 +1092,11 @@ static int ahci_dma_set_unit(IDEDMA *dma, int unit)
     return 0;
 }
 
-static void ahci_async_cmd_done(IDEDMA *dma)
+static void ahci_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
-    DPRINTF(ad->port_no, "async cmd done\n");
+    DPRINTF(ad->port_no, "cmd done\n");
 
     /* update d2h status */
     ahci_write_fis_d2h(ad, NULL);
@@ -1132,7 +1122,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .prepare_buf = ahci_dma_prepare_buf,
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
-    .async_cmd_done = ahci_async_cmd_done,
+    .cmd_done = ahci_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
 };
 
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 3b419b3..3d92b52 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -174,9 +174,9 @@ void ide_atapi_cmd_reply_end(IDEState *s)
 #endif
     if (s->packet_transfer_size <= 0) {
         /* end of transfer */
-        ide_transfer_stop(s);
         s->status = READY_STAT | SEEK_STAT;
         s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
+        ide_transfer_stop(s);
         ide_set_irq(s->bus);
 #ifdef DEBUG_IDE_ATAPI
         printf("status=0x%x\n", s->status);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 91a17e6..bdb0a80 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -440,12 +440,20 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     }
 }
 
+static void ide_cmd_done(IDEState *s)
+{
+    if (s->bus->dma->ops->cmd_done) {
+        s->bus->dma->ops->cmd_done(s->bus->dma);
+    }
+}
+
 void ide_transfer_stop(IDEState *s)
 {
     s->end_transfer_func = ide_transfer_stop;
     s->data_ptr = s->io_buffer;
     s->data_end = s->io_buffer;
     s->status &= ~DRQ_STAT;
+    ide_cmd_done(s);
 }
 
 int64_t ide_get_sector(IDEState *s)
@@ -588,20 +596,13 @@ static void dma_buf_commit(IDEState *s)
     qemu_sglist_destroy(&s->sg);
 }
 
-static void ide_async_cmd_done(IDEState *s)
-{
-    if (s->bus->dma->ops->async_cmd_done) {
-        s->bus->dma->ops->async_cmd_done(s->bus->dma);
-    }
-}
-
 void ide_set_inactive(IDEState *s, bool more)
 {
     s->bus->dma->aiocb = NULL;
     if (s->bus->dma->ops->set_inactive) {
         s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
-    ide_async_cmd_done(s);
+    ide_cmd_done(s);
 }
 
 void ide_dma_error(IDEState *s)
@@ -849,7 +850,7 @@ static void ide_flush_cb(void *opaque, int ret)
 
     bdrv_acct_done(s->bs, &s->acct);
     s->status = READY_STAT | SEEK_STAT;
-    ide_async_cmd_done(s);
+    ide_cmd_done(s);
     ide_set_irq(s->bus);
 }
 
@@ -1773,6 +1774,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
             s->status |= SEEK_STAT;
         }
 
+        ide_cmd_done(s);
         ide_set_irq(s->bus);
     }
 }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b919e96..5c19f79 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -433,7 +433,7 @@ struct IDEDMAOps {
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
     DMAStopFunc *set_inactive;
-    DMAVoidFunc *async_cmd_done;
+    DMAVoidFunc *cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
 };
-- 
1.9.3

[Qemu-devel] [PATCH v2 17/30] ahci: construct PIO Setup FIS for PIO commands

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

PIO commands should put a PIO Setup FIS in the receive area when data
transfer ends.  Currently QEMU does not do this and only places the
D2H FIS at the end of the operation.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b40ec06..4cda0d0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -587,6 +587,71 @@ static void ahci_write_fis_sdb(AHCIState *s, int port, uint32_t finished)
     ahci_trigger_irq(s, &s->dev[port], PORT_IRQ_SDB_FIS);
 }
 
+static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
+{
+    AHCIPortRegs *pr = &ad->port_regs;
+    uint8_t *pio_fis, *cmd_fis;
+    uint64_t tbl_addr;
+    dma_addr_t cmd_len = 0x80;
+
+    if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
+        return;
+    }
+
+    /* map cmd_fis */
+    tbl_addr = le64_to_cpu(ad->cur_cmd->tbl_addr);
+    cmd_fis = dma_memory_map(ad->hba->as, tbl_addr, &cmd_len,
+                             DMA_DIRECTION_TO_DEVICE);
+
+    if (cmd_fis == NULL) {
+        DPRINTF(ad->port_no, "dma_memory_map failed in ahci_write_fis_pio");
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR);
+        return;
+    }
+
+    if (cmd_len != 0x80) {
+        DPRINTF(ad->port_no,
+                "dma_memory_map mapped too few bytes in ahci_write_fis_pio");
+        dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len,
+                         DMA_DIRECTION_TO_DEVICE, cmd_len);
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_HBUS_ERR);
+        return;
+    }
+
+    pio_fis = &ad->res_fis[RES_FIS_PSFIS];
+
+    pio_fis[0] = 0x5f;
+    pio_fis[1] = (ad->hba->control_regs.irqstatus ? (1 << 6) : 0);
+    pio_fis[2] = ad->port.ifs[0].status;
+    pio_fis[3] = ad->port.ifs[0].error;
+
+    pio_fis[4] = cmd_fis[4];
+    pio_fis[5] = cmd_fis[5];
+    pio_fis[6] = cmd_fis[6];
+    pio_fis[7] = cmd_fis[7];
+    pio_fis[8] = cmd_fis[8];
+    pio_fis[9] = cmd_fis[9];
+    pio_fis[10] = cmd_fis[10];
+    pio_fis[11] = cmd_fis[11];
+    pio_fis[12] = cmd_fis[12];
+    pio_fis[13] = cmd_fis[13];
+    pio_fis[14] = 0;
+    pio_fis[15] = ad->port.ifs[0].status;
+    pio_fis[16] = len & 255;
+    pio_fis[17] = len >> 8;
+    pio_fis[18] = 0;
+    pio_fis[19] = 0;
+
+    if (pio_fis[2] & ERR_STAT) {
+        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_TF_ERR);
+    }
+
+    ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
+
+    dma_memory_unmap(ad->hba->as, cmd_fis, cmd_len,
+                     DMA_DIRECTION_TO_DEVICE, cmd_len);
+}
+
 static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
 {
     AHCIPortRegs *pr = &ad->port_regs;
@@ -1031,6 +1096,11 @@ out:
     }
 
     s->end_transfer_func(s);
+
+    if (!(s->status & DRQ_STAT)) {
+        /* done with PIO send/receive */
+        ahci_write_fis_pio(ad, le32_to_cpu(ad->cur_cmd->status));
+    }
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
-- 
1.9.3

[Qemu-devel] [PATCH v2 18/30] q35: Enable the ioapic device to be seen by qtest.

[John Snow]

Currently, the ioapic device can not be found in a qtest environment
when requesting "irq_interrupt_in ioapic" via the qtest socket.

By mirroring how the ioapic is added in i44ofx (hw/i440/pc_piix.c),
as a child of "q35," the device is able to be seen by qtest.

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

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c39ee98..0dd7e02 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -236,7 +236,7 @@ static void pc_q35_init(MachineState *machine)
         gsi_state->i8259_irq[i] = i8259[i];
     }
     if (pci_enabled) {
-        ioapic_init_gsi(gsi_state, NULL);
+        ioapic_init_gsi(gsi_state, "q35");
     }
     qdev_init_nofail(icc_bridge);
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 19/30] qtest: Adding qtest_memset and qmemset.

[John Snow]

Currently, libqtest allows for memread and memwrite, but
does not offer a simple way to zero out regions of memory.
This patch adds a simple function to do so.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqtest.c | 12 ++++++++++++
 tests/libqtest.h | 24 ++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 4a75cd3..e525e6f 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -659,6 +659,18 @@ void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
     qtest_rsp(s, 0);
 }
 
+void qtest_memset(QTestState *s, uint64_t addr, uint8_t pattern, size_t size)
+{
+    size_t i;
+
+    qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x", addr, size);
+    for (i = 0; i < size; i++) {
+        qtest_sendf(s, "%02x", pattern);
+    }
+    qtest_sendf(s, "\n");
+    qtest_rsp(s, 0);
+}
+
 QDict *qmp(const char *fmt, ...)
 {
     va_list ap;
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 8f323c7..1be0934 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -283,6 +283,17 @@ void qtest_memread(QTestState *s, uint64_t addr, void *data, size_t size);
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size);
 
 /**
+ * qtest_memset:
+ * @s: #QTestState instance to operate on.
+ * @addr: Guest address to write to.
+ * @patt: Byte pattern to fill the guest memory region with.
+ * @size: Number of bytes to write.
+ *
+ * Write a pattern to guest memory.
+ */
+void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size);
+
+/**
  * qtest_clock_step_next:
  * @s: #QTestState instance to operate on.
  *
@@ -621,6 +632,19 @@ static inline void memwrite(uint64_t addr, const void *data, size_t size)
 }
 
 /**
+ * qmemset:
+ * @addr: Guest address to write to.
+ * @patt: Byte pattern to fill the guest memory region with.
+ * @size: Number of bytes to write.
+ *
+ * Write a pattern to guest memory.
+ */
+static inline void qmemset(uint64_t addr, uint8_t patt, size_t size)
+{
+    qtest_memset(global_qtest, addr, patt, size);
+}
+
+/**
  * clock_step_next:
  *
  * Advance the QEMU_CLOCK_VIRTUAL to the next deadline.
-- 
1.9.3

[Qemu-devel] [PATCH v2 20/30] libqos: Correct memory leak

[John Snow]

Fix a small memory leak inside of libqos, in the pc_alloc_init routine.

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

diff --git a/tests/libqos/malloc-pc.c b/tests/libqos/malloc-pc.c
index db1496c..63af60a 100644
--- a/tests/libqos/malloc-pc.c
+++ b/tests/libqos/malloc-pc.c
@@ -67,5 +67,8 @@ QGuestAllocator *pc_alloc_init(void)
     /* Respect PCI hole */
     s->end = MIN(ram_size, 0xE0000000);
 
+    /* clean-up */
+    g_free(fw_cfg);
+
     return &s->alloc;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 21/30] libqtest: Correct small memory leak.

[John Snow]

Fixes a small memory leak inside of libqtest.
After we produce a test path and glib copies the string
for itself, we should clean up our temporary copy.

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

diff --git a/tests/libqtest.c b/tests/libqtest.c
index e525e6f..0bf17aa 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -644,6 +644,7 @@ void qtest_add_func(const char *str, void (*fn))
 {
     gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
     g_test_add_func(path, fn);
+    g_free(path);
 }
 
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
-- 
1.9.3

[Qemu-devel] [PATCH v2 22/30] libqos: Fixes a small memory leak.

[John Snow]

Allow users the chance to clean up the QPCIBusPC structure
by adding a small cleanup routine. Helps clear up small
memory leaks during setup/teardown, to allow for cleaner
debug output messages.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/libqos/pci-pc.c | 7 +++++++
 tests/libqos/pci-pc.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index 4adf400..f5d6469 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -237,3 +237,10 @@ QPCIBus *qpci_init_pc(void)
 
     return &ret->bus;
 }
+
+void qpci_free_pc(QPCIBus *bus)
+{
+    QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
+
+    g_free(s);
+}
diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
index 4f7475f..2621179 100644
--- a/tests/libqos/pci-pc.h
+++ b/tests/libqos/pci-pc.h
@@ -16,5 +16,6 @@
 #include "libqos/pci.h"
 
 QPCIBus *qpci_init_pc(void);
+void     qpci_free_pc(QPCIBus *bus);
 
 #endif
-- 
1.9.3

[Qemu-devel] [PATCH v2 23/30] libqos: allow qpci_iomap to return BAR mapping size

[John Snow]

This patch allows qpci_iomap to return the size of the
BAR mapping that it created, to allow driver applications
(e.g, ahci-test) to make determinations about the suitability
or the mapping size, or in the specific case of AHCI, how
many ports are supported by the HBA.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c          | 2 +-
 tests/libqos/pci-pc.c     | 5 ++++-
 tests/libqos/pci.c        | 4 ++--
 tests/libqos/pci.h        | 4 ++--
 tests/usb-hcd-ehci-test.c | 2 +-
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 151ef30..e2b4efc 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -146,7 +146,7 @@ static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
     g_assert(device_id == PCI_DEVICE_ID_INTEL_82371SB_1);
 
     /* Map bmdma BAR */
-    *bmdma_base = (uint16_t)(uintptr_t) qpci_iomap(dev, 4);
+    *bmdma_base = (uint16_t)(uintptr_t) qpci_iomap(dev, 4, NULL);
 
     qpci_device_enable(dev);
 
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
index f5d6469..0609294 100644
--- a/tests/libqos/pci-pc.c
+++ b/tests/libqos/pci-pc.c
@@ -144,7 +144,7 @@ static void qpci_pc_config_writel(QPCIBus *bus, int devfn, uint8_t offset, uint3
     outl(0xcfc, value);
 }
 
-static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno)
+static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr)
 {
     QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
     static const int bar_reg_map[] = {
@@ -173,6 +173,9 @@ static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno)
     if (size == 0) {
         return NULL;
     }
+    if (sizeptr) {
+        *sizeptr = size;
+    }
 
     if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
         uint16_t loc;
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index c9a0b91..ce0b308 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -138,9 +138,9 @@ void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t value)
     dev->bus->io_writel(dev->bus, data, value);
 }
 
-void *qpci_iomap(QPCIDevice *dev, int barno)
+void *qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr)
 {
-    return dev->bus->iomap(dev->bus, dev, barno);
+    return dev->bus->iomap(dev->bus, dev, barno, sizeptr);
 }
 
 void qpci_iounmap(QPCIDevice *dev, void *data)
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h
index 3439431..9ee048b 100644
--- a/tests/libqos/pci.h
+++ b/tests/libqos/pci.h
@@ -41,7 +41,7 @@ struct QPCIBus
     void (*config_writel)(QPCIBus *bus, int devfn,
                           uint8_t offset, uint32_t value);
 
-    void *(*iomap)(QPCIBus *bus, QPCIDevice *dev, int barno);
+    void *(*iomap)(QPCIBus *bus, QPCIDevice *dev, int barno, uint64_t *sizeptr);
     void (*iounmap)(QPCIBus *bus, void *data);
 };
 
@@ -74,7 +74,7 @@ void qpci_io_writeb(QPCIDevice *dev, void *data, uint8_t value);
 void qpci_io_writew(QPCIDevice *dev, void *data, uint16_t value);
 void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t value);
 
-void *qpci_iomap(QPCIDevice *dev, int barno);
+void *qpci_iomap(QPCIDevice *dev, int barno, uint64_t *sizeptr);
 void qpci_iounmap(QPCIDevice *dev, void *data);
 
 #endif
diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
index bcdf62f..c990492 100644
--- a/tests/usb-hcd-ehci-test.c
+++ b/tests/usb-hcd-ehci-test.c
@@ -34,7 +34,7 @@ static void pci_init_one(struct qhc *hc, uint32_t devfn, int bar)
     hc->dev = qpci_device_find(pcibus, devfn);
     g_assert(hc->dev != NULL);
     qpci_device_enable(hc->dev);
-    hc->base = qpci_iomap(hc->dev, bar);
+    hc->base = qpci_iomap(hc->dev, bar, NULL);
     g_assert(hc->base != NULL);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 24/30] qtest/ide: Fix small memory leak

[John Snow]

For libqos debugging purposes, it's nice to
be able to assert that tests and associated libraries
have no memory leaks. To that end, free up the
trivial cmdline leak.

The remaining leaks caused by pc_alloc_init are fixed
instead by my first-fit pc_alloc implementation already
on the qemu-devel mailing list.

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

diff --git a/tests/ide-test.c b/tests/ide-test.c
index e2b4efc..a77a037 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -120,6 +120,8 @@ static void ide_test_start(const char *cmdline_fmt, ...)
     qtest_start(cmdline);
     qtest_irq_intercept_in(global_qtest, "ioapic");
     guest_malloc = pc_alloc_init();
+
+    g_free(cmdline);
 }
 
 static void ide_test_quit(void)
-- 
1.9.3

[Qemu-devel] [PATCH v2 25/30] ahci: Adding basic functionality qtest.

[John Snow]

Currently, there is no qtest to test the functionality of
the AHCI functionality present within the Q35 machine type.

This patch adds a skeleton for an AHCI test suite,
and adds a simple sanity-check test case where we
identify that the AHCI device is present, then
disengage the virtual machine.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile    |   2 +
 tests/ahci-test.c | 196 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 198 insertions(+)
 create mode 100644 tests/ahci-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 4b2e1bb..0c61b22 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -132,6 +132,7 @@ check-qtest-i386-y = tests/endianness-test$(EXESUF)
 check-qtest-i386-y += tests/fdc-test$(EXESUF)
 gcov-files-i386-y = hw/block/fdc.c
 check-qtest-i386-y += tests/ide-test$(EXESUF)
+check-qtest-i386-y += tests/ahci-test$(EXESUF)
 check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
 gcov-files-i386-y += hw/block/hd-geometry.c
 check-qtest-i386-y += tests/boot-order-test$(EXESUF)
@@ -301,6 +302,7 @@ tests/endianness-test$(EXESUF): tests/endianness-test.o
 tests/spapr-phb-test$(EXESUF): tests/spapr-phb-test.o $(libqos-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o
 tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
+tests/ahci-test$(EXESUF): tests/ahci-test.o $(libqos-pc-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
 tests/boot-order-test$(EXESUF): tests/boot-order-test.o $(libqos-obj-y)
 tests/bios-tables-test$(EXESUF): tests/bios-tables-test.o $(libqos-obj-y)
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
new file mode 100644
index 0000000..cc49dba
--- /dev/null
+++ b/tests/ahci-test.c
@@ -0,0 +1,196 @@
+/*
+ * AHCI test cases
+ *
+ * Copyright (c) 2014 John Snow <jsnow@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <stdint.h>
+#include <string.h>
+#include <stdio.h>
+
+#include <glib.h>
+
+#include "libqtest.h"
+#include "libqos/pci-pc.h"
+#include "libqos/malloc-pc.h"
+
+#include "qemu-common.h"
+#include "qemu/host-utils.h"
+
+#include "hw/pci/pci_ids.h"
+#include "hw/pci/pci_regs.h"
+
+/* Test-specific defines. */
+#define TEST_IMAGE_SIZE    (64 * 1024 * 1024)
+
+/*** Supplementary PCI Config Space IDs & Masks ***/
+#define PCI_DEVICE_ID_INTEL_Q35_AHCI   (0x2922)
+
+/*** Globals ***/
+static QGuestAllocator *guest_malloc;
+static QPCIBus *pcibus;
+static char tmp_path[] = "/tmp/qtest.XXXXXX";
+
+/*** Function Declarations ***/
+static QPCIDevice *get_ahci_device(void);
+static void free_ahci_device(QPCIDevice *dev);
+
+/*** Utilities ***/
+
+/**
+ * Locate, verify, and return a handle to the AHCI device.
+ */
+static QPCIDevice *get_ahci_device(void)
+{
+    QPCIDevice *ahci;
+    uint16_t vendor_id, device_id;
+
+    pcibus = qpci_init_pc();
+
+    /* Find the AHCI PCI device and verify it's the right one. */
+    ahci = qpci_device_find(pcibus, QPCI_DEVFN(0x1F, 0x02));
+    g_assert(ahci != NULL);
+
+    vendor_id = qpci_config_readw(ahci, PCI_VENDOR_ID);
+    device_id = qpci_config_readw(ahci, PCI_DEVICE_ID);
+
+    g_assert_cmphex(vendor_id, ==, PCI_VENDOR_ID_INTEL);
+    g_assert_cmphex(device_id, ==, PCI_DEVICE_ID_INTEL_Q35_AHCI);
+
+    return ahci;
+}
+
+static void free_ahci_device(QPCIDevice *ahci)
+{
+    /* libqos doesn't have a function for this, so free it manually */
+    g_free(ahci);
+
+    if (pcibus) {
+        qpci_free_pc(pcibus);
+        pcibus = NULL;
+    }
+}
+
+/*** Test Setup & Teardown ***/
+
+/**
+ * Launch QEMU with the given command line,
+ * and then set up interrupts and our guest malloc interface.
+ */
+static void qtest_boot(const char *cmdline_fmt, ...)
+{
+    va_list ap;
+    char *cmdline;
+
+    va_start(ap, cmdline_fmt);
+    cmdline = g_strdup_vprintf(cmdline_fmt, ap);
+    va_end(ap);
+
+    qtest_start(cmdline);
+    qtest_irq_intercept_in(global_qtest, "ioapic");
+    guest_malloc = pc_alloc_init();
+
+    g_free(cmdline);
+}
+
+/**
+ * Tear down the QEMU instance.
+ */
+static void qtest_shutdown(void)
+{
+    g_free(guest_malloc);
+    guest_malloc = NULL;
+    qtest_end();
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the AHCI device.
+ */
+static QPCIDevice *ahci_boot(void)
+{
+    qtest_boot("-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
+               " -M q35 "
+               "-device ide-hd,drive=drive0 "
+               "-global ide-hd.ver=%s",
+               tmp_path, "testdisk", "version");
+
+    /* Verify that we have an AHCI device present. */
+    return get_ahci_device();
+}
+
+/**
+ * Clean up the PCI device, then terminate the QEMU instance.
+ */
+static void ahci_shutdown(QPCIDevice *ahci)
+{
+    free_ahci_device(ahci);
+    qtest_shutdown();
+}
+
+/******************************************************************************/
+/* Test Interfaces                                                            */
+/******************************************************************************/
+
+/**
+ * Basic sanity test to boot a machine, find an AHCI device, and shutdown.
+ */
+static void test_sanity(void)
+{
+    QPCIDevice *ahci;
+    ahci = ahci_boot();
+    ahci_shutdown(ahci);
+}
+
+/******************************************************************************/
+
+int main(int argc, char **argv)
+{
+    const char *arch;
+    int fd;
+    int ret;
+
+    /* Should be first to utilize g_test functionality, So we can see errors. */
+    g_test_init(&argc, &argv, NULL);
+
+    /* Check architecture */
+    arch = qtest_get_arch();
+    if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
+        g_test_message("Skipping test for non-x86");
+        return 0;
+    }
+
+    /* Create a temporary raw image */
+    fd = mkstemp(tmp_path);
+    g_assert(fd >= 0);
+    ret = ftruncate(fd, TEST_IMAGE_SIZE);
+    g_assert(ret == 0);
+    close(fd);
+
+    /* Run the tests */
+    qtest_add_func("/ahci/sanity",     test_sanity);
+
+    ret = g_test_run();
+
+    /* Cleanup */
+    unlink(tmp_path);
+
+    return ret;
+}
-- 
1.9.3

[Qemu-devel] [PATCH v2 26/30] ahci: Add test_pci_spec to ahci-test.

[John Snow]

Adds a specification adherence test for AHCI
where the boot-up values for the PCI configuration space
are compared against the AHCI 1.3 specification.

This test does not itself attempt to engage the device.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index cc49dba..29ac0d0 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -25,7 +25,7 @@
 #include <stdint.h>
 #include <string.h>
 #include <stdio.h>
-
+#include <getopt.h>
 #include <glib.h>
 
 #include "libqtest.h"
@@ -43,15 +43,36 @@
 
 /*** Supplementary PCI Config Space IDs & Masks ***/
 #define PCI_DEVICE_ID_INTEL_Q35_AHCI   (0x2922)
+#define PCI_MSI_FLAGS_RESERVED         (0xFF00)
+#define PCI_PM_CTRL_RESERVED             (0xFC)
+#define PCI_BCC(REG32)          ((REG32) >> 24)
+#define PCI_PI(REG32)   (((REG32) >> 8) & 0xFF)
+#define PCI_SCC(REG32) (((REG32) >> 16) & 0xFF)
+
+/*** Recognized AHCI Device Types ***/
+#define AHCI_INTEL_ICH9 (PCI_DEVICE_ID_INTEL_Q35_AHCI << 16 | \
+                         PCI_VENDOR_ID_INTEL)
 
 /*** Globals ***/
 static QGuestAllocator *guest_malloc;
 static QPCIBus *pcibus;
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
+static bool ahci_pedantic;
+static uint32_t ahci_fingerprint;
+
+/*** Macro Utilities ***/
+#define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, (mask))
+#define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
 
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
 static void free_ahci_device(QPCIDevice *dev);
+static void ahci_test_pci_spec(QPCIDevice *ahci);
+static void ahci_test_pci_caps(QPCIDevice *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);
 
 /*** Utilities ***/
 
@@ -61,7 +82,6 @@ static void free_ahci_device(QPCIDevice *dev);
 static QPCIDevice *get_ahci_device(void)
 {
     QPCIDevice *ahci;
-    uint16_t vendor_id, device_id;
 
     pcibus = qpci_init_pc();
 
@@ -69,11 +89,15 @@ static QPCIDevice *get_ahci_device(void)
     ahci = qpci_device_find(pcibus, QPCI_DEVFN(0x1F, 0x02));
     g_assert(ahci != NULL);
 
-    vendor_id = qpci_config_readw(ahci, PCI_VENDOR_ID);
-    device_id = qpci_config_readw(ahci, PCI_DEVICE_ID);
+    ahci_fingerprint = qpci_config_readl(ahci, PCI_VENDOR_ID);
 
-    g_assert_cmphex(vendor_id, ==, PCI_VENDOR_ID_INTEL);
-    g_assert_cmphex(device_id, ==, PCI_DEVICE_ID_INTEL_Q35_AHCI);
+    switch (ahci_fingerprint) {
+    case AHCI_INTEL_ICH9:
+        break;
+    default:
+        /* Unknown device. */
+        g_assert_not_reached();
+    }
 
     return ahci;
 }
@@ -145,6 +169,239 @@ static void ahci_shutdown(QPCIDevice *ahci)
     qtest_shutdown();
 }
 
+/*** Specification Adherence Tests ***/
+
+/**
+ * Implementation for test_pci_spec. Ensures PCI configuration space is sane.
+ */
+static void ahci_test_pci_spec(QPCIDevice *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);
+    assert_bit_clear(data, PCI_COMMAND_MEMORY);
+    assert_bit_clear(data, PCI_COMMAND_MASTER);
+    assert_bit_clear(data, PCI_COMMAND_SPECIAL);     /* Reserved */
+    assert_bit_clear(data, PCI_COMMAND_VGA_PALETTE); /* Reserved */
+    assert_bit_clear(data, PCI_COMMAND_PARITY);
+    assert_bit_clear(data, PCI_COMMAND_WAIT);        /* Reserved */
+    assert_bit_clear(data, PCI_COMMAND_SERR);
+    assert_bit_clear(data, PCI_COMMAND_FAST_BACK);
+    assert_bit_clear(data, PCI_COMMAND_INTX_DISABLE);
+    assert_bit_clear(data, 0xF800);                  /* Reserved */
+
+    data = qpci_config_readw(ahci, 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 */
+    assert_bit_clear(data, PCI_STATUS_UDF);         /* Reserved */
+    assert_bit_clear(data, PCI_STATUS_PARITY);
+    assert_bit_clear(data, PCI_STATUS_SIG_TARGET_ABORT);
+    assert_bit_clear(data, PCI_STATUS_REC_TARGET_ABORT);
+    assert_bit_clear(data, PCI_STATUS_REC_MASTER_ABORT);
+    assert_bit_clear(data, PCI_STATUS_SIG_SYSTEM_ERROR);
+    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);
+    if (ahci_pedantic) {
+        /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00,
+         * Though in practice this is likely seldom true. */
+        assert_bit_clear(datal, 0xFF);
+    }
+
+    /* BCC *must* equal 0x01. */
+    g_assert_cmphex(PCI_BCC(datal), ==, 0x01);
+    if (PCI_SCC(datal) == 0x01) {
+        /* IDE */
+        assert_bit_set(0x80000000, datal);
+        assert_bit_clear(0x60000000, datal);
+    } else if (PCI_SCC(datal) == 0x04) {
+        /* RAID */
+        g_assert_cmphex(PCI_PI(datal), ==, 0);
+    } else if (PCI_SCC(datal) == 0x06) {
+        /* AHCI */
+        g_assert_cmphex(PCI_PI(datal), ==, 0x01);
+    } else {
+        g_assert_not_reached();
+    }
+
+    datab = qpci_config_readb(ahci, PCI_CACHE_LINE_SIZE);
+    g_assert_cmphex(datab, ==, 0);
+
+    datab = qpci_config_readb(ahci, PCI_LATENCY_TIMER);
+    g_assert_cmphex(datab, ==, 0);
+
+    /* Only the bottom 7 bits must be off. */
+    datab = qpci_config_readb(ahci, 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);
+    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);
+    g_assert_cmphex(datal, ==, 0);
+
+    qpci_config_writel(ahci, PCI_BASE_ADDRESS_5, 0xFFFFFFFF);
+    datal = qpci_config_readl(ahci, 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);
+    /* 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);
+
+    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);
+        break;
+    default:
+        /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */
+        g_assert_cmphex((data & 0xFF), ==, PCI_CAP_ID_PM);
+    }
+
+    ahci_test_pci_caps(ahci, data, (uint8_t)datal);
+
+    /* Reserved. */
+    datal = qpci_config_readl(ahci, 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);
+    g_assert_cmphex(datab, ==, 0);
+}
+
+/**
+ * Test PCI capabilities for AHCI specification adherence.
+ */
+static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
+                               uint8_t offset)
+{
+    uint8_t cid = header & 0xFF;
+    uint8_t next = header >> 8;
+
+    g_test_message("CID: %02x; next: %02x", cid, next);
+
+    switch (cid) {
+    case PCI_CAP_ID_PM:
+        ahci_test_pmcap(ahci, offset);
+        break;
+    case PCI_CAP_ID_MSI:
+        ahci_test_msicap(ahci, offset);
+        break;
+    case PCI_CAP_ID_SATA:
+        ahci_test_satacap(ahci, offset);
+        break;
+
+    default:
+        g_test_message("Unknown CAP 0x%02x", cid);
+    }
+
+    if (next) {
+        ahci_test_pci_caps(ahci, qpci_config_readw(ahci, next), next);
+    }
+}
+
+/**
+ * Test SATA PCI capabilitity for AHCI specification adherence.
+ */
+static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset)
+{
+    uint16_t dataw;
+    uint32_t datal;
+
+    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);
+    g_assert_cmphex(dataw, ==, 0x10);
+
+    /* Grab the SATACR1 register. */
+    datal = qpci_config_readw(ahci, offset + 4);
+
+    switch (datal & 0x0F) {
+    case 0x04: /* BAR0 */
+    case 0x05: /* BAR1 */
+    case 0x06:
+    case 0x07:
+    case 0x08:
+    case 0x09: /* BAR5 */
+    case 0x0F: /* Immediately following SATACR1 in PCI config space. */
+        break;
+    default:
+        /* Invalid BARLOC for the Index Data Pair. */
+        g_assert_not_reached();
+    }
+
+    /* Reserved. */
+    g_assert_cmphex((datal >> 24), ==, 0x00);
+}
+
+/**
+ * Test MSI PCI capability for AHCI specification adherence.
+ */
+static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset)
+{
+    uint16_t dataw;
+    uint32_t datal;
+
+    g_test_message("Verifying MSICAP");
+
+    dataw = qpci_config_readw(ahci, 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);
+    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);
+        g_assert_cmphex(datal, ==, 0);
+        dataw = qpci_config_readw(ahci, 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);
+        g_assert_cmphex(dataw, ==, 0);
+    }
+}
+
+/**
+ * Test Power Management PCI capability for AHCI specification adherence.
+ */
+static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset)
+{
+    uint16_t dataw;
+
+    g_test_message("Verifying PMCAP");
+
+    dataw = qpci_config_readw(ahci, 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);
+    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);
+}
+
 /******************************************************************************/
 /* Test Interfaces                                                            */
 /******************************************************************************/
@@ -159,6 +416,18 @@ static void test_sanity(void)
     ahci_shutdown(ahci);
 }
 
+/**
+ * Ensure that the PCI configuration space for the AHCI device is in-line with
+ * the AHCI 1.3 specification for initial values.
+ */
+static void test_pci_spec(void)
+{
+    QPCIDevice *ahci;
+    ahci = ahci_boot();
+    ahci_test_pci_spec(ahci);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -166,10 +435,33 @@ int main(int argc, char **argv)
     const char *arch;
     int fd;
     int ret;
+    int c;
+
+    static struct option long_options[] = {
+        {"pedantic", no_argument, 0, 'p' },
+        {0, 0, 0, 0},
+    };
 
     /* Should be first to utilize g_test functionality, So we can see errors. */
     g_test_init(&argc, &argv, NULL);
 
+    while (1) {
+        c = getopt_long(argc, argv, "", long_options, NULL);
+        if (c == -1) {
+            break;
+        }
+        switch (c) {
+        case -1:
+            break;
+        case 'p':
+            ahci_pedantic = 1;
+            break;
+        default:
+            fprintf(stderr, "Unrecognized ahci_test option.\n");
+            g_assert_not_reached();
+        }
+    }
+
     /* Check architecture */
     arch = qtest_get_arch();
     if (strcmp(arch, "i386") && strcmp(arch, "x86_64")) {
@@ -186,6 +478,7 @@ int main(int argc, char **argv)
 
     /* Run the tests */
     qtest_add_func("/ahci/sanity",     test_sanity);
+    qtest_add_func("/ahci/pci_spec",   test_pci_spec);
 
     ret = g_test_run();
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 27/30] ahci: add test_pci_enable to ahci-test.

[John Snow]

This adds a test wherein we engage the PCI AHCI
device and ensure that the memory region for the
HBA functionality is now accessible.

Under Q35 environments, additional PCI configuration
is performed to ensure that the HBA functionality
will become usable.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/pci.c |  6 ++++++
 2 files changed, 63 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 29ac0d0..0ca4a20 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -56,6 +56,7 @@
 /*** 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;
@@ -66,6 +67,7 @@ static uint32_t ahci_fingerprint;
 
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
+static QPCIDevice *start_ahci_device(QPCIDevice *dev, void **hba_base);
 static void free_ahci_device(QPCIDevice *dev);
 static void ahci_test_pci_spec(QPCIDevice *ahci);
 static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
@@ -111,6 +113,9 @@ static void free_ahci_device(QPCIDevice *ahci)
         qpci_free_pc(pcibus);
         pcibus = NULL;
     }
+
+    /* Clear our cached barsize information. */
+    barsize = 0;
 }
 
 /*** Test Setup & Teardown ***/
@@ -169,6 +174,44 @@ static void ahci_shutdown(QPCIDevice *ahci)
     qtest_shutdown();
 }
 
+/*** Logical Device Initialization ***/
+
+/**
+ * Start the PCI device and sanity-check default operation.
+ */
+static void ahci_pci_enable(QPCIDevice *ahci, void **hba_base)
+{
+    uint8_t reg;
+
+    start_ahci_device(ahci, hba_base);
+
+    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 |= 0x3F;
+        qpci_config_writeb(ahci, 0x92, reg);
+        assert_bit_set(qpci_config_readb(ahci, 0x92), 0x3F);
+        break;
+    }
+
+}
+
+/**
+ * Map BAR5/ABAR, and engage the PCI device.
+ */
+static QPCIDevice *start_ahci_device(QPCIDevice *ahci, void **hba_base)
+{
+    /* Map AHCI's ABAR (BAR5) */
+    *hba_base = qpci_iomap(ahci, 5, &barsize);
+
+    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
+    qpci_device_enable(ahci);
+
+    return ahci;
+}
+
 /*** Specification Adherence Tests ***/
 
 /**
@@ -428,6 +471,19 @@ static void test_pci_spec(void)
     ahci_shutdown(ahci);
 }
 
+/**
+ * Engage the PCI AHCI device and sanity check the response.
+ * Perform additional PCI config space bringup for the HBA.
+ */
+static void test_pci_enable(void)
+{
+    QPCIDevice *ahci;
+    void *hba_base;
+    ahci = ahci_boot();
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -479,6 +535,7 @@ int main(int argc, char **argv)
     /* Run the tests */
     qtest_add_func("/ahci/sanity",     test_sanity);
     qtest_add_func("/ahci/pci_spec",   test_pci_spec);
+    qtest_add_func("/ahci/pci_enable", test_pci_enable);
 
     ret = g_test_run();
 
diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c
index ce0b308..b244689 100644
--- a/tests/libqos/pci.c
+++ b/tests/libqos/pci.c
@@ -73,6 +73,12 @@ void qpci_device_enable(QPCIDevice *dev)
     cmd = qpci_config_readw(dev, PCI_COMMAND);
     cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
     qpci_config_writew(dev, PCI_COMMAND, cmd);
+
+    /* Verify the bits are now set. */
+    cmd = qpci_config_readw(dev, PCI_COMMAND);
+    g_assert_cmphex(cmd & PCI_COMMAND_IO, ==, PCI_COMMAND_IO);
+    g_assert_cmphex(cmd & PCI_COMMAND_MEMORY, ==, PCI_COMMAND_MEMORY);
+    g_assert_cmphex(cmd & PCI_COMMAND_MASTER, ==, PCI_COMMAND_MASTER);
 }
 
 uint8_t qpci_config_readb(QPCIDevice *dev, uint8_t offset)
-- 
1.9.3

[Qemu-devel] [PATCH v2 28/30] ahci: Add test_hba_spec to ahci-test.

[John Snow]

Add a test routine that checks the boot-up values of the HBA
configuration memory space against the AHCI 1.3 specification
and Intel ICH9 data sheet (for Q35 machines) for adherence and
sane values.

The HBA is not yet engaged or put into the idle state.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 0ca4a20..1c19c9c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -53,6 +53,210 @@
 #define AHCI_INTEL_ICH9 (PCI_DEVICE_ID_INTEL_Q35_AHCI << 16 | \
                          PCI_VENDOR_ID_INTEL)
 
+/*** AHCI/HBA Register Offsets and Bitmasks ***/
+#define AHCI_CAP                          (0)
+#define AHCI_CAP_NP                    (0x1F)
+#define AHCI_CAP_SXS                   (0x20)
+#define AHCI_CAP_EMS                   (0x40)
+#define AHCI_CAP_CCCS                  (0x80)
+#define AHCI_CAP_NCS                 (0x1F00)
+#define AHCI_CAP_PSC                 (0x2000)
+#define AHCI_CAP_SSC                 (0x4000)
+#define AHCI_CAP_PMD                 (0x8000)
+#define AHCI_CAP_FBSS               (0x10000)
+#define AHCI_CAP_SPM                (0x20000)
+#define AHCI_CAP_SAM                (0x40000)
+#define AHCI_CAP_RESERVED           (0x80000)
+#define AHCI_CAP_ISS               (0xF00000)
+#define AHCI_CAP_SCLO             (0x1000000)
+#define AHCI_CAP_SAL              (0x2000000)
+#define AHCI_CAP_SALP             (0x4000000)
+#define AHCI_CAP_SSS              (0x8000000)
+#define AHCI_CAP_SMPS            (0x10000000)
+#define AHCI_CAP_SSNTF           (0x20000000)
+#define AHCI_CAP_SNCQ            (0x40000000)
+#define AHCI_CAP_S64A            (0x80000000)
+
+#define AHCI_GHC                          (1)
+#define AHCI_GHC_HR                    (0x01)
+#define AHCI_GHC_IE                    (0x02)
+#define AHCI_GHC_MRSM                  (0x04)
+#define AHCI_GHC_RESERVED        (0x7FFFFFF8)
+#define AHCI_GHC_AE              (0x80000000)
+
+#define AHCI_IS                           (2)
+#define AHCI_PI                           (3)
+#define AHCI_VS                           (4)
+
+#define AHCI_CCCCTL                       (5)
+#define AHCI_CCCCTL_EN                 (0x01)
+#define AHCI_CCCCTL_RESERVED           (0x06)
+#define AHCI_CCCCTL_CC               (0xFF00)
+#define AHCI_CCCCTL_TV           (0xFFFF0000)
+
+#define AHCI_CCCPORTS                     (6)
+#define AHCI_EMLOC                        (7)
+
+#define AHCI_EMCTL                        (8)
+#define AHCI_EMCTL_STSMR               (0x01)
+#define AHCI_EMCTL_CTLTM              (0x100)
+#define AHCI_EMCTL_CTLRST             (0x200)
+#define AHCI_EMCTL_RESERVED      (0xF0F0FCFE)
+
+#define AHCI_CAP2                         (9)
+#define AHCI_CAP2_BOH                  (0x01)
+#define AHCI_CAP2_NVMP                 (0x02)
+#define AHCI_CAP2_APST                 (0x04)
+#define AHCI_CAP2_RESERVED       (0xFFFFFFF8)
+
+#define AHCI_BOHC                        (10)
+#define AHCI_RESERVED                    (11)
+#define AHCI_NVMHCI                      (24)
+#define AHCI_VENDOR                      (40)
+#define AHCI_PORTS                       (64)
+
+/*** Port Memory Offsets & Bitmasks ***/
+#define AHCI_PX_CLB                       (0)
+#define AHCI_PX_CLB_RESERVED          (0x1FF)
+
+#define AHCI_PX_CLBU                      (1)
+
+#define AHCI_PX_FB                        (2)
+#define AHCI_PX_FB_RESERVED            (0xFF)
+
+#define AHCI_PX_FBU                       (3)
+
+#define AHCI_PX_IS                        (4)
+#define AHCI_PX_IS_DHRS                 (0x1)
+#define AHCI_PX_IS_PSS                  (0x2)
+#define AHCI_PX_IS_DSS                  (0x4)
+#define AHCI_PX_IS_SDBS                 (0x8)
+#define AHCI_PX_IS_UFS                 (0x10)
+#define AHCI_PX_IS_DPS                 (0x20)
+#define AHCI_PX_IS_PCS                 (0x40)
+#define AHCI_PX_IS_DMPS                (0x80)
+#define AHCI_PX_IS_RESERVED       (0x23FFF00)
+#define AHCI_PX_IS_PRCS            (0x400000)
+#define AHCI_PX_IS_IPMS            (0x800000)
+#define AHCI_PX_IS_OFS            (0x1000000)
+#define AHCI_PX_IS_INFS           (0x4000000)
+#define AHCI_PX_IS_IFS            (0x8000000)
+#define AHCI_PX_IS_HBDS          (0x10000000)
+#define AHCI_PX_IS_HBFS          (0x20000000)
+#define AHCI_PX_IS_TFES          (0x40000000)
+#define AHCI_PX_IS_CPDS          (0x80000000)
+
+#define AHCI_PX_IE                        (5)
+#define AHCI_PX_IE_DHRE                 (0x1)
+#define AHCI_PX_IE_PSE                  (0x2)
+#define AHCI_PX_IE_DSE                  (0x4)
+#define AHCI_PX_IE_SDBE                 (0x8)
+#define AHCI_PX_IE_UFE                 (0x10)
+#define AHCI_PX_IE_DPE                 (0x20)
+#define AHCI_PX_IE_PCE                 (0x40)
+#define AHCI_PX_IE_DMPE                (0x80)
+#define AHCI_PX_IE_RESERVED       (0x23FFF00)
+#define AHCI_PX_IE_PRCE            (0x400000)
+#define AHCI_PX_IE_IPME            (0x800000)
+#define AHCI_PX_IE_OFE            (0x1000000)
+#define AHCI_PX_IE_INFE           (0x4000000)
+#define AHCI_PX_IE_IFE            (0x8000000)
+#define AHCI_PX_IE_HBDE          (0x10000000)
+#define AHCI_PX_IE_HBFE          (0x20000000)
+#define AHCI_PX_IE_TFEE          (0x40000000)
+#define AHCI_PX_IE_CPDE          (0x80000000)
+
+#define AHCI_PX_CMD                       (6)
+#define AHCI_PX_CMD_ST                  (0x1)
+#define AHCI_PX_CMD_SUD                 (0x2)
+#define AHCI_PX_CMD_POD                 (0x4)
+#define AHCI_PX_CMD_CLO                 (0x8)
+#define AHCI_PX_CMD_FRE                (0x10)
+#define AHCI_PX_CMD_RESERVED           (0xE0)
+#define AHCI_PX_CMD_CCS              (0x1F00)
+#define AHCI_PX_CMD_MPSS             (0x2000)
+#define AHCI_PX_CMD_FR               (0x4000)
+#define AHCI_PX_CMD_CR               (0x8000)
+#define AHCI_PX_CMD_CPS             (0x10000)
+#define AHCI_PX_CMD_PMA             (0x20000)
+#define AHCI_PX_CMD_HPCP            (0x40000)
+#define AHCI_PX_CMD_MPSP            (0x80000)
+#define AHCI_PX_CMD_CPD            (0x100000)
+#define AHCI_PX_CMD_ESP            (0x200000)
+#define AHCI_PX_CMD_FBSCP          (0x400000)
+#define AHCI_PX_CMD_APSTE          (0x800000)
+#define AHCI_PX_CMD_ATAPI         (0x1000000)
+#define AHCI_PX_CMD_DLAE          (0x2000000)
+#define AHCI_PX_CMD_ALPE          (0x4000000)
+#define AHCI_PX_CMD_ASP           (0x8000000)
+#define AHCI_PX_CMD_ICC          (0xF0000000)
+
+#define AHCI_PX_RES1                      (7)
+
+#define AHCI_PX_TFD                       (8)
+#define AHCI_PX_TFD_STS                (0xFF)
+#define AHCI_PX_TFD_STS_ERR            (0x01)
+#define AHCI_PX_TFD_STS_CS1            (0x06)
+#define AHCI_PX_TFD_STS_DRQ            (0x08)
+#define AHCI_PX_TFD_STS_CS2            (0x70)
+#define AHCI_PX_TFD_STS_BSY            (0x80)
+#define AHCI_PX_TFD_ERR              (0xFF00)
+#define AHCI_PX_TFD_RESERVED     (0xFFFF0000)
+
+#define AHCI_PX_SIG                       (9)
+#define AHCI_PX_SIG_SECTOR_COUNT       (0xFF)
+#define AHCI_PX_SIG_LBA_LOW          (0xFF00)
+#define AHCI_PX_SIG_LBA_MID        (0xFF0000)
+#define AHCI_PX_SIG_LBA_HIGH     (0xFF000000)
+
+#define AHCI_PX_SSTS                     (10)
+#define AHCI_PX_SSTS_DET               (0x0F)
+#define AHCI_PX_SSTS_SPD               (0xF0)
+#define AHCI_PX_SSTS_IPM              (0xF00)
+#define AHCI_PX_SSTS_RESERVED    (0xFFFFF000)
+#define SSTS_DET_NO_DEVICE             (0x00)
+#define SSTS_DET_PRESENT               (0x01)
+#define SSTS_DET_ESTABLISHED           (0x03)
+#define SSTS_DET_OFFLINE               (0x04)
+
+#define AHCI_PX_SCTL                     (11)
+
+#define AHCI_PX_SERR                     (12)
+#define AHCI_PX_SERR_ERR             (0xFFFF)
+#define AHCI_PX_SERR_DIAG        (0xFFFF0000)
+#define  AHCI_PX_SERR_DIAG_X     (0x04000000)
+
+#define AHCI_PX_SACT                     (13)
+#define AHCI_PX_CI                       (14)
+#define AHCI_PX_SNTF                     (15)
+
+#define AHCI_PX_FBS                      (16)
+#define AHCI_PX_FBS_EN                  (0x1)
+#define AHCI_PX_FBS_DEC                 (0x2)
+#define AHCI_PX_FBS_SDE                 (0x4)
+#define AHCI_PX_FBS_DEV               (0xF00)
+#define AHCI_PX_FBS_ADO              (0xF000)
+#define AHCI_PX_FBS_DWE             (0xF0000)
+#define AHCI_PX_FBS_RESERVED     (0xFFF000F8)
+
+#define AHCI_PX_RES2                     (17)
+#define AHCI_PX_VS                       (28)
+
+#define HBA_DATA_REGION_SIZE            (256)
+#define HBA_PORT_DATA_SIZE              (128)
+#define HBA_PORT_NUM_REG (HBA_PORT_DATA_SIZE/4)
+
+#define AHCI_VERSION_0_95        (0x00000905)
+#define AHCI_VERSION_1_0         (0x00010000)
+#define AHCI_VERSION_1_1         (0x00010100)
+#define AHCI_VERSION_1_2         (0x00010200)
+#define AHCI_VERSION_1_3         (0x00010300)
+
+typedef struct hba_cap {
+    uint32_t cap;
+    uint32_t cap2;
+} hba_cap;
+
 /*** Globals ***/
 static QGuestAllocator *guest_malloc;
 static QPCIBus *pcibus;
@@ -62,13 +266,29 @@ static bool ahci_pedantic;
 static uint32_t ahci_fingerprint;
 
 /*** Macro Utilities ***/
+#define bitany(data, mask) (((data) & (mask)) != 0)
+#define bitset(data, mask) (((data) & (mask)) == (mask))
+#define bitclr(data, mask) (((data) & (mask)) == 0)
 #define assert_bit_set(data, mask) g_assert_cmphex((data) & (mask), ==, (mask))
 #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_rreg(regno)      ahci_read(4 * (regno))
+#define ahci_wreg(regno, val) ahci_write(4 * (regno), (val))
+
+/*** IO macros for port-specific offsets inside of AHCI memory. ***/
+#define px_ofst(port, regno) (HBA_PORT_NUM_REG * (port) + AHCI_PORTS + (regno))
+#define px_rreg(port, regno)      ahci_rreg(px_ofst((port), (regno)))
+#define px_wreg(port, regno, val) ahci_wreg(px_ofst((port), (regno)), (val))
+
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
 static QPCIDevice *start_ahci_device(QPCIDevice *dev, void **hba_base);
 static void free_ahci_device(QPCIDevice *dev);
+static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
+                                hba_cap *hcap, uint8_t port);
 static void ahci_test_pci_spec(QPCIDevice *ahci);
 static void ahci_test_pci_caps(QPCIDevice *ahci, uint16_t header,
                                uint8_t offset);
@@ -445,6 +665,327 @@ static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset)
     assert_bit_clear(dataw, PCI_PM_CTRL_DATA_SCALE_MASK);
 }
 
+static void ahci_test_hba_spec(QPCIDevice *ahci, void *hba_base)
+{
+    struct hba_cap hcap;
+    unsigned i;
+    uint32_t cap, cap2, reg;
+    uint32_t ports;
+    uint8_t nports_impl;
+    uint8_t maxports;
+
+    g_assert(ahci != 0);
+    g_assert(hba_base != 0);
+
+    /*
+     * Note that the AHCI spec does expect the BIOS to set up a few things:
+     * CAP.SSS    - Support for staggered spin-up            (t/f)
+     * CAP.SMPS   - Support for mechanical presence switches (t/f)
+     * PI         - Ports Implemented                        (1-32)
+     * PxCMD.HPCP - Hot Plug Capable Port
+     * PxCMD.MPSP - Mechanical Presence Switch Present
+     * PxCMD.CPD  - Cold Presence Detection support
+     *
+     * Additional items are touched if CAP.SSS is on, see AHCI 10.1.1 p.97:
+     * Foreach Port Implemented:
+     * -PxCMD.ST, PxCMD.CR, PxCMD.FRE, PxCMD.FR, PxSCTL.DET are 0
+     * -PxCLB/U and PxFB/U are set to valid regions in memory
+     * -PxSUD is set to 1.
+     * -PxSSTS.DET is polled for presence; if detected, we continue:
+     * -PxSERR is cleared with 1's.
+     * -If PxTFD.STS.BSY, PxTFD.STS.DRQ, and PxTFD.STS.ERR are all zero,
+     *  the device is ready.
+     */
+
+    /* 1 CAP - Capabilities Register */
+    cap = ahci_rreg(AHCI_CAP);
+    assert_bit_clear(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)) {
+        g_test_message("Supports AHCI-Only Mode: GHC_AE is Read-Only.");
+        assert_bit_set(reg, AHCI_GHC_AE);
+    } else {
+        g_test_message("Supports AHCI/Legacy mix.");
+        assert_bit_clear(reg, AHCI_GHC_AE);
+    }
+
+    /* 3 IS - Interrupt Status */
+    reg = ahci_rreg(AHCI_IS);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* 4 PI - Ports Implemented */
+    ports = ahci_rreg(AHCI_PI);
+    /* Ports Implemented must be non-zero. */
+    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_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;
+    /* e.g, 30 ports for 4K of memory. (4096 - 256) / 128 = 30 */
+    g_assert_cmphex((reg >> maxports), ==, 0);
+
+    /* 5 AHCI Version */
+    reg = ahci_rreg(AHCI_VS);
+    switch (reg) {
+    case AHCI_VERSION_0_95:
+    case AHCI_VERSION_1_0:
+    case AHCI_VERSION_1_1:
+    case AHCI_VERSION_1_2:
+    case AHCI_VERSION_1_3:
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    /* 6 Command Completion Coalescing Control: depends on CAP.CCCS. */
+    reg = ahci_rreg(AHCI_CCCCTL);
+    if (bitset(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);
+        assert_bit_set(reg, AHCI_CCCCTL_TV);
+    } else {
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* 7 CCC_PORTS */
+    reg = ahci_rreg(AHCI_CCCPORTS);
+    /* Must be zeroes initially regardless of CAP.CCCS */
+    g_assert_cmphex(reg, ==, 0);
+
+    /* 8 EM_LOC */
+    reg = ahci_rreg(AHCI_EMLOC);
+    if (bitclr(cap, AHCI_CAP_EMS)) {
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* 9 EM_CTL */
+    reg = ahci_rreg(AHCI_EMCTL);
+    if (bitset(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);
+        assert_bit_clear(reg, AHCI_EMCTL_RESERVED);
+    } else {
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* 10 CAP2 -- Capabilities Extended */
+    cap2 = ahci_rreg(AHCI_CAP2);
+    assert_bit_clear(cap2, AHCI_CAP2_RESERVED);
+
+    /* 11 BOHC -- Bios/OS Handoff Control */
+    reg = ahci_rreg(AHCI_BOHC);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* 12 -- 23: Reserved */
+    g_test_message("Verifying HBA reserved area is empty.");
+    for (i = AHCI_RESERVED; i < AHCI_NVMHCI; ++i) {
+        reg = ahci_rreg(i);
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* 24 -- 39: NVMHCI */
+    if (bitclr(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);
+            g_assert_cmphex(reg, ==, 0);
+        }
+    }
+
+    /* 40 -- 63: Vendor */
+    g_test_message("Verifying HBA/Vendor area is empty.");
+    for (i = AHCI_VENDOR; i < AHCI_PORTS; ++i) {
+        reg = ahci_rreg(i);
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* 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);
+        } else {
+            uint16_t j;
+            uint16_t low = AHCI_PORTS + (32 * i);
+            uint16_t high = AHCI_PORTS + (32 * (i + 1));
+            g_test_message("Asserting unimplemented port %u "
+                           "(reg [%u-%u]) is empty.",
+                           i, low, high - 1);
+            for (j = low; j < high; ++j) {
+                reg = ahci_rreg(j);
+                g_assert_cmphex(reg, ==, 0);
+            }
+        }
+    }
+}
+
+/**
+ * Test the memory space for one port for specification adherence.
+ */
+static void ahci_test_port_spec(QPCIDevice *ahci, void *hba_base,
+                                hba_cap *hcap, uint8_t port)
+{
+    uint32_t reg;
+    unsigned i;
+
+    /* (0) CLB */
+    reg = px_rreg(port, AHCI_PX_CLB);
+    assert_bit_clear(reg, AHCI_PX_CLB_RESERVED);
+
+    /* (1) CLBU */
+    if (bitclr(hcap->cap, AHCI_CAP_S64A)) {
+        reg = px_rreg(port, AHCI_PX_CLBU);
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* (2) FB */
+    reg = px_rreg(port, AHCI_PX_FB);
+    assert_bit_clear(reg, AHCI_PX_FB_RESERVED);
+
+    /* (3) FBU */
+    if (bitclr(hcap->cap, AHCI_CAP_S64A)) {
+        reg = px_rreg(port, AHCI_PX_FBU);
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* (4) IS */
+    reg = px_rreg(port, AHCI_PX_IS);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (5) IE */
+    reg = px_rreg(port, AHCI_PX_IE);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (6) CMD */
+    reg = px_rreg(port, AHCI_PX_CMD);
+    assert_bit_clear(reg, AHCI_PX_CMD_FRE);
+    assert_bit_clear(reg, AHCI_PX_CMD_RESERVED);
+    assert_bit_clear(reg, AHCI_PX_CMD_CCS);
+    assert_bit_clear(reg, AHCI_PX_CMD_FR);
+    assert_bit_clear(reg, AHCI_PX_CMD_CR);
+    assert_bit_clear(reg, AHCI_PX_CMD_PMA); /* And RW only if CAP.SPM */
+    assert_bit_clear(reg, AHCI_PX_CMD_APSTE); /* RW only if CAP2.APST */
+    assert_bit_clear(reg, AHCI_PX_CMD_ATAPI);
+    assert_bit_clear(reg, AHCI_PX_CMD_DLAE);
+    assert_bit_clear(reg, AHCI_PX_CMD_ALPE);  /* RW only if CAP.SALP */
+    assert_bit_clear(reg, AHCI_PX_CMD_ASP);   /* RW only if CAP.SALP */
+    assert_bit_clear(reg, AHCI_PX_CMD_ICC);
+    /* If CPDetect support does not exist, CPState must be off. */
+    if (bitclr(reg, AHCI_PX_CMD_CPD)) {
+        assert_bit_clear(reg, AHCI_PX_CMD_CPS);
+    }
+    /* If MPSPresence is not set, MPSState must be off. */
+    if (bitclr(reg, AHCI_PX_CMD_MPSP)) {
+        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)) {
+        assert_bit_clear(reg, AHCI_PX_CMD_MPSS);
+        assert_bit_clear(reg, AHCI_PX_CMD_MPSP);
+    }
+    /* If, via CPD or MPSP we detect a drive, HPCP must be on. */
+    if (bitany(reg, AHCI_PX_CMD_CPD || AHCI_PX_CMD_MPSP)) {
+        assert_bit_set(reg, AHCI_PX_CMD_HPCP);
+    }
+    /* 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)) {
+        assert_bit_clear(reg, AHCI_PX_CMD_FBSCP);
+    }
+
+    /* (7) RESERVED */
+    reg = px_rreg(port, AHCI_PX_RES1);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (8) TFD */
+    reg = px_rreg(port, AHCI_PX_TFD);
+    /* At boot, prior to an FIS being received, the TFD register should be 0x7F,
+     * which breaks down as follows, as seen in AHCI 1.3 sec 3.3.8, p. 27. */
+    assert_bit_set(reg, AHCI_PX_TFD_STS_ERR);
+    assert_bit_set(reg, AHCI_PX_TFD_STS_CS1);
+    assert_bit_set(reg, AHCI_PX_TFD_STS_DRQ);
+    assert_bit_set(reg, AHCI_PX_TFD_STS_CS2);
+    assert_bit_clear(reg, AHCI_PX_TFD_STS_BSY);
+    assert_bit_clear(reg, AHCI_PX_TFD_ERR);
+    assert_bit_clear(reg, AHCI_PX_TFD_RESERVED);
+
+    /* (9) SIG */
+    reg = px_rreg(port, AHCI_PX_SIG);
+    /* Similar to the TFD, until we see the first D2H Register FIS, this
+     * register should provide a characteristic default. QEMU should not
+     * pretend it has seen a D2H FIS prior to FIS Receive Enable being set.
+     * AHCI 1.3, sec 3.3.9, pp 27-28. */
+    g_assert_cmphex(reg, ==, 0xFFFFFFFF);
+
+    /* (10) SSTS / SCR0: SStatus */
+    reg = px_rreg(port, AHCI_PX_SSTS);
+    assert_bit_clear(reg, AHCI_PX_SSTS_RESERVED);
+    /* Even though the register should be 0 at boot, it is asynchronous and
+     * prone to change, so we cannot test any well known value. */
+
+    /* (11) SCTL / SCR2: SControl */
+    reg = px_rreg(port, AHCI_PX_SCTL);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (12) SERR / SCR1: SError */
+    reg = px_rreg(port, AHCI_PX_SERR);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (13) SACT / SCR3: SActive */
+    reg = px_rreg(port, AHCI_PX_SACT);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (14) CI */
+    reg = px_rreg(port, AHCI_PX_CI);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (15) SNTF */
+    reg = px_rreg(port, AHCI_PX_SNTF);
+    g_assert_cmphex(reg, ==, 0);
+
+    /* (16) FBS */
+    reg = px_rreg(port, AHCI_PX_FBS);
+    assert_bit_clear(reg, AHCI_PX_FBS_EN);
+    assert_bit_clear(reg, AHCI_PX_FBS_DEC);
+    assert_bit_clear(reg, AHCI_PX_FBS_SDE);
+    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 Port-Multiplier FIS-based switching avail, ADO must >= 2 */
+        g_assert((reg & AHCI_PX_FBS_ADO) >> ctzl(AHCI_PX_FBS_ADO) >= 2);
+    }
+
+    /* [17 -- 27] RESERVED */
+    for (i = AHCI_PX_RES2; i < AHCI_PX_VS; ++i) {
+        reg = px_rreg(port, i);
+        g_assert_cmphex(reg, ==, 0);
+    }
+
+    /* [28 -- 31] Vendor-Specific */
+    for (i = AHCI_PX_VS; i < 32; ++i) {
+        reg = px_rreg(port, i);
+        if (reg) {
+            g_test_message("INFO: Vendor register %u non-empty", i);
+        }
+    }
+}
+
 /******************************************************************************/
 /* Test Interfaces                                                            */
 /******************************************************************************/
@@ -484,6 +1025,21 @@ static void test_pci_enable(void)
     ahci_shutdown(ahci);
 }
 
+/**
+ * Investigate the memory mapped regions of the HBA,
+ * and test them for AHCI specification adherence.
+ */
+static void test_hba_spec(void)
+{
+    QPCIDevice *ahci;
+    void *hba_base;
+
+    ahci = ahci_boot();
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_test_hba_spec(ahci, hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -536,6 +1092,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/sanity",     test_sanity);
     qtest_add_func("/ahci/pci_spec",   test_pci_spec);
     qtest_add_func("/ahci/pci_enable", test_pci_enable);
+    qtest_add_func("/ahci/hba_spec",   test_hba_spec);
 
     ret = g_test_run();
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 29/30] ahci: Add test_hba_enable to ahci-test.

[John Snow]

This test engages the HBA functionality and initializes
values to sane defaults to allow for minimal HBA functionality.

Buffers are allocated and pointers are updated to allow minimal
I/O commands to complete as expected. Error registers and responses
are sanity checked for specification adherence.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 1c19c9c..f157047 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -277,11 +277,17 @@ static uint32_t ahci_fingerprint;
 #define ahci_write(OFST, VAL) qpci_io_writel(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))
+#define ahci_clr(regno, mask) ahci_wreg((regno), ahci_rreg(regno) & ~(mask))
 
 /*** IO macros for port-specific offsets inside of AHCI memory. ***/
 #define px_ofst(port, regno) (HBA_PORT_NUM_REG * (port) + AHCI_PORTS + (regno))
 #define px_rreg(port, regno)      ahci_rreg(px_ofst((port), (regno)))
 #define px_wreg(port, regno, val) ahci_wreg(px_ofst((port), (regno)), (val))
+#define px_set(port, reg, mask)   px_wreg((port), (reg),                \
+                                          px_rreg((port), (reg)) | (mask));
+#define px_clr(port, reg, mask)   px_wreg((port), (reg),                \
+                                          px_rreg((port), (reg)) & ~(mask));
 
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
@@ -432,6 +438,140 @@ static QPCIDevice *start_ahci_device(QPCIDevice *ahci, void **hba_base)
     return ahci;
 }
 
+/**
+ * Test and initialize the AHCI's HBA memory areas.
+ * 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)
+{
+    /* Bits of interest in this section:
+     * GHC.AE     Global Host Control / AHCI Enable
+     * PxCMD.ST   Port Command: Start
+     * PxCMD.SUD  "Spin Up Device"
+     * PxCMD.POD  "Power On Device"
+     * PxCMD.FRE  "FIS Receive Enable"
+     * 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;
+    uint16_t i;
+    uint8_t num_cmd_slots;
+
+    g_assert(hba_base != 0);
+
+    /* Set GHC.AE to 1 */
+    ahci_set(AHCI_GHC, AHCI_GHC_AE);
+    reg = ahci_rreg(AHCI_GHC);
+    assert_bit_set(reg, AHCI_GHC_AE);
+
+    /* Read CAP.NCS, how many command slots do we have? */
+    reg = ahci_rreg(AHCI_CAP);
+    num_cmd_slots = ((reg & AHCI_CAP_NCS) >> ctzl(AHCI_CAP_NCS)) + 1;
+    g_test_message("Number of Command Slots: %u", num_cmd_slots);
+
+    /* Determine which ports are implemented. */
+    ports_impl = ahci_rreg(AHCI_PI);
+
+    for (i = 0; ports_impl; ports_impl >>= 1, ++i) {
+        if (!(ports_impl & 0x01)) {
+            continue;
+        }
+
+        g_test_message("Initializing port %u", i);
+
+        reg = px_rreg(i, AHCI_PX_CMD);
+        if (bitclr(reg, AHCI_PX_CMD_ST | AHCI_PX_CMD_CR |
+                   AHCI_PX_CMD_FRE | AHCI_PX_CMD_FR)) {
+            g_test_message("port is idle");
+        } else {
+            g_test_message("port needs to be idled");
+            px_clr(i, AHCI_PX_CMD, (AHCI_PX_CMD_ST | AHCI_PX_CMD_FRE));
+            /* The port has 500ms to disengage. */
+            usleep(500000);
+            reg = px_rreg(i, AHCI_PX_CMD);
+            assert_bit_clear(reg, AHCI_PX_CMD_CR);
+            assert_bit_clear(reg, AHCI_PX_CMD_FR);
+            g_test_message("port is now idle");
+            /* The spec does allow for possibly needing a PORT RESET
+             * or HBA reset if we fail to idle the port. */
+        }
+
+        /* 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));
+
+        /* 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));
+
+        /* Clear PxSERR, PxIS, then IS.IPS[x] by writing '1's. */
+        px_wreg(i, AHCI_PX_SERR, 0xFFFFFFFF);
+        px_wreg(i, AHCI_PX_IS, 0xFFFFFFFF);
+        ahci_wreg(AHCI_IS, (1 << i));
+
+        /* Verify Interrupts Cleared */
+        reg = px_rreg(i, AHCI_PX_SERR);
+        g_assert_cmphex(reg, ==, 0);
+
+        reg = px_rreg(i, AHCI_PX_IS);
+        g_assert_cmphex(reg, ==, 0);
+
+        reg = ahci_rreg(AHCI_IS);
+        assert_bit_clear(reg, (1 << i));
+
+        /* Enable All Interrupts: */
+        px_wreg(i, AHCI_PX_IE, 0xFFFFFFFF);
+        reg = px_rreg(i, AHCI_PX_IE);
+        g_assert_cmphex(reg, ==, ~((uint32_t)AHCI_PX_IE_RESERVED));
+
+        /* Enable the FIS Receive Engine. */
+        px_set(i, AHCI_PX_CMD, AHCI_PX_CMD_FRE);
+        reg = px_rreg(i, AHCI_PX_CMD);
+        assert_bit_set(reg, AHCI_PX_CMD_FR);
+
+        /* AHCI 1.3 spec: if !STS.BSY, !STS.DRQ and PxSSTS.DET indicates
+         * physical presence, a device is present and may be started. However,
+         * PxSERR.DIAG.X /may/ need to be cleared a priori. */
+        reg = px_rreg(i, AHCI_PX_SERR);
+        if (bitset(reg, AHCI_PX_SERR_DIAG_X)) {
+            px_set(i, AHCI_PX_SERR, AHCI_PX_SERR_DIAG_X);
+        }
+
+        reg = px_rreg(i, AHCI_PX_TFD);
+        if (bitclr(reg, AHCI_PX_TFD_STS_BSY | AHCI_PX_TFD_STS_DRQ)) {
+            reg = px_rreg(i, AHCI_PX_SSTS);
+            if ((reg & AHCI_PX_SSTS_DET) == SSTS_DET_ESTABLISHED) {
+                /* Device Found: set PxCMD.ST := 1 */
+                px_set(i, AHCI_PX_CMD, AHCI_PX_CMD_ST);
+                assert_bit_set(px_rreg(i, AHCI_PX_CMD), AHCI_PX_CMD_CR);
+                g_test_message("Started Device %u", i);
+            } else if ((reg & AHCI_PX_SSTS_DET)) {
+                /* Device present, but in some unknown state. */
+                g_assert_not_reached();
+            }
+        }
+    }
+
+    /* Enable GHC.IE */
+    ahci_set(AHCI_GHC, AHCI_GHC_IE);
+    reg = ahci_rreg(AHCI_GHC);
+    assert_bit_set(reg, AHCI_GHC_IE);
+
+    /* TODO: The device should now be idling and waiting for commands.
+     * In the future, a small test-case to inspect the Register D2H FIS
+     * and clear the initial interrupts might be good. */
+}
+
 /*** Specification Adherence Tests ***/
 
 /**
@@ -1040,6 +1180,21 @@ static void test_hba_spec(void)
     ahci_shutdown(ahci);
 }
 
+/**
+ * Engage the HBA functionality of the AHCI PCI device,
+ * and bring it into a functional idle state.
+ */
+static void test_hba_enable(void)
+{
+    QPCIDevice *ahci;
+    void *hba_base;
+
+    ahci = ahci_boot();
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_hba_enable(ahci, hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -1093,6 +1248,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/pci_spec",   test_pci_spec);
     qtest_add_func("/ahci/pci_enable", test_pci_enable);
     qtest_add_func("/ahci/hba_spec",   test_hba_spec);
+    qtest_add_func("/ahci/hba_enable", test_hba_enable);
 
     ret = g_test_run();
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 30/30] ahci: Add test_identify case to ahci-test.

[John Snow]

Utilizing all of the bring-up code in pci_enable and hba_enable,
this test issues a simple IDENTIFY command via the HBA and retrieves
the response via the PIO receive mechanisms of the HBA.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index f157047..f834b6f 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -252,6 +252,97 @@
 #define AHCI_VERSION_1_2         (0x00010200)
 #define AHCI_VERSION_1_3         (0x00010300)
 
+/*** Structures ***/
+
+/**
+ * Generic FIS structure.
+ */
+struct fis {
+    uint8_t fis_type;
+    uint8_t flags;
+    char data[0];
+} __attribute__((__packed__));
+
+/**
+ * Register device-to-host FIS structure.
+ */
+struct reg_d2h_fis {
+    /* DW0 */
+    uint8_t fis_type;
+    uint8_t flags;
+    uint8_t status;
+    uint8_t error;
+    /* DW1 */
+    uint8_t lba_low;
+    uint8_t lba_mid;
+    uint8_t lba_high;
+    uint8_t device;
+    /* DW2 */
+    uint8_t lba3;
+    uint8_t lba4;
+    uint8_t lba5;
+    uint8_t res1;
+    /* DW3 */
+    uint16_t count;
+    uint8_t res2;
+    uint8_t res3;
+    /* DW4 */
+    uint16_t res4;
+    uint16_t res5;
+} __attribute__((__packed__));
+
+/**
+ * Register host-to-device FIS structure.
+ */
+struct reg_h2d_fis {
+    /* DW0 */
+    uint8_t fis_type;
+    uint8_t flags;
+    uint8_t command;
+    uint8_t feature_low;
+    /* DW1 */
+    uint8_t lba_low;
+    uint8_t lba_mid;
+    uint8_t lba_high;
+    uint8_t device;
+    /* DW2 */
+    uint8_t lba3;
+    uint8_t lba4;
+    uint8_t lba5;
+    uint8_t feature_high;
+    /* DW3 */
+    uint16_t count;
+    uint8_t icc;
+    uint8_t control;
+    /* DW4 */
+    uint32_t aux;
+} __attribute__((__packed__));
+
+/**
+ * Command List entry structure.
+ * The command list contains between 1-32 of these structures.
+ */
+struct ahci_command {
+    uint8_t b1;
+    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 */
+    uint32_t res[4];
+} __attribute__((__packed__));
+
+/**
+ * Physical Region Descriptor; pointed to by the Command List Header,
+ * struct ahci_command.
+ */
+struct prd {
+    uint32_t dba;  /* Data Base Address */
+    uint32_t dbau; /* Data Base Address Upper */
+    uint32_t res;  /* Reserved */
+    uint32_t dbc;  /* Data Byte Count (0-indexed) & Interrupt Flag (bit 2^31) */
+};
+
 typedef struct hba_cap {
     uint32_t cap;
     uint32_t cap2;
@@ -289,6 +380,10 @@ static uint32_t ahci_fingerprint;
 #define px_clr(port, reg, mask)   px_wreg((port), (reg),                \
                                           px_rreg((port), (reg)) & ~(mask));
 
+/* For calculating how big the PRD table needs to be: */
+#define cmd_tbl_siz(n) ((0x80 + ((n) * sizeof(struct prd)) + 0x7F) & ~0x7F)
+
+
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
 static QPCIDevice *start_ahci_device(QPCIDevice *dev, void **hba_base);
@@ -304,6 +399,17 @@ static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset);
 
 /*** Utilities ***/
 
+static void string_cpu_to_be16(uint16_t *s, size_t bytes)
+{
+    g_assert_cmphex((bytes & 1), ==, 0);
+    bytes /= 2;
+
+    while (bytes--) {
+        *s = cpu_to_be16(*s);
+        s++;
+    }
+}
+
 /**
  * Locate, verify, and return a handle to the AHCI device.
  */
@@ -1126,6 +1232,179 @@ 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)
+{
+    struct reg_d2h_fis *d2h = g_malloc0(0x20);
+    struct reg_d2h_fis *pio = g_malloc0(0x20);
+    struct reg_h2d_fis fis;
+    struct ahci_command cmd;
+    struct prd prd;
+    uint32_t ports, reg, clb, table, fb, 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
+     *     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
+     * (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
+     *     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.
+     */
+
+    /* 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);
+    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 */
+    fb = px_rreg(i, AHCI_PX_FB);
+    g_assert_cmphex(fb, !=, 0);
+    qmemset(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.    */
+    table = guest_alloc(guest_malloc, cmd_tbl_siz(1));
+    g_assert(table);
+    assert_bit_clear(table, 0x7F);
+
+    /* Create a data buffer ... where we will dump the IDENTIFY data to. */
+    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));
+    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;
+    cmd.ctbau = 0;
+
+    /* Construct our PRD, noting that DBC is 0-indexed. */
+    prd.dba = data_ptr;
+    prd.dbau = 0;
+    prd.res = 0;
+    prd.dbc = 511;
+    prd.dbc |= 0x80000000; /* Request DPS Interrupt */
+
+    /* 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, &fis, sizeof(fis));
+
+    /* Commit the PRD entry to the Command Table */
+    memwrite(table + 0x80, &prd, sizeof(prd));
+
+    /* Commit Command #0, pointing to the Table, to the Command List Buffer. */
+    memwrite(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);
+
+    /* Issue Command #0 via PxCI */
+    px_wreg(i, AHCI_PX_CI, (1 << 0));
+    while (bitset(px_rreg(i, AHCI_PX_TFD), AHCI_PX_TFD_STS_BSY)) {
+        usleep(50);
+    }
+
+    /* 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);
+    assert_bit_set(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);
+
+    /* 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 CMD #0, assert that we read 512 bytes */
+    memread(clb, &cmd, sizeof(cmd));
+    g_assert_cmphex(512, ==, cmd.prdbc);
+
+    /* Investigate FIS responses */
+    memread(fb + 0x20, pio, 0x20);
+    memread(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);
+    /* PIO FIS contains a "bytes read" field, it should match up. */
+    g_assert_cmphex(pio->res4, ==, cmd.prdbc);
+
+    /* Last, but not least: Investigate the IDENTIFY response data. */
+    memread(data_ptr, &buff, 512);
+
+    /* Check serial number/version in the buffer */
+    string_cpu_to_be16(&buff[10], 20);
+    rc = memcmp(&buff[10], "testdisk            ", 20);
+    g_assert_cmphex(rc, ==, 0);
+
+    string_cpu_to_be16(&buff[23], 8);
+    rc = memcmp(&buff[23], "version ", 8);
+    g_assert_cmphex(rc, ==, 0);
+
+    g_free(d2h);
+    g_free(pio);
+}
+
 /******************************************************************************/
 /* Test Interfaces                                                            */
 /******************************************************************************/
@@ -1195,6 +1474,22 @@ static void test_hba_enable(void)
     ahci_shutdown(ahci);
 }
 
+/**
+ * Bring up the device and issue an IDENTIFY command.
+ * Inspect the state of the HBA device and the data returned.
+ */
+static void test_identify(void)
+{
+    QPCIDevice *ahci;
+    void *hba_base;
+
+    ahci = ahci_boot();
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_hba_enable(ahci, hba_base);
+    ahci_test_identify(ahci, hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -1249,6 +1544,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/pci_enable", test_pci_enable);
     qtest_add_func("/ahci/hba_spec",   test_hba_spec);
     qtest_add_func("/ahci/hba_enable", test_hba_enable);
+    qtest_add_func("/ahci/identify",   test_identify);
 
     ret = g_test_run();
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Stefan Hajnoczi]

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

On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
> This patch series introduces a number of small fixes and tweaks to
> help support an AHCI test suite that in the future I hope to expand
> to a fuller regression suite to help guide the development of the
> AHCI device support under, in particular, the Q35 machine type in QEMU.
> 
> Paolo Bonzini has contributed a number of cleanup and refactoring patches
> that support changes to the PIO setup FIS packet construction code, which
> is necessary for testing ths specification adherence of the IDENTIFY command,
> which issues its data exclusively via PIO mechanisms.
> 
> The ahci-test code being checked in represents a minimum of functionality
> needed in order to issue and receive commands from the AHCI HBA under the
> libqos / qtest environment.
> 
> In V2, as detailed below, these tests are not currently expected to pass.
> I will post a complementary patch outside of this set that highlights
> the exact set of tests that will not pass, which can help verify at least
> the portions of these tests that do work correctly.
> 
> Assertions that currently fail:
>     - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>     - Boot-time values of the PxTFD register, which should not have valid
>       data until after a D2H FIS is received, but does in Qemu 2.1
>     - Boot-time values of the PxSIG register, which should have a specific
>       placeholder signature until the first D2H FIS is received, but is
>       currently blank.
>     - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>       command exhausts the given PRDT, but is not seen.

I guess these are the assertion failures:
ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed ((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
**
ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed ((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
**
ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed ((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d

Why publish this patch series if the test fails?  We can't merge failing
tests.

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

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
>> This patch series introduces a number of small fixes and tweaks to
>> help support an AHCI test suite that in the future I hope to expand
>> to a fuller regression suite to help guide the development of the
>> AHCI device support under, in particular, the Q35 machine type in QEMU.
>> 
>> Paolo Bonzini has contributed a number of cleanup and refactoring patches
>> that support changes to the PIO setup FIS packet construction code, which
>> is necessary for testing ths specification adherence of the IDENTIFY command,
>> which issues its data exclusively via PIO mechanisms.
>> 
>> The ahci-test code being checked in represents a minimum of functionality
>> needed in order to issue and receive commands from the AHCI HBA under the
>> libqos / qtest environment.
>> 
>> In V2, as detailed below, these tests are not currently expected to pass.
>> I will post a complementary patch outside of this set that highlights
>> the exact set of tests that will not pass, which can help verify at least
>> the portions of these tests that do work correctly.
>> 
>> Assertions that currently fail:
>>     - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>>     - Boot-time values of the PxTFD register, which should not have valid
>>       data until after a D2H FIS is received, but does in Qemu 2.1
>>     - Boot-time values of the PxSIG register, which should have a specific
>>       placeholder signature until the first D2H FIS is received, but is
>>       currently blank.
>>     - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>>       command exhausts the given PRDT, but is not seen.
>
> I guess these are the assertion failures:
> ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed
> ((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
> GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
> **
> ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed
> ((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
> GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
> **
> ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed
> ((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
> GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d
>
> Why publish this patch series if the test fails?  We can't merge failing
> tests.

Correct.

What I do when I want to start some bug fixing work with tests is to
write the tests to expect the actual, incorrect behavior, with a
greppable comment documenting the correct behavior.  Then clean that up
as the bugs get fixed.

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[John Snow]

On 08/06/2014 07:30 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
>>> This patch series introduces a number of small fixes and tweaks to
>>> help support an AHCI test suite that in the future I hope to expand
>>> to a fuller regression suite to help guide the development of the
>>> AHCI device support under, in particular, the Q35 machine type in QEMU.
>>>
>>> Paolo Bonzini has contributed a number of cleanup and refactoring patches
>>> that support changes to the PIO setup FIS packet construction code, which
>>> is necessary for testing ths specification adherence of the IDENTIFY command,
>>> which issues its data exclusively via PIO mechanisms.
>>>
>>> The ahci-test code being checked in represents a minimum of functionality
>>> needed in order to issue and receive commands from the AHCI HBA under the
>>> libqos / qtest environment.
>>>
>>> In V2, as detailed below, these tests are not currently expected to pass.
>>> I will post a complementary patch outside of this set that highlights
>>> the exact set of tests that will not pass, which can help verify at least
>>> the portions of these tests that do work correctly.
>>>
>>> Assertions that currently fail:
>>>      - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>>>      - Boot-time values of the PxTFD register, which should not have valid
>>>        data until after a D2H FIS is received, but does in Qemu 2.1
>>>      - Boot-time values of the PxSIG register, which should have a specific
>>>        placeholder signature until the first D2H FIS is received, but is
>>>        currently blank.
>>>      - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>>>        command exhausts the given PRDT, but is not seen.
>>
>> I guess these are the assertion failures:
>> ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed
>> ((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
>> GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
>> **
>> ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed
>> ((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
>> GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
>> **
>> ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed
>> ((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
>> GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d
>>
>> Why publish this patch series if the test fails?  We can't merge failing
>> tests.
>
> Correct.
>
> What I do when I want to start some bug fixing work with tests is to
> write the tests to expect the actual, incorrect behavior, with a
> greppable comment documenting the correct behavior.  Then clean that up
> as the bugs get fixed.
>

I thought it was valid to submit a failing test if... well, the behavior 
was wrong. Stefan said no warnings, so I took that to mean "This should 
fail." I didn't think it was too strange to have a failing test for 
something that was not feature complete.

So, if it's not appropriate to have a failing test at any stage 
(Regressions only?) now's a good time to let me know how you would like 
me to accomplish no warnings but have the tests pass. In my V1 I did 
just print a "WARN" string which was reasonable greppable to find the 
failure cases.

My next guess at something workable would be to stick the assertions 
behind a bool that could be toggled on/off via a flag that could be 
toggled with --all or similar to hit the expected failure cases. No 
warnings inside of the test harness, no failures, and cases could be 
found by grepping the name of the boolean and/or some accompanying comment.

--j

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 08/06/2014 07:30 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>
>>> On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
>>>> This patch series introduces a number of small fixes and tweaks to
>>>> help support an AHCI test suite that in the future I hope to expand
>>>> to a fuller regression suite to help guide the development of the
>>>> AHCI device support under, in particular, the Q35 machine type in QEMU.
>>>>
>>>> Paolo Bonzini has contributed a number of cleanup and refactoring patches
>>>> that support changes to the PIO setup FIS packet construction code, which
>>>> is necessary for testing ths specification adherence of the IDENTIFY command,
>>>> which issues its data exclusively via PIO mechanisms.
>>>>
>>>> The ahci-test code being checked in represents a minimum of functionality
>>>> needed in order to issue and receive commands from the AHCI HBA under the
>>>> libqos / qtest environment.
>>>>
>>>> In V2, as detailed below, these tests are not currently expected to pass.
>>>> I will post a complementary patch outside of this set that highlights
>>>> the exact set of tests that will not pass, which can help verify at least
>>>> the portions of these tests that do work correctly.
>>>>
>>>> Assertions that currently fail:
>>>>      - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>>>>      - Boot-time values of the PxTFD register, which should not have valid
>>>>        data until after a D2H FIS is received, but does in Qemu 2.1
>>>>      - Boot-time values of the PxSIG register, which should have a specific
>>>>        placeholder signature until the first D2H FIS is received, but is
>>>>        currently blank.
>>>>      - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>>>>        command exhausts the given PRDT, but is not seen.
>>>
>>> I guess these are the assertion failures:
>>> ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed
>>> ((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
>>> GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
>>> **
>>> ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed
>>> ((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
>>> GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
>>> **
>>> ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed
>>> ((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
>>> GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d
>>>
>>> Why publish this patch series if the test fails?  We can't merge failing
>>> tests.
>>
>> Correct.
>>
>> What I do when I want to start some bug fixing work with tests is to
>> write the tests to expect the actual, incorrect behavior, with a
>> greppable comment documenting the correct behavior.  Then clean that up
>> as the bugs get fixed.
>>
>
> I thought it was valid to submit a failing test if... well, the
> behavior was wrong. Stefan said no warnings, so I took that to mean
> "This should fail." I didn't think it was too strange to have a
> failing test for something that was not feature complete.
>
> So, if it's not appropriate to have a failing test at any stage
> (Regressions only?) now's a good time to let me know how you would
> like me to accomplish no warnings but have the tests pass. In my V1 I
> did just print a "WARN" string which was reasonable greppable to find
> the failure cases.
>
> My next guess at something workable would be to stick the assertions
> behind a bool that could be toggled on/off via a flag that could be
> toggled with --all or similar to hit the expected failure cases. No
> warnings inside of the test harness, no failures, and cases could be
> found by grepping the name of the boolean and/or some accompanying
> comment.

I reiterate my advice to simply assert the actual behavior, even though
it's incorrect, with a greppable comment documenting the correct
behavior.  No printing of warnings.  No flags to toggle assertions.

To illustrate what I mean: commit e2ec3f9.  It fixes a bunch of bugs,
and as a side effect replaces some incorrect behavior by other incorrect
behavior.  This is all visible in the patch to tests/.

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Stefan Hajnoczi]

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

On Wed, Aug 06, 2014 at 12:50:23PM -0400, John Snow wrote:
> 
> 
> On 08/06/2014 07:30 AM, Markus Armbruster wrote:
> >Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> >>On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
> >>>This patch series introduces a number of small fixes and tweaks to
> >>>help support an AHCI test suite that in the future I hope to expand
> >>>to a fuller regression suite to help guide the development of the
> >>>AHCI device support under, in particular, the Q35 machine type in QEMU.
> >>>
> >>>Paolo Bonzini has contributed a number of cleanup and refactoring patches
> >>>that support changes to the PIO setup FIS packet construction code, which
> >>>is necessary for testing ths specification adherence of the IDENTIFY command,
> >>>which issues its data exclusively via PIO mechanisms.
> >>>
> >>>The ahci-test code being checked in represents a minimum of functionality
> >>>needed in order to issue and receive commands from the AHCI HBA under the
> >>>libqos / qtest environment.
> >>>
> >>>In V2, as detailed below, these tests are not currently expected to pass.
> >>>I will post a complementary patch outside of this set that highlights
> >>>the exact set of tests that will not pass, which can help verify at least
> >>>the portions of these tests that do work correctly.
> >>>
> >>>Assertions that currently fail:
> >>>     - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
> >>>     - Boot-time values of the PxTFD register, which should not have valid
> >>>       data until after a D2H FIS is received, but does in Qemu 2.1
> >>>     - Boot-time values of the PxSIG register, which should have a specific
> >>>       placeholder signature until the first D2H FIS is received, but is
> >>>       currently blank.
> >>>     - The "Descriptor Processed" interrupt is expected after the IDENTIFY
> >>>       command exhausts the given PRDT, but is not seen.
> >>
> >>I guess these are the assertion failures:
> >>ERROR:tests/ahci-test.c:777:ahci_test_pci_spec: assertion failed
> >>((data & 0xFF) == PCI_CAP_ID_MSI): (0x00000012 == 0x00000005)
> >>GTester: last random seed: R02Sd92815a5d013e8433808b903b2b13fb0
> >>**
> >>ERROR:tests/ahci-test.c:1165:ahci_test_port_spec: assertion failed
> >>((reg) & ((0x01)) == ((0x01))): (0x00000000 == 0x00000001)
> >>GTester: last random seed: R02S4d6c05e864dc777e64141cdc6d2a18cf
> >>**
> >>ERROR:tests/ahci-test.c:1360:ahci_test_identify: assertion failed
> >>((reg) & ((0x20)) == ((0x20))): (0x00000000 == 0x00000020)
> >>GTester: last random seed: R02S2b3b330b83a66badb24da80b48120b1d
> >>
> >>Why publish this patch series if the test fails?  We can't merge failing
> >>tests.
> >
> >Correct.
> >
> >What I do when I want to start some bug fixing work with tests is to
> >write the tests to expect the actual, incorrect behavior, with a
> >greppable comment documenting the correct behavior.  Then clean that up
> >as the bugs get fixed.
> >
> 
> I thought it was valid to submit a failing test if... well, the behavior was
> wrong. Stefan said no warnings, so I took that to mean "This should fail." I
> didn't think it was too strange to have a failing test for something that
> was not feature complete.
> 
> So, if it's not appropriate to have a failing test at any stage (Regressions
> only?) now's a good time to let me know how you would like me to accomplish
> no warnings but have the tests pass. In my V1 I did just print a "WARN"
> string which was reasonable greppable to find the failure cases.
> 
> My next guess at something workable would be to stick the assertions behind
> a bool that could be toggled on/off via a flag that could be toggled with
> --all or similar to hit the expected failure cases. No warnings inside of
> the test harness, no failures, and cases could be found by grepping the name
> of the boolean and/or some accompanying comment.

Note for the mailing list: John and I had an IRC chat where I mentioned
that the git history must always be bisectable, which is the fundamental
reason why failing tests cannot be merged.  My suggestion is to
post-pone tests that are not fixed by this patch series, and I like
Markus idea to assert the incorrect behavior (with a comment) in the
meantime, too.

Stefan

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

Re: [Qemu-devel] [PATCH v2 00/30] AHCI test suite framework

[Stefan Hajnoczi]

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

On Mon, Aug 04, 2014 at 05:11:01PM -0400, John Snow wrote:
> This patch series introduces a number of small fixes and tweaks to
> help support an AHCI test suite that in the future I hope to expand
> to a fuller regression suite to help guide the development of the
> AHCI device support under, in particular, the Q35 machine type in QEMU.
> 
> Paolo Bonzini has contributed a number of cleanup and refactoring patches
> that support changes to the PIO setup FIS packet construction code, which
> is necessary for testing ths specification adherence of the IDENTIFY command,
> which issues its data exclusively via PIO mechanisms.
> 
> The ahci-test code being checked in represents a minimum of functionality
> needed in order to issue and receive commands from the AHCI HBA under the
> libqos / qtest environment.
> 
> In V2, as detailed below, these tests are not currently expected to pass.
> I will post a complementary patch outside of this set that highlights
> the exact set of tests that will not pass, which can help verify at least
> the portions of these tests that do work correctly.
> 
> Assertions that currently fail:
>     - Ordering of PCI capabilities as defined by either AHCI or Intel ICH9
>     - Boot-time values of the PxTFD register, which should not have valid
>       data until after a D2H FIS is received, but does in Qemu 2.1
>     - Boot-time values of the PxSIG register, which should have a specific
>       placeholder signature until the first D2H FIS is received, but is
>       currently blank.
>     - The "Descriptor Processed" interrupt is expected after the IDENTIFY
>       command exhausts the given PRDT, but is not seen.

Thanks, I have merged patches up to and including Patch 24 onto my block
tree:
https://github.com/stefanha/qemu/commits/block

This should make it easier to manage the next revision of the series
where we can focus on the qtest test cases.

Stefan

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