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

[Qemu-devel] [PATCH 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.
________________________________________________________________________________

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-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-tests-2.4-v1

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       | 21 +++++++++++++--
 tests/ahci-test.c   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++----
 tests/libqos/ahci.c |  6 +++--
 3 files changed, 92 insertions(+), 9 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 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."

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

Introduce a wrapper to supper 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9e5d862..55779fb 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,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
 }
 
 
+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 1/2/4 byte read does not cross 4 byte boundary */
+    if (ofst + size <= 4) {
+        return lo >> (ofst * 8);
+    }
+
+    /* 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;
+}
+
 
 static void ahci_mem_write(void *opaque, hwaddr addr,
                            uint64_t val, unsigned size)
-- 
2.1.0

[Qemu-devel] [PATCH 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 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 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 1/4] ahci: Do not ignore memory access read size

[Eric Blake]

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

On 06/15/2015 04:22 PM, 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."
> 
> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads
> regardless of alignment. Windows 7 can also be observed making 1 byte
> reads to the middle of 32 bit registers.
> 
> Introduce a wrapper to supper unaligned accesses to AHCI.

s/supper/support/

> 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 | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9e5d862..55779fb 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,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
>  }
>  
>  
> +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 1/2/4 byte read does not cross 4 byte boundary */
> +    if (ofst + size <= 4) {
> +        return lo >> (ofst * 8);
> +    }

At this point, we could assert(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;

This makes no effort to support an unaligned 2 byte (16bit) or 4 byte
(32bit) read that crosses 4-byte boundary.  Is that intentional?  I know
it is intentional that you don't care about unaligned 64bit reads;
conversely, while your commit message mentioned Windows doing 1-byte
reads in the middle of 32-bit registers, you didn't mention whether
Windows does unaligned 2- or 4-byte reads.  So either the comment should
be broadened, or the code needs further tuning.

-- 
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 1/4] ahci: Do not ignore memory access read size

[John Snow]

On 06/15/2015 06:55 PM, Eric Blake wrote:
> On 06/15/2015 04:22 PM, 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."
>>
>> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads
>> regardless of alignment. Windows 7 can also be observed making 1 byte
>> reads to the middle of 32 bit registers.
>>
>> Introduce a wrapper to supper unaligned accesses to AHCI.
> 
> s/supper/support/

Wow, I guess I'm hungry.

> 
>> 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 | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 9e5d862..55779fb 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,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
>>  }
>>  
>>  
>> +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 1/2/4 byte read does not cross 4 byte boundary */
>> +    if (ofst + size <= 4) {
>> +        return lo >> (ofst * 8);
>> +    }
> 
> At this point, we could assert(size > 1).
> 

Sure. I guess in that light my comment above is a little wacky -- 1 byte
reads can't cross the boundary ;)

>> +
>> +    /* 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;
> 
> This makes no effort to support an unaligned 2 byte (16bit) or 4 byte
> (32bit) read that crosses 4-byte boundary.  Is that intentional?  I know
> it is intentional that you don't care about unaligned 64bit reads;
> conversely, while your commit message mentioned Windows doing 1-byte
> reads in the middle of 32-bit registers, you didn't mention whether
> Windows does unaligned 2- or 4-byte reads.  So either the comment should
> be broadened, or the code needs further tuning.
> 

Good catch.

I have not observed any OS making 2 or 4 byte accesses across the
register boundary, and cannot think of a reason why you would want to,
though the AHCI spec technically doesn't discount your ability to do so
and it does work on a real Q35.

I can do this:

return (hi << 32 | lo) >> (ofst * 8);

which will give us unaligned 2 and 4 byte reads, but will really get
very wacky for unaligned 8 byte reads -- which you really should
probably not be doing anyway.

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

[Eric Blake]

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

On 06/15/2015 05:09 PM, John Snow wrote:

>>
>>> 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.
>>>

> 
>>> +
>>> +    /* 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;
>>
>> This makes no effort to support an unaligned 2 byte (16bit) or 4 byte
>> (32bit) read that crosses 4-byte boundary.  Is that intentional?  I know
>> it is intentional that you don't care about unaligned 64bit reads;
>> conversely, while your commit message mentioned Windows doing 1-byte
>> reads in the middle of 32-bit registers, you didn't mention whether
>> Windows does unaligned 2- or 4-byte reads.  So either the comment should
>> be broadened, or the code needs further tuning.
>>
> 
> Good catch.
> 

Oh, and one other comment - do we care about the contents in the
remaining bytes beyond the requested size?

> I have not observed any OS making 2 or 4 byte accesses across the
> register boundary, and cannot think of a reason why you would want to,
> though the AHCI spec technically doesn't discount your ability to do so
> and it does work on a real Q35.
> 
> I can do this:
> 
> return (hi << 32 | lo) >> (ofst * 8);
> 
> which will give us unaligned 2 and 4 byte reads, but will really get
> very wacky for unaligned 8 byte reads -- which you really should
> probably not be doing anyway.

I don't mind wacky unaligned 8 byte reads - they are in clear violation
of the spec you quoted, so the caller deserves any such garbage they
get.  But as the spec is silent on unaligned 4 byte reads, and real
hardware supports it, it's probably best to support it ourselves even if
we haven't observed clients exploiting it.

Note that while this returns the desired 16 or 32 bits in the low order
side of the result, it does not guarantee that the remaining upper bytes
are all 0.  I don't know if it matters to callers, or even what real
hardware does, but you may want to mask things at both return
statements, to guarantee a stable result limited to size bytes of
information rather than leaking nearby bytes from the rest of the
registers being read.

-- 
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 1/4] ahci: Do not ignore memory access read size

[John Snow]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 06/15/2015 07:28 PM, Eric Blake wrote:
> On 06/15/2015 05:09 PM, John Snow wrote:
> 
>>> 
>>>> 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.
>>>> 
> 
>> 
>>>> + +    /* 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;
>>> 
>>> This makes no effort to support an unaligned 2 byte (16bit) or
>>> 4 byte (32bit) read that crosses 4-byte boundary.  Is that
>>> intentional?  I know it is intentional that you don't care
>>> about unaligned 64bit reads; conversely, while your commit
>>> message mentioned Windows doing 1-byte reads in the middle of
>>> 32-bit registers, you didn't mention whether Windows does
>>> unaligned 2- or 4-byte reads.  So either the comment should be
>>> broadened, or the code needs further tuning.
>>> 
>> 
>> Good catch.
>> 
> 
> Oh, and one other comment - do we care about the contents in the 
> remaining bytes beyond the requested size?
> 

Do you mean the unmasked higher order bits, if applicable, as you
elaborate below?

>> I have not observed any OS making 2 or 4 byte accesses across
>> the register boundary, and cannot think of a reason why you would
>> want to, though the AHCI spec technically doesn't discount your
>> ability to do so and it does work on a real Q35.
>> 
>> I can do this:
>> 
>> return (hi << 32 | lo) >> (ofst * 8);
>> 
>> which will give us unaligned 2 and 4 byte reads, but will really
>> get very wacky for unaligned 8 byte reads -- which you really
>> should probably not be doing anyway.
> 
> I don't mind wacky unaligned 8 byte reads - they are in clear
> violation of the spec you quoted, so the caller deserves any such
> garbage they get.  But as the spec is silent on unaligned 4 byte
> reads, and real hardware supports it, it's probably best to support
> it ourselves even if we haven't observed clients exploiting it.
> 
> Note that while this returns the desired 16 or 32 bits in the low
> order side of the result, it does not guarantee that the remaining
> upper bytes are all 0.  I don't know if it matters to callers, or
> even what real hardware does, but you may want to mask things at
> both return statements, to guarantee a stable result limited to
> size bytes of information rather than leaking nearby bytes from the
> rest of the registers being read.
> 

I believe the masking is handled by the memory system in general, see
memory_region_read_accessor, which masks the returned uint64_t with an
appropriate value set earlier by access_with_adjusted_size:

access_mask = -1ULL >> (64 - access_size * 8);

So we're probably OK here, but I can leave a comment if you think it's
too hand-wavey.

- --js
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVf2NZAAoJEH3vgQaq/DkOLqwQAItBm26zX4FO3ecRxIBo3QEW
DBz+8wCcSHNne1qjiV41ArP+Mc1ckJ8tSob1NohlNekyxN69W5iVd8vsgjfmYZ6t
Yqf5Hd5ebEBXMxnNjcCW/pTYQwNitP9RHMuGWnTpRYQxNE2FfV0p8JRSNVk7jsRy
/24EmI7ZZv7GhOe4Okh2neiKzizhcxs/XDHrDuJPOkrby73Gc/eCjbXYKGLcLKjh
xXrziAPDHtBo4XStNCMWVtSizmLuSbabt6jeoIKIISRyEGwD5v/GFRpXcKsfQlHq
Fm/2ITeVFlE5myJLQOV0KLsC6WLtyispgQuMyBXII4+AM2vPkNMc5TdxuYXx1bLt
NT+42FYj4lnKqa77SuymsRe+fBUK5FNtJQC1PrdV3gugqUKHVydsQe10Y6me5Ywg
OnvGwi5XO4sDePIv7ANGLgtOaNuIZ007Zr+nK43RvTDyby/e2eVkZTEpzt3Lufex
zlN8YvjPYM5NwmSXDyKUYICd6Xg6KyFX5lT9KZJ2c6K0cs/bSlD313CQ8l96E+5V
Mt+WIvN6ywaJ8q3co4YVkxfbJ+SNPBhkSmlmBnfCH/NwhLtFkTvlMQyqrr4otf3N
n+FkLOjAj35jn0oC43plG//aUfTyfkGKFbk7Bw3pBCZqKSf2BT57Fh/iYgXxBw78
Gt+cgknToPRtKWcps0HW
=LzUg
-----END PGP SIGNATURE-----

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

[Eric Blake]

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

On 06/15/2015 05:44 PM, John Snow wrote:

>> Note that while this returns the desired 16 or 32 bits in the low
>> order side of the result, it does not guarantee that the remaining
>> upper bytes are all 0.  I don't know if it matters to callers, or
>> even what real hardware does, but you may want to mask things at
>> both return statements, to guarantee a stable result limited to
>> size bytes of information rather than leaking nearby bytes from the
>> rest of the registers being read.
> 
> 
> I believe the masking is handled by the memory system in general, see
> memory_region_read_accessor, which masks the returned uint64_t with an
> appropriate value set earlier by access_with_adjusted_size:
> 
> access_mask = -1ULL >> (64 - access_size * 8);

Good, the caller takes care of it.

> 
> So we're probably OK here, but I can leave a comment if you think it's
> too hand-wavey.

Yeah, always nicer to state our assumption that we rely on the caller to
truncate the result to the desired access size.

-- 
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 --]