[Qemu-devel] [PATCH 00/28] ahci refactoring to support ahci-test suite

[Qemu-devel] [PATCH 00/28] ahci refactoring to support ahci-test suite

[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.

John Snow (11):
  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.
  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            |  100 +--
 hw/ide/ahci.h            |   21 -
 hw/ide/atapi.c           |   11 +-
 hw/ide/core.c            |   96 +--
 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        | 1579 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/ide-test.c         |   74 +++
 tests/libqos/malloc-pc.c |    3 +
 tests/libqos/pci-pc.c    |    7 +
 tests/libqos/pci-pc.h    |    1 +
 tests/libqtest.c         |   20 +-
 tests/libqtest.h         |   24 +
 18 files changed, 1878 insertions(+), 181 deletions(-)
 create mode 100644 tests/ahci-test.c

-- 
1.9.3

[Qemu-devel] [PATCH 01/28] 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

Re: [Qemu-devel] [PATCH 01/28] blkdebug: report errors on flush too

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:42PM -0400, John Snow wrote:
> 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(+)

Assuming this doesn't break existing qemu-iotests that use blkdebug:

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

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

[Qemu-devel] [PATCH 02/28] 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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:43PM -0400, John Snow wrote:
>  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);
>      }

Is the QMP command also logged or are we just logging the response?

Stefan

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

[Qemu-devel] [PATCH 03/28] 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 | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a0d97f..03af481 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,72 @@ static void test_flush(void)
     ide_test_quit();
 }
 
+static void prepare_blkdebug_script(const char *debug_path, const char *event)
+{
+    FILE *debug_file = fopen(debug_path, "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;
+
+    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);
+
+    sleep(1);                    /* HACK: wait for event */
+
+    /* Complete the command */
+    s = "{'execute':'cont' }";
+    while (!qmp(s)) {
+        s = "";
+        sleep(1);
+    }
+
+    /* 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 +568,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,6 +594,8 @@ 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 */
-- 
1.9.3

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
> @@ -489,6 +490,72 @@ static void test_flush(void)
>      ide_test_quit();
>  }
>  
> +static void prepare_blkdebug_script(const char *debug_path, const char *event)
> +{
> +    FILE *debug_file = fopen(debug_path, "w");

Please avoid shadowing the global debug_path variable.  Perhaps simply
call the argument 'filename'.

> +static void test_retry_flush(void)
> +{
> +    uint8_t data;
> +    const char *s;
> +
> +    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);
> +
> +    sleep(1);                    /* HACK: wait for event */
> +
> +    /* Complete the command */
> +    s = "{'execute':'cont' }";
> +    while (!qmp(s)) {
> +        s = "";
> +        sleep(1);
> +    }

I guess we're supposed to wait for the block I/O error event when the
machine stops.  Please implement that and replace this polling loop.

See the STOP event in docs/qmp/qmp-events.txt.

> @@ -501,6 +568,11 @@ int main(int argc, char **argv)
>          return 0;
>      }
>  
> +    /* Create temporary blkdebug instructions */
> +    fd = mkstemp(debug_path);
> +    g_assert(fd >= 0);
> +    close(fd);
> +

debug_path must be unlinked at the end of main().  Tests should not
clutter up the /tmp directory, all resources must be freed.

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

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

[John Snow]

On 07/31/2014 06:58 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
>> +static void test_retry_flush(void)
>> +{
>> +    uint8_t data;
>> +    const char *s;
>> +
>> +    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);
>> +
>> +    sleep(1);                    /* HACK: wait for event */
>> +
>> +    /* Complete the command */
>> +    s = "{'execute':'cont' }";
>> +    while (!qmp(s)) {
>> +        s = "";
>> +        sleep(1);
>> +    }
> I guess we're supposed to wait for the block I/O error event when the
> machine stops.  Please implement that and replace this polling loop.
>
> See the STOP event in docs/qmp/qmp-events.txt.
>
Paolo: I edited in a bit to check for the STOP event instead of sleeping 
in V2, but what's the point of setting s = "" and sleeping and resending 
a blank string? Can't we just g_assert(qmp(s)) the first go around, 
provided the STOP event has already occurred?

It works in practice, but I am curious.

--J

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/31/2014 06:58 AM, Stefan Hajnoczi wrote:
>> On Mon, Jul 07, 2014 at 02:17:44PM -0400, John Snow wrote:
>>> +static void test_retry_flush(void)
>>> +{
>>> +    uint8_t data;
>>> +    const char *s;
>>> +
>>> +    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);
>>> +
>>> +    sleep(1);                    /* HACK: wait for event */
>>> +
>>> +    /* Complete the command */
>>> +    s = "{'execute':'cont' }";
>>> +    while (!qmp(s)) {
>>> +        s = "";
>>> +        sleep(1);
>>> +    }
>> I guess we're supposed to wait for the block I/O error event when the
>> machine stops.  Please implement that and replace this polling loop.
>>
>> See the STOP event in docs/qmp/qmp-events.txt.
>>
> Paolo: I edited in a bit to check for the STOP event instead of
> sleeping in V2, but what's the point of setting s = "" and sleeping
> and resending a blank string? Can't we just g_assert(qmp(s)) the first
> go around, provided the STOP event has already occurred?
>
> It works in practice, but I am curious.

I don't understand the need for sleep.  qmp() & friends wait for a
reply.

Sending "", i.e. nothing, is a crude way to just receive a reply.

Example: boot-order-test.c waits for an event without examining it:

    qmp_discard_response("");   /* HACK: wait for event */

Your loop seems to try to consume replies (events?) until you get NULL,
which I guess happens when the other end closes the connection.

By the way, your loop leaks the responses.  Pretty harmless, but you may
want to QDECREF() just to be orderly.

Proper support for events in libqtest would be nice.

[Qemu-devel] [PATCH 04/28] 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 3a38f1e..506db88 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -803,6 +803,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)) {
@@ -825,7 +827,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

Re: [Qemu-devel] [PATCH 04/28] ide: stash aiocb for flushes

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:45PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 05/28] 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 506db88..96e4c7c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2060,7 +2060,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)
@@ -2198,7 +2200,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

Re: [Qemu-devel] [PATCH 05/28] ide: simplify reset callbacks

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:46PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 06/28] 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 96e4c7c..dbf5030 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -578,7 +578,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);
 }
 
@@ -2198,7 +2200,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

Re: [Qemu-devel] [PATCH 06/28] ide: simplify set_inactive callbacks

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:47PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 07/28] 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

Re: [Qemu-devel] [PATCH 07/28] ide: simplify async_cmd_done callbacks

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:48PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 08/28] 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 dbf5030..4b47bf5 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)
@@ -2179,11 +2181,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;
@@ -2195,7 +2192,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

Re: [Qemu-devel] [PATCH 08/28] ide: simplify start_transfer callbacks

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:49PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 09/28] 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 4b47bf5..82aa0b9 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -722,7 +722,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)
@@ -2176,11 +2183,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;
@@ -2191,7 +2193,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

Re: [Qemu-devel] [PATCH 09/28] ide: wrap start_dma callback

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:50PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 10/28] 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

Re: [Qemu-devel] [PATCH 10/28] ide: remove wrong setting of BM_STATUS_INT

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:51PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 11/28] 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 82aa0b9..d97f323 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -577,11 +577,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);
 }
@@ -591,7 +591,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);
 }
 
@@ -696,10 +696,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)
@@ -2196,7 +2193,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

Re: [Qemu-devel] [PATCH 11/28] ide: fold add_status callback into set_inactive

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:52PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 12/28] 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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:53PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 13/28] 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 d97f323..bd3bd34 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -511,8 +511,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;
         }
     }
@@ -597,14 +597,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 {
@@ -623,12 +623,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;
@@ -746,7 +746,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;
         }
     }
@@ -815,7 +815,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;
         }
     }
@@ -2310,7 +2310,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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:54PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 14/28] 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

Re: [Qemu-devel] [PATCH 14/28] ahci: remove duplicate PORT_IRQ_* constants

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:55PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 15/28] 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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index bd3bd34..d900ba0 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -422,6 +422,9 @@ static inline void ide_abort_command(IDEState *s)
 {
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
+    if (s->end_transfer_func != ide_transfer_stop) {
+        ide_transfer_stop(s);
+    }
 }
 
 /* prepare data transfer and tell what to do after */
@@ -588,9 +591,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

Re: [Qemu-devel] [PATCH 15/28] ide: stop PIO transfer on errors

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:56PM -0400, John Snow wrote:
> 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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index bd3bd34..d900ba0 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -422,6 +422,9 @@ static inline void ide_abort_command(IDEState *s)
>  {
>      s->status = READY_STAT | ERR_STAT;
>      s->error = ABRT_ERR;
> +    if (s->end_transfer_func != ide_transfer_stop) {
> +        ide_transfer_stop(s);
> +    }
>  }

I don't understand this.

ide_transfer_stop() sets s->send_transfer_func = ide_transfer_stop.
This has the side-effect of making ide_is_pio_out() return true (not
sure if that poses a problem).

Why can't we call ide_transfer_stop() when s->end_transfer_func ==
ide_transfer_stop?

Stefan

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

Re: [Qemu-devel] [PATCH 15/28] ide: stop PIO transfer on errors

[John Snow]

On 07/31/2014 08:23 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:17:56PM -0400, John Snow wrote:
>> 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 | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index bd3bd34..d900ba0 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -422,6 +422,9 @@ static inline void ide_abort_command(IDEState *s)
>>   {
>>       s->status = READY_STAT | ERR_STAT;
>>       s->error = ABRT_ERR;
>> +    if (s->end_transfer_func != ide_transfer_stop) {
>> +        ide_transfer_stop(s);
>> +    }
>>   }
> I don't understand this.
>
> ide_transfer_stop() sets s->send_transfer_func = ide_transfer_stop.
> This has the side-effect of making ide_is_pio_out() return true (not
> sure if that poses a problem).
>
> Why can't we call ide_transfer_stop() when s->end_transfer_func ==
> ide_transfer_stop?
>
> Stefan
I'm afraid I need to defer to your judgment on this one, Paolo.

Re: [Qemu-devel] [PATCH 15/28] ide: stop PIO transfer on errors

[Paolo Bonzini]

Il 31/07/2014 14:23, Stefan Hajnoczi ha scritto:
> On Mon, Jul 07, 2014 at 02:17:56PM -0400, John Snow wrote:
>> 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 | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index bd3bd34..d900ba0 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -422,6 +422,9 @@ static inline void ide_abort_command(IDEState *s)
>>  {
>>      s->status = READY_STAT | ERR_STAT;
>>      s->error = ABRT_ERR;
>> +    if (s->end_transfer_func != ide_transfer_stop) {
>> +        ide_transfer_stop(s);
>> +    }
>>  }
> 
> I don't understand this.
> 
> ide_transfer_stop() sets s->send_transfer_func = ide_transfer_stop.
> This has the side-effect of making ide_is_pio_out() return true (not
> sure if that poses a problem).

ide_is_pio_out is only called if DRQ_STAT is set, and it is never set
after ide_transfer_stop returns, so that is not a problem.

> Why can't we call ide_transfer_stop() when s->end_transfer_func ==
> ide_transfer_stop?

The idea was to only call it for PIO commands (if s->end_transfer_func
== ide_transfer_stop, the PIO command cannot fail anymore), but we might
as well call it unconditionally like ide_dma_error does, i.e.

 {
+    ide_transfer_stop(s);
     s->status = READY_STAT | ERR_STAT;
     s->error = ABRT_ERR;
 }

It's simpler indeed.

Paolo

[Qemu-devel] [PATCH 16/28] 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 d900ba0..41ca92e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -442,12 +442,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)
@@ -573,20 +581,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)
@@ -823,7 +824,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);
 }
 
@@ -1747,6 +1748,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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:57PM -0400, John Snow wrote:
> 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(-)

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

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

[Qemu-devel] [PATCH 17/28] 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 | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b40ec06..1299920 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -587,6 +587,56 @@ 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);
+
+    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 +1081,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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:58PM -0400, John Snow wrote:
> +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);

We should check cmd_len == 0x80 and cmd_fis != NULL to avoid undefined
behavior when accessing cmd_fis.

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

[Qemu-devel] [PATCH 18/28] 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 36b6ab0..143b978 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -231,7 +231,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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:17:59PM -0400, John Snow wrote:
> 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 36b6ab0..143b978 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -231,7 +231,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

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

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

[Qemu-devel] [PATCH 19/28] 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

Re: [Qemu-devel] [PATCH 19/28] qtest: Adding qtest_memset and qmemset.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:00PM -0400, John Snow wrote:
> 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(+)

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

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

[Qemu-devel] [PATCH 20/28] 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

Re: [Qemu-devel] [PATCH 20/28] libqos: Correct memory leak

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:01PM -0400, John Snow wrote:
> 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(+)

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

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

[Qemu-devel] [PATCH 21/28] 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..8205e0a 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);
+    free(path);
 }
 
 void qtest_memwrite(QTestState *s, uint64_t addr, const void *data, size_t size)
-- 
1.9.3

Re: [Qemu-devel] [PATCH 21/28] libqtest: Correct small memory leak.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:02PM -0400, John Snow wrote:
> 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..8205e0a 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);
> +    free(path);

Glib functions use g_malloc()/g_free(), not malloc()/free().  Since this
string was allocated with g_strdup_printf() it must be freed with
g_free().

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

[Qemu-devel] [PATCH 22/28] 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 bf741a4..e0396bd 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

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

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:03PM -0400, John Snow wrote:
> 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(+)

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

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

[Qemu-devel] [PATCH 23/28] 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 | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+)
 create mode 100644 tests/ahci-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 7e53d0d..71caa26 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)
@@ -299,6 +300,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..b3ed85f
--- /dev/null
+++ b/tests/ahci-test.c
@@ -0,0 +1,207 @@
+/*
+ * 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)
+
+/* To help make it clear that the HBA is not a pointer to local memory. */
+typedef void HBA;
+
+/*** 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(vendor_id == PCI_VENDOR_ID_INTEL);
+    g_assert(device_id == PCI_DEVICE_ID_INTEL_Q35_AHCI);
+
+    return ahci;
+}
+
+static void free_ahci_device(QPCIDevice *ahci)
+{
+    if (pcibus) {
+        qpci_free_pc(pcibus);
+        pcibus = NULL;
+    }
+
+    /* libqos doesn't have a function for this, so free it manually */
+    g_free(ahci);
+}
+
+/*** 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();
+
+    free(cmdline);
+}
+
+/**
+ * Tear down the QEMU instance.
+ */
+static void qtest_shutdown(void)
+{
+    if (guest_malloc) {
+        g_free(guest_malloc);
+        guest_malloc = NULL;
+    }
+    qtest_end();
+}
+
+/**
+ * Start a Q35 machine and bookmark a handle to the AHCI device.
+ */
+static void ahci_boot(QPCIDevice **ahci)
+{
+    QPCIDevice *dev;
+
+    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. */
+    dev = get_ahci_device();
+
+    if (ahci) {
+        *ahci = dev;
+    }
+}
+
+/**
+ * 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_boot(&ahci);
+    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

Re: [Qemu-devel] [PATCH 23/28] ahci: Adding basic functionality qtest.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:04PM -0400, John Snow wrote:
> +/* To help make it clear that the HBA is not a pointer to local memory. */
> +typedef void HBA;

Unused.  Please drop it or move it to the patch that uses it.

> +static void free_ahci_device(QPCIDevice *ahci)
> +{
> +    if (pcibus) {
> +        qpci_free_pc(pcibus);
> +        pcibus = NULL;
> +    }
> +
> +    /* libqos doesn't have a function for this, so free it manually */
> +    g_free(ahci);

Since the AHCI QPCIDevice comes from the QPCIBus, it would make sense to
free the AHCI device before freeing the bus.

> +}
> +
> +/*** 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();
> +
> +    free(cmdline);

g_strdup_vprintf() needs g_free(), not free().

> +}
> +
> +/**
> + * Tear down the QEMU instance.
> + */
> +static void qtest_shutdown(void)
> +{
> +    if (guest_malloc) {
> +        g_free(guest_malloc);
> +        guest_malloc = NULL;
> +    }

g_free(NULL) is a nop, so this is equivalent:

  g_free(guest_malloc);
  guest_malloc = NULL;

> +    qtest_end();
> +}
> +
> +/**
> + * Start a Q35 machine and bookmark a handle to the AHCI device.
> + */
> +static void ahci_boot(QPCIDevice **ahci)
> +{
> +    QPCIDevice *dev;
> +
> +    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. */
> +    dev = get_ahci_device();
> +
> +    if (ahci) {
> +        *ahci = dev;
> +    }

It would be simpler to just return the QPCIDevice.

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

[Qemu-devel] [PATCH 24/28] 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 | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 271 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index b3ed85f..e0a8f34 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -43,6 +43,11 @@
 
 /*** 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)
 
 /* To help make it clear that the HBA is not a pointer to local memory. */
 typedef void HBA;
@@ -52,9 +57,22 @@ static QGuestAllocator *guest_malloc;
 static QPCIBus *pcibus;
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
 
+/*** 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)
+
 /*** 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 ***/
 
@@ -156,6 +174,246 @@ 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);
+#ifdef AHCI_13_STRICT
+    /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */
+    assert_bit_clear(datal, 0xFF);
+#else
+    if (datal & 0xFF) {
+        g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF);
+    }
+#endif
+
+    /* BCC *must* equal 0x01. */
+    g_assert(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(PCI_PI(datal) == 0);
+    } else if (PCI_SCC(datal) == 0x06) {
+        /* AHCI */
+        g_assert(PCI_PI(datal) == 0x01);
+    } else {
+        g_assert_not_reached();
+    }
+
+    datab = qpci_config_readb(ahci, PCI_CACHE_LINE_SIZE);
+    g_assert(datab == 0);
+
+    datab = qpci_config_readb(ahci, PCI_LATENCY_TIMER);
+    g_assert(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(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);
+
+#ifdef AHCI_13_STRICT
+    /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */
+    g_assert((data & 0xFF) == PCI_CAP_ID_PM);
+#elif defined Q35
+    /* Intel ICH9 Family Datasheet 14.1.19 p.550 */
+    if ((data & 0xFF) != PCI_CAP_ID_MSI) {
+        g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)",
+                       data & 0xFF, PCI_CAP_ID_MSI);
+    }
+    /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/
+#else
+    if ((data & 0xFF) != PCI_CAP_ID_PM) {
+        g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)",
+                       data & 0xFF, PCI_CAP_ID_PM);
+    }
+#endif
+
+    ahci_test_pci_caps(ahci, data, (uint8_t)datal);
+
+    /* Reserved. */
+    datal = qpci_config_readl(ahci, PCI_CAPABILITY_LIST + 4);
+    g_assert(datal == 0);
+
+    /* IPIN might vary, but ILINE must be off. */
+    datab = qpci_config_readb(ahci, PCI_INTERRUPT_LINE);
+    g_assert(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(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((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(datal == 0x00);
+
+    if (dataw & PCI_MSI_FLAGS_64BIT) {
+        g_test_message("MSICAP is 64bit");
+        g_assert(qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_HI) == 0x00);
+        g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_64) == 0x00);
+    } else {
+        g_test_message("MSICAP is 32bit");
+        g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_32) == 0x00);
+    }
+}
+
+/**
+ * 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                                                            */
 /******************************************************************************/
@@ -170,6 +428,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_boot(&ahci);
+    ahci_test_pci_spec(ahci);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -197,6 +467,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

Re: [Qemu-devel] [PATCH 24/28] ahci: Add test_pci_spec to ahci-test.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote:
> +/*** Macro Utilities ***/
> +#define bitany(data, mask) (((data) & (mask)) != 0)
> +#define bitset(data, mask) (((data) & (mask)) == (mask))
> +#define bitclr(data, mask) (((data) & (mask)) == 0)

Unused, please remove.

> +#ifdef AHCI_13_STRICT

Please drop the #ifdef.  #ifdefs mean dead code that is not being
compiled or tested.  Just decide which case we should take and keep that
one.

> +    /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */
> +    assert_bit_clear(datal, 0xFF);
> +#else
> +    if (datal & 0xFF) {
> +        g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF);
> +    }
> +#endif

Tests should not produce output.  Either this is a failure that needs to
be fixed in QEMU, or it is not a failure and we shouldn't produce
output.

The rationale is that producing "warning" output means we now need to
diff the make check output to see when it changes.  In practice no one
will pay attention and warnings will build up.

> +
> +    /* BCC *must* equal 0x01. */
> +    g_assert(PCI_BCC(datal) == 0x01);

g_assert_cmphex() would make the error message more useful by including
the incorrect value.  The same applies elsewhere in this patch.

> +#ifdef AHCI_13_STRICT
> +    /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */
> +    g_assert((data & 0xFF) == PCI_CAP_ID_PM);
> +#elif defined Q35
> +    /* Intel ICH9 Family Datasheet 14.1.19 p.550 */
> +    if ((data & 0xFF) != PCI_CAP_ID_MSI) {
> +        g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)",
> +                       data & 0xFF, PCI_CAP_ID_MSI);
> +    }
> +    /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/
> +#else
> +    if ((data & 0xFF) != PCI_CAP_ID_PM) {
> +        g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)",
> +                       data & 0xFF, PCI_CAP_ID_PM);
> +    }
> +#endif

Since the test hardcodes the q35 machine type when calling
qtest_start(), I guess the Q35 case always applies.

Please remove the #ifdef and warning (either it's a failure that needs
to be fixed, or it's not a failure).

> +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);

Debugging output that should be removed?

> +
> +    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);

This should be a failure.  If a new capability is added, the test should
be extended for that capability.

> +static void ahci_test_satacap(QPCIDevice *ahci, uint8_t offset)
> +{
> +    uint16_t dataw;
> +    uint32_t datal;
> +
> +    g_test_message("Verifying SATACAP");

Debug message that should be removed?

> +static void ahci_test_msicap(QPCIDevice *ahci, uint8_t offset)
> +{
> +    uint16_t dataw;
> +    uint32_t datal;
> +
> +    g_test_message("Verifying MSICAP");

Debug message that should be removed?

> +    if (dataw & PCI_MSI_FLAGS_64BIT) {
> +        g_test_message("MSICAP is 64bit");

Debug message that should be removed?

> +        g_assert(qpci_config_readl(ahci, offset + PCI_MSI_ADDRESS_HI) == 0x00);
> +        g_assert(qpci_config_readw(ahci, offset + PCI_MSI_DATA_64) == 0x00);
> +    } else {
> +        g_test_message("MSICAP is 32bit");

Debug message that should be removed?

> +static void ahci_test_pmcap(QPCIDevice *ahci, uint8_t offset)
> +{
> +    uint16_t dataw;
> +
> +    g_test_message("Verifying PMCAP");

Debug message that should be removed?

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

Re: [Qemu-devel] [PATCH 24/28] ahci: Add test_pci_spec to ahci-test.

[John Snow]

On 07/31/2014 09:19 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote:
>> +#ifdef AHCI_13_STRICT
> Please drop the #ifdef.  #ifdefs mean dead code that is not being
> compiled or tested.  Just decide which case we should take and keep that
> one.
OK. It might be nice to have some feedback on which is preferred, then. 
Intel Q35, perhaps?
It might be nice to leave some comments in at least to help document 
where the specifications diverge.
>
>> +    /* AHCI 1.3 specifies that at-boot, the RID should reset to 0x00. */
>> +    assert_bit_clear(datal, 0xFF);
>> +#else
>> +    if (datal & 0xFF) {
>> +        g_test_message("WARN: Revision ID (%u) != 0", datal & 0xFF);
>> +    }
>> +#endif
> Tests should not produce output.  Either this is a failure that needs to
> be fixed in QEMU, or it is not a failure and we shouldn't produce
> output.
OK. In this case, I think the specification might be "wrong" in that the 
RID should in fact be implementation-dependent, and it is perhaps a typo 
or an oversight that it is set to 0x00. It would make sense to just 
delete this check and leave a comment explaining why it's not checked. 
Let me know if I am off-base here.
>
> The rationale is that producing "warning" output means we now need to
> diff the make check output to see when it changes.  In practice no one
> will pay attention and warnings will build up.
>
>> +
>> +    /* BCC *must* equal 0x01. */
>> +    g_assert(PCI_BCC(datal) == 0x01);
> g_assert_cmphex() would make the error message more useful by including
> the incorrect value.  The same applies elsewhere in this patch.
>
>> +#ifdef AHCI_13_STRICT
>> +    /* AHCI 1.3, Section 2.1.14 -- CAP must point to PMCAP. */
>> +    g_assert((data & 0xFF) == PCI_CAP_ID_PM);
>> +#elif defined Q35
>> +    /* Intel ICH9 Family Datasheet 14.1.19 p.550 */
>> +    if ((data & 0xFF) != PCI_CAP_ID_MSI) {
>> +        g_test_message("WARN: 1st Capability ID (%u) is not MSICAP (%u)",
>> +                       data & 0xFF, PCI_CAP_ID_MSI);
>> +    }
>> +    /*g_assert((data & 0xFF) == PCI_CAP_ID_MSI);*/
>> +#else
>> +    if ((data & 0xFF) != PCI_CAP_ID_PM) {
>> +        g_test_message("WARN: 1st Capability ID (%u) is not PMCAP (%u)",
>> +                       data & 0xFF, PCI_CAP_ID_PM);
>> +    }
>> +#endif
> Since the test hardcodes the q35 machine type when calling
> qtest_start(), I guess the Q35 case always applies.
>
> Please remove the #ifdef and warning (either it's a failure that needs
> to be fixed, or it's not a failure).
This is where I was unable to really make a judgment call and some more 
input would be nice. AHCI 1.3 and Intel Q35/ICH9 specifications are 
actually at odds with one another -- The ability to test either/or might 
not be terrible.

The warnings here are somewhat minor, pedantic nitpicks ... though in a 
future patch I did fix the ordering, so I can just make these hard errors.

>
>> +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);
> Debugging output that should be removed?
The output here is only visible via --verbose. If even this is 
undesirable, I can remove it completely ... but I figured it would be 
nice to leave in some really basic debugging messages.
>
>> +
>> +    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);
> This should be a failure.  If a new capability is added, the test should
> be extended for that capability.
The specification does allow for any number of arbitrary capabilities, 
in which the specification has no say about order or compliance. Any 
further investigation really delves into the PCI specification, which 
was not my aim here. I think mentioning the inability to verify a 
capability is fine via --verbose for the purposes of basic AHCI sanity 
tests. At any rate, I don't think this is a failure -- at least not in 
THIS test, which I tried to keep as a "spec-adherent, QEMU indifferent" 
test. If we want to test QEMU-specifics, I would like to add those into 
a separate test case -- at which point failures for unrecognized 
capabilities would be appropriate.

In the future, we might even abstract out these pieces that apply to PCI 
devices as a whole to be generic PCI spec compliance tests, with an AHCI 
and then a QEMU layer on top, in order of increasing specificity.

Re: [Qemu-devel] [PATCH 24/28] ahci: Add test_pci_spec to ahci-test.

[Stefan Hajnoczi]

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

On Thu, Jul 31, 2014 at 01:42:02PM -0400, John Snow wrote:
> 
> On 07/31/2014 09:19 AM, Stefan Hajnoczi wrote:
> >On Mon, Jul 07, 2014 at 02:18:05PM -0400, John Snow wrote:
> >>+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);
> >Debugging output that should be removed?
> The output here is only visible via --verbose. If even this is undesirable,
> I can remove it completely ... but I figured it would be nice to leave in
> some really basic debugging messages.

When I come across verbose output during review I'm not sure whether you
have it there because you weren't sure about something that still needs
to be discussed before merging the patch.  I'm trying to identify loose
ends by asking these questions.

> >>+
> >>+    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);
> >This should be a failure.  If a new capability is added, the test should
> >be extended for that capability.
> The specification does allow for any number of arbitrary capabilities, in
> which the specification has no say about order or compliance. Any further
> investigation really delves into the PCI specification, which was not my aim
> here. I think mentioning the inability to verify a capability is fine via
> --verbose for the purposes of basic AHCI sanity tests. At any rate, I don't
> think this is a failure -- at least not in THIS test, which I tried to keep
> as a "spec-adherent, QEMU indifferent" test. If we want to test
> QEMU-specifics, I would like to add those into a separate test case -- at
> which point failures for unrecognized capabilities would be appropriate.
> 
> In the future, we might even abstract out these pieces that apply to PCI
> devices as a whole to be generic PCI spec compliance tests, with an AHCI and
> then a QEMU layer on top, in order of increasing specificity.

Okay, just keep in mind that only someone actively developing the test
case will notice verbose output.  By skipping unknown capabilities we
won't have a reminder that this test case needs to be updated when a new
one is added.

Stefan

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

[Qemu-devel] [PATCH 25/28] 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 | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index e0a8f34..7b45064 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -38,6 +38,13 @@
 #include "hw/pci/pci_ids.h"
 #include "hw/pci/pci_regs.h"
 
+/* Test the Q35/ICH9 behavior in preference to AHCI 1.3 behavior */
+#define Q35
+
+#ifndef Q35
+#define AHCI_13_STRICT
+#endif
+
 /* Test-specific defines. */
 #define TEST_IMAGE_SIZE    (64 * 1024 * 1024)
 
@@ -66,6 +73,7 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
 
 /*** Function Declarations ***/
 static QPCIDevice *get_ahci_device(void);
+static QPCIDevice *start_ahci_device(QPCIDevice *dev, HBA **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,
@@ -174,6 +182,58 @@ 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, HBA **hba_base)
+{
+    uint8_t reg;
+
+    start_ahci_device(ahci, hba_base);
+
+#ifdef Q35
+    /* 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);
+#endif
+}
+
+/**
+ * Map BAR5/ABAR, and engage the PCI device.
+ */
+static QPCIDevice *start_ahci_device(QPCIDevice *ahci, HBA **hba_base)
+{
+    uint16_t data;
+
+    /* Map AHCI's ABAR (BAR5) */
+    *hba_base = qpci_iomap(ahci, 5);
+
+    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
+    qpci_device_enable(ahci);
+
+#ifdef USE_MSI
+    data |= PCI_COMMAND_INTX_DISABLE;
+    qpci_config_writew(ahci, PCI_COMMAND, data);
+    data = qpci_config_readw(ahci, PCI_COMMAND);
+#endif
+
+    /* These bits should now test as on. */
+    data = qpci_config_readw(ahci, PCI_COMMAND);
+    assert_bit_set(data, PCI_COMMAND_IO);
+    assert_bit_set(data, PCI_COMMAND_MEMORY);
+    assert_bit_set(data, PCI_COMMAND_MASTER);
+#ifdef USE_MSI
+    assert_bit_set(data, PCI_COMMAND_INTX_DISABLE);
+#endif
+
+    return ahci;
+}
+
 /*** Specification Adherence Tests ***/
 
 /**
@@ -440,6 +500,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;
+    HBA *hba_base;
+    ahci_boot(&ahci);
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -468,6 +541,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();
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 25/28] ahci: add test_pci_enable to ahci-test.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:06PM -0400, John Snow wrote:
> +/* Test the Q35/ICH9 behavior in preference to AHCI 1.3 behavior */
> +#define Q35
> +
> +#ifndef Q35
> +#define AHCI_13_STRICT
> +#endif
> +

Please don't use an #ifdef.  If you really want to keep both code paths
around, make it a C variable (e.g. a bool).  That way the compiler will
parse the code at least.

But there's nothing wrong with keeping the code Q35-only for now,
especially if you comment which assertions are Q35 quirks.  I would
simply drop the non-Q35 code paths.

> +/**
> + * Map BAR5/ABAR, and engage the PCI device.
> + */
> +static QPCIDevice *start_ahci_device(QPCIDevice *ahci, HBA **hba_base)

Now I see how HBA is used.  The test case shouldn't do this, please drop
HBA.

Either decide how all of libqtest/libqos should type guest physical
memory addresses and refactor the code, or just use the types from
libqtest/libqos (void*).

If every test case author invents their own type we'll have a lot of
boilerplate and test cases will be inconsistent.

> +{
> +    uint16_t data;
> +
> +    /* Map AHCI's ABAR (BAR5) */
> +    *hba_base = qpci_iomap(ahci, 5);
> +
> +    /* turns on pci.cmd.iose, pci.cmd.mse and pci.cmd.bme */
> +    qpci_device_enable(ahci);
> +
> +#ifdef USE_MSI
> +    data |= PCI_COMMAND_INTX_DISABLE;
> +    qpci_config_writew(ahci, PCI_COMMAND, data);
> +    data = qpci_config_readw(ahci, PCI_COMMAND);
> +#endif

Same comment as other #ifdefs.  Please don't use them.  Either test both
MSI and non-MSI cases if the difference matters, or just keep one code
path without #ifdefs.

> +
> +    /* These bits should now test as on. */
> +    data = qpci_config_readw(ahci, PCI_COMMAND);
> +    assert_bit_set(data, PCI_COMMAND_IO);
> +    assert_bit_set(data, PCI_COMMAND_MEMORY);
> +    assert_bit_set(data, PCI_COMMAND_MASTER);

Please move this to qpci_device_enable() so that all tests benefit from
these assertions.

> +#ifdef USE_MSI
> +    assert_bit_set(data, PCI_COMMAND_INTX_DISABLE);
> +#endif

MSI stuff should probably be in pci.h/pci.c/pci-pc.c so other MSI users
can take advantage of it.

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

[Qemu-devel] [PATCH 26/28] 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 | 581 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 581 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 7b45064..727c939 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -56,9 +56,214 @@
 #define PCI_PI(REG32)   (((REG32) >> 8) & 0xFF)
 #define PCI_SCC(REG32) (((REG32) >> 16) & 0xFF)
 
+/*** 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)
+
+
 /* To help make it clear that the HBA is not a pointer to local memory. */
 typedef void HBA;
 
+typedef struct hba_cap {
+    uint32_t cap;
+    uint32_t cap2;
+} hba_cap;
+
 /*** Globals ***/
 static QGuestAllocator *guest_malloc;
 static QPCIBus *pcibus;
@@ -71,10 +276,31 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
 #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 void_incr(vptr, OFST) ((void *)((char *)(vptr) + (OFST)))
+#define ahci_read(OFST) qpci_io_readl(ahci, void_incr(hba_base, (OFST)))
+#define ahci_write(OFST, VAL) qpci_io_writel(ahci,                      \
+                                             void_incr(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);
 static QPCIDevice *start_ahci_device(QPCIDevice *dev, HBA **hba_base);
 static void free_ahci_device(QPCIDevice *dev);
+static void ahci_test_port_spec(QPCIDevice *ahci, HBA *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);
@@ -474,6 +700,345 @@ 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, HBA *hba_base)
+{
+    struct hba_cap hcap;
+    unsigned i;
+    uint32_t base_addr, size, 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.
+     */
+
+    /* We need to know the size of the region,
+     * but qpci_iomap doesn't save it. Recalculate it. */
+    base_addr = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5);
+    qpci_config_writel(ahci, PCI_BASE_ADDRESS_5, 0xFFFFFFFF);
+    size = qpci_config_readl(ahci, PCI_BASE_ADDRESS_5);
+    size = 1 << ctzl(size);
+    qpci_config_writel(ahci, PCI_BASE_ADDRESS_5, base_addr);
+
+    /* 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(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);
+
+    /* 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 = (size - 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(reg == 0);
+    }
+
+    /* 7 CCC_PORTS */
+    reg = ahci_rreg(AHCI_CCCPORTS);
+    /* Must be zeroes initially regardless of CAP.CCCS */
+    g_assert(reg == 0);
+
+    /* 8 EM_LOC */
+    reg = ahci_rreg(AHCI_EMLOC);
+    if (bitclr(cap, AHCI_CAP_EMS)) {
+        g_assert(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(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(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(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(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(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(reg == 0);
+            }
+        }
+    }
+}
+
+/**
+ * Test the memory space for one port for specification adherence.
+ */
+static void ahci_test_port_spec(QPCIDevice *ahci, HBA *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(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(reg == 0);
+    }
+
+    /* (4) IS */
+    reg = px_rreg(port, AHCI_PX_IS);
+    g_assert(reg == 0);
+
+    /* (5) IE */
+    reg = px_rreg(port, AHCI_PX_IE);
+    g_assert(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(reg == 0);
+
+    /* (8) TFD */
+    reg = px_rreg(port, AHCI_PX_TFD);
+#ifdef AHCI_13_STRICT
+    /* In theory, prior to an FIS being received, should be 0x0000007F.
+     * Reserved & Error bytes empty, BSY bit clear and the rest ON. */
+    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);
+#else
+    if (reg != 0x7F) {
+        g_test_message("WARN: TFD is 0x%x instead of 0x7F", reg);
+    }
+#endif
+    assert_bit_clear(reg, AHCI_PX_TFD_RESERVED);
+
+
+    /* (9) SIG */
+    reg = px_rreg(port, AHCI_PX_SIG);
+#ifdef AHCI_13_STRICT
+    /* Should be all 1s until the first D2H Register FIS is received,
+     * which should not happen prior to FIS Receive Enable being set. */
+    g_assert(reg == 0xFFFFFFFF);
+#else
+    if (reg != 0xFFFFFFFF) {
+        g_test_message("WARN: Signature is 0x%x instead of 0xFFFFFFFF", reg);
+    }
+#endif
+
+    /* (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(reg == 0);
+
+    /* (12) SERR / SCR1: SError */
+    reg = px_rreg(port, AHCI_PX_SERR);
+    g_assert(reg == 0);
+
+    /* (13) SACT / SCR3: SActive */
+    reg = px_rreg(port, AHCI_PX_SACT);
+    g_assert(reg == 0);
+
+    /* (14) CI */
+    reg = px_rreg(port, AHCI_PX_CI);
+    g_assert(reg == 0);
+
+    /* (15) SNTF */
+    reg = px_rreg(port, AHCI_PX_SNTF);
+    g_assert(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(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                                                            */
 /******************************************************************************/
@@ -513,6 +1078,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;
+    HBA *hba_base;
+
+    ahci_boot(&ahci);
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_test_hba_spec(ahci, hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -542,6 +1122,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

Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:07PM -0400, John Snow wrote:
> +/*** IO macros for the AHCI memory registers. ***/
> +#define void_incr(vptr, OFST) ((void *)((char *)(vptr) + (OFST)))

I'm pretty sure QEMU takes advantage of GCC's void pointer arithmetic
extension:
https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Pointer-Arith.html#Pointer-Arith

In other words, vptr + OFST works and you don't need a macro.

> +#define ahci_set(regno, mask) ahci_wreg((regno), ahci_rreg(regno) | (mask))
> +#define ahci_clr(regno, mask) ahci_wreg((regno), ahci_rreg(regno) & ~(mask))

Unused.  Please move to the patch that actually uses them.

> +#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));

Unused.  Please move to the patch that actually uses them.

> +    /* We need to know the size of the region,
> +     * but qpci_iomap doesn't save it. Recalculate it. */

It seems like many tests will want to check the BAR size.  Please add an
argument to qpci_iomap() so the caller gets the size.

> +    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);
> +    }

Let's just assert what QEMU implements.

> +    /* 12 -- 23: Reserved */
> +    g_test_message("Verifying HBA reserved area is empty.");

Debugging message that can be removed?

More elsewhere in this patch.

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

Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.

[John Snow]

On 07/31/2014 10:01 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:18:07PM -0400, John Snow wrote:
>> +    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);
>> +    }
> Let's just assert what QEMU implements.
It was mentioned at least once to me that supporting this would be 
interesting. As I was trying to write a spec test, I don't think this 
conditional is hurting anyone. Certainly if we DO decide to implement a 
mixed-mode device later, who will remember to add this check back here?

Especially since it's not an ugly ifdef like my other "what if" cases, I 
feel like it could stay.

Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.

[John Snow]

On 07/31/2014 10:01 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 07, 2014 at 02:18:07PM -0400, John Snow wrote:
>> +/*** IO macros for the AHCI memory registers. ***/
>> +#define void_incr(vptr, OFST) ((void *)((char *)(vptr) + (OFST)))
> I'm pretty sure QEMU takes advantage of GCC's void pointer arithmetic
> extension:
> https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Pointer-Arith.html#Pointer-Arith
>
> In other words, vptr + OFST works and you don't need a macro.
>
>> +#define ahci_set(regno, mask) ahci_wreg((regno), ahci_rreg(regno) | (mask))
>> +#define ahci_clr(regno, mask) ahci_wreg((regno), ahci_rreg(regno) & ~(mask))
> Unused.  Please move to the patch that actually uses them.
>
>> +#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));
> Unused.  Please move to the patch that actually uses them.
>
>> +    /* We need to know the size of the region,
>> +     * but qpci_iomap doesn't save it. Recalculate it. */
> It seems like many tests will want to check the BAR size.  Please add an
> argument to qpci_iomap() so the caller gets the size.
>
>> +    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);
>> +    }
> Let's just assert what QEMU implements.
>
>> +    /* 12 -- 23: Reserved */
>> +    g_test_message("Verifying HBA reserved area is empty.");
> Debugging message that can be removed?
>
> More elsewhere in this patch.

one last question -- is there a reasonable way to have assertions that 
allow the test to shamble forward, still fail at the end, and still run 
subsequent test cases?
A lot of the warnings I threw in here are actually errors, but it'd 
still be useful to see the rest of the test run to completion as best as 
it can.

You can call g_test_set_nonfatal_assertions(), but it actually appears 
as if (on my machine at least) that this is a nop, which is not super 
helpful.

It'd be nice if the checked in version of the test showed you the myriad 
failings, instead of just one at a time until you hack in/out certain 
assertions manually if you are interested in other areas.

Re: [Qemu-devel] [PATCH 26/28] ahci: Add test_hba_spec to ahci-test.

[Stefan Hajnoczi]

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

On Fri, Aug 01, 2014 at 07:27:57PM -0400, John Snow wrote:
> 
> On 07/31/2014 10:01 AM, Stefan Hajnoczi wrote:
> >On Mon, Jul 07, 2014 at 02:18:07PM -0400, John Snow wrote:
> >>+/*** IO macros for the AHCI memory registers. ***/
> >>+#define void_incr(vptr, OFST) ((void *)((char *)(vptr) + (OFST)))
> >I'm pretty sure QEMU takes advantage of GCC's void pointer arithmetic
> >extension:
> >https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/Pointer-Arith.html#Pointer-Arith
> >
> >In other words, vptr + OFST works and you don't need a macro.
> >
> >>+#define ahci_set(regno, mask) ahci_wreg((regno), ahci_rreg(regno) | (mask))
> >>+#define ahci_clr(regno, mask) ahci_wreg((regno), ahci_rreg(regno) & ~(mask))
> >Unused.  Please move to the patch that actually uses them.
> >
> >>+#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));
> >Unused.  Please move to the patch that actually uses them.
> >
> >>+    /* We need to know the size of the region,
> >>+     * but qpci_iomap doesn't save it. Recalculate it. */
> >It seems like many tests will want to check the BAR size.  Please add an
> >argument to qpci_iomap() so the caller gets the size.
> >
> >>+    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);
> >>+    }
> >Let's just assert what QEMU implements.
> >
> >>+    /* 12 -- 23: Reserved */
> >>+    g_test_message("Verifying HBA reserved area is empty.");
> >Debugging message that can be removed?
> >
> >More elsewhere in this patch.
> 
> one last question -- is there a reasonable way to have assertions that allow
> the test to shamble forward, still fail at the end, and still run subsequent
> test cases?
> A lot of the warnings I threw in here are actually errors, but it'd still be
> useful to see the rest of the test run to completion as best as it can.
> 
> You can call g_test_set_nonfatal_assertions(), but it actually appears as if
> (on my machine at least) that this is a nop, which is not super helpful.
> 
> It'd be nice if the checked in version of the test showed you the myriad
> failings, instead of just one at a time until you hack in/out certain
> assertions manually if you are interested in other areas.

I'm not aware of a glib API to do that.  You can tell gtester which test
cases to invoke if you need to skip or only run certain cases.  That
might be useful during development.

Stefan

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

[Qemu-devel] [PATCH 27/28] 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 | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 727c939..f342e2c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -460,6 +460,140 @@ static QPCIDevice *start_ahci_device(QPCIDevice *ahci, HBA **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, HBA *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(reg == 0);
+
+        reg = px_rreg(i, AHCI_PX_IS);
+        g_assert(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 ***/
 
 /**
@@ -1093,6 +1227,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;
+    HBA *hba_base;
+
+    ahci_boot(&ahci);
+    ahci_pci_enable(ahci, &hba_base);
+    ahci_hba_enable(ahci, hba_base);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 
 int main(int argc, char **argv)
@@ -1123,6 +1272,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

Re: [Qemu-devel] [PATCH 27/28] ahci: Add test_hba_enable to ahci-test.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:08PM -0400, John Snow wrote:
> 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 | 150 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)

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

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

[Qemu-devel] [PATCH 28/28] 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 f342e2c..a410b97 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -255,6 +255,96 @@
 #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) */
+};
 
 /* To help make it clear that the HBA is not a pointer to local memory. */
 typedef void HBA;
@@ -295,6 +385,10 @@ static char tmp_path[] = "/tmp/qtest.XXXXXX";
 #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, HBA **hba_base);
@@ -310,6 +404,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((bytes & 1) == 0);
+    bytes /= 2;
+
+    while (bytes--) {
+        *s = cpu_to_be16(*s);
+        s++;
+    }
+}
+
 /**
  * Locate, verify, and return a handle to the AHCI device.
  */
@@ -1173,6 +1278,180 @@ static void ahci_test_port_spec(QPCIDevice *ahci, HBA *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, HBA *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(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 != 0);
+
+    /* 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);
+    if (bitclr(reg, AHCI_PX_IS_DPS)) {
+        g_test_message("WARN: Expected to see DPS bit set in PxIS");
+    }
+    /* 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(rc == 0);
+
+    string_cpu_to_be16(&buff[23], 8);
+    rc = memcmp(&buff[23], "version ", 8);
+    g_assert(rc == 0);
+
+    free(d2h);
+    free(pio);
+}
+
 /******************************************************************************/
 /* Test Interfaces                                                            */
 /******************************************************************************/
@@ -1242,6 +1521,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;
+    HBA *hba_base;
+
+    ahci_boot(&ahci);
+    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)
@@ -1273,6 +1568,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 28/28] ahci: Add test_identify case to ahci-test.

[Stefan Hajnoczi]

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

On Mon, Jul 07, 2014 at 02:18:09PM -0400, John Snow wrote:
> +    free(d2h);
> +    free(pio);

Should be g_free() since memory was allocated with g_malloc0().

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