[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

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

[Mark Cave-Ayland]

On 20/09/17 18:55, John Snow wrote:

> Guh. From which distro does your GCC 4.7 hail?
> 
> Regardless, I suppose I will revert to Eric's workaround, though I like
> the way it reads an awful lot less.

Thanks John - it's just a standard Debian Wheezy installation on amd64.


ATB,

Mark.

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

[John Snow]

On 09/20/2017 01:02 PM, Mark Cave-Ayland wrote:
> On 18/09/17 19:14, Peter Maydell wrote:
> 
>> On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote:
>>>> On 09/16/2017 10:34 AM, Peter Maydell wrote:
>>>>> Hi; I'm afraid this doesn't build with clang:
>>>>>
>>>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
>>>>> comparison of unsigned enum expression >= 0 is always true
>>>>> [-Werror,-Wtautological-compare]
>>>>>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>>>>>         ~~~~~ ^  ~
>>>>> 1 error generated
>>
>>
>>> I think you could argue that it would at least be helpful
>>> if clang didn't warn about comparisons that only happen
>>> to be useless for this particular platform/impdef choice
>>> but are useful for the same code compiled with a different
>>> compiler.
>>
>> A bit of googling and some experimentation reveals that
>> clang deliberately suppresses this warning in the special
>> case of comparing against an enum value which happens to
>> be zero (but not for literal constant zero!). So this will
>> be fine:
>>    if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT)
>>
>> (or more sensibly you'd want to define an enum constant
>> for IDE_DMA__FIRST or something rather than relying on
>> READ being 0.)
>>
>> (found here:
>> http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
>> )
> 
> Doing a git pull and even with the applied version of this patch I get a
> build failure on my local gcc-4.7:
> 
> cc -I/home/build/src/qemu/git/qemu/hw/ide -Ihw/ide
> -I/home/build/src/qemu/git/qemu/tcg
> -I/home/build/src/qemu/git/qemu/tcg/i386
> -I/home/build/src/qemu/git/qemu/linux-headers
> -I/home/build/src/qemu/git/qemu/linux-headers -I.
> -I/home/build/src/qemu/git/qemu
> -I/home/build/src/qemu/git/qemu/accel/tcg
> -I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1
> -I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread
> -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
> -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
> -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
> -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
> -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs
> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
> -Wold-style-declaration -Wold-style-definition -Wtype-limits
> -fstack-protector-all -I/usr/include/p11-kit-1
> -I/usr/include/libpng12   -I/home/build/src/qemu/git/qemu/tests -MMD -MP
> -MT hw/ide/core.o -MF hw/ide/core.d -O2 -U_FORTIFY_SOURCE
> -D_FORTIFY_SOURCE=2 -g   -c -o hw/ide/core.o hw/ide/core.c
> hw/ide/core.c: In function ‘IDE_DMA_CMD_str’:
> hw/ide/core.c:71:5: error: comparison of unsigned expression >= 0 is
> always true [-Werror=type-limits]
> cc1: all warnings being treated as errors
> make: *** [hw/ide/core.o] Error 1
> 
> Are there any other workarounds for this at all?
> 
> 
> ATB,
> 
> Mark.
> 

Guh. From which distro does your GCC 4.7 hail?

Regardless, I suppose I will revert to Eric's workaround, though I like
the way it reads an awful lot less.

--js

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

[Mark Cave-Ayland]

On 18/09/17 19:14, Peter Maydell wrote:

> On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote:
>>> On 09/16/2017 10:34 AM, Peter Maydell wrote:
>>>> Hi; I'm afraid this doesn't build with clang:
>>>>
>>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
>>>> comparison of unsigned enum expression >= 0 is always true
>>>> [-Werror,-Wtautological-compare]
>>>>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>>>>         ~~~~~ ^  ~
>>>> 1 error generated
> 
> 
>> I think you could argue that it would at least be helpful
>> if clang didn't warn about comparisons that only happen
>> to be useless for this particular platform/impdef choice
>> but are useful for the same code compiled with a different
>> compiler.
> 
> A bit of googling and some experimentation reveals that
> clang deliberately suppresses this warning in the special
> case of comparing against an enum value which happens to
> be zero (but not for literal constant zero!). So this will
> be fine:
>    if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT)
> 
> (or more sensibly you'd want to define an enum constant
> for IDE_DMA__FIRST or something rather than relying on
> READ being 0.)
> 
> (found here:
> http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
> )

Doing a git pull and even with the applied version of this patch I get a
build failure on my local gcc-4.7:

cc -I/home/build/src/qemu/git/qemu/hw/ide -Ihw/ide
-I/home/build/src/qemu/git/qemu/tcg
-I/home/build/src/qemu/git/qemu/tcg/i386
-I/home/build/src/qemu/git/qemu/linux-headers
-I/home/build/src/qemu/git/qemu/linux-headers -I.
-I/home/build/src/qemu/git/qemu
-I/home/build/src/qemu/git/qemu/accel/tcg
-I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1
-I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread
-I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include
-m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv
-Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers
-Wold-style-declaration -Wold-style-definition -Wtype-limits
-fstack-protector-all -I/usr/include/p11-kit-1
-I/usr/include/libpng12   -I/home/build/src/qemu/git/qemu/tests -MMD -MP
-MT hw/ide/core.o -MF hw/ide/core.d -O2 -U_FORTIFY_SOURCE
-D_FORTIFY_SOURCE=2 -g   -c -o hw/ide/core.o hw/ide/core.c
hw/ide/core.c: In function ‘IDE_DMA_CMD_str’:
hw/ide/core.c:71:5: error: comparison of unsigned expression >= 0 is
always true [-Werror=type-limits]
cc1: all warnings being treated as errors
make: *** [hw/ide/core.o] Error 1

Are there any other workarounds for this at all?


ATB,

Mark.

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

[Peter Maydell]

On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote:
>> On 09/16/2017 10:34 AM, Peter Maydell wrote:
>>> Hi; I'm afraid this doesn't build with clang:
>>>
>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
>>> comparison of unsigned enum expression >= 0 is always true
>>> [-Werror,-Wtautological-compare]
>>>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>>>         ~~~~~ ^  ~
>>> 1 error generated


> I think you could argue that it would at least be helpful
> if clang didn't warn about comparisons that only happen
> to be useless for this particular platform/impdef choice
> but are useful for the same code compiled with a different
> compiler.

A bit of googling and some experimentation reveals that
clang deliberately suppresses this warning in the special
case of comparing against an enum value which happens to
be zero (but not for literal constant zero!). So this will
be fine:
   if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT)

(or more sensibly you'd want to define an enum constant
for IDE_DMA__FIRST or something rather than relying on
READ being 0.)

(found here:
http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html
)

thanks
-- PMM

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

[Peter Maydell]

On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote:
> On 09/16/2017 10:34 AM, Peter Maydell wrote:
>> Hi; I'm afraid this doesn't build with clang:
>>
>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
>> comparison of unsigned enum expression >= 0 is always true
>> [-Werror,-Wtautological-compare]
>>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>>         ~~~~~ ^  ~
>> 1 error generated.
>>
>> (It's impdef whether an enum with all positive values is
>> a signed type or unsigned type, so just deleting the
>> comparison against 0 would also be wrong...)

> Huh, impdef in the general case, but is it undefined for gnu99? I'm
> wondering why Clang can be so certain about this comparison being
> useless. Is this a Clang "bug"?

My guess is that clang as an implementation picks unsigned
in this case, that it then effectively lowers all the enums
to just being integer arithmetic, and then the warning pass
coming along later doesn't know that the unsigned thing it's
comparing is an enum.

I think you could argue that it would at least be helpful
if clang didn't warn about comparisons that only happen
to be useless for this particular platform/impdef choice
but are useful for the same code compiled with a different
compiler.

thanks
-- PMM

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

[John Snow]

On 09/16/2017 10:34 AM, Peter Maydell wrote:
> On 16 September 2017 at 02:03, John Snow <jsnow@redhat.com> wrote:
>> The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa:
>>
>>   Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100)
>>
>> are available in the git repository at:
>>
>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>
>> for you to fetch changes up to 2a94e34d3ecef91727f467cc012587c632099d40:
>>
>>   AHCI: remove DPRINTF macro (2017-09-15 20:36:18 -0400)
>>
>> ----------------------------------------------------------------
>>
>> ----------------------------------------------------------------
> 
> Hi; I'm afraid this doesn't build with clang:
> 
> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
> comparison of unsigned enum expression >= 0 is always true
> [-Werror,-Wtautological-compare]
>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>         ~~~~~ ^  ~
> 1 error generated.
> 
> (It's impdef whether an enum with all positive values is
> a signed type or unsigned type, so just deleting the
> comparison against 0 would also be wrong...)
> 
> thanks
> -- PMM
> 

Huh, impdef in the general case, but is it undefined for gnu99? I'm
wondering why Clang can be so certain about this comparison being
useless. Is this a Clang "bug"?

--js

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

[Eric Blake]

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

On 09/16/2017 09:34 AM, Peter Maydell wrote:

> Hi; I'm afraid this doesn't build with clang:
> 
> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
> comparison of unsigned enum expression >= 0 is always true
> [-Werror,-Wtautological-compare]
>     if (enval >= 0 && enval < IDE_DMA__COUNT) {
>         ~~~~~ ^  ~
> 1 error generated.
> 
> (It's impdef whether an enum with all positive values is
> a signed type or unsigned type, so just deleting the
> comparison against 0 would also be wrong...)

But if ((unsigned)enval < IDE_DMA__COUNT) {

should work, regardless of the signedness of the enum.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

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

[Peter Maydell]

On 16 September 2017 at 02:03, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa:
>
>   Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 2a94e34d3ecef91727f467cc012587c632099d40:
>
>   AHCI: remove DPRINTF macro (2017-09-15 20:36:18 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

Hi; I'm afraid this doesn't build with clang:

/home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error:
comparison of unsigned enum expression >= 0 is always true
[-Werror,-Wtautological-compare]
    if (enval >= 0 && enval < IDE_DMA__COUNT) {
        ~~~~~ ^  ~
1 error generated.

(It's impdef whether an enum with all positive values is
a signed type or unsigned type, so just deleting the
comparison against 0 would also be wrong...)

thanks
-- PMM

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

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PULL 00/11] Ide patches
Message-id: 20170916010330.10435-1-jsnow@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
bc6172dc94 AHCI: remove DPRINTF macro
2ea9b926e3 AHCI: pretty-print FIS to buffer instead of stderr
d23d77942b AHCI: Rework IRQ constants
c3e3883d53 AHCI: Replace DPRINTF with trace-events
a3f8dc5d3c IDE: replace DEBUG_AIO with trace events
3c992d8a98 ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
3beaa3f939 IDE: add tracing for data ports
1994e49cc7 IDE: Add register hints to tracing
a446948ea3 IDE: replace DEBUG_IDE with tracing system
8d2b13d3da hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false
9bc5607864 ide: ahci: unparent children buses before freeing their memory

=== OUTPUT BEGIN ===
Checking PATCH 1/11: ide: ahci: unparent children buses before freeing their memory...
Checking PATCH 2/11: hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false...
Checking PATCH 3/11: IDE: replace DEBUG_IDE with tracing system...
ERROR: spaces required around that '|' (ctx:VxV)
#146: FILE: hw/ide/core.c:1197:
+    if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) {
                                                ^

total: 1 errors, 0 warnings, 337 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/11: IDE: Add register hints to tracing...
Checking PATCH 5/11: IDE: add tracing for data ports...
Checking PATCH 6/11: ATAPI: Replace DEBUG_IDE_ATAPI with tracing events...
Checking PATCH 7/11: IDE: replace DEBUG_AIO with trace events...
Checking PATCH 8/11: AHCI: Replace DPRINTF with trace-events...
ERROR: Hex numbers must be prefixed with '0x'
#548: FILE: hw/ide/trace-events:91:
+handle_reg_h2d_fis_pmp(void *s, int port, char b0, char b1, char b2) "ahci(%p)[%d]: Port Multiplier not supported, FIS: 0x%02x-%02x-%02x"

ERROR: Hex numbers must be prefixed with '0x'
#549: FILE: hw/ide/trace-events:92:
+handle_reg_h2d_fis_res(void *s, int port, char b0, char b1, char b2) "ahci(%p)[%d]: Reserved flags set in H2D Register FIS, FIS: 0x%02x-%02x-%02x"

ERROR: Hex numbers must be prefixed with '0x'
#555: FILE: hw/ide/trace-events:98:
+handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x"

total: 3 errors, 0 warnings, 496 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 9/11: AHCI: Rework IRQ constants...
Checking PATCH 10/11: AHCI: pretty-print FIS to buffer instead of stderr...
WARNING: line over 80 characters
#60: FILE: hw/ide/ahci.c:1206:
+            char *pretty_fis = ahci_pretty_buffer_fis(ide_state->io_buffer, 0x10);

total: 0 errors, 1 warnings, 60 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/11: AHCI: remove DPRINTF macro...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[John Snow]

The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa:

  Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100)

are available in the git repository at:

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

for you to fetch changes up to 2a94e34d3ecef91727f467cc012587c632099d40:

  AHCI: remove DPRINTF macro (2017-09-15 20:36:18 -0400)

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

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

Igor Mammedov (1):
  ide: ahci: unparent children buses before freeing their memory

John Snow (9):
  IDE: replace DEBUG_IDE with tracing system
  IDE: Add register hints to tracing
  IDE: add tracing for data ports
  ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
  IDE: replace DEBUG_AIO with trace events
  AHCI: Replace DPRINTF with trace-events
  AHCI: Rework IRQ constants
  AHCI: pretty-print FIS to buffer instead of stderr
  AHCI: remove DPRINTF macro

Thomas Huth (1):
  hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable =
    false

 Makefile.objs             |   1 +
 hw/ide/ahci.c             | 244 ++++++++++++++++++++++++----------------------
 hw/ide/ahci_internal.h    |  44 +++++++--
 hw/ide/atapi.c            |  69 +++++--------
 hw/ide/cmd646.c           |  10 +-
 hw/ide/core.c             | 185 +++++++++++++++++++++++------------
 hw/ide/microdrive.c       |   3 +
 hw/ide/pci.c              |  17 +---
 hw/ide/piix.c             |  11 +--
 hw/ide/trace-events       | 111 +++++++++++++++++++++
 hw/ide/via.c              |  10 +-
 include/hw/ide/internal.h |   8 +-
 12 files changed, 441 insertions(+), 272 deletions(-)
 create mode 100644 hw/ide/trace-events

-- 
2.9.5

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

[Peter Maydell]

On 10 February 2016 at 19:37, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit c9f19dff101e2c2cf3fa3967eceec2833e845e40:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2016-02-09 19:34:46 +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 d590474922d37372c56075adb229c86d3aeae967:
>
>   ahci: prohibit "restarting" the FIS or CLB engines (2016-02-10 13:29:40 -0500)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

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

[John Snow]

The following changes since commit c9f19dff101e2c2cf3fa3967eceec2833e845e40:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2016-02-09 19:34:46 +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 d590474922d37372c56075adb229c86d3aeae967:

  ahci: prohibit "restarting" the FIS or CLB engines (2016-02-10 13:29:40 -0500)

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

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

John Snow (11):
  ide: Prohibit RESET on IDE drives
  ide: code motion
  ide: move buffered DMA cancel to core
  ide: replace blk_drain_all by blk_drain
  ide: Add silent DRQ cancellation
  ide: fix device_reset to not ignore pending AIO
  fdc: always compile-check debug prints
  ahci: Do not unmap NULL addresses
  ahci: handle LIST_ON and FIS_ON in map helpers
  ahci: explicitly reject bad engine states on post_load
  ahci: prohibit "restarting" the FIS or CLB engines

 hw/block/fdc.c    |  15 ++--
 hw/ide/ahci.c     |  96 ++++++++++++++----------
 hw/ide/core.c     | 217 ++++++++++++++++++++++++++++++++++++------------------
 hw/ide/internal.h |   1 +
 hw/ide/pci.c      |  36 +--------
 5 files changed, 213 insertions(+), 152 deletions(-)

-- 
2.4.3

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

[John Snow]

The following changes since commit c9f19dff101e2c2cf3fa3967eceec2833e845e40:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2016-02-09 19:34:46 +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 d590474922d37372c56075adb229c86d3aeae967:

  ahci: prohibit "restarting" the FIS or CLB engines (2016-02-10 13:29:40 -0500)

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

Untested with Clang.

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

John Snow (11):
  ide: Prohibit RESET on IDE drives
  ide: code motion
  ide: move buffered DMA cancel to core
  ide: replace blk_drain_all by blk_drain
  ide: Add silent DRQ cancellation
  ide: fix device_reset to not ignore pending AIO
  fdc: always compile-check debug prints
  ahci: Do not unmap NULL addresses
  ahci: handle LIST_ON and FIS_ON in map helpers
  ahci: explicitly reject bad engine states on post_load
  ahci: prohibit "restarting" the FIS or CLB engines

 hw/block/fdc.c    |  15 ++--
 hw/ide/ahci.c     |  96 ++++++++++++++----------
 hw/ide/core.c     | 217 ++++++++++++++++++++++++++++++++++++------------------
 hw/ide/internal.h |   1 +
 hw/ide/pci.c      |  36 +--------
 5 files changed, 213 insertions(+), 152 deletions(-)

-- 
2.4.3

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

[Peter Maydell]

On 18 September 2015 at 16:04, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 16a1b6e97c2a2919fd296db4bea2f9da2ad3cc4d:
>
>   target-cris: update CPU state save/load to use VMStateDescription (2015-09-17 14:31:38 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to e47f9eb148fc3b9a67d318951ebceb834205f94c:
>
>   ahci: clean up initial d2h semantics (2015-09-18 10:58:56 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>

Applied, thanks.

-- PMM

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

[John Snow]

The following changes since commit 16a1b6e97c2a2919fd296db4bea2f9da2ad3cc4d:

  target-cris: update CPU state save/load to use VMStateDescription (2015-09-17 14:31:38 +0100)

are available in the git repository at:

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

for you to fetch changes up to e47f9eb148fc3b9a67d318951ebceb834205f94c:

  ahci: clean up initial d2h semantics (2015-09-18 10:58:56 -0400)

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

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

John Snow (11):
  ide: unify io_buffer_offset increments
  qtest/ahci: use generate_pattern everywhere
  qtest/ahci: export generate_pattern
  ide-test: add cdrom pio test
  ide-test: add cdrom dma test
  ide: fix ATAPI command permissions
  atapi: abort transfers with 0 byte limits
  ahci: remove dead reset code
  ahci: fix signature generation
  ahci: remove cmd_fis argument from write_fis_d2h
  ahci: clean up initial d2h semantics

 hw/ide/ahci.c         |  75 ++++++++--------
 hw/ide/atapi.c        |  32 +++++--
 hw/ide/core.c         |  37 ++++----
 hw/ide/internal.h     |   2 +
 tests/ahci-test.c     |  43 +--------
 tests/ide-test.c      | 245 ++++++++++++++++++++++++++++++++++++++++++++++----
 tests/libqos/libqos.c |  26 ++++++
 tests/libqos/libqos.h |   1 +
 8 files changed, 342 insertions(+), 119 deletions(-)

-- 
2.4.3