| Message ID | 20190702001301.4768-6-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/pflash_cfi01: Add DeviceReset() handler | expand |
On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote: > A "system reset" sets the device state machine in READ_ARRAY mode > and, after some delay, set the SR.7 READY bit. > > We do not model timings, so we set the SR.7 bit directly. > > This pflash device is a child of TYPE_DEVICE. > The TYPE_DEVICE interface provide a DeviceReset handler which will > be called after the device is realized, and each time the machine > resets itself. > > To avoid incoherent states when the machine resets (see but report > below), factor out the reset code into pflash_cfi01_system_reset, > and register the method as a device reset callback. > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713 > Reported-by: Laszlo Ersek <lersek@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Does reset always get called as part of realize, really? Or are we just trusting that the device is probably going to get reset by the guest during bringup? > --- > hw/block/pflash_cfi01.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index dd1dfd266b..8d632ea941 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -763,8 +763,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > pfl->max_device_width = pfl->device_width; > } > > - pflash_mode_read_array(pfl); > - pfl->status = 0x80; /* WSM ready */ > /* Hardcoded CFI table */ > /* Standard "QRY" string */ > pfl->cfi_table[0x10] = 'Q'; > @@ -852,6 +850,18 @@ 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); > + > + pflash_mode_read_array(pfl); > + /* > + * 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, > @@ -896,6 +906,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; >
On Tue, 2 Jul 2019 at 04:16, John Snow <jsnow@redhat.com> wrote: > Does reset always get called as part of realize, really? > > Or are we just trusting that the device is probably going to get reset > by the guest during bringup? Reset is not called "as part of realize", but it is guaranteed to be called after realize and before we try to run the guest, as long as the device is in the qbus tree. Things are in the qbus tree if either: * they're plugged into something already in the tree (eg pci devices, scsi disks) * they're a sysbus device (which is automatically plugged into the 'main system bus' which is effectively the root of the qbus tree) In this case TYPE_PFLASH_CFI01 is a subclass of TYPE_SYS_BUS_DEVICE, so it will always be reset as part of system reset. (the main things which don't get automatically reset are direct subclasses of TYPE_DEVICE, notably CPU objects.) thanks -- PMM
On 7/2/19 11:23 AM, Peter Maydell wrote: > On Tue, 2 Jul 2019 at 04:16, John Snow <jsnow@redhat.com> wrote: >> Does reset always get called as part of realize, really? >> >> Or are we just trusting that the device is probably going to get reset >> by the guest during bringup? > > Reset is not called "as part of realize", but it is guaranteed > to be called after realize and before we try to run the guest, > as long as the device is in the qbus tree. Things are in the > qbus tree if either: > * they're plugged into something already in the tree (eg > pci devices, scsi disks) > * they're a sysbus device (which is automatically plugged into > the 'main system bus' which is effectively the root of the > qbus tree) > > In this case TYPE_PFLASH_CFI01 is a subclass of TYPE_SYS_BUS_DEVICE, > so it will always be reset as part of system reset. > > (the main things which don't get automatically reset are direct > subclasses of TYPE_DEVICE, notably CPU objects.) Thanks for the clarification! I will update the commit description paraphrasing Peter: The TYPE_DEVICE interface provides a DeviceReset handler. This pflash device is a subclass of TYPE_SYS_BUS_DEVICE (which is a subclass of TYPE_DEVICE). SYS_BUS devices are automatically plugged into the 'main system bus', which is the root of the qbus tree. Devices in the qbus tree are guaranteed to have their reset() handler called after realize() and before we try to run the guest. To avoid incoherent states when the machine resets (see but report below), factor out the reset code into pflash_cfi01_system_reset, and register the method as a device reset callback. John, Laszlo, is that OK with you? Thanks, Phil.
On 07/02/19 05:16, John Snow wrote: > > > On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote: >> A "system reset" sets the device state machine in READ_ARRAY mode >> and, after some delay, set the SR.7 READY bit. >> >> We do not model timings, so we set the SR.7 bit directly. >> >> This pflash device is a child of TYPE_DEVICE. >> The TYPE_DEVICE interface provide a DeviceReset handler which will >> be called after the device is realized, and each time the machine >> resets itself. >> >> To avoid incoherent states when the machine resets (see but report >> below), factor out the reset code into pflash_cfi01_system_reset, >> and register the method as a device reset callback. >> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713 >> Reported-by: Laszlo Ersek <lersek@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Does reset always get called as part of realize, really? I'm not an expert on this, but I believe the following happens. (1) When a device is hotplugged, the device is reset as part of realize in the following call stack: device_set_realized() [hw/core/qdev.c] device_reset() [hw/core/qdev.c] (2) For when the device is cold-plugged, we have, in main() [hw/core/qdev.c]: /* TODO: once all bus devices are qdevified, this should be done * when bus is created by qdev.c */ qemu_register_reset(qbus_reset_all_fn, sysbus_get_default()); and then a bit later qemu_system_reset(SHUTDOWN_CAUSE_NONE); AIUI it results in a recursive traversal / reset, qbus_reset_all_fn() [hw/core/qdev.c] qbus_reset_all() [hw/core/qdev.c] qbus_walk_children() [hw/core/bus.c] qdev_reset_one() [hw/core/qdev.c] device_reset() [hw/core/qdev.c] In other words, I think you have a point about clarifying the commit message. It's likely not that the device is re-set (a) after it is realized *and* (b) each time the machine is re-set. Because, pflash is not hot-plugged, so reset-on-hotplug does not apply, and so case (a) falls away. Instead, case (b), "each time the machine is re-set", includes the initial launch of the machine, and that is why pflash is ultimately re-set at startup. It does happen after realize, but not as part of it. Anyway, Phil can verify this easily: just set a breakpoint on pflash_cfi01_system_reset() in gdb before launching qemu, and get a backtrace once the breakpoint fires. :) Thanks Laszlo > Or are we just trusting that the device is probably going to get reset > by the guest during bringup? > >> --- >> hw/block/pflash_cfi01.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index dd1dfd266b..8d632ea941 100644 >> --- a/hw/block/pflash_cfi01.c >> +++ b/hw/block/pflash_cfi01.c >> @@ -763,8 +763,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) >> pfl->max_device_width = pfl->device_width; >> } >> >> - pflash_mode_read_array(pfl); >> - pfl->status = 0x80; /* WSM ready */ >> /* Hardcoded CFI table */ >> /* Standard "QRY" string */ >> pfl->cfi_table[0x10] = 'Q'; >> @@ -852,6 +850,18 @@ 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); >> + >> + pflash_mode_read_array(pfl); >> + /* >> + * 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, >> @@ -896,6 +906,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; >> >
On 7/2/19 5:23 AM, Peter Maydell wrote: > On Tue, 2 Jul 2019 at 04:16, John Snow <jsnow@redhat.com> wrote: >> Does reset always get called as part of realize, really? >> >> Or are we just trusting that the device is probably going to get reset >> by the guest during bringup? > > Reset is not called "as part of realize", but it is guaranteed > to be called after realize and before we try to run the guest, > as long as the device is in the qbus tree. Things are in the > qbus tree if either: > * they're plugged into something already in the tree (eg > pci devices, scsi disks) > * they're a sysbus device (which is automatically plugged into > the 'main system bus' which is effectively the root of the > qbus tree) > > In this case TYPE_PFLASH_CFI01 is a subclass of TYPE_SYS_BUS_DEVICE, > so it will always be reset as part of system reset. > > (the main things which don't get automatically reset are direct > subclasses of TYPE_DEVICE, notably CPU objects.) > > thanks > -- PMM > Thank you for the lesson, that makes sense -- I saw the hotplug clause, but missed the sysbus routine. Thanks to Laszlo for pointing this out in great detail, too. Phil, a clarified commit message is perfectly sufficient, and please take my RB.
On 07/02/19 11:37, Philippe Mathieu-Daudé wrote: > On 7/2/19 11:23 AM, Peter Maydell wrote: >> On Tue, 2 Jul 2019 at 04:16, John Snow <jsnow@redhat.com> wrote: >>> Does reset always get called as part of realize, really? >>> >>> Or are we just trusting that the device is probably going to get reset >>> by the guest during bringup? >> >> Reset is not called "as part of realize", but it is guaranteed >> to be called after realize and before we try to run the guest, >> as long as the device is in the qbus tree. Things are in the >> qbus tree if either: >> * they're plugged into something already in the tree (eg >> pci devices, scsi disks) >> * they're a sysbus device (which is automatically plugged into >> the 'main system bus' which is effectively the root of the >> qbus tree) >> >> In this case TYPE_PFLASH_CFI01 is a subclass of TYPE_SYS_BUS_DEVICE, >> so it will always be reset as part of system reset. >> >> (the main things which don't get automatically reset are direct >> subclasses of TYPE_DEVICE, notably CPU objects.) > > Thanks for the clarification! > > I will update the commit description paraphrasing Peter: > > The TYPE_DEVICE interface provides a DeviceReset handler. > This pflash device is a subclass of TYPE_SYS_BUS_DEVICE (which > is a subclass of TYPE_DEVICE). > SYS_BUS devices are automatically plugged into the 'main system > bus', which is the root of the qbus tree. > Devices in the qbus tree are guaranteed to have their reset() > handler called after realize() and before we try to run the guest. > > To avoid incoherent states when the machine resets (see but report > below), factor out the reset code into pflash_cfi01_system_reset, > and register the method as a device reset callback. > > John, Laszlo, is that OK with you? works for me, sure. Thanks Laszlo
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index dd1dfd266b..8d632ea941 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -763,8 +763,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) pfl->max_device_width = pfl->device_width; } - pflash_mode_read_array(pfl); - pfl->status = 0x80; /* WSM ready */ /* Hardcoded CFI table */ /* Standard "QRY" string */ pfl->cfi_table[0x10] = 'Q'; @@ -852,6 +850,18 @@ 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); + + pflash_mode_read_array(pfl); + /* + * 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, @@ -896,6 +906,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;
A "system reset" sets the device state machine in READ_ARRAY mode and, after some delay, set the SR.7 READY bit. We do not model timings, so we set the SR.7 bit directly. This pflash device is a child of TYPE_DEVICE. The TYPE_DEVICE interface provide a DeviceReset handler which will be called after the device is realized, and each time the machine resets itself. To avoid incoherent states when the machine resets (see but report below), factor out the reset code into pflash_cfi01_system_reset, and register the method as a device reset callback. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1678713 Reported-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/block/pflash_cfi01.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)