[Qemu-devel] [PULL 00/11] Ide patches

[Qemu-devel] [PULL 00/11] Ide patches

[John Snow]

The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:

  Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:

  libqos/ahci: organize header (2016-01-08 15:22:34 -0500)

----------------------------------------------------------------

----------------------------------------------------------------

John Snow (9):
  ahci-test: fix memory leak
  libqos/ahci: ATAPI support
  libqos/ahci: ATAPI identify
  libqos/ahci: Switch to mutable properties
  libqos: allow zero-size allocations
  libqos/ahci: allow nondata commands for ahci_io variants
  libqos/ahci: add ahci_exec
  qtest/ahci: ATAPI data tests
  libqos/ahci: organize header

Mark Cave-Ayland (1):
  macio: fix overflow in lba to offset conversion for ATAPI devices

Prasad J Pandit (1):
  ide: ahci: reset ncq object to unused on error

 hw/ide/ahci.c         |   1 +
 hw/ide/macio.c        |   2 +-
 tests/ahci-test.c     | 131 ++++++++++++++++++++++++++++++------
 tests/libqos/ahci.c   | 181 +++++++++++++++++++++++++++++++++++++++++++++++---
 tests/libqos/ahci.h   |  66 +++++++++++++++---
 tests/libqos/malloc.c |   4 ++
 6 files changed, 343 insertions(+), 42 deletions(-)

-- 
2.4.3

[Qemu-devel] [PULL 01/11] macio: fix overflow in lba to offset conversion for ATAPI devices

[John Snow]

From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

As the IDEState lba field is an int32_t, make sure we cast to int64_t before
shifting to calculate the offset. Otherwise we end up with an overflow when
trying to access sectors beyond 2GB as can occur when using DVD images.

[Maintainer edit: fixed extraneous parentheses. --js]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1451928613-29476-1-git-send-email-mark.cave-ayland@ilande.co.uk
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/macio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 3ee962f..1678b2f 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -280,7 +280,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
     }
 
     /* Calculate current offset */
-    offset = (int64_t)(s->lba << 11) + s->io_buffer_index;
+    offset = ((int64_t)s->lba << 11) + s->io_buffer_index;
 
     pmac_dma_read(s->blk, offset, io->len, pmac_ide_atapi_transfer_cb, io);
     return;
-- 
2.4.3

[Qemu-devel] [PULL 02/11] ide: ahci: reset ncq object to unused on error

[John Snow]

From: Prasad J Pandit <pjp@fedoraproject.org>

When processing NCQ commands, ACHI device emulation prepares a
NCQ transfer object; To which an aio control block(aiocb) object
is assigned in 'execute_ncq_command'. In case, when the NCQ
command is invalid, the 'aiocb' object is not assigned, and NCQ
transfer object is left as 'used'. This leads to a use after
free kind of error in 'bdrv_aio_cancel_async' via 'ahci_reset_port'.
Reset NCQ transfer object to 'unused' to avoid it.

Reported-by: Qinghao Tang <luodalongde@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1452282511-4116-1-git-send-email-ppandit@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index dd1912e..17f1cbd 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -910,6 +910,7 @@ static void ncq_err(NCQTransferState *ncq_tfs)
     ide_state->error = ABRT_ERR;
     ide_state->status = READY_STAT | ERR_STAT;
     ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag);
+    ncq_tfs->used = 0;
 }
 
 static void ncq_finish(NCQTransferState *ncq_tfs)
-- 
2.4.3

[Qemu-devel] [PULL 03/11] ahci-test: fix memory leak

[John Snow]

Use the proper free command to detroy an AHCICommand.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-2-git-send-email-jsnow@redhat.com
---
 tests/ahci-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 0888506..f4945dc 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1045,14 +1045,14 @@ static void test_dma_fragmented(void)
     ahci_command_commit(ahci, cmd, px);
     ahci_command_issue(ahci, cmd);
     ahci_command_verify(ahci, cmd);
-    g_free(cmd);
+    ahci_command_free(cmd);
 
     cmd = ahci_command_create(CMD_READ_DMA);
     ahci_command_adjust(cmd, 0, ptr, bufsize, 32);
     ahci_command_commit(ahci, cmd, px);
     ahci_command_issue(ahci, cmd);
     ahci_command_verify(ahci, cmd);
-    g_free(cmd);
+    ahci_command_free(cmd);
 
     /* Read back the guest's receive buffer into local memory */
     bufread(ptr, rx, bufsize);
-- 
2.4.3

[Qemu-devel] [PULL 04/11] libqos/ahci: ATAPI support

[John Snow]

Add pathways to tolerate ATAPI commands.

Notably, unlike ATA, each SCSI command's layout is a little different,
so support will have to be patched in for each command as we want to
test them in e.g. ahci_command_set_sizes and ahci_command_set_offset.

For now, I'm adding support for 0x28, READ (10).

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-3-git-send-email-jsnow@redhat.com
---
 tests/libqos/ahci.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 tests/libqos/ahci.h | 14 +++++++++
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index adb2665..59bf893 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -74,7 +74,11 @@ AHCICommandProp ahci_command_properties[] = {
                                  .lba48 = true, .write = true, .ncq = true },
     { .cmd = CMD_READ_MAX,       .lba28 = true },
     { .cmd = CMD_READ_MAX_EXT,   .lba48 = true },
-    { .cmd = CMD_FLUSH_CACHE,    .data = false }
+    { .cmd = CMD_FLUSH_CACHE,    .data = false },
+    { .cmd = CMD_PACKET,         .data = true,  .size = 16,
+                                 .atapi = true, },
+    { .cmd = CMD_PACKET_ID,      .data = true,  .pio = true,
+                                 .size = 512,   .read = true }
 };
 
 struct AHCICommand {
@@ -90,7 +94,7 @@ struct AHCICommand {
     /* Data to be transferred to the guest */
     AHCICommandHeader header;
     RegH2DFIS fis;
-    void *atapi_cmd;
+    unsigned char *atapi_cmd;
 };
 
 /**
@@ -731,6 +735,13 @@ static void command_table_init(AHCICommand *cmd)
     memset(fis->aux, 0x00, ARRAY_SIZE(fis->aux));
 }
 
+void ahci_command_enable_atapi_dma(AHCICommand *cmd)
+{
+    RegH2DFIS *fis = &(cmd->fis);
+    g_assert(cmd->props->atapi);
+    fis->feature_low |= 0x01;
+}
+
 AHCICommand *ahci_command_create(uint8_t command_name)
 {
     AHCICommandProp *props = ahci_command_find(command_name);
@@ -767,8 +778,22 @@ AHCICommand *ahci_command_create(uint8_t command_name)
     return cmd;
 }
 
+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
+{
+    AHCICommand *cmd = ahci_command_create(CMD_PACKET);
+    cmd->atapi_cmd = g_malloc0(16);
+    cmd->atapi_cmd[0] = scsi_cmd;
+    /* ATAPI needs a PIO transfer chunk size set inside of the LBA registers.
+     * The block/sector size is a natural default. */
+    cmd->fis.lba_lo[1] = ATAPI_SECTOR_SIZE >> 8 & 0xFF;
+    cmd->fis.lba_lo[2] = ATAPI_SECTOR_SIZE & 0xFF;
+
+    return cmd;
+}
+
 void ahci_command_free(AHCICommand *cmd)
 {
+    g_free(cmd->atapi_cmd);
     g_free(cmd);
 }
 
@@ -782,10 +807,33 @@ void ahci_command_clr_flags(AHCICommand *cmd, uint16_t cmdh_flags)
     cmd->header.flags &= ~cmdh_flags;
 }
 
+static void ahci_atapi_command_set_offset(AHCICommand *cmd, uint64_t lba)
+{
+    unsigned char *cbd = cmd->atapi_cmd;
+    uint32_t *lba32;
+    g_assert(cbd);
+
+    switch (cbd[0]) {
+    case CMD_ATAPI_READ_10:
+        g_assert_cmpuint(lba, <=, UINT32_MAX);
+        lba32 = (uint32_t *)&(cbd[2]);
+        *lba32 = cpu_to_be32(lba);
+        break;
+    default:
+        /* SCSI doesn't have uniform packet formats,
+         * so you have to add support for it manually. Sorry! */
+        g_assert_not_reached();
+    }
+}
+
 void ahci_command_set_offset(AHCICommand *cmd, uint64_t lba_sect)
 {
     RegH2DFIS *fis = &(cmd->fis);
-    if (cmd->props->lba28) {
+
+    if (cmd->props->atapi) {
+        ahci_atapi_command_set_offset(cmd, lba_sect);
+        return;
+    } else if (cmd->props->lba28) {
         g_assert_cmphex(lba_sect, <=, 0xFFFFFFF);
     } else if (cmd->props->lba48 || cmd->props->ncq) {
         g_assert_cmphex(lba_sect, <=, 0xFFFFFFFFFFFF);
@@ -811,6 +859,26 @@ void ahci_command_set_buffer(AHCICommand *cmd, uint64_t buffer)
     cmd->buffer = buffer;
 }
 
+static void ahci_atapi_set_size(AHCICommand *cmd, uint64_t xbytes)
+{
+    unsigned char *cbd = cmd->atapi_cmd;
+    uint64_t nsectors = xbytes / 2048;
+    uint16_t *nsector16;
+    g_assert(cbd);
+
+    switch (cbd[0]) {
+    case CMD_ATAPI_READ_10:
+        g_assert_cmpuint(nsectors, <=, UINT16_MAX);
+        nsector16 = (uint16_t *)&(cbd[7]);
+        *nsector16 = cpu_to_be16(nsectors);
+        break;
+    default:
+        /* SCSI doesn't have uniform packet formats,
+         * so you have to add support for it manually. Sorry! */
+        g_assert_not_reached();
+    }
+}
+
 void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
                             unsigned prd_size)
 {
@@ -829,6 +897,8 @@ void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
         NCQFIS *nfis = (NCQFIS *)&(cmd->fis);
         nfis->sector_low = sect_count & 0xFF;
         nfis->sector_hi = (sect_count >> 8) & 0xFF;
+    } else if (cmd->props->atapi) {
+        ahci_atapi_set_size(cmd, xbytes);
     } else {
         cmd->fis.count = sect_count;
     }
@@ -877,9 +947,14 @@ void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port)
     g_assert((table_ptr & 0x7F) == 0x00);
     cmd->header.ctba = table_ptr;
 
-    /* Commit the command header and command FIS */
+    /* Commit the command header (part of the Command List Buffer) */
     ahci_set_command_header(ahci, port, cmd->slot, &(cmd->header));
+    /* Now, write the command table (FIS, ACMD, and PRDT) -- FIS first, */
     ahci_write_fis(ahci, cmd);
+    /* Then ATAPI CMD, if needed */
+    if (cmd->props->atapi) {
+        memwrite(table_ptr + 0x40, cmd->atapi_cmd, 16);
+    }
 
     /* Construct and write the PRDs to the command table */
     g_assert_cmphex(prdtl, ==, cmd->header.prdtl);
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index cffc2c3..9ffd415 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -244,6 +244,10 @@
 #define AHCI_VERSION_1_3         (0x00010300)
 
 #define AHCI_SECTOR_SIZE                (512)
+#define ATAPI_SECTOR_SIZE              (2048)
+
+#define AHCI_SIGNATURE_CDROM     (0xeb140101)
+#define AHCI_SIGNATURE_DISK      (0x00000101)
 
 /* FIS types */
 enum {
@@ -277,11 +281,18 @@ enum {
     CMD_READ_MAX_EXT   = 0x27,
     CMD_FLUSH_CACHE    = 0xE7,
     CMD_IDENTIFY       = 0xEC,
+    CMD_PACKET         = 0xA0,
+    CMD_PACKET_ID      = 0xA1,
     /* NCQ */
     READ_FPDMA_QUEUED  = 0x60,
     WRITE_FPDMA_QUEUED = 0x61,
 };
 
+/* ATAPI Commands */
+enum {
+    CMD_ATAPI_READ_10 = 0x28,
+};
+
 /* AHCI Command Header Flags & Masks*/
 #define CMDH_CFL        (0x1F)
 #define CMDH_ATAPI      (0x20)
@@ -561,6 +572,7 @@ void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
 
 /* Command Lifecycle */
 AHCICommand *ahci_command_create(uint8_t command_name);
+AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd);
 void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port);
 void ahci_command_issue(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_command_issue_async(AHCIQState *ahci, AHCICommand *cmd);
@@ -577,6 +589,8 @@ void ahci_command_set_size(AHCICommand *cmd, uint64_t xbytes);
 void ahci_command_set_prd_size(AHCICommand *cmd, unsigned prd_size);
 void ahci_command_set_sizes(AHCICommand *cmd, uint64_t xbytes,
                             unsigned prd_size);
+void ahci_command_set_acmd(AHCICommand *cmd, void *acmd);
+void ahci_command_enable_atapi_dma(AHCICommand *cmd);
 void ahci_command_adjust(AHCICommand *cmd, uint64_t lba_sect, uint64_t gbuffer,
                          uint64_t xbytes, unsigned prd_size);
 
-- 
2.4.3

[Qemu-devel] [PULL 05/11] libqos/ahci: ATAPI identify

[John Snow]

We need to say "hello!" to our ATAPI friends
in a slightly different manner.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-4-git-send-email-jsnow@redhat.com
---
 tests/ahci-test.c   | 8 +++++++-
 tests/libqos/ahci.c | 5 +++++
 tests/libqos/ahci.h | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index f4945dc..8ebbd33 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -215,6 +215,7 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...)
     va_list ap;
     uint16_t buff[256];
     uint8_t port;
+    uint8_t hello;
 
     if (cli) {
         va_start(ap, cli);
@@ -229,7 +230,12 @@ static AHCIQState *ahci_boot_and_enable(const char *cli, ...)
     /* Initialize test device */
     port = ahci_port_select(ahci);
     ahci_port_clear(ahci, port);
-    ahci_io(ahci, port, CMD_IDENTIFY, &buff, sizeof(buff), 0);
+    if (is_atapi(ahci, port)) {
+        hello = CMD_PACKET_ID;
+    } else {
+        hello = CMD_IDENTIFY;
+    }
+    ahci_io(ahci, port, hello, &buff, sizeof(buff), 0);
 
     return ahci;
 }
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 59bf893..81edf34 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -114,6 +114,11 @@ void ahci_free(AHCIQState *ahci, uint64_t addr)
     qfree(ahci->parent, addr);
 }
 
+bool is_atapi(AHCIQState *ahci, uint8_t port)
+{
+    return ahci_px_rreg(ahci, port, AHCI_PX_SIG) == AHCI_SIGNATURE_CDROM;
+}
+
 /**
  * Locate, verify, and return a handle to the AHCI device.
  */
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 9ffd415..705fbd6 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -596,5 +596,6 @@ void ahci_command_adjust(AHCICommand *cmd, uint64_t lba_sect, uint64_t gbuffer,
 
 /* Command Misc */
 uint8_t ahci_command_slot(AHCICommand *cmd);
+bool is_atapi(AHCIQState *ahci, uint8_t port);
 
 #endif
-- 
2.4.3

[Qemu-devel] [PULL 06/11] libqos/ahci: Switch to mutable properties

[John Snow]

ATAPI commands are, unfortunately, weird in that they can
be either DMA or PIO depending on a header bit. In order to
accommodate them, I'll need to make AHCI command properties
mutable so we can toggle between which "flavor" of ATAPI command
we want to test.

The default ATAPI transfer mechanism is PIO and the default
properties are adjusted accordingly.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-5-git-send-email-jsnow@redhat.com
---
 tests/libqos/ahci.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 81edf34..a219f67 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -76,7 +76,7 @@ AHCICommandProp ahci_command_properties[] = {
     { .cmd = CMD_READ_MAX_EXT,   .lba48 = true },
     { .cmd = CMD_FLUSH_CACHE,    .data = false },
     { .cmd = CMD_PACKET,         .data = true,  .size = 16,
-                                 .atapi = true, },
+                                 .atapi = true, .pio = true },
     { .cmd = CMD_PACKET_ID,      .data = true,  .pio = true,
                                  .size = 512,   .read = true }
 };
@@ -745,6 +745,11 @@ void ahci_command_enable_atapi_dma(AHCICommand *cmd)
     RegH2DFIS *fis = &(cmd->fis);
     g_assert(cmd->props->atapi);
     fis->feature_low |= 0x01;
+    cmd->interrupts &= ~AHCI_PX_IS_PSS;
+    cmd->props->dma = true;
+    cmd->props->pio = false;
+    /* BUG: We expect the DMA Setup interrupt for DMA commands */
+    /* cmd->interrupts |= AHCI_PX_IS_DSS; */
 }
 
 AHCICommand *ahci_command_create(uint8_t command_name)
@@ -761,7 +766,7 @@ AHCICommand *ahci_command_create(uint8_t command_name)
     g_assert(!props->ncq || props->lba48);
 
     /* Defaults and book-keeping */
-    cmd->props = props;
+    cmd->props = g_memdup(props, sizeof(AHCICommandProp));
     cmd->name = command_name;
     cmd->xbytes = props->size;
     cmd->prd_size = 4096;
@@ -799,6 +804,7 @@ AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd)
 void ahci_command_free(AHCICommand *cmd)
 {
     g_free(cmd->atapi_cmd);
+    g_free(cmd->props);
     g_free(cmd);
 }
 
-- 
2.4.3

[Qemu-devel] [PULL 07/11] libqos: allow zero-size allocations

[John Snow]

As part of streamlining the AHCI tests interface, it'd be nice
if specying a size of zero could be handled without special branches
and the allocator could handle this special case gracefully.

This lets me use the "ahci_io" macros for non-data commands, too,
which moves me forward towards shepherding all AHCI qtests into
a common set of commands in a unified pipeline.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-6-git-send-email-jsnow@redhat.com
---
 tests/ahci-test.c     | 8 +-------
 tests/libqos/ahci.c   | 6 +++---
 tests/libqos/malloc.c | 4 ++++
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 8ebbd33..8c48587 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -890,18 +890,12 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
 static uint8_t ahci_test_nondata(AHCIQState *ahci, uint8_t ide_cmd)
 {
     uint8_t port;
-    AHCICommand *cmd;
 
     /* Sanitize */
     port = ahci_port_select(ahci);
     ahci_port_clear(ahci, port);
 
-    /* Issue Command */
-    cmd = ahci_command_create(ide_cmd);
-    ahci_command_commit(ahci, cmd, port);
-    ahci_command_issue(ahci, cmd);
-    ahci_command_verify(ahci, cmd);
-    ahci_command_free(cmd);
+    ahci_io(ahci, port, ide_cmd, NULL, 0, 0);
 
     return port;
 }
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index a219f67..df29560 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -668,16 +668,16 @@ void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
     props = ahci_command_find(ide_cmd);
     g_assert(props);
     ptr = ahci_alloc(ahci, bufsize);
-    g_assert(ptr);
+    g_assert(!bufsize || ptr);
     qmemset(ptr, 0x00, bufsize);
 
-    if (props->write) {
+    if (bufsize && props->write) {
         bufwrite(ptr, buffer, bufsize);
     }
 
     ahci_guest_io(ahci, port, ide_cmd, ptr, bufsize, sector);
 
-    if (props->read) {
+    if (bufsize && props->read) {
         bufread(ptr, buffer, bufsize);
     }
 
diff --git a/tests/libqos/malloc.c b/tests/libqos/malloc.c
index 82b9df5..19d05ca 100644
--- a/tests/libqos/malloc.c
+++ b/tests/libqos/malloc.c
@@ -270,6 +270,10 @@ uint64_t guest_alloc(QGuestAllocator *allocator, size_t size)
     uint64_t rsize = size;
     uint64_t naddr;
 
+    if (!size) {
+        return 0;
+    }
+
     rsize += (allocator->page_size - 1);
     rsize &= -allocator->page_size;
     g_assert_cmpint((allocator->start + rsize), <=, allocator->end);
-- 
2.4.3

[Qemu-devel] [PULL 08/11] libqos/ahci: allow nondata commands for ahci_io variants

[John Snow]

These variants try to set a data offset, even if you don't specify one.
In the cases where the offset is zero and it's a nondata command, just
ignore the instruction.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-7-git-send-email-jsnow@redhat.com
---
 tests/ahci-test.c   | 14 ++------------
 tests/libqos/ahci.c |  3 +++
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 8c48587..2bee2a2 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1080,7 +1080,6 @@ static void test_flush_retry(void)
     AHCIQState *ahci;
     AHCICommand *cmd;
     uint8_t port;
-    const char *s;
 
     prepare_blkdebug_script(debug_path, "flush_to_disk");
     ahci = ahci_boot_and_enable("-drive file=blkdebug:%s:%s,if=none,id=drive0,"
@@ -1094,19 +1093,10 @@ static void test_flush_retry(void)
     /* Issue Flush Command and wait for error */
     port = ahci_port_select(ahci);
     ahci_port_clear(ahci, port);
-    cmd = ahci_command_create(CMD_FLUSH_CACHE);
-    ahci_command_commit(ahci, cmd, port);
-    ahci_command_issue_async(ahci, cmd);
-    qmp_eventwait("STOP");
 
-    /* Complete the command */
-    s = "{'execute':'cont' }";
-    qmp_async(s);
-    qmp_eventwait("RESUME");
-    ahci_command_wait(ahci, cmd);
-    ahci_command_verify(ahci, cmd);
+    cmd = ahci_guest_io_halt(ahci, port, CMD_FLUSH_CACHE, 0, 0, 0);
+    ahci_guest_io_resume(ahci, cmd);
 
-    ahci_command_free(cmd);
     ahci_shutdown(ahci);
 }
 
diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index df29560..0fa9bf2 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -844,6 +844,9 @@ void ahci_command_set_offset(AHCICommand *cmd, uint64_t lba_sect)
     if (cmd->props->atapi) {
         ahci_atapi_command_set_offset(cmd, lba_sect);
         return;
+    } else if (!cmd->props->data && !lba_sect) {
+        /* Not meaningful, ignore. */
+        return;
     } else if (cmd->props->lba28) {
         g_assert_cmphex(lba_sect, <=, 0xFFFFFFF);
     } else if (cmd->props->lba48 || cmd->props->ncq) {
-- 
2.4.3

[Qemu-devel] [PULL 09/11] libqos/ahci: add ahci_exec

[John Snow]

add ahci_exec, which is a standard purpose flexible command dispatcher
and tester for the AHCI device. The intent is to eventually cut down on
the absurd amount of boilerplate inside of the AHCI qtest.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-8-git-send-email-jsnow@redhat.com
---
 tests/libqos/ahci.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/libqos/ahci.h | 17 ++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 0fa9bf2..6d1298b 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -601,6 +601,82 @@ inline unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd)
     return (bytes + bytes_per_prd - 1) / bytes_per_prd;
 }
 
+const AHCIOpts default_opts = { .size = 0 };
+
+/**
+ * ahci_exec: execute a given command on a specific
+ * AHCI port.
+ *
+ * @ahci: The device to send the command to
+ * @port: The port number of the SATA device we wish
+ *        to have execute this command
+ * @op:   The S/ATA command to execute, or if opts.atapi
+ *        is true, the SCSI command code.
+ * @opts: Optional arguments to modify execution behavior.
+ */
+void ahci_exec(AHCIQState *ahci, uint8_t port,
+               uint8_t op, const AHCIOpts *opts_in)
+{
+    AHCICommand *cmd;
+    int rc;
+    AHCIOpts *opts;
+
+    opts = g_memdup((opts_in == NULL ? &default_opts : opts_in),
+                    sizeof(AHCIOpts));
+
+    /* No guest buffer provided, create one. */
+    if (opts->size && !opts->buffer) {
+        opts->buffer = ahci_alloc(ahci, opts->size);
+        g_assert(opts->buffer);
+        qmemset(opts->buffer, 0x00, opts->size);
+    }
+
+    /* Command creation */
+    if (opts->atapi) {
+        cmd = ahci_atapi_command_create(op);
+        if (opts->atapi_dma) {
+            ahci_command_enable_atapi_dma(cmd);
+        }
+    } else {
+        cmd = ahci_command_create(op);
+    }
+    ahci_command_adjust(cmd, opts->lba, opts->buffer,
+                        opts->size, opts->prd_size);
+
+    if (opts->pre_cb) {
+        rc = opts->pre_cb(ahci, cmd, opts);
+        g_assert_cmpint(rc, ==, 0);
+    }
+
+    /* Write command to memory and issue it */
+    ahci_command_commit(ahci, cmd, port);
+    ahci_command_issue_async(ahci, cmd);
+    if (opts->error) {
+        qmp_eventwait("STOP");
+    }
+    if (opts->mid_cb) {
+        rc = opts->mid_cb(ahci, cmd, opts);
+        g_assert_cmpint(rc, ==, 0);
+    }
+    if (opts->error) {
+        qmp_async("{'execute':'cont' }");
+        qmp_eventwait("RESUME");
+    }
+
+    /* Wait for command to complete and verify sanity */
+    ahci_command_wait(ahci, cmd);
+    ahci_command_verify(ahci, cmd);
+    if (opts->post_cb) {
+        rc = opts->post_cb(ahci, cmd, opts);
+        g_assert_cmpint(rc, ==, 0);
+    }
+    ahci_command_free(cmd);
+    if (opts->buffer != opts_in->buffer) {
+        ahci_free(ahci, opts->buffer);
+    }
+    g_free(opts);
+}
+
 /* Issue a command, expecting it to fail and STOP the VM */
 AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t port,
                                 uint8_t ide_cmd, uint64_t buffer,
diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 705fbd6..2c2d2fc 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -462,6 +462,21 @@ typedef struct PRD {
 /* Opaque, defined within ahci.c */
 typedef struct AHCICommand AHCICommand;
 
+/* Options to ahci_exec */
+typedef struct AHCIOpts {
+    size_t size;
+    unsigned prd_size;
+    uint64_t lba;
+    uint64_t buffer;
+    bool atapi;
+    bool atapi_dma;
+    bool error;
+    int (*pre_cb)(AHCIQState*, AHCICommand*, const struct AHCIOpts *);
+    int (*mid_cb)(AHCIQState*, AHCICommand*, const struct AHCIOpts *);
+    int (*post_cb)(AHCIQState*, AHCICommand*, const struct AHCIOpts *);
+    void *opaque;
+} AHCIOpts;
+
 /*** Macro Utilities ***/
 #define BITANY(data, mask) (((data) & (mask)) != 0)
 #define BITSET(data, mask) (((data) & (mask)) == (mask))
@@ -569,6 +584,8 @@ AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
 void ahci_guest_io_resume(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
              void *buffer, size_t bufsize, uint64_t sector);
+void ahci_exec(AHCIQState *ahci, uint8_t port,
+               uint8_t op, const AHCIOpts *opts);
 
 /* Command Lifecycle */
 AHCICommand *ahci_command_create(uint8_t command_name);
-- 
2.4.3

[Qemu-devel] [PULL 10/11] qtest/ahci: ATAPI data tests

[John Snow]

Simple I/O tests for DMA and PIO pathways in the AHCI HBA.

I believe at this point in time all of the common, major IO pathways
in BMDMA and AHCI are covered by qtests now.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-9-git-send-email-jsnow@redhat.com
---
 tests/ahci-test.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 2bee2a2..31fb1f9 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1413,6 +1413,98 @@ static void test_ncq_simple(void)
     ahci_shutdown(ahci);
 }
 
+static int prepare_iso(size_t size, unsigned char **buf, char **name)
+{
+    char cdrom_path[] = "/tmp/qtest.iso.XXXXXX";
+    unsigned char *patt;
+    ssize_t ret;
+    int fd = mkstemp(cdrom_path);
+
+    g_assert(buf);
+    g_assert(name);
+    patt = g_malloc(size);
+
+    /* Generate a pattern and build a CDROM image to read from */
+    generate_pattern(patt, size, ATAPI_SECTOR_SIZE);
+    ret = write(fd, patt, size);
+    g_assert(ret == size);
+
+    *name = g_strdup(cdrom_path);
+    *buf = patt;
+    return fd;
+}
+
+static void remove_iso(int fd, char *name)
+{
+    unlink(name);
+    g_free(name);
+    close(fd);
+}
+
+static int ahci_cb_cmp_buff(AHCIQState *ahci, AHCICommand *cmd,
+                            const AHCIOpts *opts)
+{
+    unsigned char *tx = opts->opaque;
+    unsigned char *rx = g_malloc0(opts->size);
+
+    bufread(opts->buffer, rx, opts->size);
+    g_assert_cmphex(memcmp(tx, rx, opts->size), ==, 0);
+    g_free(rx);
+
+    return 0;
+}
+
+static void ahci_test_cdrom(int nsectors, bool dma)
+{
+    AHCIQState *ahci;
+    unsigned char *tx;
+    char *iso;
+    int fd;
+    AHCIOpts opts = {
+        .size = (ATAPI_SECTOR_SIZE * nsectors),
+        .atapi = true,
+        .atapi_dma = dma,
+        .post_cb = ahci_cb_cmp_buff,
+    };
+
+    /* Prepare ISO and fill 'tx' buffer */
+    fd = prepare_iso(1024 * 1024, &tx, &iso);
+    opts.opaque = tx;
+
+    /* Standard startup wonkery, but use ide-cd and our special iso file */
+    ahci = ahci_boot_and_enable("-drive if=none,id=drive0,file=%s,format=raw "
+                                "-M q35 "
+                                "-device ide-cd,drive=drive0 ", iso);
+
+    /* Build & Send AHCI command */
+    ahci_exec(ahci, ahci_port_select(ahci), CMD_ATAPI_READ_10, &opts);
+
+    /* Cleanup */
+    g_free(tx);
+    ahci_shutdown(ahci);
+    remove_iso(fd, iso);
+}
+
+static void test_cdrom_dma(void)
+{
+    ahci_test_cdrom(1, true);
+}
+
+static void test_cdrom_dma_multi(void)
+{
+    ahci_test_cdrom(3, true);
+}
+
+static void test_cdrom_pio(void)
+{
+    ahci_test_cdrom(1, false);
+}
+
+static void test_cdrom_pio_multi(void)
+{
+    ahci_test_cdrom(3, false);
+}
+
 /******************************************************************************/
 /* AHCI I/O Test Matrix Definitions                                           */
 
@@ -1697,6 +1789,11 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/io/ncq/retry", test_halted_ncq);
     qtest_add_func("/ahci/migrate/ncq/halted", test_migrate_halted_ncq);
 
+    qtest_add_func("/ahci/cdrom/dma/single", test_cdrom_dma);
+    qtest_add_func("/ahci/cdrom/dma/multi", test_cdrom_dma_multi);
+    qtest_add_func("/ahci/cdrom/pio/single", test_cdrom_pio);
+    qtest_add_func("/ahci/cdrom/pio/multi", test_cdrom_pio_multi);
+
     ret = g_test_run();
 
     /* Cleanup */
-- 
2.4.3

[Qemu-devel] [PULL 11/11] libqos/ahci: organize header

[John Snow]

Organize the prototypes into nice little sections.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1452282920-21550-10-git-send-email-jsnow@redhat.com
---
 tests/libqos/ahci.h | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h
index 2c2d2fc..69dc4d7 100644
--- a/tests/libqos/ahci.h
+++ b/tests/libqos/ahci.h
@@ -553,14 +553,28 @@ static inline void ahci_px_clr(AHCIQState *ahci, uint8_t port,
 /*** Prototypes ***/
 uint64_t ahci_alloc(AHCIQState *ahci, size_t bytes);
 void ahci_free(AHCIQState *ahci, uint64_t addr);
+void ahci_clean_mem(AHCIQState *ahci);
+
+/* Device management */
 QPCIDevice *get_ahci_device(uint32_t *fingerprint);
 void free_ahci_device(QPCIDevice *dev);
-void ahci_clean_mem(AHCIQState *ahci);
 void ahci_pci_enable(AHCIQState *ahci);
 void start_ahci_device(AHCIQState *ahci);
 void ahci_hba_enable(AHCIQState *ahci);
+
+/* Port Management */
 unsigned ahci_port_select(AHCIQState *ahci);
 void ahci_port_clear(AHCIQState *ahci, uint8_t port);
+
+/* Command header / table management */
+unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port);
+void ahci_get_command_header(AHCIQState *ahci, uint8_t port,
+                             uint8_t slot, AHCICommandHeader *cmd);
+void ahci_set_command_header(AHCIQState *ahci, uint8_t port,
+                             uint8_t slot, AHCICommandHeader *cmd);
+void ahci_destroy_command(AHCIQState *ahci, uint8_t port, uint8_t slot);
+
+/* AHCI sanity check routines */
 void ahci_port_check_error(AHCIQState *ahci, uint8_t port);
 void ahci_port_check_interrupts(AHCIQState *ahci, uint8_t port,
                                 uint32_t intr_mask);
@@ -569,14 +583,12 @@ void ahci_port_check_d2h_sanity(AHCIQState *ahci, uint8_t port, uint8_t slot);
 void ahci_port_check_pio_sanity(AHCIQState *ahci, uint8_t port,
                                 uint8_t slot, size_t buffsize);
 void ahci_port_check_cmd_sanity(AHCIQState *ahci, AHCICommand *cmd);
-void ahci_get_command_header(AHCIQState *ahci, uint8_t port,
-                             uint8_t slot, AHCICommandHeader *cmd);
-void ahci_set_command_header(AHCIQState *ahci, uint8_t port,
-                             uint8_t slot, AHCICommandHeader *cmd);
-void ahci_destroy_command(AHCIQState *ahci, uint8_t port, uint8_t slot);
-void ahci_write_fis(AHCIQState *ahci, AHCICommand *cmd);
-unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port);
+
+/* Misc */
+bool is_atapi(AHCIQState *ahci, uint8_t port);
 unsigned size_to_prdtl(unsigned bytes, unsigned bytes_per_prd);
+
+/* Command: Macro level execution */
 void ahci_guest_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
                    uint64_t gbuffer, size_t size, uint64_t sector);
 AHCICommand *ahci_guest_io_halt(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
@@ -587,7 +599,7 @@ void ahci_io(AHCIQState *ahci, uint8_t port, uint8_t ide_cmd,
 void ahci_exec(AHCIQState *ahci, uint8_t port,
                uint8_t op, const AHCIOpts *opts);
 
-/* Command Lifecycle */
+/* Command: Fine-grained lifecycle */
 AHCICommand *ahci_command_create(uint8_t command_name);
 AHCICommand *ahci_atapi_command_create(uint8_t scsi_cmd);
 void ahci_command_commit(AHCIQState *ahci, AHCICommand *cmd, uint8_t port);
@@ -597,7 +609,7 @@ void ahci_command_wait(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_command_verify(AHCIQState *ahci, AHCICommand *cmd);
 void ahci_command_free(AHCICommand *cmd);
 
-/* Command adjustments */
+/* Command: adjustments */
 void ahci_command_set_flags(AHCICommand *cmd, uint16_t cmdh_flags);
 void ahci_command_clr_flags(AHCICommand *cmd, uint16_t cmdh_flags);
 void ahci_command_set_offset(AHCICommand *cmd, uint64_t lba_sect);
@@ -611,8 +623,8 @@ void ahci_command_enable_atapi_dma(AHCICommand *cmd);
 void ahci_command_adjust(AHCICommand *cmd, uint64_t lba_sect, uint64_t gbuffer,
                          uint64_t xbytes, unsigned prd_size);
 
-/* Command Misc */
+/* Command: Misc */
 uint8_t ahci_command_slot(AHCICommand *cmd);
-bool is_atapi(AHCIQState *ahci, uint8_t port);
+void ahci_write_fis(AHCIQState *ahci, AHCICommand *cmd);
 
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PULL 00/11] Ide patches

[Peter Maydell]

On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:
>
>   Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:
>
>   libqos/ahci: organize header (2016-01-08 15:22:34 -0500)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

These seem to result in some new clang sanitizer runtime warnings
during a 'make check':

/home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:963:9:
runtime error: store to misaligned address 0x2adacfbaacd7 for type
'uint16_t' (aka 'unsigned short'), which requires 2 byte alignment
0x2adacfbaacd7: note: pointer points here
 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  21
00 00 00 00 00 00 00  6c 6f 6e
             ^
/home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:907:9:
runtime error: store to misaligned address 0x2adacfbaacd2 for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x2adacfbaacd2: note: pointer points here
 00 00  28 00 00 00 00 00 00 00  01 00 00 00 00 00 00 00  00 00 00 00
00 00 00 00  21 00 00 00 00 00
              ^

This kind of thing:

    unsigned char *cbd = cmd->atapi_cmd;
    uint32_t *lba32;

        lba32 = (uint32_t *)&(cbd[2]);
        *lba32 = cpu_to_be32(lba);

isn't valid. You probably want
 stl_be_p(&cbd[2], lba);

(defined in qemu/bswap.h).

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/11] Ide patches

[John Snow]

On 01/11/2016 06:18 AM, Peter Maydell wrote:
> On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
>> The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:
>>
>>   Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)
>>
>> are available in the git repository at:
>>
>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>
>> for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:
>>
>>   libqos/ahci: organize header (2016-01-08 15:22:34 -0500)
>>
>> ----------------------------------------------------------------
>>
>> ----------------------------------------------------------------
> 
> These seem to result in some new clang sanitizer runtime warnings
> during a 'make check':
> 
> /home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:963:9:
> runtime error: store to misaligned address 0x2adacfbaacd7 for type
> 'uint16_t' (aka 'unsigned short'), which requires 2 byte alignment
> 0x2adacfbaacd7: note: pointer points here
>  00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  21
> 00 00 00 00 00 00 00  6c 6f 6e
>              ^
> /home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:907:9:
> runtime error: store to misaligned address 0x2adacfbaacd2 for type
> 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
> 0x2adacfbaacd2: note: pointer points here
>  00 00  28 00 00 00 00 00 00 00  01 00 00 00 00 00 00 00  00 00 00 00
> 00 00 00 00  21 00 00 00 00 00
>               ^
> 
> This kind of thing:
> 

"This kind of thing" as one might say while holding up a rotting fish
with just two fingers, held at arm's length.

>     unsigned char *cbd = cmd->atapi_cmd;
>     uint32_t *lba32;
> 
>         lba32 = (uint32_t *)&(cbd[2]);
>         *lba32 = cpu_to_be32(lba);
> 
> isn't valid. You probably want
>  stl_be_p(&cbd[2], lba);
> 
> (defined in qemu/bswap.h).
> 
> thanks
> -- PMM
> 

Thanks for the pointer. Out of curiosity, is there no standard way to
perform this kind of operation in C? I want to adjust my bad habits. In
QEMU I can remember to use these macros now that I know they're there,
but not sure what I'd use in other projects. memcpy directly?

--js

Re: [Qemu-devel] [PULL 00/11] Ide patches

[Peter Maydell]

On 11 January 2016 at 17:18, John Snow <jsnow@redhat.com> wrote:
> On 01/11/2016 06:18 AM, Peter Maydell wrote:
>> On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
>> This kind of thing:
>>
>
> "This kind of thing" as one might say while holding up a rotting fish
> with just two fingers, held at arm's length.

Not the intended tone :-)

>>     unsigned char *cbd = cmd->atapi_cmd;
>>     uint32_t *lba32;
>>
>>         lba32 = (uint32_t *)&(cbd[2]);
>>         *lba32 = cpu_to_be32(lba);
>>
>> isn't valid. You probably want
>>  stl_be_p(&cbd[2], lba);

> Thanks for the pointer. Out of curiosity, is there no standard way to
> perform this kind of operation in C? I want to adjust my bad habits. In
> QEMU I can remember to use these macros now that I know they're there,
> but not sure what I'd use in other projects. memcpy directly?

You can use memcpy, or you can hand-assemble values in and out
of byte arrays, I think. memcpy() is generally recommended, because
the compiler does a decent job with it.

thanks
-- PMM