[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] ahci: Do not ignore memory access read size
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] ahci: Do not ignore memory access read size |
Date: |
Mon, 15 Jun 2015 19:44:25 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
-----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-----
[Qemu-devel] [PATCH 3/4] libqos/ahci: fix memory management bugs, John Snow, 2015/06/15
[Qemu-devel] [PATCH 2/4] qtest/ahci: add test_max, John Snow, 2015/06/15
[Qemu-devel] [PATCH 4/4] qtest/ahci: add port_reset test, John Snow, 2015/06/15