Re: [Qemu-devel] [PATCH-for-4.1 v7 1/1] hw/block/pflash_cfi01: Add missing DeviceReset() handler

[Laszlo Ersek]

On 07/18/19 12:48, Philippe Mathieu-Daudé wrote:
> To avoid incoherent states when the machine resets (see but report

(1) For the PULL request, please fix the typo: s/but/bug/

> below), add the device reset callback.
> 
> A "system reset" sets the device state machine in READ_ARRAY mode
> and, after some delay, set the SR.7 READY bit.
> 
> Since we do not model timings, we set the SR.7 bit directly.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1678713
> Reported-by: Laszlo Ersek <address@hidden>
> Reviewed-by: John Snow <address@hidden>
> Reviewed-by: Alistair Francis <address@hidden>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> ---
>  hw/block/pflash_cfi01.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 435be1e35c..a1ec1faae5 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -865,6 +865,24 @@ static void pflash_cfi01_realize(DeviceState *dev, Error 
> **errp)
>      pfl->cfi_table[0x3f] = 0x01; /* Number of protection fields */
>  }
>  
> +static void pflash_cfi01_system_reset(DeviceState *dev)
> +{
> +    PFlashCFI01 *pfl = PFLASH_CFI01(dev);
> +
> +    /*
> +     * The command 0x00 is not assigned by the CFI open standard,
> +     * but QEMU historically uses it for the READ_ARRAY command (0xff).
> +     */
> +    pfl->cmd = 0x00;
> +    pfl->wcycle = 0;
> +    memory_region_rom_device_set_romd(&pfl->mem, true);
> +    /*
> +     * The WSM ready timer occurs at most 150ns after system reset.
> +     * This model deliberately ignores this delay.
> +     */
> +    pfl->status = 0x80;
> +}
> +
>  static Property pflash_cfi01_properties[] = {
>      DEFINE_PROP_DRIVE("drive", PFlashCFI01, blk),
>      /* num-blocks is the number of blocks actually visible to the guest,
> @@ -909,6 +927,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, 
> void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +    dc->reset = pflash_cfi01_system_reset;
>      dc->realize = pflash_cfi01_realize;
>      dc->props = pflash_cfi01_properties;
>      dc->vmsd = &vmstate_pflash;
> 

(2) Reviewed-by: Laszlo Ersek <address@hidden>

A *future* improvement (meant just for this surgical reset handler --
not meaning any large cfi01 overhaul!) could be the addition of a trace
point, at the top of pflash_cfi01_system_reset().

But that is strictly "nice to have", and not necessary to include in the
present bugfix.


(3) Using OVMF IA32X64 (including the edk2 SMM stack), I've
regression-tested this patch, on top of v4.1.0-rc1, with KVM. As follows:

(3a) Normal reboot from the UEFI shell ("reset -c" command)

(3b) Normal reboot from the Linux guest prompt ("reboot" command)

(3c1) Reset as part of ACPI S3 suspend/resume
(3c2) then use "efibootmgr -n / -N" to write to pflash (by virtue of
setting / deleting the standardized BootNext UEFI variable)

(3d1) Boot to setup TUI with SB enabled
(3d2) erase Platform Key in setup TUI (disables SB)
(3d3) reboot from within setup TUI
(3d4) proceed to UEFI shell
(3d5) enable SB with EnrollDefaultKeys.efi
(3d6) reboot from UEFI shell
(3d7) proceeed to Linux guest
(3d8) verify SB enablement (dmesg, "mokutil --sb-state")

(As an added exercise, step (3d4) triggered an "FTW" (fault tolerant
write) "reclaim" (basically a defragmentation of the journaled
"filesystem" that the firmware keeps in the flash, as a logical "middle
layer"), and that worked fine too.)

Regression-tested-by: Laszlo Ersek <address@hidden>


(4) I plan to provide R-t-b in the evening from aarch64 KVM too, using
the edk2 ArmVirtQemu firmware. Only the first two steps from (3) will be
covered (no ACPI S3, no SB).

Thanks!
Laszlo