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

[Eric Blake]

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