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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]

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/

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]