[Qemu-devel] [PATCH 0/4] Ide patches for 2.3-rc1

[Qemu-devel] [PATCH 0/4] Ide patches for 2.3-rc1

[John Snow]

The following changes since commit 3c6c9fe034c0c07b77f272e4a53d7735220a16a4:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2015-03-20 12:26:09 +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 54fced034e4d32d8ba6d1e27ecb7e2e2fb2f45d4:

  ahci-test: improve rw buffer patterns (2015-03-23 12:24:16 -0400)

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

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

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 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Andreas Färber <afaerber@suse.de>
Message-id: 1426811056-2202-2-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 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 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Andreas Färber <afaerber@suse.de>
Message-id: 1426811056-2202-3-git-send-email-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 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Andreas Färber <afaerber@suse.de>
Message-id: 1426811056-2202-4-git-send-email-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 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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Andreas Färber <afaerber@suse.de>
Message-id: 1426811056-2202-5-git-send-email-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 4/4] ahci-test: improve rw buffer patterns

[Ed Maste]

On 23 March 2015 at 12:56, John Snow <jsnow@redhat.com> wrote:
> 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Tested-by: Andreas Färber <afaerber@suse.de>
> Message-id: 1426811056-2202-5-git-send-email-jsnow@redhat.com
> ---
>  tests/ahci-test.c | 36 ++++++++++++++++++++++++++++--------
>  1 file changed, 28 insertions(+), 8 deletions(-)

As of the introduction of this test the test suite fails on FreeBSD,
with 6 errors of the form:

ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
(memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
GTester: last random seed: R02S76f68c4cfb2d54af66de227ed44fd54a

I haven't yet done any investigation beyond bisecting.

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

[John Snow]

On 03/31/2015 04:55 PM, Ed Maste wrote:
> On 23 March 2015 at 12:56, John Snow <jsnow@redhat.com> wrote:
>> 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>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Tested-by: Andreas Färber <afaerber@suse.de>
>> Message-id: 1426811056-2202-5-git-send-email-jsnow@redhat.com
>> ---
>>   tests/ahci-test.c | 36 ++++++++++++++++++++++++++++--------
>>   1 file changed, 28 insertions(+), 8 deletions(-)
>
> As of the introduction of this test the test suite fails on FreeBSD,
> with 6 errors of the form:
>
> ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
> (memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
> GTester: last random seed: R02S76f68c4cfb2d54af66de227ed44fd54a
>
> I haven't yet done any investigation beyond bisecting.
>

which test case does it fail under? io_rw_simple is shared by a number 
of different tests.

I suspect deeply that this test pattern has just unearthed an existing 
problem.

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

[Ed Maste]

On 31 March 2015 at 16:58, John Snow <jsnow@redhat.com> wrote:
>
> which test case does it fail under? io_rw_simple is shared by a number of
> different tests.

I'm not sure off hand how to run an individual test to confirm, but
the testrun output up to the failure is:

  LINK  tests/spapr-phb-test
GTESTER check-qtest-aarch64
GTESTER check-qtest-alpha
GTESTER check-qtest-arm
GTESTER check-qtest-cris
GTESTER check-qtest-i386
blkdebug: Suspended request 'A'
blkdebug: Resuming request 'A'
**
ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
(memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
GTester: last random seed: R02S181acd6574060796874e02dbdeb86199

Generally speaking I've only encountered build errors on FreeBSD, and
as long as it built the full test suite passed.

> I suspect deeply that this test pattern has just unearthed an existing
> problem.

Yeah, it's almost certainly just exposing the existing failure.

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

[John Snow]

On 03/31/2015 05:29 PM, Ed Maste wrote:
> On 31 March 2015 at 16:58, John Snow <jsnow@redhat.com> wrote:
>>
>> which test case does it fail under? io_rw_simple is shared by a number of
>> different tests.
>
> I'm not sure off hand how to run an individual test to confirm, but
> the testrun output up to the failure is:
>
>    LINK  tests/spapr-phb-test
> GTESTER check-qtest-aarch64
> GTESTER check-qtest-alpha
> GTESTER check-qtest-arm
> GTESTER check-qtest-cris
> GTESTER check-qtest-i386
> blkdebug: Suspended request 'A'
> blkdebug: Resuming request 'A'
> **
> ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
> (memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
> GTester: last random seed: R02S181acd6574060796874e02dbdeb86199
>
> Generally speaking I've only encountered build errors on FreeBSD, and
> as long as it built the full test suite passed.
>
>> I suspect deeply that this test pattern has just unearthed an existing
>> problem.
>
> Yeah, it's almost certainly just exposing the existing failure.
>

My apologies;

Head into your build directory and try this:

 > make tests/ahci-test
 > export QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
 > export QTEST_QEMU_IMG=./qemu-img
 > ./tests/ahci-test

This should show you which test in particular is failing. I have a hunch 
it will be the PIO, again...

In the meantime, what is your setup? which release, what architecture?

Thank you,
--John

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

[Ed Maste]

On 31 March 2015 at 17:44, John Snow <jsnow@redhat.com> wrote:
>
> My apologies;

Not at all, thanks for your help!

> Head into your build directory and try this:
>
>> make tests/ahci-test
>> export QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
>> export QTEST_QEMU_IMG=./qemu-img
>> ./tests/ahci-test
>
> This should show you which test in particular is failing. I have a hunch it
> will be the PIO, again...

Correct:

feynman% ./tests/ahci-test
/i386/ahci/sanity: OK
/i386/ahci/pci_spec: OK
/i386/ahci/pci_enable: OK
/i386/ahci/hba_spec: OK
/i386/ahci/hba_enable: OK
/i386/ahci/identify: OK
/i386/ahci/io/pio/lba28/simple: **
ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
(memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
zsh: abort (core dumped)  ./tests/ahci-test

> In the meantime, what is your setup? which release, what architecture?

This is the FreeBSD stable/10 branch on amd64 (so part way between
10.1 and what will become 10.2):

FreeBSD feynman 10.1-STABLE FreeBSD 10.1-STABLE #28
r280427+86df2de(stable-10): Thu Mar 26 16:07:47 EDT 2015
emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC
 amd64

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

[John Snow]

On 03/31/2015 05:55 PM, Ed Maste wrote:
> On 31 March 2015 at 17:44, John Snow <jsnow@redhat.com> wrote:
>>
>> My apologies;
>
> Not at all, thanks for your help!
>
>> Head into your build directory and try this:
>>
>>> make tests/ahci-test
>>> export QTEST_QEMU_BINARY=i386-softmmu/qemu-system-i386
>>> export QTEST_QEMU_IMG=./qemu-img
>>> ./tests/ahci-test
>>
>> This should show you which test in particular is failing. I have a hunch it
>> will be the PIO, again...
>
> Correct:
>
> feynman% ./tests/ahci-test
> /i386/ahci/sanity: OK
> /i386/ahci/pci_spec: OK
> /i386/ahci/pci_enable: OK
> /i386/ahci/hba_spec: OK
> /i386/ahci/hba_enable: OK
> /i386/ahci/identify: OK
> /i386/ahci/io/pio/lba28/simple: **
> ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
> (memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
> zsh: abort (core dumped)  ./tests/ahci-test
>
>> In the meantime, what is your setup? which release, what architecture?
>
> This is the FreeBSD stable/10 branch on amd64 (so part way between
> 10.1 and what will become 10.2):
>
> FreeBSD feynman 10.1-STABLE FreeBSD 10.1-STABLE #28
> r280427+86df2de(stable-10): Thu Mar 26 16:07:47 EDT 2015
> emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC
>   amd64
>

Thanks, one more if you have the spare cycles; and then I'll go prepare 
a machine to debug for myself.

./ahci-test -p /i386/ahci/io/pio/lba28/short
./ahci-test -p /i386/ahci/io/pio/lba48/short

quite likely both work for you, while changing short for "simple" "long" 
or "double" probably all fail.

./ahci-test -p /i386/ahci/io/dma/

probably launches a series of tests that also all work correctly.

Thank you for your help,
--John

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

[Ed Maste]

On 31 March 2015 at 17:55, Ed Maste <emaste@freebsd.org> wrote:
>
> feynman% ./tests/ahci-test
> /i386/ahci/sanity: OK
> /i386/ahci/pci_spec: OK
> /i386/ahci/pci_enable: OK
> /i386/ahci/hba_spec: OK
> /i386/ahci/hba_enable: OK
> /i386/ahci/identify: OK
> /i386/ahci/io/pio/lba28/simple: **
> ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
> (memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
> zsh: abort (core dumped)  ./tests/ahci-test

Well, that's rather strange: I had to head out after your last
follow-up, and then when picking this up again from home I ended up
accidentally breaking my build directory and then did a fresh clean
build. Now I can't reproduce the error. Running tests/ahci-test
reports all OK.

So I'm not quite sure what was wrong, but it looks like this is not a
real issue -- sorry for the noise.

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

[John Snow]

On 03/31/2015 08:29 PM, Ed Maste wrote:
> On 31 March 2015 at 17:55, Ed Maste <emaste@freebsd.org> wrote:
>>
>> feynman% ./tests/ahci-test
>> /i386/ahci/sanity: OK
>> /i386/ahci/pci_spec: OK
>> /i386/ahci/pci_enable: OK
>> /i386/ahci/hba_spec: OK
>> /i386/ahci/hba_enable: OK
>> /i386/ahci/identify: OK
>> /i386/ahci/io/pio/lba28/simple: **
>> ERROR:tests/ahci-test.c:790:ahci_test_io_rw_simple: assertion failed
>> (memcmp(tx, rx, bufsize) == 0): (0xffffffffffffffff == 0x00000000)
>> zsh: abort (core dumped)  ./tests/ahci-test
>
> Well, that's rather strange: I had to head out after your last
> follow-up, and then when picking this up again from home I ended up
> accidentally breaking my build directory and then did a fresh clean
> build. Now I can't reproduce the error. Running tests/ahci-test
> reports all OK.
>
> So I'm not quite sure what was wrong, but it looks like this is not a
> real issue -- sorry for the noise.
>

I'll take a swing:

You may have updated the ahci-test binary/source-code, but failed to 
rebuild the actual qemu binary which included the fixes that the newest 
test exercises.

I hope, anyway. (It would explain the problem on your little endian 
amd64 machine, at least.)

Thanks for the report and followup :)

--js

Re: [Qemu-devel] [PATCH 0/4] Ide patches for 2.3-rc1

[John Snow]

On 03/23/2015 12:56 PM, John Snow wrote:
> The following changes since commit 3c6c9fe034c0c07b77f272e4a53d7735220a16a4:
>
>    Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2015-03-20 12:26:09 +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 54fced034e4d32d8ba6d1e27ecb7e2e2fb2f45d4:
>
>    ahci-test: improve rw buffer patterns (2015-03-23 12:24:16 -0400)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------
>
> 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(-)
>

Script failure, sorry :(

Should read [PULL], of course ...

Re: [Qemu-devel] [PATCH 0/4] Ide patches for 2.3-rc1

[Peter Maydell]

On 23 March 2015 at 16:56, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 3c6c9fe034c0c07b77f272e4a53d7735220a16a4:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2015-03-20 12:26:09 +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 54fced034e4d32d8ba6d1e27ecb7e2e2fb2f45d4:
>
>   ahci-test: improve rw buffer patterns (2015-03-23 12:24:16 -0400)
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM