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 17:50, Anthony Liguori ha scritto:
>> 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
>
> My patch works.

Yes, can you send a SoB and submit as a top level?

Right now uhci is completely broken.

Regards,

Anthony Liguori

>
> Paolo
>
>> 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
>>