From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VnwiL-0002eM-Ci for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:37:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VnwiG-0000CD-I2 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:37:01 -0500 Received: from mail-vc0-f171.google.com ([209.85.220.171]:33988) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VnwiG-0000C9-E0 for qemu-devel@nongnu.org; Tue, 03 Dec 2013 15:36:56 -0500 Received: by mail-vc0-f171.google.com with SMTP id ik5so10598559vcb.30 for ; Tue, 03 Dec 2013 12:36:56 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1382459756-6853-1-git-send-email-roy.franz@linaro.org> <1382459756-6853-3-git-send-email-roy.franz@linaro.org> Date: Tue, 3 Dec 2013 12:36:55 -0800 Message-ID: From: Roy Franz Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Kevin Wolf , QEMU Developers , Stefan Hajnoczi , Patch Tracking On Tue, Dec 3, 2013 at 12:27 PM, Peter Maydell wrote: > On 3 December 2013 20:12, Roy Franz wrote: >> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell >> wrote: >>> Other than this and the status (which you deal with in >>> the other patch) the accesses I wonder about whether we >>> have correct are: >>> * manufacturer & device ID code read >>> * cfi_table[] accesses ("query mode") >> >> I'm pretty sure these are not correct when device width >> is not equal to bank width. The manufacturer and device ID code >> looks completely broken, doing: >> >> ret = pfl->ident0 << 8 | pfl->ident1; >> >> with ident0 and ident1 being 16 bit values. >> >> I can update the device ID and cfi_table accesses to take into account >> device width, and test that with UEFI, but my concern is that this code is >> tricky to get right, and won't be well tested. The UEFI code doesn't >> care that these >> values are wrong, so my test case will likely continue to work whether the >> updates I make are correct or not. Also, all other platforms will >> continue to see >> the current behavior, as they don't set the device width, so they >> won't be testing >> the new code either. >> >> Let me know how you would like me to proceed on this. This is the last issue >> for me to resolve and I will have another version of the patch series ready. > > I'd prefer us to have some untested code which we believe to > be correct, rather than the current approach, which is to have > untested code which we know to be wrong :-) > > Cross-checking against what the real hardware reads these > ident/ID accesses as would be nice, if you have the setup to > hand (ie stuff some temporary test code into a uefi build or > something). At least then we can be reasonably sure the 16:16 case > is correct. > > -- PMM OK, I'll take a stab at this. I don't have VExpress hardware, but should be able to get someone to run some test code on it to check how actual hardware behaves. Roy