[Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[John Snow]

We don't have any CDROM tests yet.
So, add some for the PCI/BMDMA HBA.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ide-atapi-test
https://github.com/jnsnow/qemu/tree/ide-atapi-test

This version is tagged ide-atapi-test-v1:
https://github.com/jnsnow/qemu/releases/tag/ide-atapi-test-v1

John Snow (4):
  qtest/ahci: use generate_pattern everywhere
  qtest/ahci: export generate_pattern
  ide-test: add cdrom pio test
  ide-test: add cdrom dma test

 tests/ahci-test.c     |  43 +---------
 tests/ide-test.c      | 232 ++++++++++++++++++++++++++++++++++++++++++++++----
 tests/libqos/libqos.c |  26 ++++++
 tests/libqos/libqos.h |   1 +
 4 files changed, 245 insertions(+), 57 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] qtest/ahci: use generate_pattern everywhere

[John Snow]

Fix the pattern generation to actually be interesting,
and make sure all buffers in the ahci-test actually use it.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 87d7691..b1a785c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -80,9 +80,9 @@ static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
 
     /* Write an indicative pattern that varies and is unique per-cycle */
     p = rand() % 256;
-    for (i = j = 0; i < len; i++, j++) {
-        tx[i] = p;
-        if (j % cycle_len == 0) {
+    for (i = 0; i < len; i++) {
+        tx[i] = p++ % 256;
+        if (i % cycle_len == 0) {
             p = rand() % 256;
         }
     }
@@ -1155,7 +1155,6 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
     size_t bufsize = 4096;
     unsigned char *tx = g_malloc(bufsize);
     unsigned char *rx = g_malloc0(bufsize);
-    unsigned i;
     const char *uri = "tcp:127.0.0.1:1234";
 
     src = ahci_boot_and_enable("-m 1024 -M q35 "
@@ -1171,9 +1170,7 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
     ahci_port_clear(src, px);
 
     /* create pattern */
-    for (i = 0; i < bufsize; i++) {
-        tx[i] = (bufsize - i);
-    }
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
 
     /* Write, migrate, then read. */
     ahci_io(src, px, cmd_write, tx, bufsize, 0);
@@ -1213,7 +1210,6 @@ static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
     size_t bufsize = 4096;
     unsigned char *tx = g_malloc(bufsize);
     unsigned char *rx = g_malloc0(bufsize);
-    unsigned i;
     uint64_t ptr;
     AHCICommand *cmd;
 
@@ -1231,11 +1227,8 @@ static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
     port = ahci_port_select(ahci);
     ahci_port_clear(ahci, port);
 
-    for (i = 0; i < bufsize; i++) {
-        tx[i] = (bufsize - i);
-    }
-
     /* create DMA source buffer and write pattern */
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
     ptr = ahci_alloc(ahci, bufsize);
     g_assert(ptr);
     memwrite(ptr, tx, bufsize);
@@ -1282,7 +1275,6 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
     size_t bufsize = 4096;
     unsigned char *tx = g_malloc(bufsize);
     unsigned char *rx = g_malloc0(bufsize);
-    unsigned i;
     uint64_t ptr;
     AHCICommand *cmd;
     const char *uri = "tcp:127.0.0.1:1234";
@@ -1310,10 +1302,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
     /* Initialize and prepare */
     port = ahci_port_select(src);
     ahci_port_clear(src, port);
-
-    for (i = 0; i < bufsize; i++) {
-        tx[i] = (bufsize - i);
-    }
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
 
     /* create DMA source buffer and write pattern */
     ptr = ahci_alloc(src, bufsize);
-- 
2.4.3

[Qemu-devel] [PATCH 2/4] qtest/ahci: export generate_pattern

[John Snow]

Share the pattern function for ide and ahci test.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index b1a785c..59d387c 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -71,32 +71,6 @@ static void string_bswap16(uint16_t *s, size_t bytes)
     }
 }
 
-static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
-{
-    int i, j;
-    unsigned char *tx = (unsigned char *)buffer;
-    unsigned char p;
-    size_t *sx;
-
-    /* Write an indicative pattern that varies and is unique per-cycle */
-    p = rand() % 256;
-    for (i = 0; i < len; i++) {
-        tx[i] = p++ % 256;
-        if (i % cycle_len == 0) {
-            p = rand() % 256;
-        }
-    }
-
-    /* force uniqueness by writing an id per-cycle */
-    for (i = 0; i < len / cycle_len; i++) {
-        j = i * cycle_len;
-        if (j + sizeof(*sx) <= len) {
-            sx = (size_t *)&tx[j];
-            *sx = i;
-        }
-    }
-}
-
 /**
  * Verify that the transfer did not corrupt our state at all.
  */
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index fce625b..8d7c5a9 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -212,3 +212,29 @@ void prepare_blkdebug_script(const char *debug_fn, const char *event)
     ret = fclose(debug_file);
     g_assert(ret == 0);
 }
+
+void generate_pattern(void *buffer, size_t len, size_t cycle_len)
+{
+    int i, j;
+    unsigned char *tx = (unsigned char *)buffer;
+    unsigned char p;
+    size_t *sx;
+
+    /* Write an indicative pattern that varies and is unique per-cycle */
+    p = rand() % 256;
+    for (i = 0; i < len; i++) {
+        tx[i] = p++ % 256;
+        if (i % cycle_len == 0) {
+            p = rand() % 256;
+        }
+    }
+
+    /* force uniqueness by writing an id per-cycle */
+    for (i = 0; i < len / cycle_len; i++) {
+        j = i * cycle_len;
+        if (j + sizeof(*sx) <= len) {
+            sx = (size_t *)&tx[j];
+            *sx = i;
+        }
+    }
+}
diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
index e1f14ea..492a651 100644
--- a/tests/libqos/libqos.h
+++ b/tests/libqos/libqos.h
@@ -24,6 +24,7 @@ void mkqcow2(const char *file, unsigned size_mb);
 void set_context(QOSState *s);
 void migrate(QOSState *from, QOSState *to, const char *uri);
 void prepare_blkdebug_script(const char *debug_fn, const char *event);
+void generate_pattern(void *buffer, size_t len, size_t cycle_len);
 
 static inline uint64_t qmalloc(QOSState *q, size_t bytes)
 {
-- 
2.4.3

[Qemu-devel] [PATCH 3/4] ide-test: add cdrom pio test

[John Snow]

Add a simple read test for ATAPI devices,
using the PIO mechanism.

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

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 4a07e3a..90524e3 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -45,6 +45,12 @@
 #define IDE_BASE 0x1f0
 #define IDE_PRIMARY_IRQ 14
 
+#define ATAPI_BLOCK_SIZE 2048
+
+/* How many bytes to receive via ATAPI PIO at one time.
+ * Must be less than 0xFFFF. */
+#define BYTE_COUNT_LIMIT 5120
+
 enum {
     reg_data        = 0x0,
     reg_nsectors    = 0x2,
@@ -80,6 +86,7 @@ enum {
     CMD_WRITE_DMA   = 0xca,
     CMD_FLUSH_CACHE = 0xe7,
     CMD_IDENTIFY    = 0xec,
+    CMD_PACKET      = 0xa0,
 
     CMDF_ABORT      = 0x100,
     CMDF_NO_BM      = 0x200,
@@ -585,6 +592,140 @@ static void test_isa_retry_flush(const char *machine)
     test_retry_flush("isapc");
 }
 
+typedef struct Read10CDB {
+    uint8_t opcode;
+    uint8_t flags;
+    uint32_t lba;
+    uint8_t reserved;
+    uint16_t nblocks;
+    uint8_t control;
+    uint16_t padding;
+} __attribute__((__packed__)) Read10CDB;
+
+static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
+{
+    Read10CDB pkt = { .padding = 0 };
+    int i;
+
+    /* Construct SCSI CDB packet */
+    pkt.opcode = 0x28;
+    pkt.lba = cpu_to_be32(lba);
+    pkt.nblocks = cpu_to_be16(nblocks);
+
+    /* Send Packet */
+    for (i = 0; i < sizeof(Read10CDB)/2; i++) {
+        outw(IDE_BASE + reg_data, ((uint16_t *)&pkt)[i]);
+    }
+}
+
+static void nsleep(int64_t nsecs)
+{
+    const struct timespec val = { .tv_nsec = nsecs };
+    nanosleep(&val, NULL);
+    clock_set(nsecs);
+}
+
+static uint8_t ide_wait_clear(uint8_t flag)
+{
+    int i;
+    uint8_t data;
+
+    /* Wait with a 5 second timeout */
+    for (i = 0; i <= 12500000; i++) {
+        data = inb(IDE_BASE + reg_status);
+        if (!(data & flag)) {
+            return data;
+        }
+        nsleep(400);
+    }
+    g_assert_not_reached();
+    return 0xff;
+}
+
+static void cdrom_pio_impl(int nblocks)
+{
+    FILE *fh;
+    size_t patt_len = ATAPI_BLOCK_SIZE * MAX(16, nblocks);
+    char *pattern = g_malloc(patt_len);
+    size_t rxsize = ATAPI_BLOCK_SIZE * nblocks;
+    char *rx = g_malloc0(rxsize);
+    int i, j;
+    uint8_t data;
+    uint16_t limit;
+
+    /* Prepopulate the CDROM with an interesting pattern */
+    generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
+    fh = fopen(tmp_path, "w+");
+    fwrite(pattern, ATAPI_BLOCK_SIZE, MAX(16, nblocks), fh);
+    fclose(fh);
+
+    ide_test_start(
+      "-drive file=%s,if=ide,media=cdrom,cache=writeback,format=raw", tmp_path);
+    qtest_irq_intercept_in(global_qtest, "ioapic");
+
+    /* PACKET command on device 0 */
+    outb(IDE_BASE + reg_device, 0);
+    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
+    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
+    outb(IDE_BASE + reg_command, CMD_PACKET);
+    /* HPD0: Check_Status_A State */
+    nsleep(400);
+    data = ide_wait_clear(BSY);
+    /* HPD1: Send_Packet State */
+    assert_bit_set(data, DRQ | DRDY);
+    assert_bit_clear(data, ERR | DF | BSY);
+
+    /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
+    send_scsi_cdb_read10(0, nblocks);
+
+    /* HPD3: INTRQ_Wait */
+    i = 0;
+    do {
+        data = get_irq(IDE_PRIMARY_IRQ);
+        nsleep(400);
+        i++;
+        g_assert_cmpint(i, <=, 12500000);
+    } while (!data);
+
+    /* HPD2: Check_Status_B */
+    data = ide_wait_clear(BSY);
+    assert_bit_set(data, DRQ | DRDY);
+    assert_bit_clear(data, ERR | DF | BSY);
+
+    /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
+     * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
+     * We allow an odd limit only when the remaining transfer size is
+     * less than BYTE_COUNT_LIMIT.
+     * For our purposes, we can only request even multiples, so do not
+     * attempt to read remainders. */
+    limit = BYTE_COUNT_LIMIT & ~1;
+    for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
+        size_t r = (rxsize - (limit * i)) / 2;
+        for (j = 0; j < MIN((limit / 2), r); j++) {
+            ((uint16_t *)rx)[(i * (limit/2)) + j] = inw(IDE_BASE + reg_data);
+        }
+    }
+    data = ide_wait_clear(DRQ);
+    assert_bit_set(data, DRDY);
+    assert_bit_clear(data, DRQ | ERR | DF | BSY);
+
+    g_assert_cmpint(memcmp(pattern, rx, rxsize), ==, 0);
+    g_free(pattern);
+    g_free(rx);
+    test_bmdma_teardown();
+}
+
+static void test_cdrom_pio(void)
+{
+    cdrom_pio_impl(1);
+}
+
+static void test_cdrom_pio_large(void)
+{
+    /* Test a few loops of the PIO DRQ mechanism. */
+    cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -628,6 +769,9 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
     qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
 
+    qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
+    qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
+
     ret = g_test_run();
 
     /* Cleanup */
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] ide-test: add cdrom dma test

[John Snow]

Now, test the DMA functionality of the ATAPI drive.

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

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 90524e3..f4a913d 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -53,6 +53,7 @@
 
 enum {
     reg_data        = 0x0,
+    reg_feature     = 0x1,
     reg_nsectors    = 0x2,
     reg_lba_low     = 0x3,
     reg_lba_middle  = 0x4,
@@ -179,7 +180,8 @@ typedef struct PrdtEntry {
 #define assert_bit_clear(data, mask) g_assert_cmphex((data) & (mask), ==, 0)
 
 static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
-                            PrdtEntry *prdt, int prdt_entries)
+                            PrdtEntry *prdt, int prdt_entries,
+                            void(*post_exec)(uint64_t sector, int nb_sectors))
 {
     QPCIDevice *dev;
     uint16_t bmdma_base;
@@ -196,6 +198,9 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
 
     switch (cmd) {
     case CMD_READ_DMA:
+    case CMD_PACKET:
+        /* Assuming we only test data reads w/ ATAPI, otherwise we need to know
+         * the SCSI command being sent in the packet, too. */
         from_dev = true;
         break;
     case CMD_WRITE_DMA:
@@ -224,14 +229,22 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
     outl(bmdma_base + bmreg_prdt, guest_prdt);
 
     /* ATA DMA command */
-    outb(IDE_BASE + reg_nsectors, nb_sectors);
-
-    outb(IDE_BASE + reg_lba_low,    sector & 0xff);
-    outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
-    outb(IDE_BASE + reg_lba_high,   (sector >> 16) & 0xff);
+    if (cmd == CMD_PACKET) {
+        /* Enables ATAPI DMA; otherwise PIO is attempted */
+        outb(IDE_BASE + reg_feature, 0x01);
+    } else {
+        outb(IDE_BASE + reg_nsectors, nb_sectors);
+        outb(IDE_BASE + reg_lba_low,    sector & 0xff);
+        outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
+        outb(IDE_BASE + reg_lba_high,   (sector >> 16) & 0xff);
+    }
 
     outb(IDE_BASE + reg_command, cmd);
 
+    if (post_exec) {
+        post_exec(sector, nb_sectors);
+    }
+
     /* Start DMA transfer */
     outb(bmdma_base + bmreg_cmd, BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
 
@@ -285,7 +298,8 @@ static void test_bmdma_simple_rw(void)
     memset(buf, 0x55, len);
     memwrite(guest_buf, buf, len);
 
-    status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt, ARRAY_SIZE(prdt));
+    status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt,
+                              ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
@@ -293,14 +307,15 @@ static void test_bmdma_simple_rw(void)
     memset(buf, 0xaa, len);
     memwrite(guest_buf, buf, len);
 
-    status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt, ARRAY_SIZE(prdt));
+    status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
+                              ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
     /* Read and verify 0x55 pattern in sector 0 */
     memset(cmpbuf, 0x55, len);
 
-    status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt));
+    status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
@@ -310,7 +325,7 @@ static void test_bmdma_simple_rw(void)
     /* Read and verify 0xaa pattern in sector 1 */
     memset(cmpbuf, 0xaa, len);
 
-    status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt));
+    status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
@@ -335,13 +350,13 @@ static void test_bmdma_short_prdt(void)
 
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 1,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 }
@@ -360,13 +375,13 @@ static void test_bmdma_one_sector_short_prdt(void)
 
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 2,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 }
@@ -384,13 +399,13 @@ static void test_bmdma_long_prdt(void)
 
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 1,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 }
@@ -406,7 +421,7 @@ static void test_bmdma_no_busmaster(void)
     PrdtEntry prdt[4096] = { };
 
     status = send_dma_request(CMD_READ_DMA | CMDF_NO_BM, 0, 512,
-                              prdt, ARRAY_SIZE(prdt));
+                              prdt, ARRAY_SIZE(prdt), NULL);
 
     /* Not entirely clear what the expected result is, but this is what we get
      * in practice. At least we want to be aware of any changes. */
@@ -602,11 +617,15 @@ typedef struct Read10CDB {
     uint16_t padding;
 } __attribute__((__packed__)) Read10CDB;
 
-static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
+static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
 {
     Read10CDB pkt = { .padding = 0 };
     int i;
 
+    g_assert_cmpint(lba, <=, UINT32_MAX);
+    g_assert_cmpint(nblocks, <=, UINT16_MAX);
+    g_assert_cmpint(nblocks, >=, 0);
+
     /* Construct SCSI CDB packet */
     pkt.opcode = 0x28;
     pkt.lba = cpu_to_be32(lba);
@@ -726,6 +745,40 @@ static void test_cdrom_pio_large(void)
     cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
 }
 
+
+static void test_cdrom_dma(void)
+{
+    static const size_t len = ATAPI_BLOCK_SIZE;
+    char *pattern = g_malloc(ATAPI_BLOCK_SIZE * 16);
+    char *rx = g_malloc0(len);
+    uintptr_t guest_buf;
+    PrdtEntry prdt[1];
+    FILE *fh;
+
+    ide_test_start(
+      "-drive file=%s,if=ide,media=cdrom,cache=writeback,format=raw", tmp_path);
+    qtest_irq_intercept_in(global_qtest, "ioapic");
+
+    guest_buf = guest_alloc(guest_malloc, len);
+    prdt[0].addr = cpu_to_le32(guest_buf);
+    prdt[0].size = cpu_to_le32(len | PRDT_EOT);
+
+    generate_pattern(pattern, ATAPI_BLOCK_SIZE * 16, ATAPI_BLOCK_SIZE);
+    fh = fopen(tmp_path, "w+");
+    fwrite(pattern, ATAPI_BLOCK_SIZE, 16, fh);
+    fclose(fh);
+
+    send_dma_request(CMD_PACKET, 0, 1, prdt, 1, send_scsi_cdb_read10);
+
+    /* Read back data from guest memory into local qtest memory */
+    memread(guest_buf, rx, len);
+    g_assert_cmpint(memcmp(pattern, rx, len), ==, 0);
+
+    g_free(pattern);
+    g_free(rx);
+    test_bmdma_teardown();
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -771,6 +824,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
     qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
+    qtest_add_func("/ide/cdrom/dma", test_cdrom_dma);
 
     ret = g_test_run();
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[John Snow]

Ping -- apologies. :)

I'll take a simple ACK before I merge it into my tree.

On 08/26/2015 05:13 PM, John Snow wrote:
> We don't have any CDROM tests yet.
> So, add some for the PCI/BMDMA HBA.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ide-atapi-test
> https://github.com/jnsnow/qemu/tree/ide-atapi-test
> 
> This version is tagged ide-atapi-test-v1:
> https://github.com/jnsnow/qemu/releases/tag/ide-atapi-test-v1
> 
> John Snow (4):
>   qtest/ahci: use generate_pattern everywhere
>   qtest/ahci: export generate_pattern
>   ide-test: add cdrom pio test
>   ide-test: add cdrom dma test
> 
>  tests/ahci-test.c     |  43 +---------
>  tests/ide-test.c      | 232 ++++++++++++++++++++++++++++++++++++++++++++++----
>  tests/libqos/libqos.c |  26 ++++++
>  tests/libqos/libqos.h |   1 +
>  4 files changed, 245 insertions(+), 57 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[Paolo Bonzini]

On 09/09/2015 18:37, John Snow wrote:
> Ping -- apologies. :)
> 
> I'll take a simple ACK before I merge it into my tree.

I don't think you need one. :)

Paolo

> On 08/26/2015 05:13 PM, John Snow wrote:
>> We don't have any CDROM tests yet.
>> So, add some for the PCI/BMDMA HBA.
>>
>> ________________________________________________________________________________
>>
>> For convenience, this branch is available at:
>> https://github.com/jnsnow/qemu.git branch ide-atapi-test
>> https://github.com/jnsnow/qemu/tree/ide-atapi-test
>>
>> This version is tagged ide-atapi-test-v1:
>> https://github.com/jnsnow/qemu/releases/tag/ide-atapi-test-v1
>>
>> John Snow (4):
>>   qtest/ahci: use generate_pattern everywhere
>>   qtest/ahci: export generate_pattern
>>   ide-test: add cdrom pio test
>>   ide-test: add cdrom dma test
>>
>>  tests/ahci-test.c     |  43 +---------
>>  tests/ide-test.c      | 232 ++++++++++++++++++++++++++++++++++++++++++++++----
>>  tests/libqos/libqos.c |  26 ++++++
>>  tests/libqos/libqos.h |   1 +
>>  4 files changed, 245 insertions(+), 57 deletions(-)
>>

Re: [Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[John Snow]

On 09/09/2015 01:00 PM, Paolo Bonzini wrote:
> 
> 
> On 09/09/2015 18:37, John Snow wrote:
>> Ping -- apologies. :)
>>
>> I'll take a simple ACK before I merge it into my tree.
> 
> I don't think you need one. :)
> 
> Paolo
> 

I guess technically not, but I like when somebody at least takes a peek
before I just brazenly send pullreqs.

I haven't achieved the level of ATA COWBOY yet.

--js

>> On 08/26/2015 05:13 PM, John Snow wrote:
>>> We don't have any CDROM tests yet.
>>> So, add some for the PCI/BMDMA HBA.
>>>
>>> ________________________________________________________________________________
>>>
>>> For convenience, this branch is available at:
>>> https://github.com/jnsnow/qemu.git branch ide-atapi-test
>>> https://github.com/jnsnow/qemu/tree/ide-atapi-test
>>>
>>> This version is tagged ide-atapi-test-v1:
>>> https://github.com/jnsnow/qemu/releases/tag/ide-atapi-test-v1
>>>
>>> John Snow (4):
>>>   qtest/ahci: use generate_pattern everywhere
>>>   qtest/ahci: export generate_pattern
>>>   ide-test: add cdrom pio test
>>>   ide-test: add cdrom dma test
>>>
>>>  tests/ahci-test.c     |  43 +---------
>>>  tests/ide-test.c      | 232 ++++++++++++++++++++++++++++++++++++++++++++++----
>>>  tests/libqos/libqos.c |  26 ++++++
>>>  tests/libqos/libqos.h |   1 +
>>>  4 files changed, 245 insertions(+), 57 deletions(-)
>>>

Re: [Qemu-devel] [PATCH 2/4] qtest/ahci: export generate_pattern

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Share the pattern function for ide and ahci test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ahci-test.c     | 26 --------------------------
>  tests/libqos/libqos.c | 26 ++++++++++++++++++++++++++
>  tests/libqos/libqos.h |  1 +
>  3 files changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index b1a785c..59d387c 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -71,32 +71,6 @@ static void string_bswap16(uint16_t *s, size_t bytes)
>      }
>  }
>  
> -static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
> -{
> -    int i, j;
> -    unsigned char *tx = (unsigned char *)buffer;
> -    unsigned char p;
> -    size_t *sx;
> -
> -    /* Write an indicative pattern that varies and is unique per-cycle */
> -    p = rand() % 256;
> -    for (i = 0; i < len; i++) {
> -        tx[i] = p++ % 256;
> -        if (i % cycle_len == 0) {
> -            p = rand() % 256;
> -        }
> -    }
> -
> -    /* force uniqueness by writing an id per-cycle */
> -    for (i = 0; i < len / cycle_len; i++) {
> -        j = i * cycle_len;
> -        if (j + sizeof(*sx) <= len) {
> -            sx = (size_t *)&tx[j];
> -            *sx = i;
> -        }
> -    }
> -}
> -
>  /**
>   * Verify that the transfer did not corrupt our state at all.
>   */
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index fce625b..8d7c5a9 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -212,3 +212,29 @@ void prepare_blkdebug_script(const char *debug_fn, const char *event)
>      ret = fclose(debug_file);
>      g_assert(ret == 0);
>  }
> +
> +void generate_pattern(void *buffer, size_t len, size_t cycle_len)
> +{
> +    int i, j;
> +    unsigned char *tx = (unsigned char *)buffer;
> +    unsigned char p;
> +    size_t *sx;
> +
> +    /* Write an indicative pattern that varies and is unique per-cycle */
> +    p = rand() % 256;
> +    for (i = 0; i < len; i++) {
> +        tx[i] = p++ % 256;
> +        if (i % cycle_len == 0) {
> +            p = rand() % 256;
> +        }
> +    }
> +
> +    /* force uniqueness by writing an id per-cycle */
> +    for (i = 0; i < len / cycle_len; i++) {
> +        j = i * cycle_len;
> +        if (j + sizeof(*sx) <= len) {
> +            sx = (size_t *)&tx[j];

Relies on cycle_len being a multiple of size_t's alignment.  So far,
it's always AHCI_SECTOR_SIZE, which should do.

> +            *sx = i;
> +        }
> +    }
> +}

Looks like overkill to me, but since it's working code... :)

> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> index e1f14ea..492a651 100644
> --- a/tests/libqos/libqos.h
> +++ b/tests/libqos/libqos.h
> @@ -24,6 +24,7 @@ void mkqcow2(const char *file, unsigned size_mb);
>  void set_context(QOSState *s);
>  void migrate(QOSState *from, QOSState *to, const char *uri);
>  void prepare_blkdebug_script(const char *debug_fn, const char *event);
> +void generate_pattern(void *buffer, size_t len, size_t cycle_len);
>  
>  static inline uint64_t qmalloc(QOSState *q, size_t bytes)
>  {

Re: [Qemu-devel] [PATCH 3/4] ide-test: add cdrom pio test

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Add a simple read test for ATAPI devices,
> using the PIO mechanism.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ide-test.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 144 insertions(+)
>
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index 4a07e3a..90524e3 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -45,6 +45,12 @@
>  #define IDE_BASE 0x1f0
>  #define IDE_PRIMARY_IRQ 14
>  
> +#define ATAPI_BLOCK_SIZE 2048
> +
> +/* How many bytes to receive via ATAPI PIO at one time.
> + * Must be less than 0xFFFF. */
> +#define BYTE_COUNT_LIMIT 5120
> +
>  enum {
>      reg_data        = 0x0,
>      reg_nsectors    = 0x2,
> @@ -80,6 +86,7 @@ enum {
>      CMD_WRITE_DMA   = 0xca,
>      CMD_FLUSH_CACHE = 0xe7,
>      CMD_IDENTIFY    = 0xec,
> +    CMD_PACKET      = 0xa0,
>  
>      CMDF_ABORT      = 0x100,
>      CMDF_NO_BM      = 0x200,
> @@ -585,6 +592,140 @@ static void test_isa_retry_flush(const char *machine)
>      test_retry_flush("isapc");
>  }
>  
> +typedef struct Read10CDB {
> +    uint8_t opcode;
> +    uint8_t flags;
> +    uint32_t lba;
> +    uint8_t reserved;
> +    uint16_t nblocks;
> +    uint8_t control;
> +    uint16_t padding;
> +} __attribute__((__packed__)) Read10CDB;
> +
> +static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
> +{
> +    Read10CDB pkt = { .padding = 0 };
> +    int i;
> +
> +    /* Construct SCSI CDB packet */
> +    pkt.opcode = 0x28;
> +    pkt.lba = cpu_to_be32(lba);
> +    pkt.nblocks = cpu_to_be16(nblocks);
> +
> +    /* Send Packet */
> +    for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> +        outw(IDE_BASE + reg_data, ((uint16_t *)&pkt)[i]);

Requires pkt to be suitable aligned.  It is.

> +    }
> +}
> +
> +static void nsleep(int64_t nsecs)
> +{
> +    const struct timespec val = { .tv_nsec = nsecs };
> +    nanosleep(&val, NULL);
> +    clock_set(nsecs);
> +}
> +
> +static uint8_t ide_wait_clear(uint8_t flag)
> +{
> +    int i;
> +    uint8_t data;
> +
> +    /* Wait with a 5 second timeout */
> +    for (i = 0; i <= 12500000; i++) {
> +        data = inb(IDE_BASE + reg_status);
> +        if (!(data & flag)) {
> +            return data;
> +        }
> +        nsleep(400);
> +    }
> +    g_assert_not_reached();
> +    return 0xff;

Unreachable code, not needed as long as g_assert_not_reached() is
properly annotated noreturn.

> +}
> +
> +static void cdrom_pio_impl(int nblocks)
> +{
> +    FILE *fh;
> +    size_t patt_len = ATAPI_BLOCK_SIZE * MAX(16, nblocks);
> +    char *pattern = g_malloc(patt_len);
> +    size_t rxsize = ATAPI_BLOCK_SIZE * nblocks;
> +    char *rx = g_malloc0(rxsize);
> +    int i, j;
> +    uint8_t data;
> +    uint16_t limit;
> +
> +    /* Prepopulate the CDROM with an interesting pattern */
> +    generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
> +    fh = fopen(tmp_path, "w+");
> +    fwrite(pattern, ATAPI_BLOCK_SIZE, MAX(16, nblocks), fh);

I guess I would've avoided repeating MAX(16, nblocks), but it'll do.

> +    fclose(fh);
> +
> +    ide_test_start(
> +      "-drive file=%s,if=ide,media=cdrom,cache=writeback,format=raw", tmp_path);

Legacy syntax.  Okay.

> +    qtest_irq_intercept_in(global_qtest, "ioapic");
> +
> +    /* PACKET command on device 0 */
> +    outb(IDE_BASE + reg_device, 0);
> +    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> +    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> +    outb(IDE_BASE + reg_command, CMD_PACKET);

Ignorant question: why no reg_lba_low?

> +    /* HPD0: Check_Status_A State */
> +    nsleep(400);
> +    data = ide_wait_clear(BSY);

Ignorant question: why do you need to wait 400ns before you wait?

> +    /* HPD1: Send_Packet State */
> +    assert_bit_set(data, DRQ | DRDY);
> +    assert_bit_clear(data, ERR | DF | BSY);
> +
> +    /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
> +    send_scsi_cdb_read10(0, nblocks);
> +
> +    /* HPD3: INTRQ_Wait */
> +    i = 0;
> +    do {
> +        data = get_irq(IDE_PRIMARY_IRQ);
> +        nsleep(400);
> +        i++;
> +        g_assert_cmpint(i, <=, 12500000);
> +    } while (!data);

Similar to ide_wait_clear().  Why do you need to nsleep() after
get_irq() returned non-zero?

> +
> +    /* HPD2: Check_Status_B */
> +    data = ide_wait_clear(BSY);
> +    assert_bit_set(data, DRQ | DRDY);
> +    assert_bit_clear(data, ERR | DF | BSY);
> +
> +    /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
> +     * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
> +     * We allow an odd limit only when the remaining transfer size is
> +     * less than BYTE_COUNT_LIMIT.
> +     * For our purposes, we can only request even multiples, so do not
> +     * attempt to read remainders. */
> +    limit = BYTE_COUNT_LIMIT & ~1;

Does nothing, BYTE_COUNT_LIMIT is 5120.  Build-time assertion
!(BYTE_COUNT_LIMIT & 1) would do.

> +    for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
> +        size_t r = (rxsize - (limit * i)) / 2;
> +        for (j = 0; j < MIN((limit / 2), r); j++) {
> +            ((uint16_t *)rx)[(i * (limit/2)) + j] = inw(IDE_BASE + reg_data);

Would be more readable with uint16_t *rx.  Only if it doesn't require
casts elsewhere.

I guess I would've tried a single loop instead of nesting two.  But
since this works, keep it.

> +        }
> +    }
> +    data = ide_wait_clear(DRQ);
> +    assert_bit_set(data, DRDY);
> +    assert_bit_clear(data, DRQ | ERR | DF | BSY);
> +
> +    g_assert_cmpint(memcmp(pattern, rx, rxsize), ==, 0);
> +    g_free(pattern);
> +    g_free(rx);
> +    test_bmdma_teardown();
> +}
> +
> +static void test_cdrom_pio(void)
> +{
> +    cdrom_pio_impl(1);
> +}
> +
> +static void test_cdrom_pio_large(void)
> +{
> +    /* Test a few loops of the PIO DRQ mechanism. */
> +    cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -628,6 +769,9 @@ int main(int argc, char **argv)
>      qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
>      qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
>  
> +    qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
> +    qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
> +
>      ret = g_test_run();
>  
>      /* Cleanup */

Re: [Qemu-devel] [PATCH 0/4] ide: simple ATAPI tests

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/09/2015 01:00 PM, Paolo Bonzini wrote:
>> 
>> 
>> On 09/09/2015 18:37, John Snow wrote:
>>> Ping -- apologies. :)
>>>
>>> I'll take a simple ACK before I merge it into my tree.
>> 
>> I don't think you need one. :)
>> 
>> Paolo
>> 
>
> I guess technically not, but I like when somebody at least takes a peek
> before I just brazenly send pullreqs.
>
> I haven't achieved the level of ATA COWBOY yet.

I like the prudent attitude.  For tests, however, you can lower the bar
a bit.  Anyway, it looks sane to me.

Re: [Qemu-devel] [PATCH 3/4] ide-test: add cdrom pio test

[John Snow]

On 09/10/2015 05:42 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Add a simple read test for ATAPI devices,
>> using the PIO mechanism.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/ide-test.c | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 144 insertions(+)
>>
>> diff --git a/tests/ide-test.c b/tests/ide-test.c
>> index 4a07e3a..90524e3 100644
>> --- a/tests/ide-test.c
>> +++ b/tests/ide-test.c
>> @@ -45,6 +45,12 @@
>>  #define IDE_BASE 0x1f0
>>  #define IDE_PRIMARY_IRQ 14
>>  
>> +#define ATAPI_BLOCK_SIZE 2048
>> +
>> +/* How many bytes to receive via ATAPI PIO at one time.
>> + * Must be less than 0xFFFF. */
>> +#define BYTE_COUNT_LIMIT 5120
>> +
>>  enum {
>>      reg_data        = 0x0,
>>      reg_nsectors    = 0x2,
>> @@ -80,6 +86,7 @@ enum {
>>      CMD_WRITE_DMA   = 0xca,
>>      CMD_FLUSH_CACHE = 0xe7,
>>      CMD_IDENTIFY    = 0xec,
>> +    CMD_PACKET      = 0xa0,
>>  
>>      CMDF_ABORT      = 0x100,
>>      CMDF_NO_BM      = 0x200,
>> @@ -585,6 +592,140 @@ static void test_isa_retry_flush(const char *machine)
>>      test_retry_flush("isapc");
>>  }
>>  
>> +typedef struct Read10CDB {
>> +    uint8_t opcode;
>> +    uint8_t flags;
>> +    uint32_t lba;
>> +    uint8_t reserved;
>> +    uint16_t nblocks;
>> +    uint8_t control;
>> +    uint16_t padding;
>> +} __attribute__((__packed__)) Read10CDB;
>> +
>> +static void send_scsi_cdb_read10(uint32_t lba, uint16_t nblocks)
>> +{
>> +    Read10CDB pkt = { .padding = 0 };
>> +    int i;
>> +
>> +    /* Construct SCSI CDB packet */
>> +    pkt.opcode = 0x28;
>> +    pkt.lba = cpu_to_be32(lba);
>> +    pkt.nblocks = cpu_to_be16(nblocks);
>> +
>> +    /* Send Packet */
>> +    for (i = 0; i < sizeof(Read10CDB)/2; i++) {
>> +        outw(IDE_BASE + reg_data, ((uint16_t *)&pkt)[i]);
> 
> Requires pkt to be suitable aligned.  It is.
> 
>> +    }
>> +}
>> +
>> +static void nsleep(int64_t nsecs)
>> +{
>> +    const struct timespec val = { .tv_nsec = nsecs };
>> +    nanosleep(&val, NULL);
>> +    clock_set(nsecs);
>> +}
>> +
>> +static uint8_t ide_wait_clear(uint8_t flag)
>> +{
>> +    int i;
>> +    uint8_t data;
>> +
>> +    /* Wait with a 5 second timeout */
>> +    for (i = 0; i <= 12500000; i++) {
>> +        data = inb(IDE_BASE + reg_status);
>> +        if (!(data & flag)) {
>> +            return data;
>> +        }
>> +        nsleep(400);
>> +    }
>> +    g_assert_not_reached();
>> +    return 0xff;
> 
> Unreachable code, not needed as long as g_assert_not_reached() is
> properly annotated noreturn.
> 

Good point, bad habit.

>> +}
>> +
>> +static void cdrom_pio_impl(int nblocks)
>> +{
>> +    FILE *fh;
>> +    size_t patt_len = ATAPI_BLOCK_SIZE * MAX(16, nblocks);
>> +    char *pattern = g_malloc(patt_len);
>> +    size_t rxsize = ATAPI_BLOCK_SIZE * nblocks;
>> +    char *rx = g_malloc0(rxsize);
>> +    int i, j;
>> +    uint8_t data;
>> +    uint16_t limit;
>> +
>> +    /* Prepopulate the CDROM with an interesting pattern */
>> +    generate_pattern(pattern, patt_len, ATAPI_BLOCK_SIZE);
>> +    fh = fopen(tmp_path, "w+");
>> +    fwrite(pattern, ATAPI_BLOCK_SIZE, MAX(16, nblocks), fh);
> 
> I guess I would've avoided repeating MAX(16, nblocks), but it'll do.
> 
>> +    fclose(fh);
>> +
>> +    ide_test_start(
>> +      "-drive file=%s,if=ide,media=cdrom,cache=writeback,format=raw", tmp_path);
> 
> Legacy syntax.  Okay.
> 

"I expected better from you, John."

>> +    qtest_irq_intercept_in(global_qtest, "ioapic");
>> +
>> +    /* PACKET command on device 0 */
>> +    outb(IDE_BASE + reg_device, 0);
>> +    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
>> +    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
>> +    outb(IDE_BASE + reg_command, CMD_PACKET);
> 
> Ignorant question: why no reg_lba_low?
> 

It's not used by the PACKET command. Most fields aren't, but
lba_middle/lba_high (lcyl and hcyl) are re-purposed to represent a 16
bit "byte count limit" field.

>> +    /* HPD0: Check_Status_A State */
>> +    nsleep(400);
>> +    data = ide_wait_clear(BSY);
> 
> Ignorant question: why do you need to wait 400ns before you wait?
> 

Blindly following spec to a fault -- the purpose on real hardware is to
allow the drive a chance to set the BSY flag before we witness it being
reset back to zero.

QEMU of course will set and clear BSY synchronously before we ever make
it back, but I try to write these tests in a manner where they are
ignorant of QEMU's internals as much as I can, so you see some weird
timing stuff here and there.

Secretly I want to leave these tests generic enough to try and test them
with pass-through devices and real hardware someday to see how my
assertions match up against decidedly real hardware.

>> +    /* HPD1: Send_Packet State */
>> +    assert_bit_set(data, DRQ | DRDY);
>> +    assert_bit_clear(data, ERR | DF | BSY);
>> +
>> +    /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
>> +    send_scsi_cdb_read10(0, nblocks);
>> +
>> +    /* HPD3: INTRQ_Wait */
>> +    i = 0;
>> +    do {
>> +        data = get_irq(IDE_PRIMARY_IRQ);
>> +        nsleep(400);
>> +        i++;
>> +        g_assert_cmpint(i, <=, 12500000);
>> +    } while (!data);
> 
> Similar to ide_wait_clear().  Why do you need to nsleep() after
> get_irq() returned non-zero?
> 

Ran out of thinking fluid. Ugly loop.

>> +
>> +    /* HPD2: Check_Status_B */
>> +    data = ide_wait_clear(BSY);
>> +    assert_bit_set(data, DRQ | DRDY);
>> +    assert_bit_clear(data, ERR | DF | BSY);
>> +
>> +    /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
>> +     * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
>> +     * We allow an odd limit only when the remaining transfer size is
>> +     * less than BYTE_COUNT_LIMIT.
>> +     * For our purposes, we can only request even multiples, so do not
>> +     * attempt to read remainders. */
>> +    limit = BYTE_COUNT_LIMIT & ~1;
> 
> Does nothing, BYTE_COUNT_LIMIT is 5120.  Build-time assertion
> !(BYTE_COUNT_LIMIT & 1) would do.
> 

Allowing for the possibility of tests to exercise this bizarre property
of the BCL in the future, though I think there'd be more work to do in
the loop below.

I intend to expand these tests bit-by-bit over time, so this is just
some evidence of where I'm thinking on that.

>> +    for (i = 0; i < DIV_ROUND_UP(rxsize, limit); i++) {
>> +        size_t r = (rxsize - (limit * i)) / 2;
>> +        for (j = 0; j < MIN((limit / 2), r); j++) {
>> +            ((uint16_t *)rx)[(i * (limit/2)) + j] = inw(IDE_BASE + reg_data);
> 
> Would be more readable with uint16_t *rx.  Only if it doesn't require
> casts elsewhere.
> 
> I guess I would've tried a single loop instead of nesting two.  But
> since this works, keep it.
> 

It's to accommodate the DRQ blocks, and to allow myself the chance to
query the device between the distinct DRQ segments.

It could just be a flat loop and it'd work okay...

>> +        }
>> +    }
>> +    data = ide_wait_clear(DRQ);
>> +    assert_bit_set(data, DRDY);
>> +    assert_bit_clear(data, DRQ | ERR | DF | BSY);
>> +
>> +    g_assert_cmpint(memcmp(pattern, rx, rxsize), ==, 0);
>> +    g_free(pattern);
>> +    g_free(rx);
>> +    test_bmdma_teardown();
>> +}
>> +
>> +static void test_cdrom_pio(void)
>> +{
>> +    cdrom_pio_impl(1);
>> +}
>> +
>> +static void test_cdrom_pio_large(void)
>> +{
>> +    /* Test a few loops of the PIO DRQ mechanism. */
>> +    cdrom_pio_impl(BYTE_COUNT_LIMIT * 4 / ATAPI_BLOCK_SIZE);
>> +}
>> +
>>  int main(int argc, char **argv)
>>  {
>>      const char *arch = qtest_get_arch();
>> @@ -628,6 +769,9 @@ int main(int argc, char **argv)
>>      qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
>>      qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
>>  
>> +    qtest_add_func("/ide/cdrom/pio", test_cdrom_pio);
>> +    qtest_add_func("/ide/cdrom/pio_large", test_cdrom_pio_large);
>> +
>>      ret = g_test_run();
>>  
>>      /* Cleanup */

I'll polish just a pinch more. Thanks for the sanity check.

--js

Re: [Qemu-devel] [PATCH 2/4] qtest/ahci: export generate_pattern

[John Snow]

On 09/10/2015 05:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Share the pattern function for ide and ahci test.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/ahci-test.c     | 26 --------------------------
>>  tests/libqos/libqos.c | 26 ++++++++++++++++++++++++++
>>  tests/libqos/libqos.h |  1 +
>>  3 files changed, 27 insertions(+), 26 deletions(-)
>>
>> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
>> index b1a785c..59d387c 100644
>> --- a/tests/ahci-test.c
>> +++ b/tests/ahci-test.c
>> @@ -71,32 +71,6 @@ static void string_bswap16(uint16_t *s, size_t bytes)
>>      }
>>  }
>>  
>> -static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
>> -{
>> -    int i, j;
>> -    unsigned char *tx = (unsigned char *)buffer;
>> -    unsigned char p;
>> -    size_t *sx;
>> -
>> -    /* Write an indicative pattern that varies and is unique per-cycle */
>> -    p = rand() % 256;
>> -    for (i = 0; i < len; i++) {
>> -        tx[i] = p++ % 256;
>> -        if (i % cycle_len == 0) {
>> -            p = rand() % 256;
>> -        }
>> -    }
>> -
>> -    /* force uniqueness by writing an id per-cycle */
>> -    for (i = 0; i < len / cycle_len; i++) {
>> -        j = i * cycle_len;
>> -        if (j + sizeof(*sx) <= len) {
>> -            sx = (size_t *)&tx[j];
>> -            *sx = i;
>> -        }
>> -    }
>> -}
>> -
>>  /**
>>   * Verify that the transfer did not corrupt our state at all.
>>   */
>> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
>> index fce625b..8d7c5a9 100644
>> --- a/tests/libqos/libqos.c
>> +++ b/tests/libqos/libqos.c
>> @@ -212,3 +212,29 @@ void prepare_blkdebug_script(const char *debug_fn, const char *event)
>>      ret = fclose(debug_file);
>>      g_assert(ret == 0);
>>  }
>> +
>> +void generate_pattern(void *buffer, size_t len, size_t cycle_len)
>> +{
>> +    int i, j;
>> +    unsigned char *tx = (unsigned char *)buffer;
>> +    unsigned char p;
>> +    size_t *sx;
>> +
>> +    /* Write an indicative pattern that varies and is unique per-cycle */
>> +    p = rand() % 256;
>> +    for (i = 0; i < len; i++) {
>> +        tx[i] = p++ % 256;
>> +        if (i % cycle_len == 0) {
>> +            p = rand() % 256;
>> +        }
>> +    }
>> +
>> +    /* force uniqueness by writing an id per-cycle */
>> +    for (i = 0; i < len / cycle_len; i++) {
>> +        j = i * cycle_len;
>> +        if (j + sizeof(*sx) <= len) {
>> +            sx = (size_t *)&tx[j];
> 
> Relies on cycle_len being a multiple of size_t's alignment.  So far,
> it's always AHCI_SECTOR_SIZE, which should do.
> 
>> +            *sx = i;
>> +        }
>> +    }
>> +}
> 
> Looks like overkill to me, but since it's working code... :)
> 

I'll spare you the details, but I found it to be genuinely helpful to
have a pattern like this while debugging. Makes it very easy to see
which pattern came from where and so on.

The per-sector uniqueness was critical, too ...

>> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
>> index e1f14ea..492a651 100644
>> --- a/tests/libqos/libqos.h
>> +++ b/tests/libqos/libqos.h
>> @@ -24,6 +24,7 @@ void mkqcow2(const char *file, unsigned size_mb);
>>  void set_context(QOSState *s);
>>  void migrate(QOSState *from, QOSState *to, const char *uri);
>>  void prepare_blkdebug_script(const char *debug_fn, const char *event);
>> +void generate_pattern(void *buffer, size_t len, size_t cycle_len);
>>  
>>  static inline uint64_t qmalloc(QOSState *q, size_t bytes)
>>  {

-- 
—js