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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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