qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset
@ 2016-01-19 17:39 John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 1/6] ide: Prohibit RESET on IDE drives John Snow
                   ` (7 more replies)
  0 siblings, 8 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

The ATAPI software reset function is implemented somewhat lackadaisically.

Firstly, it is valid only for ATAPI drives - not HDs. If a HD should
receive this command while BSY, it should be ignored like any other
command instead of aborted. A non-BSY HD is free to abort the command
in the usual fashion to indicate it doesn't understand or doesn't support
that command.

Second, for drives that should accept a software reset, they should not
"forget" about all pending AIO during the reset. Since a software reset
resets the DRQ and BSY flags, it is possible to 'stack' multiple
concurrent reads using DMA and alternately chaining software reset and
DMA reads. We mustn't reset BSY/DRQ until we are confident that we
have canceled existing AIO.

Third, the existing software reset routine does not perform a very
rigorous reset.

This series corrects this by:

(1) Correcting ide_exec_cmd to correctly ignore, not abort, software
    reset commands for ide-hd devices that are busy executing a command.

(2) Improving the software reset routine to cancel buffered DMA, then
    fall back to synchronously waiting for any pending DMA to finish
    before returning, insuring that the reset completes sanely.

(3) Use existing reset routines to comprehensively reset the device.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>

________________________________________________________________________________

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

This version is tagged ide-reset-fix-v2:
https://github.com/jnsnow/qemu/releases/tag/ide-reset-fix-v2

John Snow (6):
  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

 hw/ide/core.c     | 215 ++++++++++++++++++++++++++++++++++++------------------
 hw/ide/internal.h |   1 +
 hw/ide/pci.c      |  36 +--------
 3 files changed, 144 insertions(+), 108 deletions(-)

-- 
2.4.3

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 1/6] ide: Prohibit RESET on IDE drives
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 2/6] ide: code motion John Snow
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

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>
---
 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 da3baab..d121f3d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1876,9 +1876,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 2/6] ide: code motion
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 1/6] ide: Prohibit RESET on IDE drives John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 3/6] ide: move buffered DMA cancel to core John Snow
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

Shuffle the reset function upwards.

Signed-off-by: John Snow <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 d121f3d..10f2702 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1174,6 +1174,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;
@@ -2182,64 +2240,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 3/6] ide: move buffered DMA cancel to core
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 1/6] ide: Prohibit RESET on IDE drives John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 2/6] ide: code motion John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 4/6] ide: replace blk_drain_all by blk_drain John Snow
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

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>
---
 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 10f2702..7d32266 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -608,6 +608,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 37dbc29..6b780b8 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -233,41 +233,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 4/6] ide: replace blk_drain_all by blk_drain
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
                   ` (2 preceding siblings ...)
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 3/6] ide: move buffered DMA cancel to core John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 5/6] ide: Add silent DRQ cancellation John Snow
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

Target the drain for just one device.

Signed-off-by: John Snow <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 7d32266..cf0b5ec 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -648,7 +648,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 5/6] ide: Add silent DRQ cancellation
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
                   ` (3 preceding siblings ...)
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 4/6] ide: replace blk_drain_all by blk_drain John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-02-08 16:09   ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 6/6] ide: fix device_reset to not ignore pending AIO John Snow
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

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>
---
 hw/ide/core.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index cf0b5ec..9bc8e58 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -486,13 +486,26 @@ static void ide_cmd_done(IDEState *s)
     }
 }
 
-void ide_transfer_stop(IDEState *s)
+static void ide_transfer_halt(IDEState *s, void(*etf)(IDEState *), bool notify)
 {
-    s->end_transfer_func = ide_transfer_stop;
+    s->end_transfer_func = etf;
     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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 6/6] ide: fix device_reset to not ignore pending AIO
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
                   ` (4 preceding siblings ...)
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 5/6] ide: Add silent DRQ cancellation John Snow
@ 2016-01-19 17:39 ` John Snow
  2016-01-26 20:39 ` [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
  2016-02-08 16:10 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-19 17:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, John Snow, qemu-devel, stefanha

Signed-off-by: John Snow <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 9bc8e58..c68d1d4 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -502,7 +502,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);
@@ -1295,6 +1294,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) {
@@ -1611,15 +1627,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
                   ` (5 preceding siblings ...)
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 6/6] ide: fix device_reset to not ignore pending AIO John Snow
@ 2016-01-26 20:39 ` John Snow
  2016-02-08 16:10 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
  7 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2016-01-26 20:39 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, pbonzini, qemu-devel, stefanha

Ping

(I know I could just send a PR, but in this case I'd like someone to
look over it. I've gone over pretty terrible detail to convince myself
this won't break anything IDE-wise, so if it looks sane block-wise, I'd
be satisfied with an ACK.)

On 01/19/2016 12:39 PM, John Snow wrote:
> The ATAPI software reset function is implemented somewhat lackadaisically.
> 
> Firstly, it is valid only for ATAPI drives - not HDs. If a HD should
> receive this command while BSY, it should be ignored like any other
> command instead of aborted. A non-BSY HD is free to abort the command
> in the usual fashion to indicate it doesn't understand or doesn't support
> that command.
> 
> Second, for drives that should accept a software reset, they should not
> "forget" about all pending AIO during the reset. Since a software reset
> resets the DRQ and BSY flags, it is possible to 'stack' multiple
> concurrent reads using DMA and alternately chaining software reset and
> DMA reads. We mustn't reset BSY/DRQ until we are confident that we
> have canceled existing AIO.
> 
> Third, the existing software reset routine does not perform a very
> rigorous reset.
> 
> This series corrects this by:
> 
> (1) Correcting ide_exec_cmd to correctly ignore, not abort, software
>     reset commands for ide-hd devices that are busy executing a command.
> 
> (2) Improving the software reset routine to cancel buffered DMA, then
>     fall back to synchronously waiting for any pending DMA to finish
>     before returning, insuring that the reset completes sanely.
> 
> (3) Use existing reset routines to comprehensively reset the device.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ide-reset-fix
> https://github.com/jnsnow/qemu/tree/ide-reset-fix
> 
> This version is tagged ide-reset-fix-v2:
> https://github.com/jnsnow/qemu/releases/tag/ide-reset-fix-v2
> 
> John Snow (6):
>   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
> 
>  hw/ide/core.c     | 215 ++++++++++++++++++++++++++++++++++++------------------
>  hw/ide/internal.h |   1 +
>  hw/ide/pci.c      |  36 +--------
>  3 files changed, 144 insertions(+), 108 deletions(-)
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH v2 5/6] ide: Add silent DRQ cancellation
  2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 5/6] ide: Add silent DRQ cancellation John Snow
@ 2016-02-08 16:09   ` Stefan Hajnoczi
  2016-02-08 17:08     ` John Snow
  0 siblings, 1 reply; 12+ messages in thread
From: Stefan Hajnoczi @ 2016-02-08 16:09 UTC (permalink / raw)
  To: John Snow; +Cc: kwolf, pbonzini, stefanha, qemu-devel, qemu-block

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

On Tue, Jan 19, 2016 at 12:39:50PM -0500, John Snow wrote:
> 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>
> ---
>  hw/ide/core.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index cf0b5ec..9bc8e58 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -486,13 +486,26 @@ static void ide_cmd_done(IDEState *s)
>      }
>  }
>  
> -void ide_transfer_stop(IDEState *s)
> +static void ide_transfer_halt(IDEState *s, void(*etf)(IDEState *), bool notify)
>  {
> -    s->end_transfer_func = ide_transfer_stop;
> +    s->end_transfer_func = etf;

Please keep using full names so the code is easier to understand:
s/etc/end_transfer_func/

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH v2 0/6] ide: fix atapi software reset
  2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
                   ` (6 preceding siblings ...)
  2016-01-26 20:39 ` [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
@ 2016-02-08 16:10 ` Stefan Hajnoczi
  7 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2016-02-08 16:10 UTC (permalink / raw)
  To: John Snow; +Cc: kwolf, pbonzini, stefanha, qemu-devel, qemu-block

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

On Tue, Jan 19, 2016 at 12:39:45PM -0500, John Snow wrote:
> The ATAPI software reset function is implemented somewhat lackadaisically.
> 
> Firstly, it is valid only for ATAPI drives - not HDs. If a HD should
> receive this command while BSY, it should be ignored like any other
> command instead of aborted. A non-BSY HD is free to abort the command
> in the usual fashion to indicate it doesn't understand or doesn't support
> that command.
> 
> Second, for drives that should accept a software reset, they should not
> "forget" about all pending AIO during the reset. Since a software reset
> resets the DRQ and BSY flags, it is possible to 'stack' multiple
> concurrent reads using DMA and alternately chaining software reset and
> DMA reads. We mustn't reset BSY/DRQ until we are confident that we
> have canceled existing AIO.
> 
> Third, the existing software reset routine does not perform a very
> rigorous reset.
> 
> This series corrects this by:
> 
> (1) Correcting ide_exec_cmd to correctly ignore, not abort, software
>     reset commands for ide-hd devices that are busy executing a command.
> 
> (2) Improving the software reset routine to cancel buffered DMA, then
>     fall back to synchronously waiting for any pending DMA to finish
>     before returning, insuring that the reset completes sanely.
> 
> (3) Use existing reset routines to comprehensively reset the device.
> 
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ide-reset-fix
> https://github.com/jnsnow/qemu/tree/ide-reset-fix
> 
> This version is tagged ide-reset-fix-v2:
> https://github.com/jnsnow/qemu/releases/tag/ide-reset-fix-v2
> 
> John Snow (6):
>   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
> 
>  hw/ide/core.c     | 215 ++++++++++++++++++++++++++++++++++++------------------
>  hw/ide/internal.h |   1 +
>  hw/ide/pci.c      |  36 +--------
>  3 files changed, 144 insertions(+), 108 deletions(-)
> 
> -- 
> 2.4.3
> 
> 

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH v2 5/6] ide: Add silent DRQ cancellation
  2016-02-08 16:09   ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-02-08 17:08     ` John Snow
  2016-02-09 13:21       ` Stefan Hajnoczi
  0 siblings, 1 reply; 12+ messages in thread
From: John Snow @ 2016-02-08 17:08 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, qemu-block, qemu-devel, stefanha



On 02/08/2016 11:09 AM, Stefan Hajnoczi wrote:
> On Tue, Jan 19, 2016 at 12:39:50PM -0500, John Snow wrote:
>> 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>
>> ---
>>  hw/ide/core.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index cf0b5ec..9bc8e58 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -486,13 +486,26 @@ static void ide_cmd_done(IDEState *s)
>>      }
>>  }
>>  
>> -void ide_transfer_stop(IDEState *s)
>> +static void ide_transfer_halt(IDEState *s, void(*etf)(IDEState *), bool notify)
>>  {
>> -    s->end_transfer_func = ide_transfer_stop;
>> +    s->end_transfer_func = etf;
> 
> Please keep using full names so the code is easier to understand:
> s/etc/end_transfer_func/
> 

If this is the only feedback, I'll just clean this up in the PR, thanks.

--js

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH v2 5/6] ide: Add silent DRQ cancellation
  2016-02-08 17:08     ` John Snow
@ 2016-02-09 13:21       ` Stefan Hajnoczi
  0 siblings, 0 replies; 12+ messages in thread
From: Stefan Hajnoczi @ 2016-02-09 13:21 UTC (permalink / raw)
  To: John Snow; +Cc: kwolf, Stefan Hajnoczi, qemu-devel, qemu-block, pbonzini

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

On Mon, Feb 08, 2016 at 12:08:08PM -0500, John Snow wrote:
> 
> 
> On 02/08/2016 11:09 AM, Stefan Hajnoczi wrote:
> > On Tue, Jan 19, 2016 at 12:39:50PM -0500, John Snow wrote:
> >> 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>
> >> ---
> >>  hw/ide/core.c | 19 ++++++++++++++++---
> >>  1 file changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/ide/core.c b/hw/ide/core.c
> >> index cf0b5ec..9bc8e58 100644
> >> --- a/hw/ide/core.c
> >> +++ b/hw/ide/core.c
> >> @@ -486,13 +486,26 @@ static void ide_cmd_done(IDEState *s)
> >>      }
> >>  }
> >>  
> >> -void ide_transfer_stop(IDEState *s)
> >> +static void ide_transfer_halt(IDEState *s, void(*etf)(IDEState *), bool notify)
> >>  {
> >> -    s->end_transfer_func = ide_transfer_stop;
> >> +    s->end_transfer_func = etf;
> > 
> > Please keep using full names so the code is easier to understand:
> > s/etc/end_transfer_func/
> > 
> 
> If this is the only feedback, I'll just clean this up in the PR, thanks.

Yes, that's fine.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-02-09 13:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-19 17:39 [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 1/6] ide: Prohibit RESET on IDE drives John Snow
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 2/6] ide: code motion John Snow
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 3/6] ide: move buffered DMA cancel to core John Snow
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 4/6] ide: replace blk_drain_all by blk_drain John Snow
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 5/6] ide: Add silent DRQ cancellation John Snow
2016-02-08 16:09   ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-02-08 17:08     ` John Snow
2016-02-09 13:21       ` Stefan Hajnoczi
2016-01-19 17:39 ` [Qemu-devel] [PATCH v2 6/6] ide: fix device_reset to not ignore pending AIO John Snow
2016-01-26 20:39 ` [Qemu-devel] [PATCH v2 0/6] ide: fix atapi software reset John Snow
2016-02-08 16:10 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).