qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes
@ 2014-10-31 20:03 John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 1/3] ide: repair PIO transfers for cases where nsector > 1 John Snow
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: John Snow @ 2014-10-31 20:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, John Snow, armbru, stefanha

This is a v2 patch series that collects patches 3, 4 and 5
from the v1 series that was only partially merged.

This series, as a subset, generally cleans up the usage and
handling of the scatter-gather list implementation in QEMU
that interacts with the PRDT (Physical Region Descriptor Table)
in AHCI.

We also repair a small oversight with PIO transfers.

This series currently applies cleanly to stefanha's block staging
tree, but can be trivially merged to master.

v2:
- Reset io_buffer_offset to 0 at the beginning of ide_exec_cmd,
  which will reset it for PIO I/O and all ATAPI commands.
  There was an oversight in V1 where we would use junk values for
  PACKET IDENTIFY if it was not the first command.

- Adjust the prepare_buf() callback to return, simply, the number
  of bytes it was able to prepare, or -1 on error. This resolves
  an ambiguity around the nature of its return code meaning
  "no sectors" or "no bytes" which led to bad error pathways.

John Snow (3):
  ide: repair PIO transfers for cases where nsector > 1
  ahci: unify sglist preparation
  ide: Correct handling of malformed/short PRDTs

 hw/ide/ahci.c     | 33 ++++++++++++++++++++++++++-------
 hw/ide/core.c     | 14 ++++++++++++--
 hw/ide/internal.h | 13 +++++++------
 hw/ide/macio.c    |  7 ++++++-
 hw/ide/pci.c      | 27 +++++++++++++++++++++------
 5 files changed, 72 insertions(+), 22 deletions(-)

-- 
1.9.3

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

* [Qemu-devel] [block PATCH v2 1/3] ide: repair PIO transfers for cases where nsector > 1
  2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
@ 2014-10-31 20:03 ` John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 2/3] ahci: unify sglist preparation John Snow
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2014-10-31 20:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, John Snow, armbru, stefanha

Currently, for emulated PIO transfers through the AHCI device,
any attempt made to request more than a single sector's worth
of data will result in the same sector being transferred over
and over.

For example, if we request 8 sectors via PIO READ SECTORS, the
AHCI device will give us the same sector eight times.

This patch adds offset tracking into the PIO pathways so that
we can fulfill these requests appropriately.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 61dbed1..3d9b018 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1092,7 +1092,7 @@ static void ahci_start_transfer(IDEDMA *dma)
         goto out;
     }
 
-    if (!ahci_populate_sglist(ad, &s->sg, 0)) {
+    if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
         has_sglist = 1;
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index d316ccf..dab21f0 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -592,6 +592,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
 
     ide_set_sector(s, ide_get_sector(s) + n);
     s->nsector -= n;
+    s->io_buffer_offset += 512 * n;
 }
 
 void ide_sector_read(IDEState *s)
@@ -832,6 +833,8 @@ static void ide_sector_write_cb(void *opaque, int ret)
         n = s->req_nb_sectors;
     }
     s->nsector -= n;
+    s->io_buffer_offset += 512 * n;
+
     if (s->nsector == 0) {
         /* no more sectors to write */
         ide_transfer_stop(s);
@@ -1824,6 +1827,7 @@ void ide_exec_cmd(IDEBus *bus, uint32_t val)
 
     s->status = READY_STAT | BUSY_STAT;
     s->error = 0;
+    s->io_buffer_offset = 0;
 
     complete = ide_cmd_table[val].handler(s, val);
     if (complete) {
-- 
1.9.3

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

* [Qemu-devel] [block PATCH v2 2/3] ahci: unify sglist preparation
  2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 1/3] ide: repair PIO transfers for cases where nsector > 1 John Snow
@ 2014-10-31 20:03 ` John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 3/3] ide: Correct handling of malformed/short PRDTs John Snow
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2014-10-31 20:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, John Snow, armbru, stefanha

The intent of this patch is to further unify the creation and
deletion of the sglist used for all AHCI transfers, including
emulated PIO, ATAPI R/W, and native DMA R/W.

By replacing ahci_start_transfer's call to ahci_populate_sglist
with ahci_dma_prepare_buf, we reduce the number of direct calls
where we manipulate the scatter-gather list in the AHCI code.

To make this switch, the constant "0" passed as an offset
in ahci_dma_prepare_buf is adjusted to use io_buffer_offset.

For DMA pathways, this has no effect: io_buffer_offset is always
updated to 0 at the beginning of a DMA transfer loop regardless.
DMA pathways through ide_dma_cb() update the io_buffer_offset
accordingly, and for circumstances where we might make several
trips through this loop, this may actually correct a design flaw.

For PIO pathways, the newly updated ahci_dma_prepare_buf will
now prepare the sglist at the correct offset. It will also set
io_buffer_size, but this is not used in the cmd_read_pio or
cmd_write_pio pathways.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3d9b018..ff7e225 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1092,7 +1092,7 @@ static void ahci_start_transfer(IDEDMA *dma)
         goto out;
     }
 
-    if (!ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset)) {
+    if (ahci_dma_prepare_buf(dma, is_write)) {
         has_sglist = 1;
     }
 
@@ -1144,7 +1144,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
 
-    ahci_populate_sglist(ad, &s->sg, 0);
+    ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset);
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-- 
1.9.3

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

* [Qemu-devel] [block PATCH v2 3/3] ide: Correct handling of malformed/short PRDTs
  2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 1/3] ide: repair PIO transfers for cases where nsector > 1 John Snow
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 2/3] ahci: unify sglist preparation John Snow
@ 2014-10-31 20:03 ` John Snow
  2014-11-03 12:35 ` [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes Paolo Bonzini
  2014-11-13 10:04 ` Stefan Hajnoczi
  4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2014-10-31 20:03 UTC (permalink / raw)
  To: qemu-devel; +Cc: pbonzini, John Snow, armbru, stefanha

This impacts both BMDMA and AHCI HBA interfaces for IDE.
Currently, we confuse the difference between a PRDT having
"0 bytes" and a PRDT having "0 complete sectors."

When we receive an incomplete sector, inconsistent error checking
leads to an infinite loop wherein the call succeeds, but it
didn't give us enough bytes -- leading us to re-call the
DMA chain over and over again. This leads to, in the BMDMA case,
leaked memory for short PRDTs, and infinite loops and resource
usage in the AHCI case.

The .prepare_buf() callback is reworked to return the number of
bytes that it successfully prepared. 0 is a valid, non-error
answer that means the table was empty and described no bytes.
-1 indicates an error.

Our current implementation uses the io_buffer in IDEState to
ultimately describe the size of a prepared scatter-gather list.
Even though the AHCI PRDT/SGList can be as large as 256GiB, the
AHCI command header limits transactions to just 4GiB. ATA8-ACS3,
however, defines the largest transaction to be an LBA48 command
that transfers 65,536 sectors. With a 512 byte sector size, this
is just 32MiB.

Since our current state structures use the int type to describe
the size of the buffer, and this state is migrated as int32, we
are limited to describing 2GiB buffer sizes unless we change the
migration protocol.

For this reason, this patch begins to unify the assertions in the
IDE pathways that the scatter-gather list provided by either the
AHCI PRDT or the PCI BMDMA PRDs can only describe, at a maximum,
2GiB. This should be resilient enough unless we need a sector
size that exceeds 32KiB.

Further, the likelihood of any guest operating system actually
attempting to transfer this much data in a single operation is
very slim.

To this end, the IDEState variables have been updated to more
explicitly clarify our maximum supported size. Callers to the
prepare_buf callback have been reworked to understand the new
return code, and all versions of the prepare_buf callback have
been adjusted accordingly.

Lastly, the ahci_populate_sglist helper, relied upon by the
AHCI implementation of .prepare_buf() as well as the PCI
implementation of the callback have had overflow assertions
added to help make clear the reasonings behind the various
type changes.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 31 +++++++++++++++++++++++++------
 hw/ide/core.c     | 10 ++++++++--
 hw/ide/internal.h | 13 +++++++------
 hw/ide/macio.c    |  7 ++++++-
 hw/ide/pci.c      | 27 +++++++++++++++++++++------
 5 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index ff7e225..27f078d 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -730,7 +730,8 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl)
     return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
 }
 
-static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
+static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
+                                int32_t offset)
 {
     AHCICmdHdr *cmd = ad->cur_cmd;
     uint32_t opts = le32_to_cpu(cmd->opts);
@@ -741,13 +742,21 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
     uint8_t *prdt;
     int i;
     int r = 0;
-    int sum = 0;
+    uint64_t sum = 0;
     int off_idx = -1;
-    int off_pos = -1;
+    int64_t off_pos = -1;
     int tbl_entry_size;
     IDEBus *bus = &ad->port;
     BusState *qbus = BUS(bus);
 
+    /*
+     * Note: AHCI PRDT can describe up to 256GiB. SATA/ATA only support
+     * transactions of up to 32MiB as of ATA8-ACS3 rev 1b, assuming a
+     * 512 byte sector size. We limit the PRDT in this implementation to
+     * a reasonably large 2GiB, which can accommodate the maximum transfer
+     * request for sector sizes up to 32K.
+     */
+
     if (!sglist_alloc_hint) {
         DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);
         return -1;
@@ -797,6 +806,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
             /* flags_size is zero-based */
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
                             prdt_tbl_entry_size(&tbl[i]));
+            if (sglist->size > INT32_MAX) {
+                error_report("AHCI Physical Region Descriptor Table describes "
+                             "more than 2 GiB.\n");
+                qemu_sglist_destroy(sglist);
+                r = -1;
+                goto out;
+            }
         }
     }
 
@@ -1139,16 +1155,19 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
  * Not currently invoked by PIO R/W chains,
  * which invoke ahci_populate_sglist via ahci_start_transfer.
  */
-static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
+static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
 
-    ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset);
+    if (ahci_populate_sglist(ad, &s->sg, s->io_buffer_offset) == -1) {
+        DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n");
+        return -1;
+    }
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-    return s->io_buffer_size != 0;
+    return s->io_buffer_size;
 }
 
 /**
diff --git a/hw/ide/core.c b/hw/ide/core.c
index dab21f0..00e21cf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -731,10 +731,11 @@ void ide_dma_cb(void *opaque, int ret)
     n = s->nsector;
     s->io_buffer_index = 0;
     s->io_buffer_size = n * 512;
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) == 0) {
+    if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) {
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
         s->status = READY_STAT | SEEK_STAT;
+        dma_buf_commit(s, 0);
         goto eot;
     }
 
@@ -2313,12 +2314,17 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
+static int32_t ide_nop_int32(IDEDMA *dma, int x)
+{
+    return 0;
+}
+
 static void ide_nop_restart(void *opaque, int x, RunState y)
 {
 }
 
 static const IDEDMAOps ide_dma_nop_ops = {
-    .prepare_buf    = ide_nop_int,
+    .prepare_buf    = ide_nop_int32,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .restart_cb     = ide_nop_restart,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 907493d..8a3eca4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -322,6 +322,7 @@ typedef void EndTransferFunc(IDEState *);
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
+typedef int32_t DMAInt32Func(IDEDMA *, int);
 typedef void DMAu32Func(IDEDMA *, uint32_t);
 typedef void DMAStopFunc(IDEDMA *, bool);
 typedef void DMARestartFunc(void *, int, RunState);
@@ -385,7 +386,7 @@ struct IDEState {
     uint8_t cdrom_changed;
     int packet_transfer_size;
     int elementary_transfer_size;
-    int io_buffer_index;
+    int32_t io_buffer_index;
     int lba;
     int cd_sector_size;
     int atapi_dma; /* true if dma is requested for the packet cmd */
@@ -394,8 +395,8 @@ struct IDEState {
     struct iovec iov;
     QEMUIOVector qiov;
     /* ATA DMA state */
-    int io_buffer_offset;
-    int io_buffer_size;
+    int32_t io_buffer_offset;
+    int32_t io_buffer_size;
     QEMUSGList sg;
     /* PIO transfer handling */
     int req_nb_sectors; /* number of sectors per interrupt */
@@ -405,8 +406,8 @@ struct IDEState {
     uint8_t *io_buffer;
     /* PIO save/restore */
     int32_t io_buffer_total_len;
-    int cur_io_buffer_offset;
-    int cur_io_buffer_len;
+    int32_t cur_io_buffer_offset;
+    int32_t cur_io_buffer_len;
     uint8_t end_transfer_fn_idx;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
@@ -430,7 +431,7 @@ struct IDEState {
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
     DMAVoidFunc *start_transfer;
-    DMAIntFunc *prepare_buf;
+    DMAInt32Func *prepare_buf;
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 9a55407..f6074f2 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -553,6 +553,11 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
+static int32_t ide_nop_int32(IDEDMA *dma, int x)
+{
+    return 0;
+}
+
 static void ide_nop_restart(void *opaque, int x, RunState y)
 {
 }
@@ -569,7 +574,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
 
 static const IDEDMAOps dbdma_ops = {
     .start_dma      = ide_dbdma_start,
-    .prepare_buf    = ide_nop_int,
+    .prepare_buf    = ide_nop_int32,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .restart_cb     = ide_nop_restart,
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 2dad50e..bee5ad3 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -28,7 +28,7 @@
 #include <hw/isa/isa.h>
 #include "sysemu/block-backend.h"
 #include "sysemu/dma.h"
-
+#include "qemu/error-report.h"
 #include <hw/ide/pci.h>
 
 #define BMDMA_PAGE_SIZE 4096
@@ -55,8 +55,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
     }
 }
 
-/* return 0 if buffer completed */
-static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
+/**
+ * Return the number of bytes successfully prepared.
+ * -1 on error.
+ */
+static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
     IDEState *s = bmdma_active_if(bm);
@@ -74,8 +77,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
         if (bm->cur_prd_len == 0) {
             /* end of table (with a fail safe of one page) */
             if (bm->cur_prd_last ||
-                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
-                return s->io_buffer_size != 0;
+                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
+                return s->io_buffer_size;
+            }
             pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
             bm->cur_addr += 8;
             prd.addr = le32_to_cpu(prd.addr);
@@ -90,12 +94,23 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
         l = bm->cur_prd_len;
         if (l > 0) {
             qemu_sglist_add(&s->sg, bm->cur_prd_addr, l);
+
+            /* Note: We limit the max transfer to be 2GiB.
+             * This should accommodate the largest ATA transaction
+             * for LBA48 (65,536 sectors) and 32K sector sizes. */
+            if (s->sg.size > INT32_MAX) {
+                error_report("IDE: sglist describes more than 2GiB.\n");
+                break;
+            }
             bm->cur_prd_addr += l;
             bm->cur_prd_len -= l;
             s->io_buffer_size += l;
         }
     }
-    return 1;
+
+    qemu_sglist_destroy(&s->sg);
+    s->io_buffer_size = 0;
+    return -1;
 }
 
 /* return 0 if buffer completed */
-- 
1.9.3

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

* Re: [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes
  2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
                   ` (2 preceding siblings ...)
  2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 3/3] ide: Correct handling of malformed/short PRDTs John Snow
@ 2014-11-03 12:35 ` Paolo Bonzini
  2014-11-03 19:51   ` John Snow
  2014-11-13 10:04 ` Stefan Hajnoczi
  4 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2014-11-03 12:35 UTC (permalink / raw)
  To: John Snow, qemu-devel; +Cc: armbru, stefanha

On 31/10/2014 21:03, John Snow wrote:
> - Adjust the prepare_buf() callback to return, simply, the number
>   of bytes it was able to prepare, or -1 on error. This resolves
>   an ambiguity around the nature of its return code meaning
>   "no sectors" or "no bytes" which led to bad error pathways.

Much nicer!  Sounds obvious, but hindsight is 20/20...

> John Snow (3):
>   ide: repair PIO transfers for cases where nsector > 1
>   ahci: unify sglist preparation
>   ide: Correct handling of malformed/short PRDTs
> 
>  hw/ide/ahci.c     | 33 ++++++++++++++++++++++++++-------
>  hw/ide/core.c     | 14 ++++++++++++--
>  hw/ide/internal.h | 13 +++++++------
>  hw/ide/macio.c    |  7 ++++++-
>  hw/ide/pci.c      | 27 +++++++++++++++++++++------
>  5 files changed, 72 insertions(+), 22 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

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

* Re: [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes
  2014-11-03 12:35 ` [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes Paolo Bonzini
@ 2014-11-03 19:51   ` John Snow
  0 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2014-11-03 19:51 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf >> Kevin Wolf, pbonzini >> Paolo Bonzini,
	armbru >> Markus Armbruster, stefan Hajnoczi

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



On 11/03/2014 07:35 AM, Paolo Bonzini wrote:
> On 31/10/2014 21:03, John Snow wrote:
>> - Adjust the prepare_buf() callback to return, simply, the number
>>    of bytes it was able to prepare, or -1 on error. This resolves
>>    an ambiguity around the nature of its return code meaning
>>    "no sectors" or "no bytes" which led to bad error pathways.
>
> Much nicer!  Sounds obvious, but hindsight is 20/20...
>
>> John Snow (3):
>>    ide: repair PIO transfers for cases where nsector > 1
>>    ahci: unify sglist preparation
>>    ide: Correct handling of malformed/short PRDTs
>>
>>   hw/ide/ahci.c     | 33 ++++++++++++++++++++++++++-------
>>   hw/ide/core.c     | 14 ++++++++++++--
>>   hw/ide/internal.h | 13 +++++++------
>>   hw/ide/macio.c    |  7 ++++++-
>>   hw/ide/pci.c      | 27 +++++++++++++++++++++------
>>   5 files changed, 72 insertions(+), 22 deletions(-)
>>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Paolo
>

There is a trivial fix to be made in hw/ide/ahci.c:

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 27f078d..f49be9f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -791,7 +791,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, 
QEMUSGList *sglist,
          }
          if ((off_idx == -1) || (off_pos < 0) || (off_pos > 
tbl_entry_size)) {
              DPRINTF(ad->port_no, "%s: Incorrect offset! "
-                            "off_idx: %d, off_pos: %d\n",
+                            "off_idx: %d, off_pos: %"PRId64"\n",
                              __func__, off_idx, off_pos);
              r = -1;
              goto out;

Missed this one because it's under a debugging #ifdef. I will fix it 
myself if a v3 is warranted otherwise.

--js

[-- Attachment #2: trivial.diff --]
[-- Type: text/x-patch, Size: 577 bytes --]

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 27f078d..f49be9f 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -791,7 +791,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
         }
         if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
             DPRINTF(ad->port_no, "%s: Incorrect offset! "
-                            "off_idx: %d, off_pos: %d\n",
+                            "off_idx: %d, off_pos: %"PRId64"\n",
                             __func__, off_idx, off_pos);
             r = -1;
             goto out;

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

* Re: [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes
  2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
                   ` (3 preceding siblings ...)
  2014-11-03 12:35 ` [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes Paolo Bonzini
@ 2014-11-13 10:04 ` Stefan Hajnoczi
  4 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2014-11-13 10:04 UTC (permalink / raw)
  To: John Snow; +Cc: pbonzini, qemu-devel, stefanha, armbru

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

On Fri, Oct 31, 2014 at 04:03:36PM -0400, John Snow wrote:
> This is a v2 patch series that collects patches 3, 4 and 5
> from the v1 series that was only partially merged.
> 
> This series, as a subset, generally cleans up the usage and
> handling of the scatter-gather list implementation in QEMU
> that interacts with the PRDT (Physical Region Descriptor Table)
> in AHCI.
> 
> We also repair a small oversight with PIO transfers.
> 
> This series currently applies cleanly to stefanha's block staging
> tree, but can be trivially merged to master.
> 
> v2:
> - Reset io_buffer_offset to 0 at the beginning of ide_exec_cmd,
>   which will reset it for PIO I/O and all ATAPI commands.
>   There was an oversight in V1 where we would use junk values for
>   PACKET IDENTIFY if it was not the first command.
> 
> - Adjust the prepare_buf() callback to return, simply, the number
>   of bytes it was able to prepare, or -1 on error. This resolves
>   an ambiguity around the nature of its return code meaning
>   "no sectors" or "no bytes" which led to bad error pathways.
> 
> John Snow (3):
>   ide: repair PIO transfers for cases where nsector > 1
>   ahci: unify sglist preparation
>   ide: Correct handling of malformed/short PRDTs
> 
>  hw/ide/ahci.c     | 33 ++++++++++++++++++++++++++-------
>  hw/ide/core.c     | 14 ++++++++++++--
>  hw/ide/internal.h | 13 +++++++------
>  hw/ide/macio.c    |  7 ++++++-
>  hw/ide/pci.c      | 27 +++++++++++++++++++++------
>  5 files changed, 72 insertions(+), 22 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

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

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

end of thread, other threads:[~2014-11-13 10:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-31 20:03 [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes John Snow
2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 1/3] ide: repair PIO transfers for cases where nsector > 1 John Snow
2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 2/3] ahci: unify sglist preparation John Snow
2014-10-31 20:03 ` [Qemu-devel] [block PATCH v2 3/3] ide: Correct handling of malformed/short PRDTs John Snow
2014-11-03 12:35 ` [Qemu-devel] [block PATCH v2 0/3] AHCI device fixes Paolo Bonzini
2014-11-03 19:51   ` John Snow
2014-11-13 10:04 ` 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).