Re: [Qemu-devel] [PATCH v2 5/9] hw/block/pflash_cfi01: Add the DeviceReset() handler

[Laszlo Ersek]

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 <address@hidden>
>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> 
> 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;
>>
>