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

[Philippe Mathieu-Daudé]

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,

Phil.

[...]
>>>>> Remove the 32-bit dead code.
>>>>>
>>>>> Reviewed-by: Alistair Francis <address@hidden>
>>>>> Message-Id: <address@hidden>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>>> ---
>>>>> hw/block/pflash_cfi02.c | 5 +----
>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
>>>>> index 83084b9d72..5392290c72 100644
>>>>> --- a/hw/block/pflash_cfi02.c
>>>>> +++ b/hw/block/pflash_cfi02.c
>>>>> @@ -317,8 +317,6 @@ static uint64_t pflash_read(void *opaque, hwaddr 
>>>>> offset, unsigned int width)
>>>>>    boff = offset & 0xFF;
>>>>>    if (pfl->width == 2) {
>>>>>        boff = boff >> 1;
>>>>> -    } else if (pfl->width == 4) {
>>>>> -        boff = boff >> 2;
>>>>>    }
>>>>>    switch (pfl->cmd) {
>>>>>    default:
>>>>> @@ -449,8 +447,6 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>>>> uint64_t value,
>>>>>    boff = offset;
>>>>>    if (pfl->width == 2) {
>>>>>        boff = boff >> 1;
>>>>> -    } else if (pfl->width == 4) {
>>>>> -        boff = boff >> 2;
>>>>>    }
>>>>>    /* Only the least-significant 11 bits are used in most cases. */
>>>>>    boff &= 0x7FF;
>>>>> @@ -710,6 +706,7 @@ static void pflash_write(void *opaque, hwaddr offset, 
>>>>> uint64_t value,
>>>>> static const MemoryRegionOps pflash_cfi02_ops = {
>>>>>    .read = pflash_read,
>>>>>    .write = pflash_write,
>>>>> +    .impl.max_access_size = 2,
>>>>>    .valid.min_access_size = 1,
>>>>>    .valid.max_access_size = 4,
>>>>>    .endianness = DEVICE_NATIVE_ENDIAN,
>>>>> -- 
>>>>> 2.20.1