Re: [Qemu-devel] [PATCH-for-4.1 v6 0/5] hw/block/pflash_cfi01: Add DeviceReset() handler

[Laszlo Ersek]

Hi Phil,

On 07/17/19 00:15, Philippe Mathieu-Daudé wrote:
> Hello it's me again, insisting with this series because there are at
> least 2 different report of guests bricked on reset due to the bug
> fixed by patch #5:
> https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> https://bugzilla.redhat.com/show_bug.cgi?id=1704584
> 
> Patches missing review: 2 and 3
> 
> The pflash device lacks a reset() function.
> When a machine is resetted, the flash might be in an
> inconsistent state, leading to unexpected behavior.
> Resolve this issue by adding a DeviceReset() handler.
> 
> Since v1: https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg00962.html
> - addressed Laszlo review comments
> 
> Since v2: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00395.html
> - consider migration (Laszlo, Peter)
> 
> Since v3: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01668.html
> - more reliable migration (Dave)
> - dropped patches 6-9 not required for next release
> 
> Since v4: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg02785.html
> - document why using READ_ARRAY value 0x00 for migration is safe
> 
> Since v5: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03366.html
> - avoid trying to be spec-compliant and messing with migration. KISS.
>   review/test tags reset, sorry.

I have a number of questions / requests.


(1) The last I recall from the v5 discussion is Markus asking about some
risky cases (corner cases?) related to migration.

So what is the new avenue taken in v6? What does "continue ignoring spec
compliance" mean, with regard to migration?

My vague understanding is that we're not trying to answer Markus's
questions; instead, we're side-stepping them, by doing something else.
That works for me, but can we please summarize in a bit more detail?
Like, "in v6, we're not mapping 0x00 vs. 0xff across migration because..."

Yes, I could inter-diff v5 vs. v6, but I know way too little about
pflash. I'd miss how our *dropping* of the special 0xff mapping impacts
our conformance to the data sheet.

I'm not asking for commit message updates, just a bit more explanation
in this free-form discussion. (I looked for it under v5, and couldn't
find it.)


(2) Has someone regression-tested v6 for migration specifically?

Or, is v6 not "risky" wrt. migration any longer?


(3) I'm fine regression testing v6 too (without migration, again).
Please ping me separately once the reviews have converged and the series
is otherwise ready for merging.

Thanks!
Laszlo


> $ git backport-diff -u v5
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, 
> respectively
> 
> 001/5:[----] [-C] 'hw/block/pflash_cfi01: Removed an unused timer'
> 002/5:[down] 'hw/block/pflash_cfi01: Document use of non-CFI compliant 
> command '0x00''
> 003/5:[0031] [FC] 'hw/block/pflash_cfi01: Extract pflash_mode_read_array()'
> 004/5:[down] 'hw/block/pflash_cfi01: Rename 'reset_flash' label as 
> 'mode_read_array''
> 005/5:[----] [--] 'hw/block/pflash_cfi01: Add the DeviceReset() handler'
> 
> Regards,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (5):
>   hw/block/pflash_cfi01: Removed an unused timer
>   hw/block/pflash_cfi01: Document use of non-CFI compliant command
>     '0x00'
>   hw/block/pflash_cfi01: Extract pflash_mode_read_array()
>   hw/block/pflash_cfi01: Rename 'reset_flash' label as 'mode_read_array'
>   hw/block/pflash_cfi01: Add the DeviceReset() handler
> 
>  hw/block/pflash_cfi01.c | 77 +++++++++++++++++++++--------------------
>  hw/block/trace-events   |  1 +
>  2 files changed, 41 insertions(+), 37 deletions(-)
>