[Qemu-devel] [PATCH v2 0/2] ahci: test varying sector offsets

[Qemu-devel] [PATCH v2 0/2] ahci: test varying sector offsets

[John Snow]

This is a re-send of patches 7 & 8 from an earlier series,
"[PATCH v2 0/8] ahci: add more IO tests" which ultimately got bounced
back because I used some glib functions that were too new.

v2:
- Patchew caught a pathing problem with the qemu-img binary;
  the relative path produced by the Makefile does not prepend
  "./", so I was relying on the /distro's/ qemu-img by accident.
  Fix that by using realpath().

v1:
- Removed "./" from the execution CLI. Now you can set an absolute or
  relative path for QTEST_QEMU_IMG and it will work either way. The default
  as generated by the Makefile will be a relative path.

- Removed the g_spawn_check_exit_status glib call from mkimg(). See the
  in-line comments in patch 1/2 for correctness justification.

John Snow (2):
  qtest/ahci: add qcow2 support to ahci-test
  qtest/ahci: test different disk sectors

 tests/Makefile        |  1 +
 tests/ahci-test.c     | 84 +++++++++++++++++++++++++++++++++++++--------------
 tests/libqos/ahci.c   | 10 +++---
 tests/libqos/ahci.h   |  4 +--
 tests/libqos/libqos.c | 44 +++++++++++++++++++++++++++
 tests/libqos/libqos.h |  2 ++
 6 files changed, 116 insertions(+), 29 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/2] qtest/ahci: add qcow2 support to ahci-test

[John Snow]

This will enable the testing of high offsets without
wasting a lot of disk space, and does not impact the
previous tests.

mkimg and mkqcow2 are added to libqos for other tests.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile        |  1 +
 tests/ahci-test.c     | 16 ++++++----------
 tests/libqos/libqos.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/libqos.h |  2 ++
 4 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 1ef95c9..eb71778 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -413,6 +413,7 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 $(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
+		QTEST_QEMU_IMG=qemu-img$(EXESUF) \
 		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
 		gtester $(GTESTER_OPTIONS) -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
 	$(if $(CONFIG_GCOV),@for f in $(gcov-files-$*-y); do \
diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index cf0b98b..3f93c15 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -39,8 +39,8 @@
 #include "hw/pci/pci_ids.h"
 #include "hw/pci/pci_regs.h"
 
-/* Test-specific defines. */
-#define TEST_IMAGE_SIZE    (64 * 1024 * 1024)
+/* Test-specific defines -- in MiB */
+#define TEST_IMAGE_SIZE_MB (200 * 1024)
 
 /*** Globals ***/
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
@@ -81,7 +81,7 @@ static AHCIQState *ahci_boot(void)
     s = g_malloc0(sizeof(AHCIQState));
 
     cli = "-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
-        ",format=raw"
+        ",format=qcow2"
         " -M q35 "
         "-device ide-hd,drive=drive0 "
         "-global ide-hd.ver=%s";
@@ -1051,7 +1051,6 @@ static void create_ahci_io_test(enum IOMode type, enum AddrMode addr,
 int main(int argc, char **argv)
 {
     const char *arch;
-    int fd;
     int ret;
     int c;
     int i, j, k;
@@ -1088,12 +1087,9 @@ int main(int argc, char **argv)
         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);
+    /* Create a temporary qcow2 image */
+    close(mkstemp(tmp_path));
+    mkqcow2(tmp_path, TEST_IMAGE_SIZE_MB);
 
     /* Run the tests */
     qtest_add_func("/ahci/sanity",     test_sanity);
diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index bc8beb2..097091c 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -61,3 +61,47 @@ void qtest_shutdown(QOSState *qs)
     qtest_quit(qs->qts);
     g_free(qs);
 }
+
+void mkimg(const char *file, const char *fmt, unsigned size_mb)
+{
+    gchar *cli;
+    bool ret;
+    int rc;
+    GError *err = NULL;
+    char *qemu_img_path;
+    gchar *out, *out2;
+    char *abs_path;
+
+    qemu_img_path = getenv("QTEST_QEMU_IMG");
+    abs_path = realpath(qemu_img_path, NULL);
+    assert(qemu_img_path);
+
+    cli = g_strdup_printf("%s create -f %s %s %uM", abs_path,
+                          fmt, file, size_mb);
+    ret = g_spawn_command_line_sync(cli, &out, &out2, &rc, &err);
+    if (err) {
+        fprintf(stderr, "%s\n", err->message);
+        g_error_free(err);
+    }
+    g_assert(ret && !err);
+
+    /* In glib 2.34, we have g_spawn_check_exit_status. in 2.12, we don't.
+     * glib 2.43.91 implementation assumes that any non-zero is an error for
+     * windows, but uses extra precautions for Linux. However,
+     * 0 is only possible if the program exited normally, so that should be
+     * sufficient for our purposes on all platforms, here. */
+    if (rc) {
+        fprintf(stderr, "qemu-img returned status code %d\n", rc);
+    }
+    g_assert(!rc);
+
+    g_free(out);
+    g_free(out2);
+    g_free(cli);
+    free(abs_path);
+}
+
+void mkqcow2(const char *file, unsigned size_mb)
+{
+    return mkimg(file, "qcow2", size_mb);
+}
diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
index 612d41e..8cb2987 100644
--- a/tests/libqos/libqos.h
+++ b/tests/libqos/libqos.h
@@ -19,6 +19,8 @@ typedef struct QOSState {
 QOSState *qtest_vboot(QOSOps *ops, const char *cmdline_fmt, va_list ap);
 QOSState *qtest_boot(QOSOps *ops, const char *cmdline_fmt, ...);
 void qtest_shutdown(QOSState *qs);
+void mkimg(const char *file, const char *fmt, unsigned size_mb);
+void mkqcow2(const char *file, unsigned size_mb);
 
 static inline uint64_t qmalloc(QOSState *q, size_t bytes)
 {
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] qtest/ahci: test different disk sectors

[John Snow]

Test sector offset 0, 1, and the last sector(s)
in LBA28 and LBA48 modes.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c   | 68 +++++++++++++++++++++++++++++++++++++++++++----------
 tests/libqos/ahci.c | 10 ++++----
 tests/libqos/ahci.h |  4 ++--
 3 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 3f93c15..fb4739f 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -41,6 +41,8 @@
 
 /* Test-specific defines -- in MiB */
 #define TEST_IMAGE_SIZE_MB (200 * 1024)
+#define TEST_IMAGE_SECTORS ((TEST_IMAGE_SIZE_MB / AHCI_SECTOR_SIZE)     \
+                            * 1024 * 1024)
 
 /*** Globals ***/
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
@@ -712,7 +714,7 @@ static void ahci_test_identify(AHCIQState *ahci)
     ahci_port_clear(ahci, px);
 
     /* "Read" 512 bytes using CMD_IDENTIFY into the host buffer. */
-    ahci_io(ahci, px, CMD_IDENTIFY, &buff, buffsize);
+    ahci_io(ahci, px, CMD_IDENTIFY, &buff, buffsize, 0);
 
     /* Check serial number/version in the buffer */
     /* NB: IDENTIFY strings are packed in 16bit little endian chunks.
@@ -728,11 +730,12 @@ static void ahci_test_identify(AHCIQState *ahci)
     g_assert_cmphex(rc, ==, 0);
 
     sect_size = le16_to_cpu(*((uint16_t *)(&buff[5])));
-    g_assert_cmphex(sect_size, ==, 0x200);
+    g_assert_cmphex(sect_size, ==, AHCI_SECTOR_SIZE);
 }
 
 static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
-                                   uint8_t read_cmd, uint8_t write_cmd)
+                                   uint64_t sector, uint8_t read_cmd,
+                                   uint8_t write_cmd)
 {
     uint64_t ptr;
     uint8_t port;
@@ -758,9 +761,9 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
     memwrite(ptr, tx, bufsize);
 
     /* Write this buffer to disk, then read it back to the DMA buffer. */
-    ahci_guest_io(ahci, port, write_cmd, ptr, bufsize);
+    ahci_guest_io(ahci, port, write_cmd, ptr, bufsize, sector);
     qmemset(ptr, 0x00, bufsize);
-    ahci_guest_io(ahci, port, read_cmd, ptr, bufsize);
+    ahci_guest_io(ahci, port, read_cmd, ptr, bufsize, sector);
 
     /*** Read back the Data ***/
     memread(ptr, rx, bufsize);
@@ -948,12 +951,45 @@ enum IOOps {
     NUM_IO_OPS
 };
 
+enum OffsetType {
+    OFFSET_BEGIN = 0,
+    OFFSET_ZERO = OFFSET_BEGIN,
+    OFFSET_LOW,
+    OFFSET_HIGH,
+    NUM_OFFSETS
+};
+
+static const char *offset_str[NUM_OFFSETS] = { "zero", "low", "high" };
+
 typedef struct AHCIIOTestOptions {
     enum BuffLen length;
     enum AddrMode address_type;
     enum IOMode io_type;
+    enum OffsetType offset;
 } AHCIIOTestOptions;
 
+static uint64_t offset_sector(enum OffsetType ofst,
+                              enum AddrMode addr_type,
+                              uint64_t buffsize)
+{
+    uint64_t ceil;
+    uint64_t nsectors;
+
+    switch (ofst) {
+    case OFFSET_ZERO:
+        return 0;
+    case OFFSET_LOW:
+        return 1;
+    case OFFSET_HIGH:
+        ceil = (addr_type == ADDR_MODE_LBA28) ? 0xfffffff : 0xffffffffffff;
+        ceil = MIN(ceil, TEST_IMAGE_SECTORS - 1);
+        nsectors = buffsize / AHCI_SECTOR_SIZE;
+        return ceil - nsectors + 1;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 /**
  * Table of possible I/O ATA commands given a set of enumerations.
  */
@@ -981,12 +1017,12 @@ static const uint8_t io_cmds[NUM_MODES][NUM_ADDR_MODES][NUM_IO_OPS] = {
  * transfer modes, and buffer sizes.
  */
 static void test_io_rw_interface(enum AddrMode lba48, enum IOMode dma,
-                                 unsigned bufsize)
+                                 unsigned bufsize, uint64_t sector)
 {
     AHCIQState *ahci;
 
     ahci = ahci_boot_and_enable();
-    ahci_test_io_rw_simple(ahci, bufsize,
+    ahci_test_io_rw_simple(ahci, bufsize, sector,
                            io_cmds[dma][lba48][IO_READ],
                            io_cmds[dma][lba48][IO_WRITE]);
     ahci_shutdown(ahci);
@@ -999,6 +1035,7 @@ static void test_io_interface(gconstpointer opaque)
 {
     AHCIIOTestOptions *opts = (AHCIIOTestOptions *)opaque;
     unsigned bufsize;
+    uint64_t sector;
 
     switch (opts->length) {
     case LEN_SIMPLE:
@@ -1017,13 +1054,14 @@ static void test_io_interface(gconstpointer opaque)
         g_assert_not_reached();
     }
 
-    test_io_rw_interface(opts->address_type, opts->io_type, bufsize);
+    sector = offset_sector(opts->offset, opts->address_type, bufsize);
+    test_io_rw_interface(opts->address_type, opts->io_type, bufsize, sector);
     g_free(opts);
     return;
 }
 
 static void create_ahci_io_test(enum IOMode type, enum AddrMode addr,
-                                enum BuffLen len)
+                                enum BuffLen len, enum OffsetType offset)
 {
     static const char *arch;
     char *name;
@@ -1032,15 +1070,17 @@ static void create_ahci_io_test(enum IOMode type, enum AddrMode addr,
     opts->length = len;
     opts->address_type = addr;
     opts->io_type = type;
+    opts->offset = offset;
 
     if (!arch) {
         arch = qtest_get_arch();
     }
 
-    name = g_strdup_printf("/%s/ahci/io/%s/%s/%s", arch,
+    name = g_strdup_printf("/%s/ahci/io/%s/%s/%s/%s", arch,
                            io_mode_str[type],
                            addr_mode_str[addr],
-                           buff_len_str[len]);
+                           buff_len_str[len],
+                           offset_str[offset]);
 
     g_test_add_data_func(name, opts, test_io_interface);
     g_free(name);
@@ -1053,7 +1093,7 @@ int main(int argc, char **argv)
     const char *arch;
     int ret;
     int c;
-    int i, j, k;
+    int i, j, k, m;
 
     static struct option long_options[] = {
         {"pedantic", no_argument, 0, 'p' },
@@ -1102,7 +1142,9 @@ int main(int argc, char **argv)
     for (i = MODE_BEGIN; i < NUM_MODES; i++) {
         for (j = ADDR_MODE_BEGIN; j < NUM_ADDR_MODES; j++) {
             for (k = LEN_BEGIN; k < NUM_LENGTHS; k++) {
-                create_ahci_io_test(i, j, k);
+                for (m = OFFSET_BEGIN; m < NUM_OFFSETS; m++) {
+                    create_ahci_io_test(i, j, k, m);
+                }
             }
         }
     }
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index b0f39a5..a18c12b 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -568,13 +568,15 @@ inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
 
 /* Given a guest buffer address, perform an IO operation */
 void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
-                   uint64_t buffer, size_t bufsize)
+                   uint64_t buffer, size_t bufsize, uint64_t sector)
 {
     AHCICommand *cmd;
-
     cmd = ahci_command_create(ide_cmd);
     ahci_command_set_buffer(cmd, buffer);
     ahci_command_set_size(cmd, bufsize);
+    if (sector) {
+        ahci_command_set_offset(cmd, sector);
+    }
     ahci_command_commit(ahci, cmd, port);
     ahci_command_issue(ahci, cmd);
     ahci_command_verify(ahci, cmd);
@@ -612,7 +614,7 @@ static AHCICommandProp *ahci_command_find(uint8_t command_name)
 
 /* Given a HOST buffer, create a buffer address and perform an IO operation. */
 void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
-             void *buffer, size_t bufsize)
+             void *buffer, size_t bufsize, uint64_t sector)
 {
     uint64_t ptr;
     AHCICommandProp *props;
@@ -626,7 +628,7 @@ void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
         memwrite(ptr, buffer, bufsize);
     }
 
-    ahci_guest_io(ahci, port, ide_cmd, ptr, bufsize);
+    ahci_guest_io(ahci, port, ide_cmd, ptr, bufsize, sector);
 
     if (props->read) {
         memread(ptr, buffer, bufsize);
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 888545d..40e8ca4 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -523,9 +523,9 @@ void ahci_write_fis(AHCIQState *ahci, RegH2DFIS *fis, uint64_t addr);
 unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port);
 unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd);
 void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
-                   uint64_t gbuffer, size_t size);
+                   uint64_t gbuffer, size_t size, uint64_t sector);
 void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
-             void *buffer, size_t bufsize);
+             void *buffer, size_t bufsize, uint64_t sector);
 
 /* Command Lifecycle */
 AHCICommand *ahci_command_create(uint8_t command_name);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 0/2] ahci: test varying sector offsets

[John Snow]

Ping: I'll pull both this series and the 'ahci: rerror/werror=stop 
resume tests' series into my ide-next branch if just these two patches 
get a re-review.

They were excised from a pullreq due to glib compatibility issues, so 
the following series has no changes.

--js

On 03/13/2015 03:22 PM, John Snow wrote:
> This is a re-send of patches 7 & 8 from an earlier series,
> "[PATCH v2 0/8] ahci: add more IO tests" which ultimately got bounced
> back because I used some glib functions that were too new.
>
> v2:
> - Patchew caught a pathing problem with the qemu-img binary;
>    the relative path produced by the Makefile does not prepend
>    "./", so I was relying on the /distro's/ qemu-img by accident.
>    Fix that by using realpath().
>
> v1:
> - Removed "./" from the execution CLI. Now you can set an absolute or
>    relative path for QTEST_QEMU_IMG and it will work either way. The default
>    as generated by the Makefile will be a relative path.
>
> - Removed the g_spawn_check_exit_status glib call from mkimg(). See the
>    in-line comments in patch 1/2 for correctness justification.
>
> John Snow (2):
>    qtest/ahci: add qcow2 support to ahci-test
>    qtest/ahci: test different disk sectors
>
>   tests/Makefile        |  1 +
>   tests/ahci-test.c     | 84 +++++++++++++++++++++++++++++++++++++--------------
>   tests/libqos/ahci.c   | 10 +++---
>   tests/libqos/ahci.h   |  4 +--
>   tests/libqos/libqos.c | 44 +++++++++++++++++++++++++++
>   tests/libqos/libqos.h |  2 ++
>   6 files changed, 116 insertions(+), 29 deletions(-)
>

Re: [Qemu-devel] [PATCH v2 0/2] ahci: test varying sector offsets

[Stefan Hajnoczi]

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

On Fri, Mar 13, 2015 at 03:22:01PM -0400, John Snow wrote:
> This is a re-send of patches 7 & 8 from an earlier series,
> "[PATCH v2 0/8] ahci: add more IO tests" which ultimately got bounced
> back because I used some glib functions that were too new.
> 
> v2:
> - Patchew caught a pathing problem with the qemu-img binary;
>   the relative path produced by the Makefile does not prepend
>   "./", so I was relying on the /distro's/ qemu-img by accident.
>   Fix that by using realpath().
> 
> v1:
> - Removed "./" from the execution CLI. Now you can set an absolute or
>   relative path for QTEST_QEMU_IMG and it will work either way. The default
>   as generated by the Makefile will be a relative path.
> 
> - Removed the g_spawn_check_exit_status glib call from mkimg(). See the
>   in-line comments in patch 1/2 for correctness justification.
> 
> John Snow (2):
>   qtest/ahci: add qcow2 support to ahci-test
>   qtest/ahci: test different disk sectors
> 
>  tests/Makefile        |  1 +
>  tests/ahci-test.c     | 84 +++++++++++++++++++++++++++++++++++++--------------
>  tests/libqos/ahci.c   | 10 +++---
>  tests/libqos/ahci.h   |  4 +--
>  tests/libqos/libqos.c | 44 +++++++++++++++++++++++++++
>  tests/libqos/libqos.h |  2 ++
>  6 files changed, 116 insertions(+), 29 deletions(-)

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

As in traditional Linux Acked-by.  I've looked briefly at the patches
and am happy.

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

Re: [Qemu-devel] [PATCH v2 0/2] ahci: test varying sector offsets

[John Snow]

On 03/25/2015 08:55 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 13, 2015 at 03:22:01PM -0400, John Snow wrote:
>> This is a re-send of patches 7 & 8 from an earlier series,
>> "[PATCH v2 0/8] ahci: add more IO tests" which ultimately got bounced
>> back because I used some glib functions that were too new.
>>
>> v2:
>> - Patchew caught a pathing problem with the qemu-img binary;
>>    the relative path produced by the Makefile does not prepend
>>    "./", so I was relying on the /distro's/ qemu-img by accident.
>>    Fix that by using realpath().
>>
>> v1:
>> - Removed "./" from the execution CLI. Now you can set an absolute or
>>    relative path for QTEST_QEMU_IMG and it will work either way. The default
>>    as generated by the Makefile will be a relative path.
>>
>> - Removed the g_spawn_check_exit_status glib call from mkimg(). See the
>>    in-line comments in patch 1/2 for correctness justification.
>>
>> John Snow (2):
>>    qtest/ahci: add qcow2 support to ahci-test
>>    qtest/ahci: test different disk sectors
>>
>>   tests/Makefile        |  1 +
>>   tests/ahci-test.c     | 84 +++++++++++++++++++++++++++++++++++++--------------
>>   tests/libqos/ahci.c   | 10 +++---
>>   tests/libqos/ahci.h   |  4 +--
>>   tests/libqos/libqos.c | 44 +++++++++++++++++++++++++++
>>   tests/libqos/libqos.h |  2 ++
>>   6 files changed, 116 insertions(+), 29 deletions(-)
>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> As in traditional Linux Acked-by.  I've looked briefly at the patches
> and am happy.
>

Great, thanks.
Staged for 2.4.