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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 17/07/2013 16:29, Richard Henderson ha scritto:
> 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.

They are correct.  The device was usb-uhci, FWIW.

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

No, it won't.  access_with_adjusted_size will take care of taking a size
that IMPL rejects, and producing one or more accesses in a size that
IMPL accepts.

Now of course access_with_adjusted_size may have bugs handling
misaligned addresses.  That's possible.

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

Agreed.

> (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.

I think you misunderstand what the impl.*_access_size are.
impl.min/max_access_size is a private interface between the device and
memory.c, to avoid having code all over the place to combine/split MMIO
accesses.  The public interface of the device is valid.*_access_size.

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

Actually, no, for two reasons:

- address_space_rw memory accesses are exactly the same as memory
accesses started by the guest.  In many cases, they use addr/range pairs
passed directly by the guest.  It is not acceptable to crash on these.

- as said above, impl.*_access_size is not visible outside the device
itself, the public interface of the device is valid.*_access_size.

Paolo