Re: [Qemu-devel] [PULL 27/27] hw/block/pflash_cfi02: Reduce I/O accesses to 16-bit

[Stephen Checkoway]

> On Jul 4, 2019, at 08:45, Philippe Mathieu-Daudé <address@hidden> wrote:
> 
> Cc'ing PPC/taihu_405ep and ARM/Digic4 maintainers.
> 
> On 7/3/19 6:36 PM, Philippe Mathieu-Daudé wrote:
>> On 7/3/19 6:20 PM, Stephen Checkoway wrote:
>>>> On Jul 3, 2019, at 12:02, Philippe Mathieu-Daudé <address@hidden> wrote:
>>>> On 7/3/19 5:52 PM, Stephen Checkoway wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 1, 2019, at 20:59, Philippe Mathieu-Daudé <address@hidden> wrote:
>>>>>> 
>>>>>> Parallel NOR flashes are limited to 16-bit bus accesses.
>>>>> 
>>>>> I don't think this is correct. The CFI spec defines an x32 interface for 
>>>>> parallel NOR. CFI addresses 0x28 and 0x29 specify the interface and value 
>>>>> 3 is x32 and value 5 is x16/x32.
>>>>> 
>>>>> Here's an example of an x32 device 
>>>>> <https://www.mouser.com/datasheet/2/100/002-00948_29CD032J_S29CD016J_S29CL032J_S29CL016J_3-1316792.pdf>.
>>>> 
>>>> OK, I was not aware of these.
>>>> 
>>>> QEMU never CFI-announced itself as x32 capable:
>>>> 
>>>>   /* Flash device interface (8 & 16 bits) */
>>>>   pfl->cfi_table[0x28] = 0x02;
>>>>   pfl->cfi_table[0x29] = 0x00;
>>>> 
>>>> So while the commit description is incorrect, the code is safe with the
>>>> current device model.
>>>> 
>>>> I am not comfortable keeping untested 32-bit mode.
>>>> Were you using it?
>>> 
>>> I'm not using it. I did have some code to set these CFI values based on the 
>>> parameters used to control the interleaving 
>>> <https://github.com/stevecheckoway/qemu/commit/f9a79a6e18b2c7c5a05e344ff554a7d980a56042#diff-d33881bd0ef099e2f46ebd4797c653bcR599>.
>>> 
>>> In general, a better testing harness would be nice though.
>> 
>> We can revert it if it helps you.
> 
> So after further analysis, there are not guest visible changes, because
> 32-bit accesses are still valid (.valid.max_access_size = 4) but now
> they are processed as two 16-bit accesses, shifted by
> access_with_adjusted_size().
> Well, I haven't checked (yet) when the guest and the flash are in
> different endianess, and I wish we don't use that.
> 
> Now I see 2 different guests "registering" the flash in 32-bit access:
> 
> - PPC/taihu_405ep
> 
>  The CFI id matches the S29AL008J that is a 1MB in x16, while the code
>  QEMU forces it to be 2MB, and checking Linux it expects a 4MB flash
>  there, Yay \o/
> 
> - ARM/Digic4
> 
>  While the comment says "Samsung K8P3215UQB 64M Bit (4Mx16)", this
>  flash is 32Mb (2MB). Also note the CFI id does not match the comment.
>  Again, Yay.
> 
> Anyway, better safe than sorry, so I'll revert.
> 
> Thanks for following and catching this,

Great, thanks!

-- 
Stephen Checkoway