On 7/18/19 8:38 PM, Laszlo Ersek wrote: > 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; >>> >>
s/but/bug/ typo fixed. >> (2) Reviewed-by: Laszlo Ersek <ler...@redhat.com> Thanks! >> >> 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> Thank you a lot again for all your testing, I also noted your steps and will try to automate them. Best regards, Phil.