Re: [Qemu-devel] [PATCH 4/4] exec: fix incorrect assumptions in memory_access_size

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 19 Jul 2013 22:07:58 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> access_size_min can be 1 because erroneous accesses must not crash
> QEMU, they should trigger exceptions in the guest or just return
> garbage (depending on the CPU).  I am not sure I understand the
> comment: placing a 4-byte field at the last byte of a region
> makes no sense (unless impl.unaligned is true), and that is
> why memory.c:access_with_adjusted_size does not bother with
> minimums larger than the remaining length.
> 
> access_size_max can be mr->ops->valid.max_access_size because memory.c
> can and will still break accesses bigger than
> mr->ops->impl.max_access_size.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Tested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Yeah, works for me now:

Tested-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  exec.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index d312bb4..c8658c6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1898,14 +1898,10 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
> -    unsigned access_size_min = mr->ops->impl.min_access_size;
> -    unsigned access_size_max = mr->ops->impl.max_access_size;
> +    unsigned access_size_max = mr->ops->valid.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;
>      }
> @@ -1922,9 +1918,6 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>      if (l > access_size_max) {
>          l = access_size_max;
>      }
> -    /* ??? 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;
>  }