Re: [Qemu-devel] [PATCH 2/4 v5] block: Add device-width property to pflash_cfi01

[Peter Maydell]

On 3 December 2013 20:12, Roy Franz <address@hidden> wrote:
> On Thu, Nov 28, 2013 at 11:03 AM, Peter Maydell
> <address@hidden> 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