[Qemu-devel] [PATCH v2 0/4] ahci: misc fixes/tests for 2.4

[Qemu-devel] [PATCH v2 0/4] ahci: misc fixes/tests for 2.4

[John Snow]

This is a small handful of fixes for the ahci-tests/ahci device,
alongside two new tests. I have a larger series of NCQ patches
coming shortly, but these patches were unrelated so I cleaned them
up and am sending them out separately.

v2:
 - Fixed unaligned 2/4 byte read logic
 - Tidied comments and justification for patch 1.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-tests-2.4
https://github.com/jnsnow/qemu/tree/ahci-tests-2.4

This version is tagged ahci-tests-2.4-v2:
https://github.com/jnsnow/qemu/releases/tag/ahci-tests-2.4-v2

John Snow (4):
  ahci: Do not ignore memory access read size
  qtest/ahci: add test_max
  libqos/ahci: fix memory management bugs
  qtest/ahci: add port_reset test

 hw/ide/ahci.c       | 27 +++++++++++++++++--
 tests/ahci-test.c   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
 tests/libqos/ahci.c |  6 +++--
 3 files changed, 98 insertions(+), 9 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] ahci: Do not ignore memory access read size

[John Snow]

The only guidance the AHCI specification gives on memory access is:
"Register accesses shall have a maximum size of 64-bits; 64-bit access
must not cross an 8-byte alignment boundary."

I interpret this to mean that aligned or unaligned 1, 2 and 4 byte
accesses should work, as well as aligned 8 byte accesses.

In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads
regardless of alignment. Windows 7 can be observed making 1 byte
reads to the middle of 32 bit registers to fetch error codes.

Introduce a wrapper to support unaligned accesses to AHCI.
This wrapper will support aligned 8 byte reads, but will make
no effort to support unaligned 8 byte reads, which although they
will work on real hardware, are not guaranteed to work and do
not appear to be used by either Windows or Linux.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9e5d862..26df2ca 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -331,8 +331,7 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
     }
 }
 
-static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
-                              unsigned size)
+static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
 {
     AHCIState *s = opaque;
     uint32_t val = 0;
@@ -368,6 +367,30 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
 }
 
 
+/**
+ * AHCI 1.3 section 3 ("HBA Memory Registers")
+ * Support unaligned 8/16/32 bit reads, and 64 bit aligned reads.
+ * Caller is responsible for masking unwanted higher order bytes.
+ */
+static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size)
+{
+    hwaddr aligned = addr & ~0x3;
+    int ofst = addr - aligned;
+    uint64_t lo = ahci_mem_read_32(opaque, aligned);
+    uint64_t hi;
+
+    /* if < 8 byte read does not cross 4 byte boundary */
+    if (ofst + size <= 4) {
+        return lo >> (ofst * 8);
+    }
+    g_assert_cmpint(size, >, 1);
+
+    /* If the 64bit read is unaligned, we will produce undefined
+     * results. AHCI does not support unaligned 64bit reads. */
+    hi = ahci_mem_read_32(opaque, aligned + 4);
+    return (hi << 32 | lo) >> (ofst * 8);
+}
+
 
 static void ahci_mem_write(void *opaque, hwaddr addr,
                            uint64_t val, unsigned size)
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/4] qtest/ahci: add test_max

[John Snow]

Test that the FIS delivered after a nondata command has appropriately
updated registers, just as we'd expect a data command to do.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 6e3fa81..5148328 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -890,21 +890,23 @@ static void ahci_test_io_rw_simple(AHCIQState *ahci, unsigned bufsize,
     g_free(rx);
 }
 
-static void ahci_test_nondata(AHCIQState *ahci, uint8_t ide_cmd)
+static uint8_t ahci_test_nondata(AHCIQState *ahci, uint8_t ide_cmd)
 {
-    uint8_t px;
+    uint8_t port;
     AHCICommand *cmd;
 
     /* Sanitize */
-    px = ahci_port_select(ahci);
-    ahci_port_clear(ahci, px);
+    port = ahci_port_select(ahci);
+    ahci_port_clear(ahci, port);
 
     /* Issue Command */
     cmd = ahci_command_create(ide_cmd);
-    ahci_command_commit(ahci, cmd, px);
+    ahci_command_commit(ahci, cmd, port);
     ahci_command_issue(ahci, cmd);
     ahci_command_verify(ahci, cmd);
     ahci_command_free(cmd);
+
+    return port;
 }
 
 static void ahci_test_flush(AHCIQState *ahci)
@@ -912,6 +914,33 @@ static void ahci_test_flush(AHCIQState *ahci)
     ahci_test_nondata(ahci, CMD_FLUSH_CACHE);
 }
 
+static void ahci_test_max(AHCIQState *ahci)
+{
+    RegD2HFIS *d2h = g_malloc0(0x20);
+    uint64_t nsect;
+    uint8_t port;
+    uint8_t cmd;
+    uint64_t config_sect = TEST_IMAGE_SECTORS - 1;
+
+    if (config_sect > 0xFFFFFF) {
+        cmd = CMD_READ_MAX_EXT;
+    } else {
+        cmd = CMD_READ_MAX;
+    }
+
+    port = ahci_test_nondata(ahci, cmd);
+    memread(ahci->port[port].fb + 0x40, d2h, 0x20);
+    nsect = (uint64_t)d2h->lba_hi[2] << 40 |
+        (uint64_t)d2h->lba_hi[1] << 32 |
+        (uint64_t)d2h->lba_hi[0] << 24 |
+        (uint64_t)d2h->lba_lo[2] << 16 |
+        (uint64_t)d2h->lba_lo[1] << 8 |
+        (uint64_t)d2h->lba_lo[0];
+
+    g_assert_cmphex(nsect, ==, config_sect);
+    g_free(d2h);
+}
+
 
 /******************************************************************************/
 /* Test Interfaces                                                            */
@@ -1334,6 +1363,15 @@ static void test_flush_migrate(void)
     ahci_shutdown(dst);
 }
 
+static void test_max(void)
+{
+    AHCIQState *ahci;
+
+    ahci = ahci_boot_and_enable(NULL);
+    ahci_test_max(ahci);
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 /* AHCI I/O Test Matrix Definitions                                           */
 
@@ -1589,6 +1627,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/io/dma/lba28/retry", test_halted_dma);
     qtest_add_func("/ahci/migrate/dma/halted", test_migrate_halted_dma);
 
+    qtest_add_func("/ahci/max", test_max);
+
     ret = g_test_run();
 
     /* Cleanup */
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/4] libqos/ahci: fix memory management bugs

[John Snow]

There's a handful of trivial bugs in the libqos/ahci functions,
squish them together.

- Zero cached pointers after freeing them
- The Command List Buffer is an array of 32x 32 byte structures, not
  32x 8 byte pointers -- it's 1MiB, not 256 bytes. Zero it ALL.
- Free the correct command in ahci_pick_cmd.

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

diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
index 7e17bb6..08e1c98 100644
--- a/tests/libqos/ahci.c
+++ b/tests/libqos/ahci.c
@@ -138,12 +138,14 @@ void ahci_clean_mem(AHCIQState *ahci)
     for (port = 0; port < 32; ++port) {
         if (ahci->port[port].fb) {
             ahci_free(ahci, ahci->port[port].fb);
+            ahci->port[port].fb = 0;
         }
         if (ahci->port[port].clb) {
             for (slot = 0; slot < 32; slot++) {
                 ahci_destroy_command(ahci, port, slot);
             }
             ahci_free(ahci, ahci->port[port].clb);
+            ahci->port[port].clb = 0;
         }
     }
 }
@@ -252,7 +254,7 @@ void ahci_hba_enable(AHCIQState *ahci)
         /* Allocate Memory for the Command List Buffer & FIS Buffer */
         /* PxCLB space ... 0x20 per command, as in 4.2.2 p 36 */
         ahci->port[i].clb = ahci_alloc(ahci, num_cmd_slots * 0x20);
-        qmemset(ahci->port[i].clb, 0x00, 0x100);
+        qmemset(ahci->port[i].clb, 0x00, num_cmd_slots * 0x20);
         g_test_message("CLB: 0x%08" PRIx64, ahci->port[i].clb);
         ahci_px_wreg(ahci, i, AHCI_PX_CLB, ahci->port[i].clb);
         g_assert_cmphex(ahci->port[i].clb, ==,
@@ -549,7 +551,7 @@ unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t port)
         if (reg & (1 << j)) {
             continue;
         }
-        ahci_destroy_command(ahci, port, i);
+        ahci_destroy_command(ahci, port, j);
         ahci->port[port].next = (j + 1) % 32;
         return j;
     }
-- 
2.1.0

[Qemu-devel] [PATCH v2 4/4] qtest/ahci: add port_reset test

[John Snow]

Test that we can survive a couple of cycles of running a basic identify
test, some IO, and resetting the HBA. Ensures that we can bring the HBA
back to compliant spec during the lifecycle of the VM.

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 5148328..43da3d1 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1372,6 +1372,29 @@ static void test_max(void)
     ahci_shutdown(ahci);
 }
 
+static void test_reset(void)
+{
+    AHCIQState *ahci;
+    int i;
+
+    ahci = ahci_boot(NULL);
+    ahci_test_pci_spec(ahci);
+    ahci_pci_enable(ahci);
+
+    for (i = 0; i < 2; i++) {
+        ahci_test_hba_spec(ahci);
+        ahci_hba_enable(ahci);
+        ahci_test_identify(ahci);
+        ahci_test_io_rw_simple(ahci, 4096, 0,
+                               CMD_READ_DMA_EXT,
+                               CMD_WRITE_DMA_EXT);
+        ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
+        ahci_clean_mem(ahci);
+    }
+
+    ahci_shutdown(ahci);
+}
+
 /******************************************************************************/
 /* AHCI I/O Test Matrix Definitions                                           */
 
@@ -1628,6 +1651,7 @@ int main(int argc, char **argv)
     qtest_add_func("/ahci/migrate/dma/halted", test_migrate_halted_dma);
 
     qtest_add_func("/ahci/max", test_max);
+    qtest_add_func("/ahci/reset", test_reset);
 
     ret = g_test_run();
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/4] ahci: Do not ignore memory access read size

[Eric Blake]

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

On 06/16/2015 10:02 AM, John Snow wrote:
> The only guidance the AHCI specification gives on memory access is:
> "Register accesses shall have a maximum size of 64-bits; 64-bit access
> must not cross an 8-byte alignment boundary."
> 
> I interpret this to mean that aligned or unaligned 1, 2 and 4 byte
> accesses should work, as well as aligned 8 byte accesses.
> 
> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads
> regardless of alignment. Windows 7 can be observed making 1 byte
> reads to the middle of 32 bit registers to fetch error codes.
> 
> Introduce a wrapper to support unaligned accesses to AHCI.
> This wrapper will support aligned 8 byte reads, but will make
> no effort to support unaligned 8 byte reads, which although they
> will work on real hardware, are not guaranteed to work and do
> not appear to be used by either Windows or Linux.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 

> +/**
> + * AHCI 1.3 section 3 ("HBA Memory Registers")
> + * Support unaligned 8/16/32 bit reads, and 64 bit aligned reads.
> + * Caller is responsible for masking unwanted higher order bytes.
> + */
> +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    hwaddr aligned = addr & ~0x3;

This actually supports 4-byte aligned 8-byte reads (which is an
unaligned 8-byte read).  Doesn't matter; no guest should be relying on it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 1/4] ahci: Do not ignore memory access read size

[John Snow]

On 06/16/2015 12:25 PM, Eric Blake wrote:
> On 06/16/2015 10:02 AM, John Snow wrote:
>> The only guidance the AHCI specification gives on memory access
>> is: "Register accesses shall have a maximum size of 64-bits;
>> 64-bit access must not cross an 8-byte alignment boundary."
>> 
>> I interpret this to mean that aligned or unaligned 1, 2 and 4
>> byte accesses should work, as well as aligned 8 byte accesses.
>> 
>> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte
>> reads regardless of alignment. Windows 7 can be observed making 1
>> byte reads to the middle of 32 bit registers to fetch error
>> codes.
>> 
>> Introduce a wrapper to support unaligned accesses to AHCI. This
>> wrapper will support aligned 8 byte reads, but will make no
>> effort to support unaligned 8 byte reads, which although they 
>> will work on real hardware, are not guaranteed to work and do not
>> appear to be used by either Windows or Linux.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com> --- hw/ide/ahci.c |
>> 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+),
>> 2 deletions(-)
>> 
> 
>> +/** + * AHCI 1.3 section 3 ("HBA Memory Registers") + * Support
>> unaligned 8/16/32 bit reads, and 64 bit aligned reads. + * Caller
>> is responsible for masking unwanted higher order bytes. + */ 
>> +static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
>> unsigned size) +{ +    hwaddr aligned = addr & ~0x3;
> 
> This actually supports 4-byte aligned 8-byte reads (which is an 
> unaligned 8-byte read).  Doesn't matter; no guest should be relying
> on it.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Easier to say "indeterminate" than rely on wonko behavior, even if it
sometimes accidentally works :)

Thanks.

Re: [Qemu-devel] [PATCH v2 0/4] ahci: misc fixes/tests for 2.4

[John Snow]

final ping:

I'll pull this as a pre-requisite for the NCQ fixes (part 1) series that
has been reviewed unless there are objections to this series.

--js

On 06/16/2015 12:02 PM, John Snow wrote:
> This is a small handful of fixes for the ahci-tests/ahci device,
> alongside two new tests. I have a larger series of NCQ patches
> coming shortly, but these patches were unrelated so I cleaned them
> up and am sending them out separately.
> 
> v2:
>  - Fixed unaligned 2/4 byte read logic
>  - Tidied comments and justification for patch 1.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-tests-2.4
> https://github.com/jnsnow/qemu/tree/ahci-tests-2.4
> 
> This version is tagged ahci-tests-2.4-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-tests-2.4-v2
> 
> John Snow (4):
>   ahci: Do not ignore memory access read size
>   qtest/ahci: add test_max
>   libqos/ahci: fix memory management bugs
>   qtest/ahci: add port_reset test
> 
>  hw/ide/ahci.c       | 27 +++++++++++++++++--
>  tests/ahci-test.c   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  tests/libqos/ahci.c |  6 +++--
>  3 files changed, 98 insertions(+), 9 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 0/4] ahci: misc fixes/tests for 2.4

[John Snow]

(Staging as a pre-req for NCQ fixes, last chance to whine!)

On 06/16/2015 12:02 PM, John Snow wrote:
> This is a small handful of fixes for the ahci-tests/ahci device,
> alongside two new tests. I have a larger series of NCQ patches
> coming shortly, but these patches were unrelated so I cleaned them
> up and am sending them out separately.
> 
> v2:
>  - Fixed unaligned 2/4 byte read logic
>  - Tidied comments and justification for patch 1.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-tests-2.4
> https://github.com/jnsnow/qemu/tree/ahci-tests-2.4
> 
> This version is tagged ahci-tests-2.4-v2:
> https://github.com/jnsnow/qemu/releases/tag/ahci-tests-2.4-v2
> 
> John Snow (4):
>   ahci: Do not ignore memory access read size
>   qtest/ahci: add test_max
>   libqos/ahci: fix memory management bugs
>   qtest/ahci: add port_reset test
> 
>  hw/ide/ahci.c       | 27 +++++++++++++++++--
>  tests/ahci-test.c   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  tests/libqos/ahci.c |  6 +++--
>  3 files changed, 98 insertions(+), 9 deletions(-)
> 

Thanks, applied to my IDE tree:
https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js