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

[Philippe Mathieu-Daudé]

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 <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;
>>>
>>

s/but/bug/ typo fixed.

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

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

Thank you a lot again for all your testing, I also noted your steps and
will try to automate them.

Best regards,

Phil.