[Qemu-devel] [PATCH for-2.3 0/4] ahci: fix big endian PIO failures

[Qemu-devel] [PATCH for-2.3 0/4] ahci: fix big endian PIO failures

[John Snow]

Two issues were unearthed from ahci-test on ppc64:

(1) The ahci_populate_sglist function had endian issues,
    which is only likely to impact PIO transfers for buffers
    greater than one sector, and

(2) multiple-sector PIO which I attempted to repair in
    36334faf has been broken for years. ahci-test didn't catch
    this because it used a pattern that was identical for each
    sector.

    So the pattern has been corrected and the underlying issue
    fixed.

This should clear up the test failures (properly) for ppc64.

John Snow (4):
  ide: fix cmd_write_pio when nsectors > 1
  ide: fix cmd_read_pio when nsectors > 1
  ahci: Fix sglist offset manipulation for BE machines
  ahci-test: improve rw buffer patterns

 hw/ide/ahci.c     |  2 +-
 hw/ide/core.c     | 10 ++++------
 tests/ahci-test.c | 36 ++++++++++++++++++++++++++++--------
 3 files changed, 33 insertions(+), 15 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH for-2.3 1/4] ide: fix cmd_write_pio when nsectors > 1

[John Snow]

We need to adjust the sector being written to
prior to calling ide_transfer_start, otherwise
we'll write to the same sector again.

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 ef52f35..0e9da64 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -846,6 +846,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
     s->nsector -= n;
     s->io_buffer_offset += 512 * n;
 
+    ide_set_sector(s, ide_get_sector(s) + n);
     if (s->nsector == 0) {
         /* no more sectors to write */
         ide_transfer_stop(s);
@@ -857,7 +858,6 @@ static void ide_sector_write_cb(void *opaque, int ret)
         ide_transfer_start(s, s->io_buffer, n1 * BDRV_SECTOR_SIZE,
                            ide_sector_write);
     }
-    ide_set_sector(s, ide_get_sector(s) + n);
 
     if (win2k_install_hack && ((++s->irq_count % 16) == 0)) {
         /* It seems there is a bug in the Windows 2000 installer HDD
-- 
2.1.0

[Qemu-devel] [PATCH for-2.3 2/4] ide: fix cmd_read_pio when nsectors > 1

[John Snow]

Similar to the cmd_write_pio fix, update the nsector count and
ide sector before we invoke ide_transfer_start.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0e9da64..a895fd8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -587,14 +587,12 @@ static void ide_sector_read_cb(void *opaque, int ret)
         n = s->req_nb_sectors;
     }
 
+    ide_set_sector(s, ide_get_sector(s) + n);
+    s->nsector -= n;
     /* Allow the guest to read the io_buffer */
     ide_transfer_start(s, s->io_buffer, n * BDRV_SECTOR_SIZE, ide_sector_read);
-
-    ide_set_irq(s->bus);
-
-    ide_set_sector(s, ide_get_sector(s) + n);
-    s->nsector -= n;
     s->io_buffer_offset += 512 * n;
+    ide_set_irq(s->bus);
 }
 
 static void ide_sector_read(IDEState *s)
-- 
2.1.0

[Qemu-devel] [PATCH for-2.3 3/4] ahci: Fix sglist offset manipulation for BE machines

[John Snow]

This does not bother DMA, because DMA generally transfers
the entire SGList in one shot if it can.

PIO, on the other hand, tries to transfer just one sector
at a time, and will make multiple visits to the sglist
to fetch memory addresses.

Fix the memory address calculaton when we have an offset
by moving the offset addition OUTSIDE of the le64_to_cpu
calculation.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e1ae36f..7a223be 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -799,7 +799,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
 
         qemu_sglist_init(sglist, qbus->parent, (sglist_alloc_hint - off_idx),
                          ad->hba->as);
-        qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
+        qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr) + off_pos,
                         prdt_tbl_entry_size(&tbl[off_idx]) - off_pos);
 
         for (i = off_idx + 1; i < sglist_alloc_hint; i++) {
-- 
2.1.0

[Qemu-devel] [PATCH for-2.3 4/4] ahci-test: improve rw buffer patterns

[John Snow]

My pattern was cyclical every 256 bytes, so it missed a fairly obvious
failure case. Add some rand() pepper into the test pattern, and for large
patterns that exceed 256 sectors, start writing an ID per-sector so that
we never generate identical sector patterns.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ahci-test.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 169e83b..ea62e24 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -68,6 +68,32 @@ static void string_bswap16(uint16_t *s, size_t bytes)
     }
 }
 
+static void generate_pattern(void *buffer, size_t len, size_t cycle_len)
+{
+    int i, j;
+    unsigned char *tx = (unsigned char *)buffer;
+    unsigned char p;
+    size_t *sx;
+
+    /* Write an indicative pattern that varies and is unique per-cycle */
+    p = rand() % 256;
+    for (i = j = 0; i < len; i++, j++) {
+        tx[i] = p;
+        if (j % cycle_len == 0) {
+            p = rand() % 256;
+        }
+    }
+
+    /* force uniqueness by writing an id per-cycle */
+    for (i = 0; i < len / cycle_len; i++) {
+        j = i * cycle_len;
+        if (j + sizeof(*sx) <= len) {
+            sx = (size_t *)&tx[j];
+            *sx = i;
+        }
+    }
+}
+
 /*** Test Setup & Teardown ***/
 
 /**
@@ -736,7 +762,6 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
 {
     uint64_t ptr;
     uint8_t port;
-    unsigned i;
     unsigned char *tx = g_malloc(bufsize);
     unsigned char *rx = g_malloc0(bufsize);
 
@@ -752,9 +777,7 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
     g_assert(ptr);
 
     /* Write some indicative pattern to our buffer. */
-    for (i = 0; i < bufsize; i++) {
-        tx[i] = (bufsize - i);
-    }
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
     memwrite(ptr, tx, bufsize);
 
     /* Write this buffer to disk, then read it back to the DMA buffer. */
@@ -865,7 +888,6 @@ static void test_dma_fragmented(void)
     size_t bufsize = 4096;
     unsigned char *tx = g_malloc(bufsize);
     unsigned char *rx = g_malloc0(bufsize);
-    unsigned i;
     uint64_t ptr;
 
     ahci = ahci_boot_and_enable();
@@ -873,9 +895,7 @@ static void test_dma_fragmented(void)
     ahci_port_clear(ahci, px);
 
     /* create pattern */
-    for (i = 0; i < bufsize; i++) {
-        tx[i] = (bufsize - i);
-    }
+    generate_pattern(tx, bufsize, AHCI_SECTOR_SIZE);
 
     /* Create a DMA buffer in guest memory, and write our pattern to it. */
     ptr = guest_alloc(ahci->parent->alloc, bufsize);
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.3 0/4] ahci: fix big endian PIO failures

[Andreas Färber]

Am 20.03.2015 um 01:24 schrieb John Snow:
> Two issues were unearthed from ahci-test on ppc64:
> 
> (1) The ahci_populate_sglist function had endian issues,
>     which is only likely to impact PIO transfers for buffers
>     greater than one sector, and
> 
> (2) multiple-sector PIO which I attempted to repair in
>     36334faf has been broken for years. ahci-test didn't catch
>     this because it used a pattern that was identical for each
>     sector.
> 
>     So the pattern has been corrected and the underlying issue
>     fixed.
> 
> This should clear up the test failures (properly) for ppc64.
> 
> John Snow (4):
>   ide: fix cmd_write_pio when nsectors > 1
>   ide: fix cmd_read_pio when nsectors > 1
>   ahci: Fix sglist offset manipulation for BE machines
>   ahci-test: improve rw buffer patterns

Confirming that it resolves the observed failure,

Tested-by: Andreas Färber <afaerber@suse.de>

Thanks for the quick fix,

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.3 0/4] ahci: fix big endian PIO failures

[Stefan Hajnoczi]

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

On Thu, Mar 19, 2015 at 08:24:12PM -0400, John Snow wrote:
> Two issues were unearthed from ahci-test on ppc64:
> 
> (1) The ahci_populate_sglist function had endian issues,
>     which is only likely to impact PIO transfers for buffers
>     greater than one sector, and
> 
> (2) multiple-sector PIO which I attempted to repair in
>     36334faf has been broken for years. ahci-test didn't catch
>     this because it used a pattern that was identical for each
>     sector.
> 
>     So the pattern has been corrected and the underlying issue
>     fixed.
> 
> This should clear up the test failures (properly) for ppc64.
> 
> John Snow (4):
>   ide: fix cmd_write_pio when nsectors > 1
>   ide: fix cmd_read_pio when nsectors > 1
>   ahci: Fix sglist offset manipulation for BE machines
>   ahci-test: improve rw buffer patterns
> 
>  hw/ide/ahci.c     |  2 +-
>  hw/ide/core.c     | 10 ++++------
>  tests/ahci-test.c | 36 ++++++++++++++++++++++++++++--------
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 
> -- 
> 2.1.0
> 

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

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