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

[Richard Henderson <rth@twiddle.net>]

On 07/17/2013 06:45 AM, Paolo Bonzini wrote:
>> NAK.
>>
>> If you remove the check here, you're just trading it for one in the device.
>> The device told you that it can't support a 1 byte read.  (Either that, or the
>> device incorrectly reported what it can actually do.)
> 
> There are two parts to this.
> 
> First of all, mr->ops->impl.min_access_size is definitely wrong.  The
> device told me that the MMIO functions only know about 2-byte accesses,
> but that it _can_ support 1-, 2- and 4- byte reads (with coalescing done
> by memory.c). 

I don't know enough about the specific device (or even which device it was)
to know whether the IMPL and VALID fields are correct.

> So I could change access_size_min to
> mr->ops->valid.min_access_size, which would also fix Markus's problem.

No, you can't.  At least not without changing all of the callers.

If you do as you suggest, the callers will invoke the device with a value of
SIZE that is illegal according to IMPL.  We might as well crash now than later.

There are three possible solutions:

(1) Return an error from memory_access_size, change the callers to propagate
    the error in some fashion.  This isn't ideal, since in this case VALID
    indicates that the guest access is correct.

(2) Return the implementation minimum, change the callers to interact with
    the device using that minimum.  With this scenario, we should likely
    share code with access_with_adjusted_size.

(3) Determine that the device's impl.min_access_size is wrong and adjust it.

Responding to your earlier

> erroneous accesses must not crash 
> QEMU, they should trigger exceptions in the guest or just return 
> garbage (depending on the CPU).

I completely agree -- if we were talking about VALID.  Since this is IMPL, it's
not an "erroneous access", but rather QEMU not being self-consistent.
And for internal logic errors, we've got asserts and aborts all over.


r~