[PATCH V2 1/3] Introduce a new flag for controlling shutdown/reboot

[PATCH V2 1/3] Introduce a new flag for controlling shutdown/reboot

[Jim Fehlig]

Add a new flag to virDomain{Reboot,Shutdown}FlagValues to allow
shutting down and rebooting a domain via the Xen paravirt control
interface.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 include/libvirt/libvirt.h.in |  2 ++
 tools/virsh-domain.c         | 14 ++++++++++----
 tools/virsh.pod              |  8 ++++----
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 930b7e8..2c7565a 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -1652,6 +1652,7 @@ typedef enum {
     VIR_DOMAIN_SHUTDOWN_GUEST_AGENT    = (1 << 1), /* Use guest agent */
     VIR_DOMAIN_SHUTDOWN_INITCTL        = (1 << 2), /* Use initctl */
     VIR_DOMAIN_SHUTDOWN_SIGNAL         = (1 << 3), /* Send a signal */
+    VIR_DOMAIN_SHUTDOWN_PARAVIRT       = (1 << 4), /* Use paravirt guest control */
 } virDomainShutdownFlagValues;
 
 int                     virDomainShutdown       (virDomainPtr domain);
@@ -1664,6 +1665,7 @@ typedef enum {
     VIR_DOMAIN_REBOOT_GUEST_AGENT    = (1 << 1), /* Use guest agent */
     VIR_DOMAIN_REBOOT_INITCTL        = (1 << 2), /* Use initctl */
     VIR_DOMAIN_REBOOT_SIGNAL         = (1 << 3), /* Send a signal */
+    VIR_DOMAIN_REBOOT_PARAVIRT       = (1 << 4), /* Use paravirt guest control */
 } virDomainRebootFlagValues;
 
 int                     virDomainReboot         (virDomainPtr domain,
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 73414f8..3a7c260 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -4837,7 +4837,7 @@ static const vshCmdOptDef opts_shutdown[] = {
     },
     {.name = "mode",
      .type = VSH_OT_STRING,
-     .help = N_("shutdown mode: acpi|agent|initctl|signal")
+     .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt")
     },
     {.name = NULL}
 };
@@ -4872,9 +4872,12 @@ cmdShutdown(vshControl *ctl, const vshCmd *cmd)
             flags |= VIR_DOMAIN_SHUTDOWN_INITCTL;
         } else if (STREQ(mode, "signal")) {
             flags |= VIR_DOMAIN_SHUTDOWN_SIGNAL;
+        } else if (STREQ(mode, "paravirt")) {
+            flags |= VIR_DOMAIN_SHUTDOWN_PARAVIRT;
         } else {
             vshError(ctl, _("Unknown mode %s value, expecting "
-                            "'acpi', 'agent', 'initctl' or 'signal'"), mode);
+                            "'acpi', 'agent', 'initctl', 'signal', "
+                            "or 'paravirt'"), mode);
             goto cleanup;
         }
         tmp++;
@@ -4923,7 +4926,7 @@ static const vshCmdOptDef opts_reboot[] = {
     },
     {.name = "mode",
      .type = VSH_OT_STRING,
-     .help = N_("shutdown mode: acpi|agent|initctl|signal")
+     .help = N_("shutdown mode: acpi|agent|initctl|signal|paravirt")
     },
     {.name = NULL}
 };
@@ -4957,9 +4960,12 @@ cmdReboot(vshControl *ctl, const vshCmd *cmd)
             flags |= VIR_DOMAIN_REBOOT_INITCTL;
         } else if (STREQ(mode, "signal")) {
             flags |= VIR_DOMAIN_REBOOT_SIGNAL;
+        } else if (STREQ(mode, "paravirt")) {
+            flags |= VIR_DOMAIN_REBOOT_PARAVIRT;
         } else {
             vshError(ctl, _("Unknown mode %s value, expecting "
-                            "'acpi', 'agent', 'initctl' or 'signal'"), mode);
+                            "'acpi', 'agent', 'initctl', 'signal' "
+                            "or 'paravirt'"), mode);
             goto cleanup;
         }
         tmp++;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index abd2e93..9104804 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1302,8 +1302,8 @@ I<on_reboot> parameter in the domain's XML definition.
 By default the hypervisor will try to pick a suitable shutdown
 method. To specify an alternative method, the I<--mode> parameter
 can specify a comma separated list which includes C<acpi>, C<agent>,
-C<initctl> and C<signal>. The order in which drivers will try each
-mode is undefined, and not related to the order specified to virsh.
+C<initctl>, C<signal> and C<paravirt>. The order in which drivers will
+try each mode is undefined, and not related to the order specified to virsh.
 For strict control over ordering, use a single mode at a time and
 repeat the command.
 
@@ -1781,8 +1781,8 @@ snapshot metadata with B<snapshot-create>.
 By default the hypervisor will try to pick a suitable shutdown
 method. To specify an alternative method, the I<--mode> parameter
 can specify a comma separated list which includes C<acpi>, C<agent>,
-C<initctl> and C<signal>. The order in which drivers will try each
-mode is undefined, and not related to the order specified to virsh.
+C<initctl>, C<signal> and C<paravirt>. The order in which drivers will
+try each mode is undefined, and not related to the order specified to virsh.
 For strict control over ordering, use a single mode at a time and
 repeat the command.
 
-- 
1.8.1.4

[PATCH V2 2/3] libxl: support PARAVIRT and ACPI shutdown flags

[Jim Fehlig]

Add support for VIR_DOMAIN_SHUTDOWN_PARAVIRT and
VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flags in
libxlDomainShutdownFlags().

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index dcc3ac2..28e8512 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -873,7 +873,11 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
     int ret = -1;
     libxlDomainObjPrivatePtr priv;
 
-    virCheckFlags(0, -1);
+    virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
+                  VIR_DOMAIN_SHUTDOWN_PARAVIRT, -1);
+    if (flags == 0)
+        flags = VIR_DOMAIN_SHUTDOWN_PARAVIRT |
+            VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN;
 
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
@@ -888,18 +892,32 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
     }
 
     priv = vm->privateData;
-    if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) {
+    if (flags & VIR_DOMAIN_SHUTDOWN_PARAVIRT) {
+        ret = libxl_domain_shutdown(priv->ctx, vm->def->id);
+        if (ret == 0)
+            goto cleanup;
+
+        if (ret != ERROR_NOPARAVIRT) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to shutdown domain '%d' with libxenlight"),
+                           vm->def->id);
+            ret = -1;
+            goto cleanup;
+        }
+    }
+
+    if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) {
+        ret = libxl_send_trigger(priv->ctx, vm->def->id,
+                                 LIBXL_TRIGGER_POWER, 0);
+        if (ret == 0)
+            goto cleanup;
+
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to shutdown domain '%d' with libxenlight"),
                        vm->def->id);
-        goto cleanup;
+        ret = -1;
     }
 
-    /* vm is marked shutoff (or removed from domains list if not persistent)
-     * in shutdown event handler.
-     */
-    ret = 0;
-
  cleanup:
     if (vm)
         virObjectUnlock(vm);
-- 
1.8.1.4

[PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Jim Fehlig]

Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
libxlDomainReboot().

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 28e8512..6c63251 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -938,7 +938,11 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     int ret = -1;
     libxlDomainObjPrivatePtr priv;
 
-    virCheckFlags(0, -1);
+    virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
+                  VIR_DOMAIN_REBOOT_PARAVIRT, -1);
+    if (flags == 0)
+        flags = VIR_DOMAIN_REBOOT_PARAVIRT |
+            VIR_DOMAIN_REBOOT_ACPI_POWER_BTN;
 
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
@@ -953,13 +957,31 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     }
 
     priv = vm->privateData;
-    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
+    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
+        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
+        if (ret == 0)
+            goto cleanup;
+
+        if (ret != ERROR_NOPARAVIRT) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("Failed to reboot domain '%d' with libxenlight"),
+                           vm->def->id);
+            ret = -1;
+            goto cleanup;
+        }
+    }
+
+    if (flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) {
+        ret = libxl_send_trigger(priv->ctx, vm->def->id,
+                                 LIBXL_TRIGGER_RESET, 0);
+        if (ret == 0)
+            goto cleanup;
+
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to reboot domain '%d' with libxenlight"),
                        vm->def->id);
-        goto cleanup;
+        ret = -1;
     }
-    ret = 0;
 
  cleanup:
     if (vm)
-- 
1.8.1.4

Re: [PATCH V2 2/3] libxl: support PARAVIRT and ACPI shutdown flags

[Ian Campbell]

On Thu, 2014-05-01 at 16:14 -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_SHUTDOWN_PARAVIRT and
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flags in
> libxlDomainShutdownFlags().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>

For the libxl use: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [libvirt] [PATCH V2 2/3] libxl: support PARAVIRT and ACPI shutdown flags

[Daniel P. Berrange]

On Thu, May 01, 2014 at 04:14:38PM -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_SHUTDOWN_PARAVIRT and
> VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN flags in
> libxlDomainShutdownFlags().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index dcc3ac2..28e8512 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -873,7 +873,11 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      int ret = -1;
>      libxlDomainObjPrivatePtr priv;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN |
> +                  VIR_DOMAIN_SHUTDOWN_PARAVIRT, -1);
> +    if (flags == 0)
> +        flags = VIR_DOMAIN_SHUTDOWN_PARAVIRT |
> +            VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN;
>  
>      if (!(vm = libxlDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -888,18 +892,32 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags)
>      }
>  
>      priv = vm->privateData;
> -    if (libxl_domain_shutdown(priv->ctx, vm->def->id) != 0) {
> +    if (flags & VIR_DOMAIN_SHUTDOWN_PARAVIRT) {
> +        ret = libxl_domain_shutdown(priv->ctx, vm->def->id);
> +        if (ret == 0)
> +            goto cleanup;
> +
> +        if (ret != ERROR_NOPARAVIRT) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to shutdown domain '%d' with libxenlight"),
> +                           vm->def->id);
> +            ret = -1;
> +            goto cleanup;
> +        }

I think you need 'ret = -1' here too, otherwise if domain shutdown
fails, but flags does not request ACPI, then we can leak
ret == ERROR_NOPARAVIRT back to the public API.

> +    }
> +
> +    if (flags & VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN) {
> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
> +                                 LIBXL_TRIGGER_POWER, 0);
> +        if (ret == 0)
> +            goto cleanup;
> +
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to shutdown domain '%d' with libxenlight"),
>                         vm->def->id);
> -        goto cleanup;
> +        ret = -1;
>      }
>  
> -    /* vm is marked shutoff (or removed from domains list if not persistent)
> -     * in shutdown event handler.
> -     */
> -    ret = 0;
> -
>   cleanup:
>      if (vm)
>          virObjectUnlock(vm);

ACK to the patch with that fix added  (post 1.2.4 release of course)


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH V2 1/3] Introduce a new flag for controlling shutdown/reboot

[Daniel P. Berrange]

On Thu, May 01, 2014 at 04:14:37PM -0600, Jim Fehlig wrote:
> Add a new flag to virDomain{Reboot,Shutdown}FlagValues to allow
> shutting down and rebooting a domain via the Xen paravirt control
> interface.
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  include/libvirt/libvirt.h.in |  2 ++
>  tools/virsh-domain.c         | 14 ++++++++++----
>  tools/virsh.pod              |  8 ++++----
>  3 files changed, 16 insertions(+), 8 deletions(-)

ACK for merge post-1.2.4 release

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Ian Campbell]

On Thu, 2014-05-01 at 16:14 -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
> VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
> libxlDomainReboot().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>

libxl side looks correct; Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [libvirt] [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Daniel P. Berrange]

On Thu, May 01, 2014 at 04:14:39PM -0600, Jim Fehlig wrote:
> Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
> VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
> libxlDomainReboot().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 28e8512..6c63251 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -938,7 +938,11 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>      int ret = -1;
>      libxlDomainObjPrivatePtr priv;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
> +                  VIR_DOMAIN_REBOOT_PARAVIRT, -1);
> +    if (flags == 0)
> +        flags = VIR_DOMAIN_REBOOT_PARAVIRT |
> +            VIR_DOMAIN_REBOOT_ACPI_POWER_BTN;
>  
>      if (!(vm = libxlDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -953,13 +957,31 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>      }
>  
>      priv = vm->privateData;
> -    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
> +    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
> +        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
> +        if (ret == 0)
> +            goto cleanup;
> +
> +        if (ret != ERROR_NOPARAVIRT) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to reboot domain '%d' with libxenlight"),
> +                           vm->def->id);
> +            ret = -1;
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) {
> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
> +                                 LIBXL_TRIGGER_RESET, 0);

What does this trigger in ACPI ? IIUC, it'll do a hard reset of the
board, which is not the same as a controlled reboot which this API
wants.  There isn't any ACPI button that I know of that guests will
interpret todo a controlled reboot, so in the QEMU driver we actually
just send a normal ACPI shutdown event. We have QEMU configured with
the '-no-shutdown' flag so when it finishes doing an controlled APCI
shutdown, we can then reset the board and start the CPUs again, which
gives the illusion of a controlled reboot.

Given that Xen has a decent paravirt reboot facility I'd probably
just not bother with trying to fake the controlled reboot via ACPI.
We only did this with QEMU since we didn't have any alternative to
make reboot work with QEMU at the time, since the QEMU guest agent
did not exist then.

A hard reset of the machine board is something you can implement
in the virDomainReset() API if you like though.

> +        if (ret == 0)
> +            goto cleanup;
> +
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to reboot domain '%d' with libxenlight"),
>                         vm->def->id);
> -        goto cleanup;
> +        ret = -1;
>      }
> -    ret = 0;
>  
>   cleanup:
>      if (vm)


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Thu, May 01, 2014 at 04:14:39PM -0600, Jim Fehlig wrote:
>   
>> Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
>> VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
>> libxlDomainReboot().
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>>  src/libxl/libxl_driver.c | 30 ++++++++++++++++++++++++++----
>>  1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 28e8512..6c63251 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -938,7 +938,11 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>>      int ret = -1;
>>      libxlDomainObjPrivatePtr priv;
>>  
>> -    virCheckFlags(0, -1);
>> +    virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
>> +                  VIR_DOMAIN_REBOOT_PARAVIRT, -1);
>> +    if (flags == 0)
>> +        flags = VIR_DOMAIN_REBOOT_PARAVIRT |
>> +            VIR_DOMAIN_REBOOT_ACPI_POWER_BTN;
>>  
>>      if (!(vm = libxlDomObjFromDomain(dom)))
>>          goto cleanup;
>> @@ -953,13 +957,31 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>>      }
>>  
>>      priv = vm->privateData;
>> -    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
>> +    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
>> +        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
>> +        if (ret == 0)
>> +            goto cleanup;
>> +
>> +        if (ret != ERROR_NOPARAVIRT) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Failed to reboot domain '%d' with libxenlight"),
>> +                           vm->def->id);
>> +            ret = -1;
>> +            goto cleanup;
>> +        }
>> +    }
>> +
>> +    if (flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) {
>> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
>> +                                 LIBXL_TRIGGER_RESET, 0);
>>     
>
> What does this trigger in ACPI ? IIUC, it'll do a hard reset of the
> board,

Yes, I think you are right.  Similar to pushing the reset button.

>  which is not the same as a controlled reboot which this API
> wants.  There isn't any ACPI button that I know of that guests will
> interpret todo a controlled reboot, so in the QEMU driver we actually
> just send a normal ACPI shutdown event. We have QEMU configured with
> the '-no-shutdown' flag so when it finishes doing an controlled APCI
> shutdown, we can then reset the board and start the CPUs again, which
> gives the illusion of a controlled reboot.
>
> Given that Xen has a decent paravirt reboot facility I'd probably
> just not bother with trying to fake the controlled reboot via ACPI.
>   

Ok, that sounds reasonable to me.  I'll drop this patch when pushing the
others, post 1.2.4.  Should 1/3 retain the VIR_DOMAIN_REBOOT_PARAVIRT
addition tovirDomainRebootFlagValues?

> We only did this with QEMU since we didn't have any alternative to
> make reboot work with QEMU at the time, since the QEMU guest agent
> did not exist then.
>
> A hard reset of the machine board is something you can implement
> in the virDomainReset() API if you like though.
>   

Ah, yes, good point.  domainReset is missing in the libxl driver.

Regards,
Jim

Re: [libvirt] [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Daniel P. Berrange]

On Fri, May 02, 2014 at 08:01:00AM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Thu, May 01, 2014 at 04:14:39PM -0600, Jim Fehlig wrote:
> >   
> >> Add support for VIR_DOMAIN_REBOOT_PARAVIRT and
> >> VIR_DOMAIN_REBOOT_ACPI_POWER_BTN flags in
> >> libxlDomainReboot().
> >>
> >> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> >> ---
> >>  src/libxl/libxl_driver.c | 30 ++++++++++++++++++++++++++----
> >>  1 file changed, 26 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >> index 28e8512..6c63251 100644
> >> --- a/src/libxl/libxl_driver.c
> >> +++ b/src/libxl/libxl_driver.c
> >> @@ -938,7 +938,11 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
> >>      int ret = -1;
> >>      libxlDomainObjPrivatePtr priv;
> >>  
> >> -    virCheckFlags(0, -1);
> >> +    virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN |
> >> +                  VIR_DOMAIN_REBOOT_PARAVIRT, -1);
> >> +    if (flags == 0)
> >> +        flags = VIR_DOMAIN_REBOOT_PARAVIRT |
> >> +            VIR_DOMAIN_REBOOT_ACPI_POWER_BTN;
> >>  
> >>      if (!(vm = libxlDomObjFromDomain(dom)))
> >>          goto cleanup;
> >> @@ -953,13 +957,31 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
> >>      }
> >>  
> >>      priv = vm->privateData;
> >> -    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
> >> +    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
> >> +        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
> >> +        if (ret == 0)
> >> +            goto cleanup;
> >> +
> >> +        if (ret != ERROR_NOPARAVIRT) {
> >> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                           _("Failed to reboot domain '%d' with libxenlight"),
> >> +                           vm->def->id);
> >> +            ret = -1;
> >> +            goto cleanup;
> >> +        }
> >> +    }
> >> +
> >> +    if (flags & VIR_DOMAIN_REBOOT_ACPI_POWER_BTN) {
> >> +        ret = libxl_send_trigger(priv->ctx, vm->def->id,
> >> +                                 LIBXL_TRIGGER_RESET, 0);
> >>     
> >
> > What does this trigger in ACPI ? IIUC, it'll do a hard reset of the
> > board,
> 
> Yes, I think you are right.  Similar to pushing the reset button.
> 
> >  which is not the same as a controlled reboot which this API
> > wants.  There isn't any ACPI button that I know of that guests will
> > interpret todo a controlled reboot, so in the QEMU driver we actually
> > just send a normal ACPI shutdown event. We have QEMU configured with
> > the '-no-shutdown' flag so when it finishes doing an controlled APCI
> > shutdown, we can then reset the board and start the CPUs again, which
> > gives the illusion of a controlled reboot.
> >
> > Given that Xen has a decent paravirt reboot facility I'd probably
> > just not bother with trying to fake the controlled reboot via ACPI.
> >   
> 
> Ok, that sounds reasonable to me.  I'll drop this patch when pushing the
> others, post 1.2.4.  Should 1/3 retain the VIR_DOMAIN_REBOOT_PARAVIRT
> addition tovirDomainRebootFlagValues?

I don't think you need to drop the patch/code. It is still useful, IMHO,
to have the explicit flag for VIR_DOMAIN_REBOOT_PARAVIRT. I'd just
suggest you remove the block of code for VIR_DOMAIN_REBOOT_ACPI_POWER_BTN
impl in the reboot method.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Jim Fehlig]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

Daniel P. Berrange wrote:
> On Fri, May 02, 2014 at 08:01:00AM -0600, Jim Fehlig wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> Given that Xen has a decent paravirt reboot facility I'd probably
>>> just not bother with trying to fake the controlled reboot via ACPI.
>>>   
>>>       
>> Ok, that sounds reasonable to me.  I'll drop this patch when pushing the
>> others, post 1.2.4.  Should 1/3 retain the VIR_DOMAIN_REBOOT_PARAVIRT
>> addition tovirDomainRebootFlagValues?
>>     
>
> I don't think you need to drop the patch/code. It is still useful, IMHO,
> to have the explicit flag for VIR_DOMAIN_REBOOT_PARAVIRT. I'd just
> suggest you remove the block of code for VIR_DOMAIN_REBOOT_ACPI_POWER_BTN
> impl in the reboot method.
>   

Just to clarify, do you mean changing this patch to the attached one?

Regards,
Jim


[-- Attachment #2: libxl-PARAVIRT-reboot-flag.patch --]
[-- Type: text/x-patch, Size: 1529 bytes --]

>From e76f891bd843dd4f7b895d3929c9f561162a69a9 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
Date: Thu, 1 May 2014 15:00:47 -0600
Subject: [PATCH 3/3] libxl: support PARAVIRT reboot flag

Add support for the VIR_DOMAIN_REBOOT_PARAVIRT flag in
libxlDomainReboot().

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_driver.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 28e8512..edbfa57 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -938,7 +938,9 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     int ret = -1;
     libxlDomainObjPrivatePtr priv;
 
-    virCheckFlags(0, -1);
+    virCheckFlags(VIR_DOMAIN_REBOOT_PARAVIRT, -1);
+    if (flags == 0)
+        flags = VIR_DOMAIN_REBOOT_PARAVIRT;
 
     if (!(vm = libxlDomObjFromDomain(dom)))
         goto cleanup;
@@ -953,13 +955,16 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
     }
 
     priv = vm->privateData;
-    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
+    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
+        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
+        if (ret == 0)
+            goto cleanup;
+
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Failed to reboot domain '%d' with libxenlight"),
                        vm->def->id);
-        goto cleanup;
+        ret = -1;
     }
-    ret = 0;
 
  cleanup:
     if (vm)
-- 
1.8.1.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [libvirt] [PATCH V2 3/3] libxl: support PARAVIRT and ACPI reboot flags

[Daniel P. Berrange]

On Fri, May 02, 2014 at 08:43:57AM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Fri, May 02, 2014 at 08:01:00AM -0600, Jim Fehlig wrote:
> >   
> >> Daniel P. Berrange wrote:
> >>     
> >>> Given that Xen has a decent paravirt reboot facility I'd probably
> >>> just not bother with trying to fake the controlled reboot via ACPI.
> >>>   
> >>>       
> >> Ok, that sounds reasonable to me.  I'll drop this patch when pushing the
> >> others, post 1.2.4.  Should 1/3 retain the VIR_DOMAIN_REBOOT_PARAVIRT
> >> addition tovirDomainRebootFlagValues?
> >>     
> >
> > I don't think you need to drop the patch/code. It is still useful, IMHO,
> > to have the explicit flag for VIR_DOMAIN_REBOOT_PARAVIRT. I'd just
> > suggest you remove the block of code for VIR_DOMAIN_REBOOT_ACPI_POWER_BTN
> > impl in the reboot method.
> >   
> 
> Just to clarify, do you mean changing this patch to the attached one?

Yes, you got it.

> From e76f891bd843dd4f7b895d3929c9f561162a69a9 Mon Sep 17 00:00:00 2001
> From: Jim Fehlig <jfehlig@suse.com>
> Date: Thu, 1 May 2014 15:00:47 -0600
> Subject: [PATCH 3/3] libxl: support PARAVIRT reboot flag
> 
> Add support for the VIR_DOMAIN_REBOOT_PARAVIRT flag in
> libxlDomainReboot().
> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
>  src/libxl/libxl_driver.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 28e8512..edbfa57 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -938,7 +938,9 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>      int ret = -1;
>      libxlDomainObjPrivatePtr priv;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_REBOOT_PARAVIRT, -1);
> +    if (flags == 0)
> +        flags = VIR_DOMAIN_REBOOT_PARAVIRT;
>  
>      if (!(vm = libxlDomObjFromDomain(dom)))
>          goto cleanup;
> @@ -953,13 +955,16 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags)
>      }
>  
>      priv = vm->privateData;
> -    if (libxl_domain_reboot(priv->ctx, vm->def->id) != 0) {
> +    if (flags & VIR_DOMAIN_REBOOT_PARAVIRT) {
> +        ret = libxl_domain_reboot(priv->ctx, vm->def->id);
> +        if (ret == 0)
> +            goto cleanup;
> +
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to reboot domain '%d' with libxenlight"),
>                         vm->def->id);
> -        goto cleanup;
> +        ret = -1;
>      }
> -    ret = 0;
>  
>   cleanup:
>      if (vm)

ACK


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [libvirt] [PATCH V2 0/3] support flags in libxlDomain{Shutdown, Reboot}

[Jim Fehlig]

Jim Fehlig wrote:
> This small series is a V2 of
>
>   https://www.redhat.com/archives/libvir-list/2014-April/msg00837.html
>
> Patch 1 adds VIR_DOMAIN_SHUTDOWN_PARAVIRT and
> VIR_REBOOT_SHUTDOWN_PARAVIRT flags to the API as requested by danpb.
>
> Patch 2 makes use of the shutdown flags in libxlDomainShutdownFlags.
> Patch 3 does the same for libxlDomainReboot.
>
> Jim Fehlig (3):
>   Introduce a new flag for controlling shutdown/reboot
>   libxl: support PARAVIRT and ACPI shutdown flags
>   libxl: support PARAVIRT and ACPI reboot flags
>   

Now that 1.2.4 is out, I've pushed this series after squashing in the
suggested changes to 2 and 3.

Regards,
Jim

>  include/libvirt/libvirt.h.in |  2 ++
>  src/libxl/libxl_driver.c     | 64 +++++++++++++++++++++++++++++++++++---------
>  tools/virsh-domain.c         | 14 +++++++---
>  tools/virsh.pod              |  8 +++---
>  4 files changed, 68 insertions(+), 20 deletions(-)
>
>