[PATCH-for-4.1,v7,1/1] hw/block/pflash_cfi01: Add missing DeviceReset() handler
diff mbox series

Message ID 20190718104837.13905-2-philmd@redhat.com
State New
Headers show
Series
  • hw/block/pflash_cfi01: Add DeviceReset() handler
Related show

Commit Message

Philippe Mathieu-Daudé July 18, 2019, 10:48 a.m. UTC
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(+)

Comments

Laszlo Ersek July 18, 2019, 3:03 p.m. UTC | #1
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
Laszlo Ersek July 18, 2019, 6:38 p.m. UTC | #2
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>
Philippe Mathieu-Daudé July 18, 2019, 7:35 p.m. UTC | #3
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.
Philippe Mathieu-Daudé July 19, 2019, 4:19 p.m. UTC | #4
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.
>
Laszlo Ersek July 22, 2019, 4:51 p.m. UTC | #5
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.
>>
Philippe Mathieu-Daudé July 22, 2019, 4:55 p.m. UTC | #6
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.
Philippe Mathieu-Daudé July 22, 2019, 4:55 p.m. UTC | #7
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.
Peter Maydell July 22, 2019, 5:12 p.m. UTC | #8
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
Laszlo Ersek July 23, 2019, 7:38 a.m. UTC | #9
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
>

Patch
diff mbox series

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;