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

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

[Philippe Mathieu-Daudé]

The pflash device lacks a reset() function.
When a machine is resetted, the flash might be in an
inconsistent state, leading to unexpected behavior.
Resolve this issue by adding a DeviceReset() handler.

v7: Surgical bugfix, do not attempt to improve the model in any
    way, thus ignoring all comments from previous versions.
    No migration impact.

Usual regression testing is welcomed, but probably pointless,
except done in the way describes in the following bug reports:
https://bugzilla.redhat.com/show_bug.cgi?id=1678713
https://bugzilla.redhat.com/show_bug.cgi?id=1704584
That is, resetting the machine when it is accessing the flash
device. IMO testing after the guest is done accessing the flash
device is totally pointless.

If no objection on this series after a day, I plan to send a
pull request to get this bugfix into 4.1.0-rc2.

Regards,

Phil.

Philippe Mathieu-Daudé (1):
  hw/block/pflash_cfi01: Add missing DeviceReset() handler

 hw/block/pflash_cfi01.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.20.1

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

[Philippe Mathieu-Daudé]

To avoid incoherent states when the machine resets (see but report
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 <lersek@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@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.20.1

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 <lersek@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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 <lersek@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 <lersek@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).

Thanks!
Laszlo

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 <lersek@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@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 <lersek@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 <lersek@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 <lersek@redhat.com>

Re: [Qemu-devel] [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 <lersek@redhat.com>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@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 <lersek@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 <lersek@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 <lersek@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.

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

[Philippe Mathieu-Daudé]

Hi Laszlo,

On 7/18/19 9:35 PM, Philippe Mathieu-Daudé wrote:
> 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
[...]>>> (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 <lersek@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 <lersek@redhat.com>

Patchwork doesn't recognize your R-t-b tag:

https://patchwork.ozlabs.org/patch/1133671/

Should I change it for a Tested-by, or add as it?

Thanks,

Phil.

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

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

[Laszlo Ersek]

On 07/19/19 18:19, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 7/18/19 9:35 PM, Philippe Mathieu-Daudé wrote:
>> 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
> [...]>>> (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 <lersek@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 <lersek@redhat.com>
> 
> Patchwork doesn't recognize your R-t-b tag:
> 
> https://patchwork.ozlabs.org/patch/1133671/
> 
> Should I change it for a Tested-by, or add as it?

Please pick it up manually, as it is, if that's possible.

I prefer to dedicate "Tested-by" to cases where my before-after
comparison highlights a difference (i.e., bug disappears, or feature
appears). I dedicate "R-t-b" to cases where nothing observable changes
(in accordance with my expectation).

Thanks!
Laszlo

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

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

[Philippe Mathieu-Daudé]

On 7/22/19 6:51 PM, Laszlo Ersek wrote:
> On 07/19/19 18:19, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 7/18/19 9:35 PM, Philippe Mathieu-Daudé wrote:
>>> 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
>> [...]>>> (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 <lersek@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 <lersek@redhat.com>
>>
>> Patchwork doesn't recognize your R-t-b tag:
>>
>> https://patchwork.ozlabs.org/patch/1133671/
>>
>> Should I change it for a Tested-by, or add as it?
> 
> Please pick it up manually, as it is, if that's possible.
> 
> I prefer to dedicate "Tested-by" to cases where my before-after
> comparison highlights a difference (i.e., bug disappears, or feature
> appears). I dedicate "R-t-b" to cases where nothing observable changes
> (in accordance with my expectation).

OK, thanks for your explanation!

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

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

[Peter Maydell]

On Mon, 22 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 07/19/19 18:19, Philippe Mathieu-Daudé wrote:
> > Hi Laszlo,
> >
> > On 7/18/19 9:35 PM, Philippe Mathieu-Daudé wrote:
> >> On 7/18/19 8:38 PM, Laszlo Ersek wrote:
> >>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> >
> > Patchwork doesn't recognize your R-t-b tag:
> >
> > https://patchwork.ozlabs.org/patch/1133671/
> >
> > Should I change it for a Tested-by, or add as it?
>
> Please pick it up manually, as it is, if that's possible.
>
> I prefer to dedicate "Tested-by" to cases where my before-after
> comparison highlights a difference (i.e., bug disappears, or feature
> appears). I dedicate "R-t-b" to cases where nothing observable changes
> (in accordance with my expectation).

The counter-argument to this is that nobody else is using
this convention (there are exactly 0 instances of
"Regression-tested-by" in the project git log as far as
I can see), and so in practice people reading the commits
won't really know what you meant by it. Everybody else
on the project uses "Tested-by" to mean either of the
two cases you describe above, without distinction...

(At one point we talked about using checkpatch to enforce
that we used a particular set of tags, mostly to avoid
people managing to typo the tagname, but also partly to
retain some consistency of usage.)

thanks
-- PMM

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

[Laszlo Ersek]

On 07/22/19 19:12, Peter Maydell wrote:
> On Mon, 22 Jul 2019 at 17:51, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 07/19/19 18:19, Philippe Mathieu-Daudé wrote:
>>> Hi Laszlo,
>>>
>>> On 7/18/19 9:35 PM, Philippe Mathieu-Daudé wrote:
>>>> On 7/18/19 8:38 PM, Laszlo Ersek wrote:
>>>>> Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Patchwork doesn't recognize your R-t-b tag:
>>>
>>> https://patchwork.ozlabs.org/patch/1133671/
>>>
>>> Should I change it for a Tested-by, or add as it?
>>
>> Please pick it up manually, as it is, if that's possible.
>>
>> I prefer to dedicate "Tested-by" to cases where my before-after
>> comparison highlights a difference (i.e., bug disappears, or feature
>> appears). I dedicate "R-t-b" to cases where nothing observable changes
>> (in accordance with my expectation).
> 
> The counter-argument to this is that nobody else is using
> this convention (there are exactly 0 instances of
> "Regression-tested-by" in the project git log as far as
> I can see), and so in practice people reading the commits
> won't really know what you meant by it. Everybody else
> on the project uses "Tested-by" to mean either of the
> two cases you describe above, without distinction...

OK. If "Tested-by" carries both meanings in the QEMU git log, then I'm
fine with either tag (T-b or R-t-b) from me on this patch. (Or I'll try
to remember this in the future anyway, seeing that Phil has submitted a
pull request already.)

Thanks
Laszlo

> (At one point we talked about using checkpatch to enforce
> that we used a particular set of tags, mostly to avoid
> people managing to typo the tagname, but also partly to
> retain some consistency of usage.)
> 
> thanks
> -- PMM
> 

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

[Philippe Mathieu-Daudé]

On 7/18/19 12:48 PM, Philippe Mathieu-Daudé wrote:
> To avoid incoherent states when the machine resets (see but report
> 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 <lersek@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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;
> 

Queued to pflash/next, thanks.