[Qemu-devel] [PATCH][RESEND] Add monitor command for system_reboot

[Qemu-devel] [PATCH][RESEND] Add monitor command for system_reboot

[Ryan Harper]

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.

V2:
    -added reset handler to lower the reboot flag on reset.

Tested with:
    - Ubuntu 9.04 64-bit guest.
    - SLES 10 SP2 32-bit guest.
    - RHEL 5.3 32 and 64 bit guests.

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


diffstat output:
 hw/acpi.c       |   15 ++++++++++++++-
 monitor.c       |    5 +++++
 qemu-monitor.hx |    8 ++++++++
 sysemu.h        |    2 ++
 vl.c            |   22 +++++++++++++++++++++-
 5 files changed, 50 insertions(+), 2 deletions(-)

---
From: Ryan Harper <ryanh@us.ibm.com>
Subject: [PATCH] Add system_reboot monitor function
Cc: Anthony Liguori <aliguori@us.ibm.com>

This patch adds a new monitor command to trigger a powerdown followed by
system_reset.

Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
---
 hw/acpi.c       |   15 ++++++++++++++-
 monitor.c       |    5 +++++
 qemu-monitor.hx |    8 ++++++++
 sysemu.h        |    2 ++
 vl.c            |   21 +++++++++++++++++++++
 5 files changed, 50 insertions(+), 1 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 0465201..aba384c 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;
                 case 1:
                     /* RSM_STS should be set on resume. Pretend that resume
@@ -497,6 +503,12 @@ static void piix4_reset(void *opaque)
     }
 }
 
+static void system_reboot_reset(void *opaque)
+{
+    /* clear reboot flag */
+    qemu_reboot_requested();
+}
+
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                        qemu_irq sci_irq)
 {
@@ -551,6 +563,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     s->smbus = i2c_init_bus(NULL, "i2c");
     s->irq = sci_irq;
     qemu_register_reset(piix4_reset, s);
+    qemu_register_reset(system_reboot_reset, s);
 
     return s->smbus;
 }
diff --git a/monitor.c b/monitor.c
index bad79fe..346e0db 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1223,6 +1223,11 @@ static void do_system_powerdown(Monitor *mon)
     qemu_system_powerdown_request();
 }
 
+static void do_system_reboot(Monitor *mon)
+{
+    qemu_system_reboot_request();
+}
+
 #if defined(TARGET_I386)
 static void print_pte(Monitor *mon, uint32_t addr, uint32_t pte, uint32_t mask)
 {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index dc10b75..91799d0 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -339,6 +339,14 @@ STEXI
 Power down the system (if supported).
 ETEXI
 
+    { "system_reboot", "", do_system_reboot,
+      "", "send system power down event, and then reset" },
+STEXI
+@item system_reboot
+
+Power down the system (if supported), and then reset.
+ETEXI
+
     { "sum", "ii", do_sum,
       "addr size", "compute the checksum of a memory region" },
 STEXI
diff --git a/sysemu.h b/sysemu.h
index 06dc4c6..056a491 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -40,10 +40,12 @@ void cpu_enable_ticks(void);
 void cpu_disable_ticks(void);
 
 void qemu_system_reset_request(void);
+void qemu_system_reboot_request(void);
 void qemu_system_shutdown_request(void);
 void qemu_system_powerdown_request(void);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
+int qemu_reboot_requested(void);
 int qemu_powerdown_requested(void);
 #ifdef NEED_CPU_H
 #if !defined(TARGET_SPARC) && !defined(TARGET_I386)
diff --git a/vl.c b/vl.c
index 7b7489c..634d386 100644
--- a/vl.c
+++ b/vl.c
@@ -3618,6 +3618,7 @@ static QEMUResetEntry *first_reset_entry;
 static int reset_requested;
 static int shutdown_requested;
 static int powerdown_requested;
+static int reboot_requested;
 static int debug_requested;
 static int vmstop_requested;
 
@@ -3642,6 +3643,13 @@ int qemu_powerdown_requested(void)
     return r;
 }
 
+int qemu_reboot_requested(void)
+{
+    int r = reboot_requested;
+    reboot_requested = 0;
+    return r;
+}
+
 static int qemu_debug_requested(void)
 {
     int r = debug_requested;
@@ -3712,6 +3720,17 @@ void qemu_system_powerdown_request(void)
     qemu_notify_event();
 }
 
+void qemu_system_reboot_request(void)
+{
+    /* Raise the powerdown request to trigger system_powerdown event.
+     * Also raise reboot flag so powerdown handler knows to request
+     * a reset instead of shutdown after the powerdown.
+     */
+    powerdown_requested = 1;
+    reboot_requested = 1;
+    qemu_notify_event();
+}
+
 #ifdef CONFIG_IOTHREAD
 static void qemu_system_vmstop_request(int reason)
 {
@@ -4461,6 +4480,8 @@ static int vm_can_run(void)
 {
     if (powerdown_requested)
         return 0;
+    if (reboot_requested)
+        return 0;
     if (reset_requested)
         return 0;
     if (shutdown_requested)
-- 
1.6.0.4

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

[Avi Kivity]

On 07/07/2009 10:26 PM, 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.
>
> V2:
>      -added reset handler to lower the reboot flag on reset.
>
> Tested with:
>      - Ubuntu 9.04 64-bit guest.
>      - SLES 10 SP2 32-bit guest.
>      - RHEL 5.3 32 and 64 bit guests.
>
>    

I think a combination of system_powerdown, avoiding exit on shutdown, 
and system_reset is more flexible as it allows the controller to modify 
the guest before rebooting it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Gleb Natapov]

On Tue, Jul 07, 2009 at 02:26:31PM -0500, 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.
> 
> V2:
>     -added reset handler to lower the reboot flag on reset.
OS is free to ignore ACPI shutdown request and in this case
reboot_requested will not be reset. On the next user initiated
guest power down qemu will reboot instead of exit.

> 
> Tested with:
>     - Ubuntu 9.04 64-bit guest.
>     - SLES 10 SP2 32-bit guest.
>     - RHEL 5.3 32 and 64 bit guests.
> 
> -- 
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> ryanh@us.ibm.com
> 
> 
> diffstat output:
>  hw/acpi.c       |   15 ++++++++++++++-
>  monitor.c       |    5 +++++
>  qemu-monitor.hx |    8 ++++++++
>  sysemu.h        |    2 ++
>  vl.c            |   22 +++++++++++++++++++++-
>  5 files changed, 50 insertions(+), 2 deletions(-)
> 
> ---
> From: Ryan Harper <ryanh@us.ibm.com>
> Subject: [PATCH] Add system_reboot monitor function
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> 
> This patch adds a new monitor command to trigger a powerdown followed by
> system_reset.
> 
> Signed-off-by: Ryan Harper <ryanh@us.ibm.com>
> ---
>  hw/acpi.c       |   15 ++++++++++++++-
>  monitor.c       |    5 +++++
>  qemu-monitor.hx |    8 ++++++++
>  sysemu.h        |    2 ++
>  vl.c            |   21 +++++++++++++++++++++
>  5 files changed, 50 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 0465201..aba384c 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;
>                  case 1:
>                      /* RSM_STS should be set on resume. Pretend that resume
> @@ -497,6 +503,12 @@ static void piix4_reset(void *opaque)
>      }
>  }
>  
> +static void system_reboot_reset(void *opaque)
> +{
> +    /* clear reboot flag */
> +    qemu_reboot_requested();
> +}
> +
>  i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                         qemu_irq sci_irq)
>  {
> @@ -551,6 +563,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>      s->smbus = i2c_init_bus(NULL, "i2c");
>      s->irq = sci_irq;
>      qemu_register_reset(piix4_reset, s);
> +    qemu_register_reset(system_reboot_reset, s);
>  
>      return s->smbus;
>  }
> diff --git a/monitor.c b/monitor.c
> index bad79fe..346e0db 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1223,6 +1223,11 @@ static void do_system_powerdown(Monitor *mon)
>      qemu_system_powerdown_request();
>  }
>  
> +static void do_system_reboot(Monitor *mon)
> +{
> +    qemu_system_reboot_request();
> +}
> +
>  #if defined(TARGET_I386)
>  static void print_pte(Monitor *mon, uint32_t addr, uint32_t pte, uint32_t mask)
>  {
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index dc10b75..91799d0 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -339,6 +339,14 @@ STEXI
>  Power down the system (if supported).
>  ETEXI
>  
> +    { "system_reboot", "", do_system_reboot,
> +      "", "send system power down event, and then reset" },
> +STEXI
> +@item system_reboot
> +
> +Power down the system (if supported), and then reset.
> +ETEXI
> +
>      { "sum", "ii", do_sum,
>        "addr size", "compute the checksum of a memory region" },
>  STEXI
> diff --git a/sysemu.h b/sysemu.h
> index 06dc4c6..056a491 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -40,10 +40,12 @@ void cpu_enable_ticks(void);
>  void cpu_disable_ticks(void);
>  
>  void qemu_system_reset_request(void);
> +void qemu_system_reboot_request(void);
>  void qemu_system_shutdown_request(void);
>  void qemu_system_powerdown_request(void);
>  int qemu_shutdown_requested(void);
>  int qemu_reset_requested(void);
> +int qemu_reboot_requested(void);
>  int qemu_powerdown_requested(void);
>  #ifdef NEED_CPU_H
>  #if !defined(TARGET_SPARC) && !defined(TARGET_I386)
> diff --git a/vl.c b/vl.c
> index 7b7489c..634d386 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3618,6 +3618,7 @@ static QEMUResetEntry *first_reset_entry;
>  static int reset_requested;
>  static int shutdown_requested;
>  static int powerdown_requested;
> +static int reboot_requested;
>  static int debug_requested;
>  static int vmstop_requested;
>  
> @@ -3642,6 +3643,13 @@ int qemu_powerdown_requested(void)
>      return r;
>  }
>  
> +int qemu_reboot_requested(void)
> +{
> +    int r = reboot_requested;
> +    reboot_requested = 0;
> +    return r;
> +}
> +
>  static int qemu_debug_requested(void)
>  {
>      int r = debug_requested;
> @@ -3712,6 +3720,17 @@ void qemu_system_powerdown_request(void)
>      qemu_notify_event();
>  }
>  
> +void qemu_system_reboot_request(void)
> +{
> +    /* Raise the powerdown request to trigger system_powerdown event.
> +     * Also raise reboot flag so powerdown handler knows to request
> +     * a reset instead of shutdown after the powerdown.
> +     */
> +    powerdown_requested = 1;
> +    reboot_requested = 1;
> +    qemu_notify_event();
> +}
> +
>  #ifdef CONFIG_IOTHREAD
>  static void qemu_system_vmstop_request(int reason)
>  {
> @@ -4461,6 +4480,8 @@ static int vm_can_run(void)
>  {
>      if (powerdown_requested)
>          return 0;
> +    if (reboot_requested)
> +        return 0;
>      if (reset_requested)
>          return 0;
>      if (shutdown_requested)
> -- 
> 1.6.0.4
> 
> 

--
			Gleb.

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

[Anthony Liguori]

Gleb Natapov wrote:
> On Tue, Jul 07, 2009 at 02:26:31PM -0500, 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.
>>
>> V2:
>>     -added reset handler to lower the reboot flag on reset.
>>     
> OS is free to ignore ACPI shutdown request and in this case
> reboot_requested will not be reset. On the next user initiated
> guest power down qemu will reboot instead of exit.
>   

Indeed.  This is what has kept me from applying this but I just can't 
think of any better solution.

Any ideas?

The drive behind implementing this feature is so that we can implement a 
proper virDomainReboot in libvirt.

-- 
Regards,

Anthony Liguori

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

[Anthony Liguori]

Avi Kivity wrote:
> On 07/07/2009 10:26 PM, 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.
>>
>> V2:
>>      -added reset handler to lower the reboot flag on reset.
>>
>> Tested with:
>>      - Ubuntu 9.04 64-bit guest.
>>      - SLES 10 SP2 32-bit guest.
>>      - RHEL 5.3 32 and 64 bit guests.
>>
>>    
>
> I think a combination of system_powerdown, avoiding exit on shutdown, 
> and system_reset is more flexible as it allows the controller to 
> modify the guest before rebooting it.

You need notification that the shutdown has occurred.  That's the only 
bit we're missing today.  It's still not perfect though.

libvirt does system_powerdown, guest completely ignores it, user decides 
to power off, libvirt cannot tell the difference between user requested 
power off and ACPI driven power down.

Regards,

Anthony Liguori

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

[Avi Kivity]

On 07/08/2009 04:07 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> On 07/07/2009 10:26 PM, 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.
>>>
>>> V2:
>>>      -added reset handler to lower the reboot flag on reset.
>>>
>>> Tested with:
>>>      - Ubuntu 9.04 64-bit guest.
>>>      - SLES 10 SP2 32-bit guest.
>>>      - RHEL 5.3 32 and 64 bit guests.
>>>
>>
>> I think a combination of system_powerdown, avoiding exit on shutdown, 
>> and system_reset is more flexible as it allows the controller to 
>> modify the guest before rebooting it.
>
> You need notification that the shutdown has occurred.  That's the only 
> bit we're missing today.  It's still not perfect though.
>
> libvirt does system_powerdown, guest completely ignores it, user 
> decides to power off, libvirt cannot tell the difference between user 
> requested power off and ACPI driven power down.

Does the patch handle this case?

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

Avi Kivity wrote:
> Does the patch handle this case?

Nope.

-- 
Regards,

Anthony Liguori

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

[Daniel P. Berrange]

On Wed, Jul 08, 2009 at 08:05:20AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
> >On Tue, Jul 07, 2009 at 02:26:31PM -0500, 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.
> >>
> >>V2:
> >>    -added reset handler to lower the reboot flag on reset.
> >>    
> >OS is free to ignore ACPI shutdown request and in this case
> >reboot_requested will not be reset. On the next user initiated
> >guest power down qemu will reboot instead of exit.
> >  
> 
> Indeed.  This is what has kept me from applying this but I just can't 
> think of any better solution.
> 
> Any ideas?

Only other option I think of is a pure paravirt shutdown/reboot
handler, ala Xen. That sucks in different ways though, namely
needing a driver in the guest, which is probably worse.


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Avi Kivity]

On 07/08/2009 04:16 PM, Anthony Liguori wrote:
> Avi Kivity wrote:
>> Does the patch handle this case?
>
> Nope.
>

In this case, separate shutdown/reset are better since they don't imply 
anything.

-- 
error compiling committee.c: too many arguments to function

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

[Anthony Liguori]

Daniel P. Berrange wrote:
>
> Only other option I think of is a pure paravirt shutdown/reboot
> handler, ala Xen. That sucks in different ways though, namely
> needing a driver in the guest, which is probably worse.
>   

What do you think about Avi's suggestion of using -no-shutdown, doing a 
system_powerdown, waiting to receive a powerdown (you can poll 'info 
status' too to see when you entered into the paused state), and then 
doing a system_reset?  I'd suggesting doing a timeout too while waiting 
to avoid waiting forever.

> Daniel
>   


-- 
Regards,

Anthony Liguori

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

[Gleb Natapov]

On Wed, Jul 08, 2009 at 08:05:20AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote:
>> On Tue, Jul 07, 2009 at 02:26:31PM -0500, 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.
>>>
>>> V2:
>>>     -added reset handler to lower the reboot flag on reset.
>>>     
>> OS is free to ignore ACPI shutdown request and in this case
>> reboot_requested will not be reset. On the next user initiated
>> guest power down qemu will reboot instead of exit.
>>   
>
> Indeed.  This is what has kept me from applying this but I just can't  
> think of any better solution.
>
> Any ideas?
>
> The drive behind implementing this feature is so that we can implement a  
> proper virDomainReboot in libvirt.
>
I don't know what is the definition of virDomainReboot is, but what this
patch is trying to do is to change a guest OS behaviour from the host.
Guest OSes can control their behaviour on ACPI shutdown event by
themselves (on Windows you can configure what power button action should
be, on Linux I don't know I never power it down), so why would we want
to override this? 

--
			Gleb.

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

[Daniel P. Berrange]

On Wed, Jul 08, 2009 at 08:47:19AM -0500, Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >
> >Only other option I think of is a pure paravirt shutdown/reboot
> >handler, ala Xen. That sucks in different ways though, namely
> >needing a driver in the guest, which is probably worse.
> >  
> 
> What do you think about Avi's suggestion of using -no-shutdown, doing a 
> system_powerdown, waiting to receive a powerdown (you can poll 'info 
> status' too to see when you entered into the paused state), and then 
> doing a system_reset?  I'd suggesting doing a timeout too while waiting 
> to avoid waiting forever.

That suffers from the same problem as a system_reboot command in that
the guest may never start the shutdown. If that's an acceptable 
limitation, then we might as well implement it directly in QEMU as
system_reboot so its easily available to everyone. Make 'system_reboot'
take an optional timeout arg (default to 60 seconds) after which it
cancels its plans. 

Regards,
Dainel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> That suffers from the same problem as a system_reboot command in that
> the guest may never start the shutdown. If that's an acceptable 
> limitation, then we might as well implement it directly in QEMU as
> system_reboot so its easily available to everyone. Make 'system_reboot'
> take an optional timeout arg (default to 60 seconds) after which it
> cancels its plans. 
>   

That's a heuristic and we tend to avoid implementing heuristics in 
QEMU.  It's really the role of a management tool to make policy decisions.

-- 
Regards,

Anthony Liguori

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

[Avi Kivity]

On 07/08/2009 04:05 PM, Anthony Liguori wrote:
>> OS is free to ignore ACPI shutdown request and in this case
>> reboot_requested will not be reset. On the next user initiated
>> guest power down qemu will reboot instead of exit.
>
> Indeed.  This is what has kept me from applying this but I just can't 
> think of any better solution.
>
> Any ideas?
>
> The drive behind implementing this feature is so that we can implement 
> a proper virDomainReboot in libvirt.
>

How does Xen hvm implement it?

-- 
error compiling committee.c: too many arguments to function

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

[Daniel P. Berrange]

On Wed, Jul 08, 2009 at 05:04:45PM +0300, Avi Kivity wrote:
> On 07/08/2009 04:05 PM, Anthony Liguori wrote:
> >>OS is free to ignore ACPI shutdown request and in this case
> >>reboot_requested will not be reset. On the next user initiated
> >>guest power down qemu will reboot instead of exit.
> >
> >Indeed.  This is what has kept me from applying this but I just can't 
> >think of any better solution.
> >
> >Any ideas?
> >
> >The drive behind implementing this feature is so that we can implement 
> >a proper virDomainReboot in libvirt.
> >
> 
> How does Xen hvm implement it?

You have to install the Xen paravirt drivers, so Xen HVM handles graceful
shutdown/reboot in the same way as pure Xen paravirt. They watch a flag
in xenstore and invoke shutdown -r / -h according to the flag.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Avi Kivity]

On 07/08/2009 05:10 PM, Daniel P. Berrange wrote:
>>> Indeed.  This is what has kept me from applying this but I just can't
>>> think of any better solution.
>>>
>>> Any ideas?
>>>
>>> The drive behind implementing this feature is so that we can implement
>>> a proper virDomainReboot in libvirt.
>>>
>>>        
>> How does Xen hvm implement it?
>>      
>
> You have to install the Xen paravirt drivers, so Xen HVM handles graceful
> shutdown/reboot in the same way as pure Xen paravirt. They watch a flag
> in xenstore and invoke shutdown -r / -h according to the flag.
>    

What happens if the driver is not installed?  The guest just keeps 
going, right?

What's the libvirt motivation for the feature?

-- 
error compiling committee.c: too many arguments to function

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

[Ryan Harper]

* Daniel P. Berrange <berrange@redhat.com> [2009-07-08 08:58]:
> On Wed, Jul 08, 2009 at 08:47:19AM -0500, Anthony Liguori wrote:
> > Daniel P. Berrange wrote:
> > >
> > >Only other option I think of is a pure paravirt shutdown/reboot
> > >handler, ala Xen. That sucks in different ways though, namely
> > >needing a driver in the guest, which is probably worse.
> > >  
> > 
> > What do you think about Avi's suggestion of using -no-shutdown, doing a 
> > system_powerdown, waiting to receive a powerdown (you can poll 'info 
> > status' too to see when you entered into the paused state), and then 
> > doing a system_reset?  I'd suggesting doing a timeout too while waiting 
> > to avoid waiting forever.
> 
> That suffers from the same problem as a system_reboot command in that
> the guest may never start the shutdown. If that's an acceptable 
> limitation, then we might as well implement it directly in QEMU as
> system_reboot so its easily available to everyone. Make 'system_reboot'
> take an optional timeout arg (default to 60 seconds) after which it
> cancels its plans. 

This seems the most reasonable thing to me.  As already mentioned,
system_powerdown already is non-deterministic since the guest could
ignore the ACPI event.



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

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

[Stefano Stabellini]

On Wed, 8 Jul 2009, Avi Kivity wrote:
> On 07/08/2009 05:10 PM, Daniel P. Berrange wrote:
> >>> Indeed.  This is what has kept me from applying this but I just can't
> >>> think of any better solution.
> >>>
> >>> Any ideas?
> >>>
> >>> The drive behind implementing this feature is so that we can implement
> >>> a proper virDomainReboot in libvirt.
> >>>
> >>>        
> >> How does Xen hvm implement it?
> >>      
> >
> > You have to install the Xen paravirt drivers, so Xen HVM handles graceful
> > shutdown/reboot in the same way as pure Xen paravirt. They watch a flag
> > in xenstore and invoke shutdown -r / -h according to the flag.
> >    
> 
> What happens if the driver is not installed?  The guest just keeps 
> going, right?
> 

There are ways to detect if the guest has pv drivers installed or not,
if they are present soft shutdown/reboot is available; if they are
not installed soft shutdown/reboot is not available.

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

[Anthony Liguori]

Ryan Harper wrote:
>> That suffers from the same problem as a system_reboot command in that
>> the guest may never start the shutdown. If that's an acceptable 
>> limitation, then we might as well implement it directly in QEMU as
>> system_reboot so its easily available to everyone. Make 'system_reboot'
>> take an optional timeout arg (default to 60 seconds) after which it
>> cancels its plans. 
>>     
>
> This seems the most reasonable thing to me.  As already mentioned,
> system_powerdown already is non-deterministic since the guest could
> ignore the ACPI event.
>   

It's a different class of non-determinism though.  With 
system_powerdown, you make a request to the guest.  You don't know 
whether the guest will comply with the request.  The same level of 
non-determinism exists with ballooning.

With system_reboot, you're taking an action (system_reset) and what's 
not deterministic is whether you're taking that action based on what 
someone requested on the monitor (system_reboot) verses whether you're 
taking that action based on something the user did (shutdown).  In the 
later case, you're action is functionally incorrect.

-- 
Regards,

Anthony Liguori

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

[Daniel P. Berrange]

On Wed, Jul 08, 2009 at 04:47:46PM +0100, Stefano Stabellini wrote:
> On Wed, 8 Jul 2009, Avi Kivity wrote:
> > On 07/08/2009 05:10 PM, Daniel P. Berrange wrote:
> > >>> Indeed.  This is what has kept me from applying this but I just can't
> > >>> think of any better solution.
> > >>>
> > >>> Any ideas?
> > >>>
> > >>> The drive behind implementing this feature is so that we can implement
> > >>> a proper virDomainReboot in libvirt.
> > >>>
> > >>>        
> > >> How does Xen hvm implement it?
> > >>      
> > >
> > > You have to install the Xen paravirt drivers, so Xen HVM handles graceful
> > > shutdown/reboot in the same way as pure Xen paravirt. They watch a flag
> > > in xenstore and invoke shutdown -r / -h according to the flag.
> > >    
> > 
> > What happens if the driver is not installed?  The guest just keeps 
> > going, right?
> > 
> 
> There are ways to detect if the guest has pv drivers installed or not,
> if they are present soft shutdown/reboot is available; if they are
> not installed soft shutdown/reboot is not available.

Having the drivers installed though, still doesn't guarentee that the
guest will honour the operation :-) Both graceful shutdown & reboot are
just *requests* by the host admin, which cannot be expected to work
all the time, it is just best effort. The admin can choose to forcably
destroy a VM if it didn't honour their request to shutdown/reboot.

Similarly this is all the libvirt APIs for shutdown/reboot define
for semantics - best effort, not a firm guarentee.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

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

[Ryan Harper]

* Anthony Liguori <aliguori@us.ibm.com> [2009-07-08 10:50]:
> Ryan Harper wrote:
> >>That suffers from the same problem as a system_reboot command in that
> >>the guest may never start the shutdown. If that's an acceptable 
> >>limitation, then we might as well implement it directly in QEMU as
> >>system_reboot so its easily available to everyone. Make 'system_reboot'
> >>take an optional timeout arg (default to 60 seconds) after which it
> >>cancels its plans. 
> >>    
> >
> >This seems the most reasonable thing to me.  As already mentioned,
> >system_powerdown already is non-deterministic since the guest could
> >ignore the ACPI event.
> >  
> 
> It's a different class of non-determinism though.  With 
> system_powerdown, you make a request to the guest.  You don't know 
> whether the guest will comply with the request.  The same level of 
> non-determinism exists with ballooning.
> 
> With system_reboot, you're taking an action (system_reset) and what's 
> not deterministic is whether you're taking that action based on what 
> someone requested on the monitor (system_reboot) verses whether you're 
> taking that action based on something the user did (shutdown).  In the 
> later case, you're action is functionally incorrect.

Only if the shutdown from the user occurs in timeout period.  So, yes,
it's still present, but the window of the behavior can be much
smaller.

I still think this is a reasonable approach even with that window.


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

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

[Stefano Stabellini]

On Wed, 8 Jul 2009, Daniel P. Berrange wrote:
> Having the drivers installed though, still doesn't guarentee that the
> guest will honour the operation :-) Both graceful shutdown & reboot are
> just *requests* by the host admin, which cannot be expected to work
> all the time, it is just best effort. The admin can choose to forcably
> destroy a VM if it didn't honour their request to shutdown/reboot.

well... pv drivers or no pv drivers the guest should never be trusted
for anything.

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

[Anthony Liguori]

Ryan Harper wrote:
>> It's a different class of non-determinism though.  With 
>> system_powerdown, you make a request to the guest.  You don't know 
>> whether the guest will comply with the request.  The same level of 
>> non-determinism exists with ballooning.
>>
>> With system_reboot, you're taking an action (system_reset) and what's 
>> not deterministic is whether you're taking that action based on what 
>> someone requested on the monitor (system_reboot) verses whether you're 
>> taking that action based on something the user did (shutdown).  In the 
>> later case, you're action is functionally incorrect.
>>     
>
> Only if the shutdown from the user occurs in timeout period.  So, yes,
> it's still present, but the window of the behavior can be much
> smaller.
>
> I still think this is a reasonable approach even with that window.
>   

It's still a policy decision and the question is whether QEMU is the 
right place to implement that policy.  QEMU provides a mechanism today 
to implement this policy (system_powerdown followed by polling via info 
status followed by system_reset).

I tend to agree with Avi that we don't want to implement these sort of 
policies in QEMU and that we instead want to take a PV approach that can 
let us eliminate the nastier non-determinism.

-- 
Regards,

Anthony Liguori

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

[Avi Kivity]

On 07/08/2009 06:43 PM, Ryan Harper wrote:
>> That suffers from the same problem as a system_reboot command in that
>> the guest may never start the shutdown. If that's an acceptable
>> limitation, then we might as well implement it directly in QEMU as
>> system_reboot so its easily available to everyone. Make 'system_reboot'
>> take an optional timeout arg (default to 60 seconds) after which it
>> cancels its plans.
>>      
>
> This seems the most reasonable thing to me.  As already mentioned,
> system_powerdown already is non-deterministic since the guest could
> ignore the ACPI event.
>    

It could also send the guest to sleep or initiate the self destruct 
sequence.  I don't think we should press the power button unless the 
user explicitly asked us to.  This means we'll need a guest agent like Xen.

I believe ACPI supports additional buttons, we can add a new one and 
designate it as a shutdown button.  It will only work with modified 
guests though.

-- 
error compiling committee.c: too many arguments to function

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

[Amit Shah]

On (Wed) Jul 08 2009 [19:02:16], Avi Kivity wrote:
> On 07/08/2009 06:43 PM, Ryan Harper wrote:
>>> That suffers from the same problem as a system_reboot command in that
>>> the guest may never start the shutdown. If that's an acceptable
>>> limitation, then we might as well implement it directly in QEMU as
>>> system_reboot so its easily available to everyone. Make 'system_reboot'
>>> take an optional timeout arg (default to 60 seconds) after which it
>>> cancels its plans.
>>>      
>>
>> This seems the most reasonable thing to me.  As already mentioned,
>> system_powerdown already is non-deterministic since the guest could
>> ignore the ACPI event.
>>    
>
> It could also send the guest to sleep or initiate the self destruct  
> sequence.  I don't think we should press the power button unless the  
> user explicitly asked us to.  This means we'll need a guest agent like 
> Xen.

This is another use-case for vmchannel (or virtio-serial) where  the user
exists within qemu.

		Amit

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

[Jamie Lokier]

Amit Shah wrote:
> On (Wed) Jul 08 2009 [19:02:16], Avi Kivity wrote:
> > On 07/08/2009 06:43 PM, Ryan Harper wrote:
> >>> That suffers from the same problem as a system_reboot command in that
> >>> the guest may never start the shutdown. If that's an acceptable
> >>> limitation, then we might as well implement it directly in QEMU as
> >>> system_reboot so its easily available to everyone. Make 'system_reboot'
> >>> take an optional timeout arg (default to 60 seconds) after which it
> >>> cancels its plans.
> >>>      
> >>
> >> This seems the most reasonable thing to me.  As already mentioned,
> >> system_powerdown already is non-deterministic since the guest could
> >> ignore the ACPI event.
> >>    
> >
> > It could also send the guest to sleep or initiate the self destruct  
> > sequence.  I don't think we should press the power button unless the  
> > user explicitly asked us to.  This means we'll need a guest agent like 
> > Xen.
> 
> This is another use-case for vmchannel (or virtio-serial) where  the user
> exists within qemu.

What happens then the guest is non-responsive because it crashed, or
the agent isn't working, or the guest doesn't support virtio?

Some way to override the guest is needed for all reboot/powerdown options.

"kill -9 $(pidof kvm)" is really unacceptable, but I've had to do it
on several occasions unfortunately.

Ideally, an option to those commands "--force" to ignore the guest and
just do it, and an option like "--timeout-force=5" to behave like
holding down the power button on a real computer.

(It would be nice to be able to change the value of -no-shutdown and
-no-reboot from the monitor, too.)

-- Jamie

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

[Amit Shah]

On (Thu) Jul 16 2009 [15:39:59], Jamie Lokier wrote:
> Amit Shah wrote:
> > On (Wed) Jul 08 2009 [19:02:16], Avi Kivity wrote:
> > > On 07/08/2009 06:43 PM, Ryan Harper wrote:
> > >>> That suffers from the same problem as a system_reboot command in that
> > >>> the guest may never start the shutdown. If that's an acceptable
> > >>> limitation, then we might as well implement it directly in QEMU as
> > >>> system_reboot so its easily available to everyone. Make 'system_reboot'
> > >>> take an optional timeout arg (default to 60 seconds) after which it
> > >>> cancels its plans.
> > >>>      
> > >>
> > >> This seems the most reasonable thing to me.  As already mentioned,
> > >> system_powerdown already is non-deterministic since the guest could
> > >> ignore the ACPI event.
> > >>    
> > >
> > > It could also send the guest to sleep or initiate the self destruct  
> > > sequence.  I don't think we should press the power button unless the  
> > > user explicitly asked us to.  This means we'll need a guest agent like 
> > > Xen.
> > 
> > This is another use-case for vmchannel (or virtio-serial) where  the user
> > exists within qemu.
> 
> What happens then the guest is non-responsive because it crashed, or
> the agent isn't working, or the guest doesn't support virtio?

In the first two cases it has to be forced (as for a physical machine).
The last case can be detected before using any virtio-serial
functionality.

> Some way to override the guest is needed for all reboot/powerdown options.

Sure, just that a PV reboot/powerdown option is more reliable if the
guest ignores the ACPI events.

> "kill -9 $(pidof kvm)" is really unacceptable, but I've had to do it
> on several occasions unfortunately.
> 
> Ideally, an option to those commands "--force" to ignore the guest and
> just do it, and an option like "--timeout-force=5" to behave like
> holding down the power button on a real computer.
> 
> (It would be nice to be able to change the value of -no-shutdown and
> -no-reboot from the monitor, too.)
> 
> -- Jamie

		Amit