From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58768) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4e3O-0008GY-0C for qemu-devel@nongnu.org; Mon, 15 Jun 2015 19:44:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4e3M-0005ej-BG for qemu-devel@nongnu.org; Mon, 15 Jun 2015 19:44:33 -0400 Message-ID: <557F6359.9060002@redhat.com> Date: Mon, 15 Jun 2015 19:44:25 -0400 From: John Snow MIME-Version: 1.0 References: <1434406965-30883-1-git-send-email-jsnow@redhat.com> <1434406965-30883-2-git-send-email-jsnow@redhat.com> <557F57CF.2090102@redhat.com> <557F5B2D.4090408@redhat.com> <557F5FAF.9030707@redhat.com> In-Reply-To: <557F5FAF.9030707@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/4] ahci: Do not ignore memory access read size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@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: >=20 >>>=20 >>>> 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. >>>>=20 >=20 >>=20 >>>> + + /* If the 64bit read is unaligned, we will produce >>>> undefined + * results. AHCI does not support unaligned >>>> 64bit reads. */ + hi =3D ahci_mem_read_32(opaque, aligned + >>>> 4); + return (hi << 32) | lo; >>>=20 >>> 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. >>>=20 >>=20 >> Good catch. >>=20 >=20 > Oh, and one other comment - do we care about the contents in the=20 > remaining bytes beyond the requested size? >=20 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. >>=20 >> I can do this: >>=20 >> return (hi << 32 | lo) >> (ofst * 8); >>=20 >> 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. >=20 > 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. >=20 > 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. >=20 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 =3D -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 =3DLzUg -----END PGP SIGNATURE-----