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
signature.asc
Description: OpenPGP digital signature
