PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
BUG", which sounds like a warning, then calls exit(1), followed by
unreachable goto reset_flash.  All this commit does is expanding the
macro, so the smell becomes more poignant, and the macro can be
deleted.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/block/pflash_cfi01.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 9efa7aa9af..f73c30a3ee 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -49,12 +49,6 @@
 #include "sysemu/sysemu.h"
 #include "trace.h"
 
-#define PFLASH_BUG(fmt, ...) \
-do { \
-    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
-    exit(1); \
-} while(0)
-
 /* #define PFLASH_DEBUG */
 #ifdef PFLASH_DEBUG
 #define DPRINTF(fmt, ...)                                   \
@@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
                 pfl->status |= 0x80;
             } else {
                 DPRINTF("%s: unknown command for \"write block\"\n", __func__);
-                PFLASH_BUG("Write block confirm");
-                goto reset_flash;
+                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
+                exit(1);
             }
             break;
         default:
-- 
2.17.2

On 02/18/19 13:56, Markus Armbruster wrote:
> PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
> BUG", which sounds like a warning, then calls exit(1), followed by
> unreachable goto reset_flash.  All this commit does is expanding the
> macro, so the smell becomes more poignant, and the macro can be
> deleted.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9efa7aa9af..f73c30a3ee 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -49,12 +49,6 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  
> -#define PFLASH_BUG(fmt, ...) \
> -do { \
> -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
> -    exit(1); \
> -} while(0)
> -
>  /* #define PFLASH_DEBUG */
>  #ifdef PFLASH_DEBUG
>  #define DPRINTF(fmt, ...)                                   \
> @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->status |= 0x80;
>              } else {
>                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
> -                PFLASH_BUG("Write block confirm");
> -                goto reset_flash;
> +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
> +                exit(1);
>              }
>              break;
>          default:
> 

Technically speaking, the commit message is slightly incorrect, where it
says "all this commit does is expanding the macro" -- the "goto" is
being removed as well.

I like the attention to detail in that you didn't add the missing
newline character in the expanded fprintf() ;)

With the commit message tweaked, or not:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thakns
Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

> On 02/18/19 13:56, Markus Armbruster wrote:
>> PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
>> BUG", which sounds like a warning, then calls exit(1), followed by
>> unreachable goto reset_flash.  All this commit does is expanding the
>> macro, so the smell becomes more poignant, and the macro can be
>> deleted.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/block/pflash_cfi01.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 9efa7aa9af..f73c30a3ee 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -49,12 +49,6 @@
>>  #include "sysemu/sysemu.h"
>>  #include "trace.h"
>>  
>> -#define PFLASH_BUG(fmt, ...) \
>> -do { \
>> -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
>> -    exit(1); \
>> -} while(0)
>> -
>>  /* #define PFLASH_DEBUG */
>>  #ifdef PFLASH_DEBUG
>>  #define DPRINTF(fmt, ...)                                   \
>> @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>>                  pfl->status |= 0x80;
>>              } else {
>>                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
>> -                PFLASH_BUG("Write block confirm");
>> -                goto reset_flash;
>> +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
>> +                exit(1);
>>              }
>>              break;
>>          default:
>> 
>
> Technically speaking, the commit message is slightly incorrect, where it
> says "all this commit does is expanding the macro" -- the "goto" is
> being removed as well.

You're right.  I'll amend the commit message.

> I like the attention to detail in that you didn't add the missing
> newline character in the expanded fprintf() ;)

I wish I could claim it was intentional preservation of a bad smell as
fair warning to future readers...

> With the commit message tweaked, or not:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!

On 2/18/19 1:56 PM, Markus Armbruster wrote:
> PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
> BUG", which sounds like a warning, then calls exit(1), followed by
> unreachable goto reset_flash.  All this commit does is expanding the
> macro, so the smell becomes more poignant, and the macro can be
> deleted.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 9efa7aa9af..f73c30a3ee 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -49,12 +49,6 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  
> -#define PFLASH_BUG(fmt, ...) \
> -do { \
> -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
> -    exit(1); \
> -} while(0)
> -
>  /* #define PFLASH_DEBUG */
>  #ifdef PFLASH_DEBUG
>  #define DPRINTF(fmt, ...)                                   \
> @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>                  pfl->status |= 0x80;
>              } else {
>                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
> -                PFLASH_BUG("Write block confirm");
> -                goto reset_flash;
> +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
> +                exit(1);

Don't you want to use hw_error here?

      hw_error("PFLASH: Possible BUG - Write block confirm");

>              }
>              break;
>          default:
> 

On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 2/18/19 1:56 PM, Markus Armbruster wrote:
> > PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
> > BUG", which sounds like a warning, then calls exit(1), followed by
> > unreachable goto reset_flash.  All this commit does is expanding the
> > macro, so the smell becomes more poignant, and the macro can be
> > deleted.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  hw/block/pflash_cfi01.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> > index 9efa7aa9af..f73c30a3ee 100644
> > --- a/hw/block/pflash_cfi01.c
> > +++ b/hw/block/pflash_cfi01.c
> > @@ -49,12 +49,6 @@
> >  #include "sysemu/sysemu.h"
> >  #include "trace.h"
> >
> > -#define PFLASH_BUG(fmt, ...) \
> > -do { \
> > -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
> > -    exit(1); \
> > -} while(0)
> > -
> >  /* #define PFLASH_DEBUG */
> >  #ifdef PFLASH_DEBUG
> >  #define DPRINTF(fmt, ...)                                   \
> > @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
> >                  pfl->status |= 0x80;
> >              } else {
> >                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
> > -                PFLASH_BUG("Write block confirm");
> > -                goto reset_flash;
> > +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
> > +                exit(1);
>
> Don't you want to use hw_error here?
>
>       hw_error("PFLASH: Possible BUG - Write block confirm");

This should just be
   qemu_log_mask(LOG_GUEST_ERROR, ...);
(replacing both the DPRINTF and the PFLASH_BUG()).

It's triggerable by a guest (if it puts the device into write-block
mode and then feeds it a bogus command byte), so it's just a guest
error, not an issue with our model of the pflash.

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>> > PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
>> > BUG", which sounds like a warning, then calls exit(1), followed by
>> > unreachable goto reset_flash.  All this commit does is expanding the
>> > macro, so the smell becomes more poignant, and the macro can be
>> > deleted.
>> >
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  hw/block/pflash_cfi01.c | 10 ++--------
>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> > index 9efa7aa9af..f73c30a3ee 100644
>> > --- a/hw/block/pflash_cfi01.c
>> > +++ b/hw/block/pflash_cfi01.c
>> > @@ -49,12 +49,6 @@
>> >  #include "sysemu/sysemu.h"
>> >  #include "trace.h"
>> >
>> > -#define PFLASH_BUG(fmt, ...) \
>> > -do { \
>> > -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
>> > -    exit(1); \
>> > -} while(0)
>> > -
>> >  /* #define PFLASH_DEBUG */
>> >  #ifdef PFLASH_DEBUG
>> >  #define DPRINTF(fmt, ...)                                   \
>> > @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>> >                  pfl->status |= 0x80;
>> >              } else {
>> >                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
>> > -                PFLASH_BUG("Write block confirm");
>> > -                goto reset_flash;
>> > +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
>> > +                exit(1);
>>
>> Don't you want to use hw_error here?
>>
>>       hw_error("PFLASH: Possible BUG - Write block confirm");
>
> This should just be
>    qemu_log_mask(LOG_GUEST_ERROR, ...);
> (replacing both the DPRINTF and the PFLASH_BUG()).
>
> It's triggerable by a guest (if it puts the device into write-block
> mode and then feeds it a bogus command byte), so it's just a guest
> error, not an issue with our model of the pflash.

I can do that.  Thanks!

Markus Armbruster <armbru@redhat.com> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>>> > PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
>>> > BUG", which sounds like a warning, then calls exit(1), followed by
>>> > unreachable goto reset_flash.  All this commit does is expanding the
>>> > macro, so the smell becomes more poignant, and the macro can be
>>> > deleted.
>>> >
>>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> > ---
>>> >  hw/block/pflash_cfi01.c | 10 ++--------
>>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>>> >
>>> > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>> > index 9efa7aa9af..f73c30a3ee 100644
>>> > --- a/hw/block/pflash_cfi01.c
>>> > +++ b/hw/block/pflash_cfi01.c
>>> > @@ -49,12 +49,6 @@
>>> >  #include "sysemu/sysemu.h"
>>> >  #include "trace.h"
>>> >
>>> > -#define PFLASH_BUG(fmt, ...) \
>>> > -do { \
>>> > -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
>>> > -    exit(1); \
>>> > -} while(0)
>>> > -
>>> >  /* #define PFLASH_DEBUG */
>>> >  #ifdef PFLASH_DEBUG
>>> >  #define DPRINTF(fmt, ...)                                   \
>>> > @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>>> >                  pfl->status |= 0x80;
>>> >              } else {
>>> >                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
>>> > -                PFLASH_BUG("Write block confirm");
>>> > -                goto reset_flash;
>>> > +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
>>> > +                exit(1);
>>>
>>> Don't you want to use hw_error here?
>>>
>>>       hw_error("PFLASH: Possible BUG - Write block confirm");
>>
>> This should just be
>>    qemu_log_mask(LOG_GUEST_ERROR, ...);
>> (replacing both the DPRINTF and the PFLASH_BUG()).
>>
>> It's triggerable by a guest (if it puts the device into write-block
>> mode and then feeds it a bogus command byte), so it's just a guest
>> error, not an issue with our model of the pflash.
>
> I can do that.  Thanks!

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;

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Tue, 19 Feb 2019 at 12:41, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>
>>>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>>>> > PFLASH_BUG()'s lone use has a suspicious smell: it prints "Possible
>>>> > BUG", which sounds like a warning, then calls exit(1), followed by
>>>> > unreachable goto reset_flash.  All this commit does is expanding the
>>>> > macro, so the smell becomes more poignant, and the macro can be
>>>> > deleted.
>>>> >
>>>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> > ---
>>>> >  hw/block/pflash_cfi01.c | 10 ++--------
>>>> >  1 file changed, 2 insertions(+), 8 deletions(-)
>>>> >
>>>> > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>>> > index 9efa7aa9af..f73c30a3ee 100644
>>>> > --- a/hw/block/pflash_cfi01.c
>>>> > +++ b/hw/block/pflash_cfi01.c
>>>> > @@ -49,12 +49,6 @@
>>>> >  #include "sysemu/sysemu.h"
>>>> >  #include "trace.h"
>>>> >
>>>> > -#define PFLASH_BUG(fmt, ...) \
>>>> > -do { \
>>>> > -    fprintf(stderr, "PFLASH: Possible BUG - " fmt, ## __VA_ARGS__); \
>>>> > -    exit(1); \
>>>> > -} while(0)
>>>> > -
>>>> >  /* #define PFLASH_DEBUG */
>>>> >  #ifdef PFLASH_DEBUG
>>>> >  #define DPRINTF(fmt, ...)                                   \
>>>> > @@ -624,8 +618,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>>>> >                  pfl->status |= 0x80;
>>>> >              } else {
>>>> >                  DPRINTF("%s: unknown command for \"write block\"\n", __func__);
>>>> > -                PFLASH_BUG("Write block confirm");
>>>> > -                goto reset_flash;
>>>> > +                fprintf(stderr, "PFLASH: Possible BUG - Write block confirm");
>>>> > +                exit(1);
>>>>
>>>> Don't you want to use hw_error here?
>>>>
>>>>       hw_error("PFLASH: Possible BUG - Write block confirm");
>>>
>>> This should just be
>>>    qemu_log_mask(LOG_GUEST_ERROR, ...);
>>> (replacing both the DPRINTF and the PFLASH_BUG()).
>>>
>>> It's triggerable by a guest (if it puts the device into write-block
>>> mode and then feeds it a bogus command byte), so it's just a guest
>>> error, not an issue with our model of the pflash.
>>
>> I can do that.  Thanks!
>
> 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;

With this change:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <armbru@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.)

thanks
-- PMM

On 2/21/19 10:38 AM, Peter Maydell wrote:
> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <armbru@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");

Regards,

Phil.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 2/21/19 10:38 AM, Peter Maydell wrote:
>> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <armbru@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.

Markus Armbruster <armbru@redhat.com> writes:

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

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

--
Alex Bennée

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 2/21/19 10:38 AM, Peter Maydell wrote:
>> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <armbru@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):

Since I'm a clueless idiot on pflash, I need to process your argument
real slow, so I can write a commit message that doesn't document my
cluelessness forever.

We have a little state machine, and its state is encoded in
pfl->wcycle. pfl->cmd, pfl->counter.  I'm going to show it as

    (value of pfl->wcycle, value of pfl->cmd, value of pfl->counter)

for brevity.

We start with (0, don't care, don't care).

A guest write sends us a width, an address, and a value.
pflash_mem_write_with_attrs() does permission checking, and
pflash_write() the actual work.  We enter it with @offset, @value and
@width holding the message.

    cmd = value;

    trace_pflash_write(offset, value, width, pfl->wcycle);
    if (!pfl->wcycle) {
        /* Set the device in I/O access mode */
        memory_region_rom_device_set_romd(&pfl->mem, false);
    }

@cmd is @value truncated to 8 bits.

> 1/ On first write cycle we have
>
>   - address = flash_page_address (we store it in pfl->counter)
>   - data = flash_command (0xe8: enter Multi-WRITE)

    switch (pfl->wcycle) {
    case 0:
        /* read mode */
        switch (cmd) {
        [...]
        case 0xe8: /* Write to buffer */
            DPRINTF("%s: Write to buffer\n", __func__);
            pfl->status |= 0x80; /* Ready! */
            break;
        [...]
        pfl->wcycle++;
        pfl->cmd = cmd;
        break;

Transition from (0, don't care, don't care) to (1, 0xE8, don't care).

I can't see "we store it in pfl->counter".

Note that the address (passed in @offset) is entirely ignored.

> 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.

    case 1:
        switch (pfl->cmd) {
        [...]
        case 0xe8:
            /* Mask writeblock size based on device width, or bank width if
             * device width not specified.
             */
            if (pfl->device_width) {
                value = extract32(value, 0, pfl->device_width * 8);
            } else {
                value = extract32(value, 0, pfl->bank_width * 8);
            }
            DPRINTF("%s: block write of %x bytes\n", __func__, value);
            pfl->counter = value;
            pfl->wcycle++;
            break;
        [...]
        }
        break;

Transition from (1, 0xE8, don't care) to (2, 0xE8, N), where N is passed
in @value.

Again, the address (passed in @offset) is ignored.

Nothing is written to the block backend, yet.

>     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.

    case 2:
        switch (pfl->cmd) {
        case 0xe8: /* Block write */
            if (!pfl->ro) {
                pflash_data_write(pfl, offset, value, width, be);
            } else {
                pfl->status |= 0x10; /* Programming error */
            }

Write to memory, with pflash_data_write(), but don't flush to the
backend, yet.  This is (guest-visibly!) wrong.  It's not quite "instead
of starting to write the data in a buffer, we write it directly to the
block backend."

Note that we happily accept any address and width.  I suspect we should
only accept consecutive addresses and consistent width.

            pfl->status |= 0x80;

            if (!pfl->counter) {
                hwaddr mask = pfl->writeblock_size - 1;
                mask = ~mask;

                DPRINTF("%s: block write finished\n", __func__);
                pfl->wcycle++;
                if (!pfl->ro) {
                    /* Flush the entire write buffer onto backing storage.  */
                    pflash_update(pfl, offset & mask, pfl->writeblock_size);
                } else {
                    pfl->status |= 0x10; /* Programming error */
                }
            }

If this is the multi-write's last write, flush the entire write block to
the backend *boggle*.

            pfl->counter--;
            break;
        [...]
        }
        break;

Transition from (2, 0xE8, N) to

    (2, 0xE8, N-1)          if N>0
    (3, 0xE8, don't care)   if N==0

> 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.

    case 3: /* Confirm mode */
        switch (pfl->cmd) {
        case 0xe8: /* Block write */
            if (cmd == 0xd0) {
                pfl->wcycle = 0;
                pfl->status |= 0x80;

On CONFIRM, transition from (3, 0xE8, don't care) back to the intial
state (0, don't care, don't care).

            } else {
                DPRINTF("%s: unknown command for \"write block\"\n", __func__);
                PFLASH_BUG("Write block confirm");
                goto reset_flash;
            }

On anything else, pea brain implodes, and we exit(1).

            break;

The fact that we let a hack job like this anywhere near "secure boot" is
either frightening or amusing, depending on one's level of cynicism
towards virtual secure boot.

> 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");

Yup, makes sense.

Perhaps we should also LOG_UNIMP on the first write of a multi-write
already, because the guest-visible wrongness starts right there.  You
tell me.

Two things left to do for this series: FIXME comments, and a commit
message.  Before I tackle them, I'll give you a chance to correct
misunderstandings in my reasoning.

somewhat off-topic:

On 02/21/19 10:38, Peter Maydell wrote:
> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <armbru@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.)

since we're talking "reset_flash", I'll note that there is no actual
reset handler for cfi.pflash01. I found out recently, via:

https://bugzilla.redhat.com/show_bug.cgi?id=1678713

Thanks
Laszlo

On Thu, 21 Feb 2019 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
> since we're talking "reset_flash", I'll note that there is no actual
> reset handler for cfi.pflash01. I found out recently, via:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1678713

Yes; this isn't uncommon for some of the really old
device models. It should definitely have one added.

You are correct also that the timer in the pflash_cfi01
model is dead code -- it has always been so, since the
device was added in 2007. The reason it is there is that
pflash_cfi01 was created as a copy-and-hack of the
cfi02 device. In cfi02 we do use the timer, as a way
of simulating "make full-chip and sector erases take a
guest-visible amount of time rather than completing
instantaneously". cfi01 doesn't do that (and I think
may not implement anything other than block erase),
but the timer initialization code was left in rather
than being deleted as part of the copy-and-hack.

thanks
-- PMM

On 02/21/19 13:38, Peter Maydell wrote:
> On Thu, 21 Feb 2019 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
>> since we're talking "reset_flash", I'll note that there is no actual
>> reset handler for cfi.pflash01. I found out recently, via:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> 
> Yes; this isn't uncommon for some of the really old
> device models. It should definitely have one added.
> 
> You are correct also that the timer in the pflash_cfi01
> model is dead code -- it has always been so, since the
> device was added in 2007. The reason it is there is that
> pflash_cfi01 was created as a copy-and-hack of the
> cfi02 device. In cfi02 we do use the timer, as a way
> of simulating "make full-chip and sector erases take a
> guest-visible amount of time rather than completing
> instantaneously". cfi01 doesn't do that (and I think
> may not implement anything other than block erase),
> but the timer initialization code was left in rather
> than being deleted as part of the copy-and-hack.

Thank you, I've linked this back into the RHBZ.

Laszlo

On 2/21/19 1:46 PM, Laszlo Ersek wrote:
> On 02/21/19 13:38, Peter Maydell wrote:
>> On Thu, 21 Feb 2019 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>> since we're talking "reset_flash", I'll note that there is no actual
>>> reset handler for cfi.pflash01. I found out recently, via:
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>>
>> Yes; this isn't uncommon for some of the really old
>> device models. It should definitely have one added.
>>
>> You are correct also that the timer in the pflash_cfi01
>> model is dead code -- it has always been so, since the
>> device was added in 2007. The reason it is there is that
>> pflash_cfi01 was created as a copy-and-hack of the
>> cfi02 device. In cfi02 we do use the timer, as a way
>> of simulating "make full-chip and sector erases take a
>> guest-visible amount of time rather than completing
>> instantaneously". cfi01 doesn't do that (and I think
>> may not implement anything other than block erase),

This time is flash-device specific and is currently hardcoded.

The guest learn from the CFI table how long it should wait
before polling/accessing the flash, or take measures in case
of timeout.

We set these values in _realize():

    ...
    /* Typical timeout for block erase (512 ms) */
    pfl->cfi_table[0x21] = 0x09;
    /* Typical timeout for full chip erase (4096 ms) */
    pfl->cfi_table[0x22] = 0x0C;
    ...
    /* Max timeout for block erase */
    pfl->cfi_table[0x25] = 0x0A;
    /* Max timeout for chip erase */
    pfl->cfi_table[0x26] = 0x0D;

The timer is triggered in _write(), where we use other hardcoded
values (which are luckily in range with the CFI announced ones):

        /* Let's wait 5 seconds before chip erase is done */
        timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                  (NANOSECONDS_PER_SECOND * 5));

        /* Let's wait 1/2 second before sector erase is done */
        timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                  (NANOSECONDS_PER_SECOND / 2));

When emulating embedded devices, it is very desirable to have those
timers working, and I'd prefer we fix that on CFI01 rather than simply
removing the unused timer.

Now for the case of "Virt" machines, it is probably pointless to add
flash delay and we should remove the timer use.

>> but the timer initialization code was left in rather
>> than being deleted as part of the copy-and-hack.
> 
> Thank you, I've linked this back into the RHBZ.
> 
> Laszlo
> 

On 02/21/19 17:39, Philippe Mathieu-Daudé wrote:
> On 2/21/19 1:46 PM, Laszlo Ersek wrote:
>> On 02/21/19 13:38, Peter Maydell wrote:
>>> On Thu, 21 Feb 2019 at 12:07, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> since we're talking "reset_flash", I'll note that there is no actual
>>>> reset handler for cfi.pflash01. I found out recently, via:
>>>>
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
>>>
>>> Yes; this isn't uncommon for some of the really old
>>> device models. It should definitely have one added.
>>>
>>> You are correct also that the timer in the pflash_cfi01
>>> model is dead code -- it has always been so, since the
>>> device was added in 2007. The reason it is there is that
>>> pflash_cfi01 was created as a copy-and-hack of the
>>> cfi02 device. In cfi02 we do use the timer, as a way
>>> of simulating "make full-chip and sector erases take a
>>> guest-visible amount of time rather than completing
>>> instantaneously". cfi01 doesn't do that (and I think
>>> may not implement anything other than block erase),
> 
> This time is flash-device specific and is currently hardcoded.
> 
> The guest learn from the CFI table how long it should wait
> before polling/accessing the flash, or take measures in case
> of timeout.
> 
> We set these values in _realize():
> 
>     ...
>     /* Typical timeout for block erase (512 ms) */
>     pfl->cfi_table[0x21] = 0x09;
>     /* Typical timeout for full chip erase (4096 ms) */
>     pfl->cfi_table[0x22] = 0x0C;
>     ...
>     /* Max timeout for block erase */
>     pfl->cfi_table[0x25] = 0x0A;
>     /* Max timeout for chip erase */
>     pfl->cfi_table[0x26] = 0x0D;
> 
> The timer is triggered in _write(), where we use other hardcoded
> values (which are luckily in range with the CFI announced ones):
> 
>         /* Let's wait 5 seconds before chip erase is done */
>         timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                   (NANOSECONDS_PER_SECOND * 5));
> 
>         /* Let's wait 1/2 second before sector erase is done */
>         timer_mod(pfl->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                   (NANOSECONDS_PER_SECOND / 2));
> 
> When emulating embedded devices, it is very desirable to have those
> timers working, and I'd prefer we fix that on CFI01 rather than simply
> removing the unused timer.
> 
> Now for the case of "Virt" machines, it is probably pointless to add
> flash delay and we should remove the timer use.

If the timer logic can be completed in such a way that existent OVMF
code sees no change at all, on the machine types that it currently runs
on (that is, on *unversioned* i440fx and q35), I'm OK with the idea.

Thanks,
Laszlo

> 
>>> but the timer initialization code was left in rather
>>> than being deleted as part of the copy-and-hack.
>>
>> Thank you, I've linked this back into the RHBZ.
>>
>> Laszlo
>>