[libvirt] [PATCH V2 0/3] support flags in libxlDomain{Shutdown, Reboot}
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 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(-) -- 1.8.1.4
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
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 :|
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
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>
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 :|
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
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>
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 :|
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
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 :|
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
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 :|
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(-)
participants (3)
-
Daniel P. Berrange -
Ian Campbell -
Jim Fehlig