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

[John Snow <jsnow@redhat.com>]

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