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