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

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(a)suse.com&gt; >> --- >> 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?