| Message ID | 20190718104837.13905-2-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/pflash_cfi01: Add DeviceReset() handler | expand |
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
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>
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.
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. >
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. >>
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.
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.
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
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 >
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;