On 07/18/19 17:03, Laszlo Ersek wrote: > 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 <ler...@redhat.com> >> Reviewed-by: John Snow <js...@redhat.com> >> Reviewed-by: Alistair Francis <alistair.fran...@wdc.com> >> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> --- >> 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 <ler...@redhat.com> > > 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 <ler...@redhat.com> > > > (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).
Regression-tested-by: Laszlo Ersek <ler...@redhat.com>