From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57211) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UzSvk-0006Z6-Sd for qemu-devel@nongnu.org; Wed, 17 Jul 2013 10:42:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UzSvj-0008H6-Ga for qemu-devel@nongnu.org; Wed, 17 Jul 2013 10:42:12 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59415) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UzSvj-0008H0-8f for qemu-devel@nongnu.org; Wed, 17 Jul 2013 10:42:11 -0400 Message-ID: <51E6AD37.9070502@redhat.com> Date: Wed, 17 Jul 2013 16:41:59 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1373840171-25556-1-git-send-email-rth@twiddle.net> <1373840171-25556-4-git-send-email-rth@twiddle.net> <8761w9wm50.fsf@blackfin.pond.sub.org> <51E67B7A.8000800@redhat.com> <51E69AE1.1060809@twiddle.net> <51E6A003.8070106@redhat.com> <51E6AA4A.9020101@twiddle.net> In-Reply-To: <51E6AA4A.9020101@twiddle.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 3/5] exec: Support 64-bit operations in address_space_rw List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: aliguori@us.ibm.com, Gerd Hoffmann , Markus Armbruster , qemu-devel@nongnu.org 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