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
