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

[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

[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 01/11] ide: Prohibit RESET on IDE drives

[John Snow]

This command is meant for ATAPI devices only, prohibit acknowledging it with
a command aborted response when an IDE device is busy.

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1453225191-11871-2-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 4c46453..88d5fab 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1877,9 +1877,13 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
         return;
     }
 
-    /* Only DEVICE RESET is allowed while BSY or/and DRQ are set */
-    if ((s->status & (BUSY_STAT|DRQ_STAT)) && val != WIN_DEVICE_RESET)
-        return;
+    /* Only RESET is allowed while BSY and/or DRQ are set,
+     * and only to ATAPI devices. */
+    if (s->status & (BUSY_STAT|DRQ_STAT)) {
+        if (val != WIN_DEVICE_RESET || s->drive_kind != IDE_CD) {
+            return;
+        }
+    }
 
     if (!ide_cmd_permitted(s, val)) {
         ide_abort_command(s);
-- 
2.4.3

[Qemu-devel] [PULL 02/11] ide: code motion

[John Snow]

Shuffle the reset function upwards.

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1453225191-11871-3-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c | 116 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 58 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 88d5fab..37d1058 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1175,6 +1175,64 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+static void ide_reset(IDEState *s)
+{
+#ifdef DEBUG_IDE
+    printf("ide: reset\n");
+#endif
+
+    if (s->pio_aiocb) {
+        blk_aio_cancel(s->pio_aiocb);
+        s->pio_aiocb = NULL;
+    }
+
+    if (s->drive_kind == IDE_CFATA)
+        s->mult_sectors = 0;
+    else
+        s->mult_sectors = MAX_MULT_SECTORS;
+    /* ide regs */
+    s->feature = 0;
+    s->error = 0;
+    s->nsector = 0;
+    s->sector = 0;
+    s->lcyl = 0;
+    s->hcyl = 0;
+
+    /* lba48 */
+    s->hob_feature = 0;
+    s->hob_sector = 0;
+    s->hob_nsector = 0;
+    s->hob_lcyl = 0;
+    s->hob_hcyl = 0;
+
+    s->select = 0xa0;
+    s->status = READY_STAT | SEEK_STAT;
+
+    s->lba48 = 0;
+
+    /* ATAPI specific */
+    s->sense_key = 0;
+    s->asc = 0;
+    s->cdrom_changed = 0;
+    s->packet_transfer_size = 0;
+    s->elementary_transfer_size = 0;
+    s->io_buffer_index = 0;
+    s->cd_sector_size = 0;
+    s->atapi_dma = 0;
+    s->tray_locked = 0;
+    s->tray_open = 0;
+    /* ATA DMA state */
+    s->io_buffer_size = 0;
+    s->req_nb_sectors = 0;
+
+    ide_set_signature(s);
+    /* init the transfer handler so that 0xffff is returned on data
+       accesses */
+    s->end_transfer_func = ide_dummy_transfer_stop;
+    ide_dummy_transfer_stop(s);
+    s->media_changed = 0;
+}
+
 static bool cmd_nop(IDEState *s, uint8_t cmd)
 {
     return true;
@@ -2183,64 +2241,6 @@ static void ide_dummy_transfer_stop(IDEState *s)
     s->io_buffer[3] = 0xff;
 }
 
-static void ide_reset(IDEState *s)
-{
-#ifdef DEBUG_IDE
-    printf("ide: reset\n");
-#endif
-
-    if (s->pio_aiocb) {
-        blk_aio_cancel(s->pio_aiocb);
-        s->pio_aiocb = NULL;
-    }
-
-    if (s->drive_kind == IDE_CFATA)
-        s->mult_sectors = 0;
-    else
-        s->mult_sectors = MAX_MULT_SECTORS;
-    /* ide regs */
-    s->feature = 0;
-    s->error = 0;
-    s->nsector = 0;
-    s->sector = 0;
-    s->lcyl = 0;
-    s->hcyl = 0;
-
-    /* lba48 */
-    s->hob_feature = 0;
-    s->hob_sector = 0;
-    s->hob_nsector = 0;
-    s->hob_lcyl = 0;
-    s->hob_hcyl = 0;
-
-    s->select = 0xa0;
-    s->status = READY_STAT | SEEK_STAT;
-
-    s->lba48 = 0;
-
-    /* ATAPI specific */
-    s->sense_key = 0;
-    s->asc = 0;
-    s->cdrom_changed = 0;
-    s->packet_transfer_size = 0;
-    s->elementary_transfer_size = 0;
-    s->io_buffer_index = 0;
-    s->cd_sector_size = 0;
-    s->atapi_dma = 0;
-    s->tray_locked = 0;
-    s->tray_open = 0;
-    /* ATA DMA state */
-    s->io_buffer_size = 0;
-    s->req_nb_sectors = 0;
-
-    ide_set_signature(s);
-    /* init the transfer handler so that 0xffff is returned on data
-       accesses */
-    s->end_transfer_func = ide_dummy_transfer_stop;
-    ide_dummy_transfer_stop(s);
-    s->media_changed = 0;
-}
-
 void ide_bus_reset(IDEBus *bus)
 {
     bus->unit = 0;
-- 
2.4.3

[Qemu-devel] [PULL 03/11] ide: move buffered DMA cancel to core

[John Snow]

Buffered DMA cancellation was added to ATAPI devices and implemented
for the BMDMA HBA. Move the code over to common IDE code and allow
it to be used for any HBA.

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1453225191-11871-4-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/internal.h |  1 +
 hw/ide/pci.c      | 36 +-----------------------------------
 3 files changed, 47 insertions(+), 35 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 37d1058..5cafcf5 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -609,6 +609,51 @@ BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
     return aioreq;
 }
 
+/**
+ * Cancel all pending DMA requests.
+ * Any buffered DMA requests are instantly canceled,
+ * but any pending unbuffered DMA requests must be waited on.
+ */
+void ide_cancel_dma_sync(IDEState *s)
+{
+    IDEBufferedRequest *req;
+
+    /* First invoke the callbacks of all buffered requests
+     * and flag those requests as orphaned. Ideally there
+     * are no unbuffered (Scatter Gather DMA Requests or
+     * write requests) pending and we can avoid to drain. */
+    QLIST_FOREACH(req, &s->buffered_requests, list) {
+        if (!req->orphaned) {
+#ifdef DEBUG_IDE
+            printf("%s: invoking cb %p of buffered request %p with"
+                   " -ECANCELED\n", __func__, req->original_cb, req);
+#endif
+            req->original_cb(req->original_opaque, -ECANCELED);
+        }
+        req->orphaned = true;
+    }
+
+    /*
+     * We can't cancel Scatter Gather DMA in the middle of the
+     * operation or a partial (not full) DMA transfer would reach
+     * the storage so we wait for completion instead (we beahve
+     * like if the DMA was completed by the time the guest trying
+     * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
+     * set).
+     *
+     * In the future we'll be able to safely cancel the I/O if the
+     * whole DMA operation will be submitted to disk with a single
+     * aio operation with preadv/pwritev.
+     */
+    if (s->bus->dma->aiocb) {
+#ifdef DEBUG_IDE
+        printf("%s: draining all remaining requests", __func__);
+#endif
+        blk_drain_all();
+        assert(s->bus->dma->aiocb == NULL);
+    }
+}
+
 static void ide_sector_read(IDEState *s);
 
 static void ide_sector_read_cb(void *opaque, int ret)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 2d1e2d2..86bde26 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -586,6 +586,7 @@ BlockAIOCB *ide_issue_trim(BlockBackend *blk,
 BlockAIOCB *ide_buffered_readv(IDEState *s, int64_t sector_num,
                                QEMUIOVector *iov, int nb_sectors,
                                BlockCompletionFunc *cb, void *opaque);
+void ide_cancel_dma_sync(IDEState *s);
 
 /* hw/ide/atapi.c */
 void ide_atapi_cmd(IDEState *s);
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index fa5b63b..92ffee7 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -234,41 +234,7 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
     /* Ignore writes to SSBM if it keeps the old value */
     if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
         if (!(val & BM_CMD_START)) {
-            /* First invoke the callbacks of all buffered requests
-             * and flag those requests as orphaned. Ideally there
-             * are no unbuffered (Scatter Gather DMA Requests or
-             * write requests) pending and we can avoid to drain. */
-            IDEBufferedRequest *req;
-            IDEState *s = idebus_active_if(bm->bus);
-            QLIST_FOREACH(req, &s->buffered_requests, list) {
-                if (!req->orphaned) {
-#ifdef DEBUG_IDE
-                    printf("%s: invoking cb %p of buffered request %p with"
-                           " -ECANCELED\n", __func__, req->original_cb, req);
-#endif
-                    req->original_cb(req->original_opaque, -ECANCELED);
-                }
-                req->orphaned = true;
-            }
-            /*
-             * We can't cancel Scatter Gather DMA in the middle of the
-             * operation or a partial (not full) DMA transfer would reach
-             * the storage so we wait for completion instead (we beahve
-             * like if the DMA was completed by the time the guest trying
-             * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
-             * set).
-             *
-             * In the future we'll be able to safely cancel the I/O if the
-             * whole DMA operation will be submitted to disk with a single
-             * aio operation with preadv/pwritev.
-             */
-            if (bm->bus->dma->aiocb) {
-#ifdef DEBUG_IDE
-                printf("%s: draining all remaining requests", __func__);
-#endif
-                blk_drain_all();
-                assert(bm->bus->dma->aiocb == NULL);
-            }
+            ide_cancel_dma_sync(idebus_active_if(bm->bus));
             bm->status &= ~BM_STATUS_DMAING;
         } else {
             bm->cur_addr = bm->addr;
-- 
2.4.3

[Qemu-devel] [PULL 04/11] ide: replace blk_drain_all by blk_drain

[John Snow]

Target the drain for just one device.

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1453225191-11871-5-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5cafcf5..40b6cc8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -649,7 +649,7 @@ void ide_cancel_dma_sync(IDEState *s)
 #ifdef DEBUG_IDE
         printf("%s: draining all remaining requests", __func__);
 #endif
-        blk_drain_all();
+        blk_drain(s->blk);
         assert(s->bus->dma->aiocb == NULL);
     }
 }
-- 
2.4.3

[Qemu-devel] [PULL 05/11] ide: Add silent DRQ cancellation

[John Snow]

Split apart the ide_transfer_stop function into two versions: one that
interrupts and one that doesn't. The one that doesn't can be used to
halt any PIO transfers that are in the DRQ phase. It will not halt
any PIO transfers that are currently in the process of buffering data
for the guest to read.

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[Renamed 'etf' to 'end_transfer_func' --js]
Message-id: 1453225191-11871-6-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 40b6cc8..3c32b39 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -487,13 +487,28 @@ static void ide_cmd_done(IDEState *s)
     }
 }
 
-void ide_transfer_stop(IDEState *s)
+static void ide_transfer_halt(IDEState *s,
+                              void(*end_transfer_func)(IDEState *),
+                              bool notify)
 {
-    s->end_transfer_func = ide_transfer_stop;
+    s->end_transfer_func = end_transfer_func;
     s->data_ptr = s->io_buffer;
     s->data_end = s->io_buffer;
     s->status &= ~DRQ_STAT;
-    ide_cmd_done(s);
+    if (notify) {
+        ide_cmd_done(s);
+    }
+}
+
+void ide_transfer_stop(IDEState *s)
+{
+    ide_transfer_halt(s, ide_transfer_stop, true);
+}
+
+__attribute__((__unused__))
+static void ide_transfer_cancel(IDEState *s)
+{
+    ide_transfer_halt(s, ide_transfer_cancel, false);
 }
 
 int64_t ide_get_sector(IDEState *s)
-- 
2.4.3

[Qemu-devel] [PULL 06/11] ide: fix device_reset to not ignore pending AIO

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1453225191-11871-7-git-send-email-jsnow@redhat.com
---
 hw/ide/core.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3c32b39..241e840 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -505,7 +505,6 @@ void ide_transfer_stop(IDEState *s)
     ide_transfer_halt(s, ide_transfer_stop, true);
 }
 
-__attribute__((__unused__))
 static void ide_transfer_cancel(IDEState *s)
 {
     ide_transfer_halt(s, ide_transfer_cancel, false);
@@ -1298,6 +1297,23 @@ static bool cmd_nop(IDEState *s, uint8_t cmd)
     return true;
 }
 
+static bool cmd_device_reset(IDEState *s, uint8_t cmd)
+{
+    /* Halt PIO (in the DRQ phase), then DMA */
+    ide_transfer_cancel(s);
+    ide_cancel_dma_sync(s);
+
+    /* Reset any PIO commands, reset signature, etc */
+    ide_reset(s);
+
+    /* RESET: ATA8-ACS3 7.10.4 "Normal Outputs";
+     * ATA8-ACS3 Table 184 "Device Signatures for Normal Output" */
+    s->status = 0x00;
+
+    /* Do not overwrite status register */
+    return false;
+}
+
 static bool cmd_data_set_management(IDEState *s, uint8_t cmd)
 {
     switch (s->feature) {
@@ -1614,15 +1630,6 @@ static bool cmd_exec_dev_diagnostic(IDEState *s, uint8_t cmd)
     return false;
 }
 
-static bool cmd_device_reset(IDEState *s, uint8_t cmd)
-{
-    ide_set_signature(s);
-    s->status = 0x00; /* NOTE: READY is _not_ set */
-    s->error = 0x01;
-
-    return false;
-}
-
 static bool cmd_packet(IDEState *s, uint8_t cmd)
 {
     /* overlapping commands not supported */
-- 
2.4.3

[Qemu-devel] [PULL 07/11] fdc: always compile-check debug prints

[John Snow]

Coverity noticed that some variables are only used by debug prints, and
called them unused. Always compile the print statements. While we're
here, print to stderr as well.

Bonus: Fix a debug printf I broke in f31937aa8

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[Touched up commit message. --js]
Message-id: 1454971529-14830-1-git-send-email-jsnow@redhat.com
---
 hw/block/fdc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a6f22ef..9838d21 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -41,14 +41,15 @@
 
 /********************************************************/
 /* debug Floppy devices */
-//#define DEBUG_FLOPPY
 
-#ifdef DEBUG_FLOPPY
+#define DEBUG_FLOPPY 0
+
 #define FLOPPY_DPRINTF(fmt, ...)                                \
-    do { printf("FLOPPY: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define FLOPPY_DPRINTF(fmt, ...)
-#endif
+    do {                                                        \
+        if (DEBUG_FLOPPY) {                                     \
+            fprintf(stderr, "FLOPPY: " fmt , ## __VA_ARGS__);   \
+        }                                                       \
+    } while (0)
 
 /********************************************************/
 /* Floppy drive emulation                               */
@@ -353,7 +354,7 @@ static int pick_geometry(FDrive *drv)
             parse = &fd_formats[size_match];
             FLOPPY_DPRINTF("User requested floppy drive type '%s', "
                            "but inserted medium appears to be a "
-                           "%d sector '%s' type\n",
+                           "%"PRId64" sector '%s' type\n",
                            FloppyDriveType_lookup[drv->drive],
                            nb_sectors,
                            FloppyDriveType_lookup[parse->drive]);
-- 
2.4.3

[Qemu-devel] [PULL 08/11] ahci: Do not unmap NULL addresses

[John Snow]

Definitely don't try to unmap a garbage address.

Reported-by: Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1454103689-13042-2-git-send-email-jsnow@redhat.com
---
 hw/ide/ahci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7e87b18..3a95dad 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -662,6 +662,10 @@ static bool ahci_map_fis_address(AHCIDevice *ad)
 
 static void ahci_unmap_fis_address(AHCIDevice *ad)
 {
+    if (ad->res_fis == NULL) {
+        DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
+        return;
+    }
     dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
                      DMA_DIRECTION_FROM_DEVICE, 256);
     ad->res_fis = NULL;
@@ -678,6 +682,10 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
 
 static void ahci_unmap_clb_address(AHCIDevice *ad)
 {
+    if (ad->lst == NULL) {
+        DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
+        return;
+    }
     dma_memory_unmap(ad->hba->as, ad->lst, 1024,
                      DMA_DIRECTION_FROM_DEVICE, 1024);
     ad->lst = NULL;
-- 
2.4.3

[Qemu-devel] [PULL 09/11] ahci: handle LIST_ON and FIS_ON in map helpers

[John Snow]

Instead of relying on ahci_cond_start_engines to maintain the
engine status indicators itself, have the lower-layer CLB and FIS mapper
helpers do it themselves.

This makes the cond_start routine slightly nicer to read, and makes sure
that the status indicators will always be correct.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1454103689-13042-3-git-send-email-jsnow@redhat.com
---
 hw/ide/ahci.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3a95dad..ff338fe 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -210,9 +210,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     AHCIPortRegs *pr = &ad->port_regs;
 
     if (pr->cmd & PORT_CMD_START) {
-        if (ahci_map_clb_address(ad)) {
-            pr->cmd |= PORT_CMD_LIST_ON;
-        } else {
+        if (!ahci_map_clb_address(ad)) {
             error_report("AHCI: Failed to start DMA engine: "
                          "bad command list buffer address");
             return -1;
@@ -220,7 +218,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_LIST_ON) {
         if (allow_stop) {
             ahci_unmap_clb_address(ad);
-            pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
         } else {
             error_report("AHCI: DMA engine should be off, "
                          "but appears to still be running");
@@ -229,9 +226,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     }
 
     if (pr->cmd & PORT_CMD_FIS_RX) {
-        if (ahci_map_fis_address(ad)) {
-            pr->cmd |= PORT_CMD_FIS_ON;
-        } else {
+        if (!ahci_map_fis_address(ad)) {
             error_report("AHCI: Failed to start FIS receive engine: "
                          "bad FIS receive buffer address");
             return -1;
@@ -239,7 +234,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_FIS_ON) {
         if (allow_stop) {
             ahci_unmap_fis_address(ad);
-            pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
         } else {
             error_report("AHCI: FIS receive engine should be off, "
                          "but appears to still be running");
@@ -657,7 +651,13 @@ static bool ahci_map_fis_address(AHCIDevice *ad)
     AHCIPortRegs *pr = &ad->port_regs;
     map_page(ad->hba->as, &ad->res_fis,
              ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
-    return ad->res_fis != NULL;
+    if (ad->res_fis != NULL) {
+        pr->cmd |= PORT_CMD_FIS_ON;
+        return true;
+    }
+
+    pr->cmd &= ~PORT_CMD_FIS_ON;
+    return false;
 }
 
 static void ahci_unmap_fis_address(AHCIDevice *ad)
@@ -666,6 +666,7 @@ static void ahci_unmap_fis_address(AHCIDevice *ad)
         DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
         return;
     }
+    ad->port_regs.cmd &= ~PORT_CMD_FIS_ON;
     dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
                      DMA_DIRECTION_FROM_DEVICE, 256);
     ad->res_fis = NULL;
@@ -677,7 +678,13 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
     ad->cur_cmd = NULL;
     map_page(ad->hba->as, &ad->lst,
              ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
-    return ad->lst != NULL;
+    if (ad->lst != NULL) {
+        pr->cmd |= PORT_CMD_LIST_ON;
+        return true;
+    }
+
+    pr->cmd &= ~PORT_CMD_LIST_ON;
+    return false;
 }
 
 static void ahci_unmap_clb_address(AHCIDevice *ad)
@@ -686,6 +693,7 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
         DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
         return;
     }
+    ad->port_regs.cmd &= ~PORT_CMD_LIST_ON;
     dma_memory_unmap(ad->hba->as, ad->lst, 1024,
                      DMA_DIRECTION_FROM_DEVICE, 1024);
     ad->lst = NULL;
-- 
2.4.3

[Qemu-devel] [PULL 10/11] ahci: explicitly reject bad engine states on post_load

[John Snow]

Currently, we let ahci_cond_start_engines reject weird configurations
where either the DMA (CLB) or FIS engines are said to be started, but
their matching on/off control bit is toggled off.

There should be no way to achieve this, since any time you toggle the
control bit off, the status bit should always follow synchronously.

Preparing for a refactor in cond_start_engines, move the rejection logic
straight up into post_load.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1454103689-13042-4-git-send-email-jsnow@redhat.com
---
 hw/ide/ahci.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ff338fe..9f4a672 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -218,10 +218,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_LIST_ON) {
         if (allow_stop) {
             ahci_unmap_clb_address(ad);
-        } else {
-            error_report("AHCI: DMA engine should be off, "
-                         "but appears to still be running");
-            return -1;
         }
     }
 
@@ -234,10 +230,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
     } else if (pr->cmd & PORT_CMD_FIS_ON) {
         if (allow_stop) {
             ahci_unmap_fis_address(ad);
-        } else {
-            error_report("AHCI: FIS receive engine should be off, "
-                         "but appears to still be running");
-            return -1;
         }
     }
 
@@ -1568,10 +1560,23 @@ static int ahci_state_post_load(void *opaque, int version_id)
     int i, j;
     struct AHCIDevice *ad;
     NCQTransferState *ncq_tfs;
+    AHCIPortRegs *pr;
     AHCIState *s = opaque;
 
     for (i = 0; i < s->ports; i++) {
         ad = &s->dev[i];
+        pr = &ad->port_regs;
+
+        if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
+            error_report("AHCI: DMA engine should be off, but status bit "
+                         "indicates it is still running.");
+            return -1;
+        }
+        if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) {
+            error_report("AHCI: FIS RX engine should be off, but status bit "
+                         "indicates it is still running.");
+            return -1;
+        }
 
         /* Only remap the CLB address if appropriate, disallowing a state
          * transition from 'on' to 'off' it should be consistent here. */
-- 
2.4.3

[Qemu-devel] [PULL 11/11] ahci: prohibit "restarting" the FIS or CLB engines

[John Snow]

If the FIS or DMA engines are already started, do not allow them to be
"restarted." As a side-effect of this change, the migration post-load
routine must be modified to cope. If the engines are listed as "on"
in the migrated registers, they must be cleared to allow the startup
routine to see the transition from "off" to "on".

As a second side-effect, the extra argument to ahci_cond_engine_start
is removed in favor of consistent behavior.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1454103689-13042-5-git-send-email-jsnow@redhat.com
---
 hw/ide/ahci.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9f4a672..f244bc0 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -199,38 +199,38 @@ static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
  * Check the cmd register to see if we should start or stop
  * the DMA or FIS RX engines.
  *
- * @ad: Device to engage.
- * @allow_stop: Allow device to transition from started to stopped?
- *   'no' is useful for migration post_load, which does not expect a transition.
+ * @ad: Device to dis/engage.
  *
  * @return 0 on success, -1 on error.
  */
-static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
+static int ahci_cond_start_engines(AHCIDevice *ad)
 {
     AHCIPortRegs *pr = &ad->port_regs;
+    bool cmd_start = pr->cmd & PORT_CMD_START;
+    bool cmd_on    = pr->cmd & PORT_CMD_LIST_ON;
+    bool fis_start = pr->cmd & PORT_CMD_FIS_RX;
+    bool fis_on    = pr->cmd & PORT_CMD_FIS_ON;
 
-    if (pr->cmd & PORT_CMD_START) {
+    if (cmd_start && !cmd_on) {
         if (!ahci_map_clb_address(ad)) {
+            pr->cmd &= ~PORT_CMD_START;
             error_report("AHCI: Failed to start DMA engine: "
                          "bad command list buffer address");
             return -1;
         }
-    } else if (pr->cmd & PORT_CMD_LIST_ON) {
-        if (allow_stop) {
-            ahci_unmap_clb_address(ad);
-        }
+    } else if (!cmd_start && cmd_on) {
+        ahci_unmap_clb_address(ad);
     }
 
-    if (pr->cmd & PORT_CMD_FIS_RX) {
+    if (fis_start && !fis_on) {
         if (!ahci_map_fis_address(ad)) {
+            pr->cmd &= ~PORT_CMD_FIS_RX;
             error_report("AHCI: Failed to start FIS receive engine: "
                          "bad FIS receive buffer address");
             return -1;
         }
-    } else if (pr->cmd & PORT_CMD_FIS_ON) {
-        if (allow_stop) {
-            ahci_unmap_fis_address(ad);
-        }
+    } else if (!fis_start && fis_on) {
+        ahci_unmap_fis_address(ad);
     }
 
     return 0;
@@ -272,8 +272,8 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
                       (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
 
-            /* Check FIS RX and CLB engines, allow transition to false: */
-            ahci_cond_start_engines(&s->dev[port], true);
+            /* Check FIS RX and CLB engines */
+            ahci_cond_start_engines(&s->dev[port]);
 
             /* XXX usually the FIS would be pending on the bus here and
                    issuing deferred until the OS enables FIS receival.
@@ -1578,9 +1578,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
             return -1;
         }
 
-        /* Only remap the CLB address if appropriate, disallowing a state
-         * transition from 'on' to 'off' it should be consistent here. */
-        if (ahci_cond_start_engines(ad, false) != 0) {
+        /* After a migrate, the DMA/FIS engines are "off" and
+         * need to be conditionally restarted */
+        pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
+        if (ahci_cond_start_engines(ad) != 0) {
             return -1;
         }
 
-- 
2.4.3

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

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

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

[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

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