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

[Anthony Liguori <aliguori@us.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 17/07/2013 11:50, Markus Armbruster ha scritto:
>> Richard Henderson <rth@twiddle.net> writes:
>> 
>>> Honor the implementation maximum access size, and at least check
>>> the minimum access size.
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> 
>> Fails for me:
>> 
>> qemu-system-x86_64: /work/armbru/qemu/exec.c:1927: memory_access_size: Assertion `l >= access_size_min' failed.
>
> This:
>
>     unsigned access_size_min = mr->ops->impl.min_access_size;
>     unsigned access_size_max = mr->ops->impl.max_access_size;
>
> must be respectively:
>
>     unsigned access_size_min = 1;
>     unsigned access_size_max = mr->ops->valid.max_access_size;
>
> 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'm 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).
>
> 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.
>
> Markus, can you try the minimal patch above?  Or this one that also
> does the consequent simplifications.

FYI, the reproducer is very simple:

qemu-system-x86_64 -usb

Regards,

Anthony Liguori

>
> diff --git a/exec.c b/exec.c
> index c99a883..0904283 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1898,14 +1898,8 @@ 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 +1916,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;
>  }
>
> Paolo