Re: [Qemu-devel] Re: [PATCH 5/5] RFC: distinguish warm reset from cold reset.

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Mon, Aug 30, 2010 at 07:59:22AM -0500, Anthony Liguori wrote:
> On 08/30/2010 02:49 AM, Isaku Yamahata wrote:
>> Distinguish warm reset from cold reset by introducing
>> cold/warm reset helper function instead of single reset routines.
>>
>> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp>
>> ---
>>   hw/hw.h  |    7 +++++
>>   sysemu.h |    4 +++
>>   vl.c     |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>>   3 files changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/hw.h b/hw/hw.h
>> index 4405092..6fb844e 100644
>> --- a/hw/hw.h
>> +++ b/hw/hw.h
>> @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr,
>>
>>   typedef void QEMUResetHandler(void *opaque);
>>
>> +
>> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque);
>> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque);
>>    
>
> I was thinking that we should stick entirely within the qdev abstraction.
>
> The patchset I sent out introduced a cold reset as a qdev property on  
> the devices.
>
> For warm reset, if I understand correctly, we need two things.  We need  
> to 1) control propagation order and we need to 2) differentiate  
> per-device between cold reset and warm reset.
>
> For (2), I don't know that we truly do need it.  For something like PCI  
> AER, wouldn't we just move the AER initialization to the qdev init  
> function and then never change the AER registers during reset?
>
> IOW, the only way to do a cold reset would be to destroy and recreate  
> the device.

I'm lost here. Then, what should qdev_reset() do?
So far you have strongly claimed that qdev_reset() should be cold reset.
(the current implementation is imperfect though)
So I had to create the patch that distinguishes warm reset from cold reset.
But here you suggest that qdev_reset() be warm reset and only way to
cold-reset be destroy/re-create.

Can you elaborate on what qdev_reset() API should do?
If qdev_reset() is cold reset, something like qdev_warm_reset()
is necessary.
If qdev_reset() is warm reset and the only way to cold-reset is
destroy/re-create, I'm fine with it.

> For (1), I still don't fully understand what's needed here.  Do we just  
> care about doing a certain transversal order (like depth-first) or do we  
> actually care about withholding the reset signal for devices if as Avi  
> said, we SCSI devices don't get reset.
>
> Changing transversal order is trivial.  I think we should be very clear  
> though if we're going to introduce bus-specific mechanisms to modify  
> transversal though.
>
> Regards,
>
> Anthony Liguori
>
>> +/* those two functions are obsoleted by cold/warm reset API. */
>>   void qemu_register_reset(QEMUResetHandler *func, void *opaque);
>>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>>
>> diff --git a/sysemu.h b/sysemu.h
>> index 7fc4e20..927892a 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void);
>>   void cpu_enable_ticks(void);
>>   void cpu_disable_ticks(void);
>>
>> +/* transitional compat = qemu_system_warm_reset_request() */
>>   void qemu_system_reset_request(void);
>> +
>> +void qemu_system_cold_reset_request(void);
>> +void qemu_system_warm_reset_request(void);
>>   void qemu_system_shutdown_request(void);
>>   void qemu_system_powerdown_request(void);
>>   extern qemu_irq qemu_system_powerdown;
>> diff --git a/vl.c b/vl.c
>> index a919a32..609d74c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry {
>>   } QEMUResetEntry;
>>
>>   QTAILQ_HEAD(reset_handlers, QEMUResetEntry);
>> -static struct reset_handlers reset_handlers =
>> -    QTAILQ_HEAD_INITIALIZER(reset_handlers);
>> -static int reset_requested;
>> +
>> +static struct reset_handlers cold_reset_handlers =
>> +    QTAILQ_HEAD_INITIALIZER(cold_reset_handlers);
>> +static struct reset_handlers warm_reset_handlers =
>> +    QTAILQ_HEAD_INITIALIZER(warm_reset_handlers);
>> +static int cold_reset_requested;
>> +static int warm_reset_requested;
>>   static int shutdown_requested;
>>   static int powerdown_requested;
>>   int debug_requested;
>> @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void)
>>       return qemu_requested(&shutdown_requested);
>>   }
>>
>> -static int qemu_reset_requested(void)
>> +static int qemu_cold_reset_requested(void)
>> +{
>> +    return qemu_requested(&cold_reset_requested);
>> +}
>> +
>> +static int qemu_warm_reset_requested(void)
>>   {
>> -    return qemu_requested(&reset_requested);
>> +    return qemu_requested(&warm_reset_requested);
>>   }
>>
>>   static int qemu_powerdown_requested(void)
>> @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers)
>>       }
>>   }
>>
>> +/* obsolated by qemu_register_cold/warm_reset() */
>>   void qemu_register_reset(QEMUResetHandler *func, void *opaque)
>>   {
>> -    qemu_register_reset_handler(func, opaque,&reset_handlers);
>> +    qemu_register_cold_reset(func, opaque);
>> +    qemu_register_warm_reset(func, opaque);
>>   }
>>
>> +/* obsolated by qemu_unregister_cold/warm_reset() */
>>   void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
>>   {
>> -    qemu_unregister_reset_handler(func, opaque,&reset_handlers);
>> +    qemu_unregister_cold_reset(func, opaque);
>> +    qemu_unregister_warm_reset(func, opaque);
>> +}
>> +
>> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_register_reset_handler(func, opaque,&cold_reset_handlers);
>> +}
>> +
>> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_unregister_reset_handler(func, opaque,&cold_reset_handlers);
>> +}
>> +
>> +static void qemu_system_cold_reset(void)
>> +{
>> +    qemu_system_reset_handler(&cold_reset_handlers);
>> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */
>> +    cpu_synchronize_all_post_reset();
>> +}
>> +
>> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_register_reset_handler(func, opaque,&warm_reset_handlers);
>> +}
>> +
>> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque)
>> +{
>> +    qemu_unregister_reset_handler(func, opaque,&warm_reset_handlers);
>>   }
>>
>> -static void qemu_system_reset(void)
>> +static void qemu_system_warm_reset(void)
>>   {
>> -    qemu_system_reset_handler(&reset_handlers);
>> -    monitor_protocol_event(QEVENT_RESET, NULL);
>> +    qemu_system_reset_handler(&warm_reset_handlers);
>> +    monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */
>>       cpu_synchronize_all_post_reset();
>>   }
>>
>> @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested)
>>       qemu_system_request(requested);
>>   }
>>
>> +void qemu_system_cold_reset_request(void)
>> +{
>> +    qemu_system_request_reboot_check(&cold_reset_requested);
>> +}
>> +
>> +void qemu_system_warm_reset_request(void)
>> +{
>> +    qemu_system_request_reboot_check(&warm_reset_requested);
>> +}
>> +
>> +/* trantitional compat */
>>   void qemu_system_reset_request(void)
>>   {
>> -    qemu_system_request_reboot_check(&reset_requested);
>> +    qemu_system_request_reboot_check(&warm_reset_requested);
>>   }
>>
>>   void qemu_system_shutdown_request(void)
>> @@ -1322,7 +1373,9 @@ static int vm_can_run(void)
>>   {
>>       if (powerdown_requested)
>>           return 0;
>> -    if (reset_requested)
>> +    if (cold_reset_requested)
>> +        return 0;
>> +    if (warm_reset_requested)
>>           return 0;
>>       if (shutdown_requested)
>>           return 0;
>> @@ -1368,9 +1421,15 @@ static void main_loop(void)
>>               } else
>>                   break;
>>           }
>> -        if (qemu_reset_requested()) {
>> +        if (qemu_warm_reset_requested()) {
>> +            pause_all_vcpus();
>> +            qemu_system_warm_reset();
>> +            resume_all_vcpus();
>> +        }
>> +        if (qemu_cold_reset_requested()) {
>> +            /* power cycle */
>>               pause_all_vcpus();
>> -            qemu_system_reset();
>> +            qemu_system_cold_reset();
>>               resume_all_vcpus();
>>           }
>>           if (qemu_powerdown_requested()) {
>> @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp)
>>           exit(1);
>>       }
>>
>> -    qemu_system_reset();
>> +    qemu_system_cold_reset();
>>       if (loadvm) {
>>           if (load_vmstate(loadvm)<  0) {
>>               autostart = 0;
>>    
>
>

-- 
yamahata