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

[Laszlo Ersek]

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

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