Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On 2/21/19 10:38 AM, Peter Maydell wrote:
>> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <arm...@redhat.com> 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.
