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

[John Snow <jsnow@redhat.com>]

On 06/15/2015 06:55 PM, Eric Blake wrote:
> On 06/15/2015 04:22 PM, John Snow wrote:
>> The only guidance the AHCI specification gives on memory access is:
>> "Register accesses shall have a maximum size of 64-bits; 64-bit access
>> must not cross an 8-byte alignment boundary."
>>
>> In practice, a real Q35/ICH9 responds to 1, 2, 4 and 8 byte reads
>> regardless of alignment. Windows 7 can also be observed making 1 byte
>> reads to the middle of 32 bit registers.
>>
>> Introduce a wrapper to supper unaligned accesses to AHCI.
> 
> s/supper/support/

Wow, I guess I'm hungry.

> 
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/ide/ahci.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index 9e5d862..55779fb 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -331,8 +331,7 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
>>      }
>>  }
>>  
>> -static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
>> -                              unsigned size)
>> +static uint64_t ahci_mem_read_32(void *opaque, hwaddr addr)
>>  {
>>      AHCIState *s = opaque;
>>      uint32_t val = 0;
>> @@ -368,6 +367,24 @@ static uint64_t ahci_mem_read(void *opaque, hwaddr addr,
>>  }
>>  
>>  
>> +static uint64_t ahci_mem_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    hwaddr aligned = addr & ~0x3;
>> +    int ofst = addr - aligned;
>> +    uint64_t lo = ahci_mem_read_32(opaque, aligned);
>> +    uint64_t hi;
>> +
>> +    /* if 1/2/4 byte read does not cross 4 byte boundary */
>> +    if (ofst + size <= 4) {
>> +        return lo >> (ofst * 8);
>> +    }
> 
> At this point, we could assert(size > 1).
> 

Sure. I guess in that light my comment above is a little wacky -- 1 byte
reads can't cross the boundary ;)

>> +
>> +    /* 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.

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.