[Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Anthony Liguori]

Ryan Harper wrote:
> Add a new monitor command (system_reboot) for a soft reboot which uses
> system_powerdown to trigger ACPI shutdown in the guest and once shutdown is
> complete, trigger a reset instead of exiting qemu.
>
> Depends on commit a6d6552426dcbf726e5549f08b70c9318d6be14b which enabled ACPI
> power button support.
>
> Tested with Ubuntu 9.04 64-bit guest.
>
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>
> diff --git a/hw/acpi.c b/hw/acpi.c
> index dbaf18a..346d100 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -151,7 +151,13 @@ static void pm_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
>                  sus_typ = (val >> 10) & 7;
>                  switch(sus_typ) {
>                  case 0: /* soft power off */
> -                    qemu_system_shutdown_request();
> +                    /* after powerdown, if on system_reboot path, call reset 
> +                       instead of shutdown */
> +                    if (qemu_reboot_requested()) {
> +                        qemu_system_reset_request();
> +                    } else {
> +                        qemu_system_shutdown_request();
> +                    }
>                      break;
>   

If qemu_shutdown_requested(), then we'll immediately shutdown the system.

qemu_reboot_requested() has different semantics, it's really a soft 
request.  I think the name needs to reflect that.

Also, the soft reset flag ought to get reset whenever a VM changes it's 
state.  That is, if you do a system_reboot, then a system_reset (imagine 
that the reboot fails), then you do a normal powerdown in the guest, 
instead of shutting off like the user would expect, we'll reboot because 
the qemu_reboot_requested() is still true.

A good way to do that would be by registering a reset handler in 
hw/acpi.c.  In fact, I think that the whole functionality probably 
should live in hw/acpi.c.

And I think this also needs to be stored as part of the savevm state for 
hw/acpi.c.  If you do a system_reboot followed by an immediate live 
migration, without the savevm handler, the VM will shutdown completely 
after the migration instead of rebooting as expected.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2009-05-08 12:00]:
> Ryan Harper wrote:
> >Add a new monitor command (system_reboot) for a soft reboot which uses
> >system_powerdown to trigger ACPI shutdown in the guest and once shutdown is
> >complete, trigger a reset instead of exiting qemu.
> >
> >Depends on commit a6d6552426dcbf726e5549f08b70c9318d6be14b which enabled 
> >ACPI
> >power button support.
> >
> >Tested with Ubuntu 9.04 64-bit guest.
> >
> >Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> >
> >diff --git a/hw/acpi.c b/hw/acpi.c
> >index dbaf18a..346d100 100644
> >--- a/hw/acpi.c
> >+++ b/hw/acpi.c
> >@@ -151,7 +151,13 @@ static void pm_ioport_writew(void *opaque, uint32_t 
> >addr, uint32_t val)
> >                 sus_typ = (val >> 10) & 7;
> >                 switch(sus_typ) {
> >                 case 0: /* soft power off */
> >-                    qemu_system_shutdown_request();
> >+                    /* after powerdown, if on system_reboot path, call 
> >reset +                       instead of shutdown */
> >+                    if (qemu_reboot_requested()) {
> >+                        qemu_system_reset_request();
> >+                    } else {
> >+                        qemu_system_shutdown_request();
> >+                    }
> >                     break;
> >  
> 
> If qemu_shutdown_requested(), then we'll immediately shutdown the system.

Right, but this request happens after we've sent the ACPI powerdown
event.  After we've done the powerdown (acpi aware guests can do a
shutdown), instead of then calling shutdown, which exits, we call
reset.

> 
> qemu_reboot_requested() has different semantics, it's really a soft 
> request.  I think the name needs to reflect that.

Not sure what you mean here.

> 
> Also, the soft reset flag ought to get reset whenever a VM changes it's 
> state.  That is, if you do a system_reboot, then a system_reset (imagine 
> that the reboot fails), then you do a normal powerdown in the guest, 
> instead of shutting off like the user would expect, we'll reboot because 
> the qemu_reboot_requested() is still true.
> 
> A good way to do that would be by registering a reset handler in 
> hw/acpi.c.  In fact, I think that the whole functionality probably 
> should live in hw/acpi.c.

OK

> 
> And I think this also needs to be stored as part of the savevm state for 
> hw/acpi.c.  If you do a system_reboot followed by an immediate live 
> migration, without the savevm handler, the VM will shutdown completely 
> after the migration instead of rebooting as expected.

Would it? I don't see that we are saving powerdown|shutdown|reset
request flags?  Sounds like all of those flags need to be in the save
state, and separate patch IMHO.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

[Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Anthony Liguori]

Ryan Harper wrote:
> * Anthony Liguori <aliguori@us.ibm.com> [2009-05-08 12:00]:
>   
>> Ryan Harper wrote:
>>     
>>> Add a new monitor command (system_reboot) for a soft reboot which uses
>>> system_powerdown to trigger ACPI shutdown in the guest and once shutdown is
>>> complete, trigger a reset instead of exiting qemu.
>>>
>>> Depends on commit a6d6552426dcbf726e5549f08b70c9318d6be14b which enabled 
>>> ACPI
>>> power button support.
>>>
>>> Tested with Ubuntu 9.04 64-bit guest.
>>>
>>> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
>>>
>>> diff --git a/hw/acpi.c b/hw/acpi.c
>>> index dbaf18a..346d100 100644
>>> --- a/hw/acpi.c
>>> +++ b/hw/acpi.c
>>> @@ -151,7 +151,13 @@ static void pm_ioport_writew(void *opaque, uint32_t 
>>> addr, uint32_t val)
>>>                 sus_typ = (val >> 10) & 7;
>>>                 switch(sus_typ) {
>>>                 case 0: /* soft power off */
>>> -                    qemu_system_shutdown_request();
>>> +                    /* after powerdown, if on system_reboot path, call 
>>> reset +                       instead of shutdown */
>>> +                    if (qemu_reboot_requested()) {
>>> +                        qemu_system_reset_request();
>>> +                    } else {
>>> +                        qemu_system_shutdown_request();
>>> +                    }
>>>                     break;
>>>  
>>>       
>> If qemu_shutdown_requested(), then we'll immediately shutdown the system.
>>     
>
> Right, but this request happens after we've sent the ACPI powerdown
> event.  After we've done the powerdown (acpi aware guests can do a
> shutdown), instead of then calling shutdown, which exits, we call
> reset.
>   

I'm saying, semantically, if you call 'qemu_shutdown_requested()', if it 
returns 1, it means, immediately shutdown the VM--regardless of where 
it's called.

The semantics of qemu_reboot_requested() are, if it returns 1, then only 
when you see an ACPI soft power off, reset the VM.  It's that difference 
in semantics that I think could lead to confusion.

>> Also, the soft reset flag ought to get reset whenever a VM changes it's 
>> state.  That is, if you do a system_reboot, then a system_reset (imagine 
>> that the reboot fails), then you do a normal powerdown in the guest, 
>> instead of shutting off like the user would expect, we'll reboot because 
>> the qemu_reboot_requested() is still true.
>>
>> A good way to do that would be by registering a reset handler in 
>> hw/acpi.c.  In fact, I think that the whole functionality probably 
>> should live in hw/acpi.c.
>>     
>
> OK
>
>   
>> And I think this also needs to be stored as part of the savevm state for 
>> hw/acpi.c.  If you do a system_reboot followed by an immediate live 
>> migration, without the savevm handler, the VM will shutdown completely 
>> after the migration instead of rebooting as expected.
>>     
>
> Would it? I don't see that we are saving powerdown|shutdown|reset
> request flags?  Sounds like all of those flags need to be in the save
> state, and separate patch IMHO.
>   

No, they don't.

A qemu_powerdown_request() call happens from the monitor.  This is to 
allow a graceful shutdown (as opposed to exiting from the monitor).  
This will trigger the TCG loop to immediately exit.  The state doesn't 
need to be saved because you cannot do a migration in between when 
qemu_powerdown_request() is called and when the shutdown actually happens.

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2009-05-08 12:38]:
> Ryan Harper wrote:
> >* Anthony Liguori <aliguori@us.ibm.com> [2009-05-08 12:00]:
> >  

> >>If qemu_shutdown_requested(), then we'll immediately shutdown the system.
> >>    
> >
> >Right, but this request happens after we've sent the ACPI powerdown
> >event.  After we've done the powerdown (acpi aware guests can do a
> >shutdown), instead of then calling shutdown, which exits, we call
> >reset.
> >  
> 
> I'm saying, semantically, if you call 'qemu_shutdown_requested()', if it 
> returns 1, it means, immediately shutdown the VM--regardless of where 
> it's called.
> 
> The semantics of qemu_reboot_requested() are, if it returns 1, then only 
> when you see an ACPI soft power off, reset the VM.  It's that difference 
> in semantics that I think could lead to confusion.

well, I think this is where we're missing each other.  I figured if the
users requested a reboot, that we also trigger the powerdown, and that's
what I'm doing.  If in the monitor you issue system_reboot, I'm
triggering a powerdown automatically.  Are you saying you want the users
to do, system_reset, and then system_powerdown in their own?   And if
that is the case, I can see why you're asking to maintain the state of
the flag.  IMO, I think triggering the powerdown from the reboot call
makes sense.

> >>And I think this also needs to be stored as part of the savevm state for 
> >>hw/acpi.c.  If you do a system_reboot followed by an immediate live 
> >>migration, without the savevm handler, the VM will shutdown completely 
> >>after the migration instead of rebooting as expected.
> >>    
> >
> >Would it? I don't see that we are saving powerdown|shutdown|reset
> >request flags?  Sounds like all of those flags need to be in the save
> >state, and separate patch IMHO.
> >  
> 
> No, they don't.
> 
> A qemu_powerdown_request() call happens from the monitor.  This is to 
> allow a graceful shutdown (as opposed to exiting from the monitor).  
> This will trigger the TCG loop to immediately exit.  The state doesn't 
> need to be saved because you cannot do a migration in between when 
> qemu_powerdown_request() is called and when the shutdown actually happens.

Sure, and I don't see why reboot semantics would be any different unless
you thinking about the above case I think you might be meaning.


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Anthony Liguori]

Ryan Harper wrote:
> well, I think this is where we're missing each other.  I figured if the
> users requested a reboot, that we also trigger the powerdown, and that's
> what I'm doing.  If in the monitor you issue system_reboot, I'm
> triggering a powerdown automatically.  Are you saying you want the users
> to do, system_reset, and then system_powerdown in their own?

No, I'm saying that if a user does a system_reboot, and that fails for 
one reason or another (like the user hits Cancel in the GUI that pops 
up), the flag is still present.

If the user later does a proper shutdown of his system (in the guest 
itself), instead of shutting down, it'll reboot.

Try this:

1) do system_reboot in the monitor
2) In the guest, when you get the Gnome GUI, hit cancel
3) Go to the Gnome menu to do a normal system shutdown.

It'll end up doing a reboot.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Add monitor command for system_reboot

[Ryan Harper]

* Anthony Liguori <anthony@codemonkey.ws> [2009-05-08 13:15]:
> Ryan Harper wrote:
> >well, I think this is where we're missing each other.  I figured if the
> >users requested a reboot, that we also trigger the powerdown, and that's
> >what I'm doing.  If in the monitor you issue system_reboot, I'm
> >triggering a powerdown automatically.  Are you saying you want the users
> >to do, system_reset, and then system_powerdown in their own?
> 
> No, I'm saying that if a user does a system_reboot, and that fails for 
> one reason or another (like the user hits Cancel in the GUI that pops 
> up), the flag is still present.
> 
> If the user later does a proper shutdown of his system (in the guest 
> itself), instead of shutting down, it'll reboot.
> 
> Try this:
> 
> 1) do system_reboot in the monitor
> 2) In the guest, when you get the Gnome GUI, hit cancel
> 3) Go to the Gnome menu to do a normal system shutdown.
> 
> It'll end up doing a reboot.

Hrm, yeah -- I'll have to see what path a guest shutdown travels and if
it's the same as what would happen with a system_powerdown via the
monitor we can clear the reboot flag.

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com