Re: [Qemu-block] [Qemu-devel] [PATCH 02/10] pflash: Macro PFLASH_BUG() is used just once, expand

[Alex Bennée]

Markus Armbruster <address@hidden> writes:

> Philippe Mathieu-Daudé <address@hidden> writes:
>
>> On 2/21/19 10:38 AM, Peter Maydell wrote:
>>> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <address@hidden> wrote:
>>>> Double-checking... you want me to keep goto reset_flash, like this:
>>>>
>>>> @@ -623,8 +617,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr 
>>>> offset,
>>>>                  pfl->wcycle = 0;
>>>>                  pfl->status |= 0x80;
>>>>              } else {
>>>> -                DPRINTF("%s: unknown command for \"write block\"\n", 
>>>> __func__);
>>>> -                PFLASH_BUG("Write block confirm");
>>>> +                qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                              "unknown command for \"write block\"\n");
>>>>                  goto reset_flash;
>>>>              }
>>>>              break;
>>>
>>> Yes. (We seem to handle most kinds of guest errors in programming
>>> the flash by reset_flash.)
>>
>> Oh I missed the context of the patch here.
>>
>> So for the case of the Multi-WRITE command (0xe8):
>>
>> 1/ On first write cycle we have
>>
>>   - address = flash_page_address (we store it in pfl->counter)
>>   - data = flash_command (0xe8: enter Multi-WRITE)
>>
>> 2/ Second cycle:
>>
>>   - address = flash_page_address
>>     We should check it matches flash_page_address
>>     of cycle 1/, but we don't.
>>   - data: N
>>
>>     "N is the number of elements (bytes / words / double words),
>>     minus one, to be written to the write buffer. Expected count
>>     ranges are N = 00h to N = 7Fh (e.g., 1 to 128 bytes) in 8-bit
>>     mode, N = 00h to N = 003Fh in 16-bit mode, and N = 00h to
>>     N = 1Fh in 32-bit mode. Bus cycles 3 and higher are for writing
>>     data into the write buffer. The confirm command (D0h) is
>>     expected after exactly N + 1 write cycles; any other command at
>>     that point in the sequence will prevent the transfer of the
>>     buffer to the array (the write will be aborted)."
>>
>>     Instead of starting to write the data in a buffer, we write it
>>     directly to the block backend.
>>     Instead of starting to write from cycle 3+, we write now in 2,
>>     and keep cycle count == 2 (pfl->wcycle) until all data is
>>     written, where we increment at 3.
>>
>> 3/ Cycles 3+
>>
>>   - address = word index (relative to the page address)
>>   - data = word value
>>
>>     We check for the CONFIRM command, and switch the device back
>>     to READ mode (setting Status), or reset the device (which is
>>     modelled the same way).
>>
>>     Very silly: If the guest cancelled and never sent the CONFIRM
>>     command, the data is already written/flushed back.
>>
>> So back to the previous code snippet, regardless the value, this
>> is neither a hw_error() nor a GUEST_ERROR. This code is simply not
>> correct. Eventually the proper (polite) error message should be:
>>
>>     qemu_log_mask(LOG_UNIMP, "MULTI_WRITE: Abort not implemented,"
>>                              " the data is already written"
>>                              " on storage!\n"
>>                              "Nevertheless resetting the flash"
>>                              " into READ mode.\n");
>
> Oww.
>
> This code is a swamp.
>
> Peter, Alex, do you agree with Phil's analysis?  If yes, I'll update my
> patch once more.

I'm happy to defer to Phil's obvious expertise here ;-)

--
Alex Bennée