Re: [Qemu-devel] [PATCH 3/4] exec: Support 64-bit operations in address_space_rw

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/07/2013 03:01, Richard Henderson ha scritto:
> Honor the implementation maximum access size, and at least check
> the minimum access size.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  exec.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 03fdf7e..5da22dc 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1902,15 +1902,37 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>      return false;
>  }
>  
> -static inline int memory_access_size(MemoryRegion *mr, int l, hwaddr addr)
> +static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
> -    if (l >= 4 && (((addr & 3) == 0 || mr->ops->impl.unaligned))) {
> -        return 4;
> +    unsigned access_size_min = mr->ops->impl.min_access_size;
> +    unsigned access_size_max = mr->ops->impl.max_access_size;
> +
> +    /* Regions are assumed to support 1-4 byte accesses unless
> +       otherwise specified.  */
> +    if (access_size_min == 0) {
> +        access_size_min = 1;
> +    }
> +    if (access_size_max == 0) {
> +        access_size_max = 4;
> +    }
> +
> +    /* Bound the maximum access by the alignment of the address.  */
> +    if (!mr->ops->impl.unaligned) {
> +        unsigned align_size_max = addr & -addr;
> +        if (align_size_max != 0 && align_size_max < access_size_max) {
> +            access_size_max = align_size_max;
> +        }
>      }
> -    if (l >= 2 && (((addr & 1) == 0) || mr->ops->impl.unaligned)) {
> -        return 2;
> +
> +    /* Don't attempt accesses larger than the maximum.  */
> +    if (l > access_size_max) {
> +        l = access_size_max;
>      }
> -    return 1;
> +    /* ??? The users of this function are wrong, not supporting minimums larger
> +       than the remaining length.  C.f. memory.c:access_with_adjusted_size.  */
> +    assert(l >= access_size_min);
> +
> +    return l;
>  }
>  
>  bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
> @@ -1932,18 +1954,29 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  l = memory_access_size(mr, l, addr1);
>                  /* XXX: could force cpu_single_env to NULL to avoid
>                     potential bugs */
> -                if (l == 4) {
> +                switch (l) {
> +                case 8:
> +                    /* 64 bit write access */
> +                    val = ldq_p(buf);
> +                    error |= io_mem_write(mr, addr1, val, 8);
> +                    break;
> +                case 4:
>                      /* 32 bit write access */
>                      val = ldl_p(buf);
>                      error |= io_mem_write(mr, addr1, val, 4);
> -                } else if (l == 2) {
> +                    break;
> +                case 2:
>                      /* 16 bit write access */
>                      val = lduw_p(buf);
>                      error |= io_mem_write(mr, addr1, val, 2);
> -                } else {
> +                    break;
> +                case 1:
>                      /* 8 bit write access */
>                      val = ldub_p(buf);
>                      error |= io_mem_write(mr, addr1, val, 1);
> +                    break;
> +                default:
> +                    abort();
>                  }
>              } else {
>                  addr1 += memory_region_get_ram_addr(mr);
> @@ -1956,18 +1989,29 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>              if (!memory_access_is_direct(mr, is_write)) {
>                  /* I/O case */
>                  l = memory_access_size(mr, l, addr1);
> -                if (l == 4) {
> +                switch (l) {
> +                case 8:
> +                    /* 64 bit read access */
> +                    error |= io_mem_read(mr, addr1, &val, 8);
> +                    stq_p(buf, val);
> +                    break;
> +                case 4:
>                      /* 32 bit read access */
>                      error |= io_mem_read(mr, addr1, &val, 4);
>                      stl_p(buf, val);
> -                } else if (l == 2) {
> +                    break;
> +                case 2:
>                      /* 16 bit read access */
>                      error |= io_mem_read(mr, addr1, &val, 2);
>                      stw_p(buf, val);
> -                } else {
> +                    break;
> +                case 1:
>                      /* 8 bit read access */
>                      error |= io_mem_read(mr, addr1, &val, 1);
>                      stb_p(buf, val);
> +                    break;
> +                default:
> +                    abort();
>                  }
>              } else {
>                  /* RAM case */
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>