kvm-devel

[kvm-devel] [PATCH] system_powerdown via acpi power button

[Guido G. <ag...@si...>]

Hi,
in order to be able to shutdown kvm cleanly with libvirt (currently
libvirt simply sends a SIGTERM) I implemented system_powerdown() to
simulate the pressing of a fixed feature acpi power button. The patch
has one issue though, it currently needs --no-kvm-irqchip (see my other
mail). Please apply if appropriate.

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 314df94..7a96ece 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1318,8 +1318,8 @@ void acpi_bios_init(void)
     fadt->pm_tmr_len = 4;
     fadt->plvl2_lat = cpu_to_le16(0x0fff); // C2 state not supported
     fadt->plvl3_lat = cpu_to_le16(0x0fff); // C3 state not supported
-    /* WBINVD + PROC_C1 + PWR_BUTTON + SLP_BUTTON + FIX_RTC */
-    fadt->flags = cpu_to_le32((1 << 0) | (1 << 2) | (1 << 4) | (1 << 5) | (1 << 6));
+    /* WBINVD + PROC_C1 + SLP_BUTTON + FIX_RTC */
+    fadt->flags = cpu_to_le32((1 << 0) | (1 << 2) | (1 << 5) | (1 << 6));
     acpi_build_table_header((struct acpi_table_header *)fadt, "FACP", 
                             sizeof(*fadt));
 
diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index a2efd9c..fa78b81 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -71,6 +71,8 @@ typedef struct PIIX4PMState {
 #define SMBHSTDAT1 0x06
 #define SMBBLKDAT 0x07
 
+PIIX4PMState *pm_state;
+
 static uint32_t get_pmtmr(PIIX4PMState *s)
 {
     uint32_t d;
@@ -475,6 +477,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
     s = (PIIX4PMState *)pci_register_device(bus,
                                          "PM", sizeof(PIIX4PMState),
                                          devfn, NULL, pm_write_config);
+    pm_state = s;
     pci_conf = s->dev.config;
     pci_conf[0x00] = 0x86;
     pci_conf[0x01] = 0x80;
@@ -516,3 +519,13 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
     s->smbus = i2c_init_bus();
     return s->smbus;
 }
+
+#if defined(TARGET_I386)
+void qemu_system_powerdown(void)
+{
+    if(pm_state->pmen & PWRBTN_EN) {
+	pm_state->pmsts |= PWRBTN_EN;
+    	pm_update_sci(pm_state);
+    }
+}
+#endif
diff --git a/qemu/pc-bios/bios.bin b/qemu/pc-bios/bios.bin
index d2729cd..fb3a72b 100644
Binary files a/qemu/pc-bios/bios.bin and b/qemu/pc-bios/bios.bin differ
diff --git a/qemu/sysemu.h b/qemu/sysemu.h
index c8478ec..5fd7fc2 100644
--- a/qemu/sysemu.h
+++ b/qemu/sysemu.h
@@ -33,7 +33,7 @@ void qemu_system_powerdown_request(void);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
-#if !defined(TARGET_SPARC)
+#if !defined(TARGET_SPARC) && !defined(TARGET_I386)
 // Please implement a power failure function to signal the OS
 #define qemu_system_powerdown() do{}while(0)
 #else

Cheers,
 -- Guido

[kvm-devel] acpi sci polarity

[Guido G. <ag...@si...>]

On Thu, Jan 03, 2008 at 07:11:32PM +0100, Guido Guenther wrote:
> simulate the pressing of a fixed feature acpi power button. The patch
> has one issue though, it currently needs --no-kvm-irqchip (see my other
> mail). Please apply if appropriate.
When running without --no-kvm-irqchip the ACPI irq never gets delivered,
the ACPI interrupt always stays at zero in /proc/interrupt. However I
can hack around this with:

--- a/kernel/ioapic.c
+++ b/kernel/ioapic.c
@@ -248,6 +253,8 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];
+		if(irq == 10) /* ACPI interrupt */
+			entry.fields.polarity = 0;
 		level ^= entry.fields.polarity;
 		if (!level)
 			ioapic->irr &= ~mask;

If I understand the docs correctly the ACPI SCI should be an (active low
not active high) leveled interrupt. Where would be the proper place to
fix this?
 -- Guido

Re: [kvm-devel] acpi sci polarity

[Avi K. <av...@qu...>]

Guido Guenther wrote:
> On Thu, Jan 03, 2008 at 07:11:32PM +0100, Guido Guenther wrote:
>   
>> simulate the pressing of a fixed feature acpi power button. The patch
>> has one issue though, it currently needs --no-kvm-irqchip (see my other
>> mail). Please apply if appropriate.
>>     
> When running without --no-kvm-irqchip the ACPI irq never gets delivered,
> the ACPI interrupt always stays at zero in /proc/interrupt. However I
> can hack around this with:
>
> --- a/kernel/ioapic.c
> +++ b/kernel/ioapic.c
> @@ -248,6 +253,8 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
>  
>  	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>  		entry = ioapic->redirtbl[irq];
> +		if(irq == 10) /* ACPI interrupt */
> +			entry.fields.polarity = 0;
>  		level ^= entry.fields.polarity;
>  		if (!level)
>  			ioapic->irr &= ~mask;
>
> If I understand the docs correctly the ACPI SCI should be an (active low
> not active high) leveled interrupt. Where would be the proper place to
> fix this?
>   

While pci interrupts are documented as active high, the documentation 
for the piix4 pic elcr registers suggests piix4 level-triggered 
interrupts are active high.  So there is some inconsistency somewhere 
involving qemu interrupt generation (which IIRC treats all interrupts as 
active high), the ioapic (qemu doesn't implement polarity), bios setup, 
and the acpi dsdt.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [kvm-devel] acpi sci polarity

[Guido G. <ag...@si...>]

Hi Avi,
On Thu, Jan 03, 2008 at 11:17:50PM +0200, Avi Kivity wrote:
> While pci interrupts are documented as active high, the documentation for 
> the piix4 pic elcr registers suggests piix4 level-triggered interrupts are 
> active high.  So there is some inconsistency somewhere involving qemu 
> interrupt generation (which IIRC treats all interrupts as active high), the 
> ioapic (qemu doesn't implement polarity), bios setup, and the acpi dsdt.
We currently specify INT10 as active high in the dsl so I added an
override entry to the madt an now the OS knows about it and things work
as expected - we this be o.k. until all the IRQ setup gets a revamp:

>From 7950892de473d42ce57e0727594190b4bfafab14 Mon Sep 17 00:00:00 2001
From: Guido Guenther <ag...@si...>
Date: Fri, 4 Jan 2008 19:26:33 +0100
Subject: [PATCH] int 10 is currently active high

so add a proper interrupt override entry to the MADT so the OS knows about it.
Fixes the ACPI powerbutton and should also fix the PM timer.

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 7a96ece..99f7d23 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1181,6 +1181,14 @@ struct madt_io_apic
 			  * lines start */
 };
 
+struct madt_intsrcovr {
+	APIC_HEADER_DEF
+	uint8_t  bus;
+	uint8_t  source;
+	uint32_t gsi;
+	uint16_t flags;
+};
+
 #include "acpi-dsdt.hex"
 
 static inline uint16_t cpu_to_le16(uint16_t x)
@@ -1271,9 +1279,10 @@ void acpi_bios_init(void)
 
     addr = (addr + 7) & ~7;
     madt_addr = addr;
-    madt_size = sizeof(*madt) + 
+    madt_size = sizeof(*madt) +
         sizeof(struct madt_processor_apic) * smp_cpus +
-        sizeof(struct madt_io_apic);
+        sizeof(struct madt_io_apic) +
+        sizeof(struct madt_intsrcovr);
     madt = (void *)(addr);
     addr += madt_size;
 
@@ -1335,6 +1344,7 @@ void acpi_bios_init(void)
     {
         struct madt_processor_apic *apic;
         struct madt_io_apic *io_apic;
+        struct madt_intsrcovr *intsrcovr;
 
         memset(madt, 0, madt_size);
         madt->local_apic_address = cpu_to_le32(0xfee00000);
@@ -1355,6 +1365,15 @@ void acpi_bios_init(void)
         io_apic->address = cpu_to_le32(0xfec00000);
         io_apic->interrupt = cpu_to_le32(0);
 
+	/* int 10 (SCI) is active high atm */
+        intsrcovr = (struct madt_intsrcovr*)(io_apic + 1);
+        memset(intsrcovr, 0, sizeof(*intsrcovr));
+        intsrcovr->type   = APIC_XRUPT_OVERRIDE;
+        intsrcovr->length = sizeof(*intsrcovr);
+        intsrcovr->source = 10;
+        intsrcovr->gsi    = 10;
+        intsrcovr->flags  = 0xd;
+
         acpi_build_table_header((struct acpi_table_header *)madt, 
                                 "APIC", madt_size);
     }

Cheers,
 -- Guido

Re: [kvm-devel] acpi sci polarity

[Avi K. <av...@qu...>]

Guido Guenther wrote:
> Hi Avi,
> On Thu, Jan 03, 2008 at 11:17:50PM +0200, Avi Kivity wrote:
>   
>> While pci interrupts are documented as active high, the documentation for 
>> the piix4 pic elcr registers suggests piix4 level-triggered interrupts are 
>> active high.  So there is some inconsistency somewhere involving qemu 
>> interrupt generation (which IIRC treats all interrupts as active high), the 
>> ioapic (qemu doesn't implement polarity), bios setup, and the acpi dsdt.
>>     
> We currently specify INT10 as active high in the dsl so I added an
> override entry to the madt an now the OS knows about it and things work
> as expected - we this be o.k. until all the IRQ setup gets a revamp:
>
> >From 7950892de473d42ce57e0727594190b4bfafab14 Mon Sep 17 00:00:00 2001
> From: Guido Guenther <ag...@si...>
> Date: Fri, 4 Jan 2008 19:26:33 +0100
> Subject: [PATCH] int 10 is currently active high
>
> so add a proper interrupt override entry to the MADT so the OS knows about it.
> Fixes the ACPI powerbutton and should also fix the PM timer.
>   

Since it is the OS that assigns SCI to irq10, we can't be sure it will 
always be there.  So I think all PCI IRQs need such an override.

Also, does this work if you remove the sci hack in piix3_set_irq()

    piix3_dev->config[0x60 + irq_num] &= ~0x80;   // enable bit

?

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

Re: [kvm-devel] acpi sci polarity

[Guido G. <ag...@si...>]

On Sun, Jan 06, 2008 at 11:08:31AM +0200, Avi Kivity wrote:
> Since it is the OS that assigns SCI to irq10, we can't be sure it will 
> always be there.  So I think all PCI IRQs need such an override.
The patch below adds the override to IRQs 5,9,10 & 11. The code is
basically from Xen. Xen fixes the SCI interrupt to 9, we should probably
do the same in the long term. However adding the proper MADT entries
should be fine anyway.

> 
> Also, does this work if you remove the sci hack in piix3_set_irq()
> 
>     piix3_dev->config[0x60 + irq_num] &= ~0x80;   // enable bit
Without that bit it doesn't work anymore.

>From 918338e3f37f91a14d230a9ccf3c3387a2b58617 Mon Sep 17 00:00:00 2001
From: Guido Guenther <ag...@si...>
Date: Fri, 4 Jan 2008 19:26:33 +0100
Subject: [PATCH] add interrupt override entries for IRQs 5,9,10,11 to the MADT

so the OS knows that they're active high, level triggered. This allows
for proper ACPI event reporting such as the (emulated) power button and
should also fix the ACPI timer.

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 7a96ece..e5a9bd5 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -55,6 +55,9 @@ typedef unsigned long long uint64_t;
 #define APIC_ID      0x020
 #define APIC_LVT3    0x370
 
+/* IRQs 5,9,10,11 */
+#define PCI_ISA_IRQ_MASK    0x0e20U
+
 #define APIC_ENABLED 0x0100
 
 #define AP_BOOT_ADDR 0x10000
@@ -1181,6 +1184,14 @@ struct madt_io_apic
 			  * lines start */
 };
 
+struct madt_intsrcovr {
+	APIC_HEADER_DEF
+	uint8_t  bus;
+	uint8_t  source;
+	uint32_t gsi;
+	uint16_t flags;
+};
+
 #include "acpi-dsdt.hex"
 
 static inline uint16_t cpu_to_le16(uint16_t x)
@@ -1271,7 +1282,7 @@ void acpi_bios_init(void)
 
     addr = (addr + 7) & ~7;
     madt_addr = addr;
-    madt_size = sizeof(*madt) + 
+    madt_size = sizeof(*madt) +
         sizeof(struct madt_processor_apic) * smp_cpus +
         sizeof(struct madt_io_apic);
     madt = (void *)(addr);
@@ -1335,6 +1346,7 @@ void acpi_bios_init(void)
     {
         struct madt_processor_apic *apic;
         struct madt_io_apic *io_apic;
+        struct madt_intsrcovr *intsrcovr;
 
         memset(madt, 0, madt_size);
         madt->local_apic_address = cpu_to_le32(0xfee00000);
@@ -1355,6 +1367,22 @@ void acpi_bios_init(void)
         io_apic->address = cpu_to_le32(0xfec00000);
         io_apic->interrupt = cpu_to_le32(0);
 
+        intsrcovr = (struct madt_intsrcovr*)(io_apic + 1);
+        for ( i = 0; i < 16; i++ ) {
+            if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
+                memset(intsrcovr, 0, sizeof(*intsrcovr));
+                intsrcovr->type   = APIC_XRUPT_OVERRIDE;
+                intsrcovr->length = sizeof(*intsrcovr);
+                intsrcovr->source = i;
+                intsrcovr->gsi    = i;
+                intsrcovr->flags  = 0xd; /* active high, level triggered */
+            } else {
+                /* No need for a INT source override structure. */
+                continue;
+            }
+            intsrcovr++;
+            madt_size += sizeof(struct madt_intsrcovr);
+        }
         acpi_build_table_header((struct acpi_table_header *)madt, 
                                 "APIC", madt_size);
     }

[kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

(needs either --no-kvm-irqchip or the previous patch)
 -- Guido

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 314df94..7a96ece 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1318,8 +1318,8 @@ void acpi_bios_init(void)
     fadt->pm_tmr_len = 4;
     fadt->plvl2_lat = cpu_to_le16(0x0fff); // C2 state not supported
     fadt->plvl3_lat = cpu_to_le16(0x0fff); // C3 state not supported
-    /* WBINVD + PROC_C1 + PWR_BUTTON + SLP_BUTTON + FIX_RTC */
-    fadt->flags = cpu_to_le32((1 << 0) | (1 << 2) | (1 << 4) | (1 << 5) | (1 << 6));
+    /* WBINVD + PROC_C1 + SLP_BUTTON + FIX_RTC */
+    fadt->flags = cpu_to_le32((1 << 0) | (1 << 2) | (1 << 5) | (1 << 6));
     acpi_build_table_header((struct acpi_table_header *)fadt, "FACP", 
                             sizeof(*fadt));
 
diff --git a/qemu/hw/acpi.c b/qemu/hw/acpi.c
index a2efd9c..fa78b81 100644
--- a/qemu/hw/acpi.c
+++ b/qemu/hw/acpi.c
@@ -71,6 +71,8 @@ typedef struct PIIX4PMState {
 #define SMBHSTDAT1 0x06
 #define SMBBLKDAT 0x07
 
+PIIX4PMState *pm_state;
+
 static uint32_t get_pmtmr(PIIX4PMState *s)
 {
     uint32_t d;
@@ -475,6 +477,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
     s = (PIIX4PMState *)pci_register_device(bus,
                                          "PM", sizeof(PIIX4PMState),
                                          devfn, NULL, pm_write_config);
+    pm_state = s;
     pci_conf = s->dev.config;
     pci_conf[0x00] = 0x86;
     pci_conf[0x01] = 0x80;
@@ -516,3 +519,13 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
     s->smbus = i2c_init_bus();
     return s->smbus;
 }
+
+#if defined(TARGET_I386)
+void qemu_system_powerdown(void)
+{
+    if(pm_state->pmen & PWRBTN_EN) {
+        pm_state->pmsts |= PWRBTN_EN;
+    	pm_update_sci(pm_state);
+    }
+}
+#endif
diff --git a/qemu/sysemu.h b/qemu/sysemu.h
index c8478ec..5fd7fc2 100644
--- a/qemu/sysemu.h
+++ b/qemu/sysemu.h
@@ -33,7 +33,7 @@ void qemu_system_powerdown_request(void);
 int qemu_shutdown_requested(void);
 int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
-#if !defined(TARGET_SPARC)
+#if !defined(TARGET_SPARC) && !defined(TARGET_I386)
 // Please implement a power failure function to signal the OS
 #define qemu_system_powerdown() do{}while(0)
 #else

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Guido Guenther wrote:
> (needs either --no-kvm-irqchip or the previous patch)
>  -- Guido
>
>   

Applied both, thanks.

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Jun K. <jun...@gm...>]

On 1/7/08, Avi Kivity <av...@qu...> wrote:
> Guido Guenther wrote:
> > (needs either --no-kvm-irqchip or the previous patch)
> >  -- Guido
> >
> >
>
> Applied both, thanks.
>

Sorry for my ignorance, but .... what is the effect of this patch? So
I can shutdown guest VM cleanly, or smt else??

Thanks,
Jun

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

On Tue, Jan 08, 2008 at 01:34:30PM +0900, Jun Koi wrote:
> Sorry for my ignorance, but .... what is the effect of this patch? So
> I can shutdown guest VM cleanly, or smt else??
system_powerdown in the commmand monitor now simulates the pressing of
the acpi power button. This allows you to shutdown the system cleanly.
This is nice for things like libvirt where you can now do a 

virsh shutdown <vm>

and the machine doesn't simply get killed. You need this patch for
libvirt:

diff --git a/src/qemu_driver.c b/src/qemu_driver.c
index f792eba..55adb18 100644
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -1849,6 +1849,27 @@ static int qemudDomainResume(virDomainPtr dom) {
 }
 
 
+static int qemudDomainShutdown(virDomainPtr dom) {
+    struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
+    struct qemud_vm *vm = qemudFindVMByID(driver, dom->id);
+    char* info;
+
+    if (!vm) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
+                         "no domain with matching id %d", dom->id);
+        return -1;
+    }
+
+    if (qemudMonitorCommand(driver, vm, "system_powerdown", &info) < 0) {
+        qemudReportError(dom->conn, dom, NULL, VIR_ERR_OPERATION_FAILED,
+                         "shutdown operation failed");
+        return -1;
+    }
+    return 0;
+
+}
+
+
 static int qemudDomainDestroy(virDomainPtr dom) {
     struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
     struct qemud_vm *vm = qemudFindVMByID(driver, dom->id);
@@ -2855,7 +2876,7 @@ static virDriver qemuDriver = {
     qemudDomainLookupByName, /* domainLookupByName */
     qemudDomainSuspend, /* domainSuspend */
     qemudDomainResume, /* domainResume */
-    qemudDomainDestroy, /* domainShutdown */
+    qemudDomainShutdown, /* domainShutdown */
     NULL, /* domainReboot */
     qemudDomainDestroy, /* domainDestroy */
     qemudDomainGetOSType, /* domainGetOSType */

Cheers,
 -- Guido

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Jan K. <jan...@we...>]

Guido Guenther wrote:
> On Tue, Jan 08, 2008 at 01:34:30PM +0900, Jun Koi wrote:
>> Sorry for my ignorance, but .... what is the effect of this patch? So
>> I can shutdown guest VM cleanly, or smt else??
> system_powerdown in the commmand monitor now simulates the pressing of
> the acpi power button. This allows you to shutdown the system cleanly.
> This is nice for things like libvirt where you can now do a=20
>=20
> virsh shutdown <vm>
>=20
> and the machine doesn't simply get killed. You need this patch for
> libvirt:

What about additionally listening on signals? If you run qemu from the
console, you can then just press ctrl-c to shut the guest down (instead
of killing it that way). The same happens on host shutdown (if the guest
is faster than the host's grace period before SIGKILL...).

Jan

---
 qemu/sysemu.h |    2 +-
 qemu/vl.c     |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

Index: kvm-userspace/qemu/vl.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- kvm-userspace.orig/qemu/vl.c
+++ kvm-userspace/qemu/vl.c
@@ -8501,6 +8501,11 @@ void qemu_get_launch_info(int *argc, cha
     *opt_incoming =3D incoming;
 }
=20
+void qemu_powerdown_sighand(int signal)
+{
+    qemu_system_powerdown_request();
+}
+
 int main(int argc, char **argv)
 {
 #ifdef CONFIG_GDBSTUB
@@ -9475,6 +9480,9 @@ int main(int argc, char **argv)
         }
     }
=20
+    signal(SIGINT, qemu_powerdown_sighand);
+    signal(SIGTERM, qemu_powerdown_sighand);
+
     machine->init(ram_size, vga_ram_size, boot_devices, ds,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_=
model);
=20
Index: kvm-userspace/qemu/sysemu.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- kvm-userspace.orig/qemu/sysemu.h
+++ kvm-userspace/qemu/sysemu.h
@@ -35,7 +35,7 @@ int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
 #if !defined(TARGET_SPARC) && !defined(TARGET_I386)
 // Please implement a power failure function to signal the OS
-#define qemu_system_powerdown() do{}while(0)
+#define qemu_system_powerdown() exit(0)
 #else
 void qemu_system_powerdown(void);
 #endif

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

Hi Jan,
On Sat, Jan 19, 2008 at 04:40:06PM +0100, Jan Kiszka wrote:
> What about additionally listening on signals? If you run qemu from the
> console, you can then just press ctrl-c to shut the guest down (instead
Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
probably kill qemu then, since the machine might have no acpid running -
in that case hitting ctrl-c would have no effect.

> of killing it that way). The same happens on host shutdown (if the guest
> is faster than the host's grace period before SIGKILL...).

> +    signal(SIGINT, qemu_powerdown_sighand);
> +    signal(SIGTERM, qemu_powerdown_sighand);
We shouldn't catch SIGTERM here since libvirt uses it for
domainDestroy() (in contrast to domainShutdown() which uses
system_powerdown).
Cheers,
 -- Guido

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Jan K. <jan...@si...>]

Hi Guido,

[posting via gmane sucks, just re-enabled mail delivery in this account...]

Guido Guenther wrote:
> Hi Jan,
> On Sat, Jan 19, 2008 at 04:40:06PM +0100, Jan Kiszka wrote:
>> What about additionally listening on signals? If you run qemu from the
>> console, you can then just press ctrl-c to shut the guest down (instead
> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
> probably kill qemu then, since the machine might have no acpid running -
> in that case hitting ctrl-c would have no effect.

Good idea.

> 
>> of killing it that way). The same happens on host shutdown (if the guest
>> is faster than the host's grace period before SIGKILL...).
> 
>> +    signal(SIGINT, qemu_powerdown_sighand);
>> +    signal(SIGTERM, qemu_powerdown_sighand);
> We shouldn't catch SIGTERM here since libvirt uses it for
> domainDestroy() (in contrast to domainShutdown() which uses
> system_powerdown).

Something like this? I also included the SDL window this time, and at
least I like it this way now :->

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux


---
 qemu/sdl.c    |    2 +-
 qemu/sysemu.h |    2 +-
 qemu/vl.c     |   21 ++++++++++++++++++++-
 3 files changed, 22 insertions(+), 3 deletions(-)

Index: b/qemu/vl.c
===================================================================
--- a/qemu/vl.c
+++ b/qemu/vl.c
@@ -7653,9 +7653,21 @@ void qemu_system_shutdown_request(void)
         cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
 }
 
+/* more than one requests within 2 s => hard powerdown */
+#define HARD_POWERDOWN_WINDOW	2
+
 void qemu_system_powerdown_request(void)
 {
-    powerdown_requested = 1;
+    static time_t last_request;
+    time_t now, delta;
+
+    now = time(NULL);
+    delta = now-last_request;
+    last_request = now;
+    if (delta < 0 || delta > HARD_POWERDOWN_WINDOW)
+        powerdown_requested = 1;
+    else
+        shutdown_requested = 1;
     if (cpu_single_env)
         cpu_interrupt(cpu_single_env, CPU_INTERRUPT_EXIT);
 }
@@ -8501,6 +8513,11 @@ void qemu_get_launch_info(int *argc, cha
     *opt_incoming = incoming;
 }
 
+void qemu_powerdown_sighand(int signal)
+{
+    qemu_system_powerdown_request();
+}
+
 int main(int argc, char **argv)
 {
 #ifdef CONFIG_GDBSTUB
@@ -9475,6 +9492,8 @@ int main(int argc, char **argv)
         }
     }
 
+    signal(SIGINT, qemu_powerdown_sighand);
+
     machine->init(ram_size, vga_ram_size, boot_devices, ds,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
Index: b/qemu/sysemu.h
===================================================================
--- a/qemu/sysemu.h
+++ b/qemu/sysemu.h
@@ -35,7 +35,7 @@ int qemu_reset_requested(void);
 int qemu_powerdown_requested(void);
 #if !defined(TARGET_SPARC) && !defined(TARGET_I386)
 // Please implement a power failure function to signal the OS
-#define qemu_system_powerdown() do{}while(0)
+#define qemu_system_powerdown() exit(0)
 #else
 void qemu_system_powerdown(void);
 #endif
Index: b/qemu/sdl.c
===================================================================
--- a/qemu/sdl.c
+++ b/qemu/sdl.c
@@ -469,7 +469,7 @@ static void sdl_refresh(DisplayState *ds
             break;
         case SDL_QUIT:
             if (!no_quit) {
-                qemu_system_shutdown_request();
+                qemu_system_powerdown_request();
                 vm_start();	/* In case we're paused */
             }
             break;

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Jan Kiszka wrote:
> Hi Guido,
>
> [posting via gmane sucks, just re-enabled mail delivery in this account...]
>
>   

Much appreciated.

> Guido Guenther wrote:
>   
>> Hi Jan,
>> On Sat, Jan 19, 2008 at 04:40:06PM +0100, Jan Kiszka wrote:
>>     
>>> What about additionally listening on signals? If you run qemu from the
>>> console, you can then just press ctrl-c to shut the guest down (instead
>>>       
>> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
>> probably kill qemu then, since the machine might have no acpid running -
>> in that case hitting ctrl-c would have no effect.
>>     
>
> Good idea.
>   

I'm worried about the 30+ second shutdown latency.  Is there precedent 
for SIGTERM or SIGINT requiring this long to take effect?

Maybe we should suspend the VM instead (using qemu suspend, not guest 
suspend).

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Gerd H. <kr...@re...>]

  Hi,

>>> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
>>> probably kill qemu then, since the machine might have no acpid running -
>>> in that case hitting ctrl-c would have no effect.
>>>     
>> Good idea.
>>   
> 
> I'm worried about the 30+ second shutdown latency.  Is there precedent 
> for SIGTERM or SIGINT requiring this long to take effect?

xenner signals a shutdown request to the guest for the first SIGINT (and
prints a message to the user saying so).  Sending SIGINT twice kills the
guest and cleans up.  I find that very useful, you can shutdown the
guest cleanly with a convenient Ctrl-C and also kill it off quickly by
simply pressing Ctrl-C again.

SIGTERM kills the guest instantly.  Applictions are expected to react
quickly on SIGTERM, there is no way you can wait for a clean guest
shutdown then.  It is used on (host) shutdown for example, where you'll
get a SIGKILL when you don't exit within three seconds.

cheers,
  Gerd

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Jan K. <jan...@si...>]

Gerd Hoffmann wrote:
>   Hi,
> 
>>>> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
>>>> probably kill qemu then, since the machine might have no acpid running -
>>>> in that case hitting ctrl-c would have no effect.
>>>>     
>>> Good idea.
>>>   
>> I'm worried about the 30+ second shutdown latency.  Is there precedent 
>> for SIGTERM or SIGINT requiring this long to take effect?
> 
> xenner signals a shutdown request to the guest for the first SIGINT (and
> prints a message to the user saying so).  Sending SIGINT twice kills the
> guest and cleans up.  I find that very useful, you can shutdown the
> guest cleanly with a convenient Ctrl-C and also kill it off quickly by
> simply pressing Ctrl-C again.

Yeah. After having worked with this feature for a few days (I'm mostly
running qemu/kvm from command line, w/ and w/o SDL GUI), I can only
underline the usefulness of it again.

> 
> SIGTERM kills the guest instantly.  Applictions are expected to react
> quickly on SIGTERM, there is no way you can wait for a clean guest
> shutdown then.  It is used on (host) shutdown for example, where you'll
> get a SIGKILL when you don't exit within three seconds.
> 

Ack, let's stick with SIGINT.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Jan K. <jan...@si...>]

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Guido Guenther wrote:
>>   
>>> Hi Jan,
>>> On Sat, Jan 19, 2008 at 04:40:06PM +0100, Jan Kiszka wrote:
>>>     
>>>> What about additionally listening on signals? If you run qemu from the
>>>> console, you can then just press ctrl-c to shut the guest down (instead
>>>>       
>>> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
>>> probably kill qemu then, since the machine might have no acpid running -
>>> in that case hitting ctrl-c would have no effect.
>>>     
>> Good idea.
>>   
> 
> I'm worried about the 30+ second shutdown latency.  Is there precedent 
> for SIGTERM or SIGINT requiring this long to take effect?

Sorry, can't follow this yet: Are you talking about host system shutdown
that should wait on the guest system that long?

> 
> Maybe we should suspend the VM instead (using qemu suspend, not guest 
> suspend).

You mean on SIGTERM? I think Guido's concern was that this signal is
expected to actually kill the qemu instance. Therefore I dropped this
signal from my second patch.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Jan Kiszka wrote:
> Avi Kivity wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Guido Guenther wrote:
>>>   
>>>       
>>>> Hi Jan,
>>>> On Sat, Jan 19, 2008 at 04:40:06PM +0100, Jan Kiszka wrote:
>>>>     
>>>>         
>>>>> What about additionally listening on signals? If you run qemu from the
>>>>> console, you can then just press ctrl-c to shut the guest down (instead
>>>>>       
>>>>>           
>>>> Catching ctrl-c sounds like a good idea but "ctrl-c, ctrl-c" should
>>>> probably kill qemu then, since the machine might have no acpid running -
>>>> in that case hitting ctrl-c would have no effect.
>>>>     
>>>>         
>>> Good idea.
>>>   
>>>       
>> I'm worried about the 30+ second shutdown latency.  Is there precedent 
>> for SIGTERM or SIGINT requiring this long to take effect?
>>     
>
> Sorry, can't follow this yet: Are you talking about host system shutdown
> that should wait on the guest system that long?
>
>   

Mostly that ctrl-C shouldn't take long, especially without some kind of 
visual indication (say with headless or vnc).

>> Maybe we should suspend the VM instead (using qemu suspend, not guest 
>> suspend).
>>     
>
> You mean on SIGTERM? I think Guido's concern was that this signal is
> expected to actually kill the qemu instance. Therefore I dropped this
> signal from my second patch.
>   

As a fast, non-guest dependent to shutdown, whichever signals we 
eventually hook into it.

I think that for the unmanaged case we need to improve the windowed 
interface instead of hooking signals.  The managed case can handle 
anything we throw at it as long as it's well defined.

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Jun Koi wrote:
> On 1/7/08, Avi Kivity <av...@qu...> wrote:
>   
>> Guido Guenther wrote:
>>     
>>> (needs either --no-kvm-irqchip or the previous patch)
>>>  -- Guido
>>>
>>>
>>>       
>> Applied both, thanks.
>>
>>     
>
> Sorry for my ignorance, but .... what is the effect of this patch? So
> I can shutdown guest VM cleanly, or smt else??
>   

Yes, you can press the VM's power button (system_powerdown in the 
monitor) and it will invoke the OS shutdown sequence.

Only worked on Linux in my testing though.

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

On Tue, Jan 08, 2008 at 10:50:00AM +0200, Avi Kivity wrote:
> Yes, you can press the VM's power button (system_powerdown in the 
> monitor) and it will invoke the OS shutdown sequence.
> 
> Only worked on Linux in my testing though.
It works for me _when_ I pass --no-kvm-irqchip. So it seems Windows
ignores the MADT entries or something.
Cheers,
 -- Guido

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Guido Guenther wrote:
> On Tue, Jan 08, 2008 at 10:50:00AM +0200, Avi Kivity wrote:
>   
>> Yes, you can press the VM's power button (system_powerdown in the 
>> monitor) and it will invoke the OS shutdown sequence.
>>
>> Only worked on Linux in my testing though.
>>     
> It works for me _when_ I pass --no-kvm-irqchip. So it seems Windows
> ignores the MADT entries or something.
>   

-no-kvm-irqchip implies ignoring the polarity, since qemu doesn't 
implement ioapic polarity.

I think we need to go back to active low pci irqs, and to have no-ioapic 
working we need to insert an inverter between the pci irq links and the 
pic.  I base this on the following:

- piix doesn't contain an ioapic, so the actual lines must be active low
- the piix pic elcr is documented as active-high for level-triggered irqs

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

On Sun, Jan 13, 2008 at 11:35:48AM +0200, Avi Kivity wrote:
> Guido Guenther wrote:
>> On Tue, Jan 08, 2008 at 10:50:00AM +0200, Avi Kivity wrote:
>>   
>>> Yes, you can press the VM's power button (system_powerdown in the 
>>> monitor) and it will invoke the OS shutdown sequence.
>>>
>>> Only worked on Linux in my testing though.
>>>     
>> It works for me _when_ I pass --no-kvm-irqchip. So it seems Windows
>> ignores the MADT entries or something.
>>   
>
> -no-kvm-irqchip implies ignoring the polarity, since qemu doesn't implement 
> ioapic polarity.
Yes, sure. That's what I meant to say.
 -- Guido

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Guido G. <ag...@si...>]

On Sun, Jan 13, 2008 at 03:48:04PM +0200, Avi Kivity wrote:
> Okay, this is likely right as I was able to shutdown both Windows and Linux 
> with the following:
Cool.

> - backout the madt interrupt source override changes
Should these stay, but with active low instead of active high?
 -- Guido

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Guido Guenther wrote:
>   
>> - backout the madt interrupt source override changes
>>     
> Should these stay, but with active low instead of active high?
>   

They can go away, since active low is the default.

For reference, I'm attaching the patches I'm using.

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

Re: [kvm-devel] [PATCH] add acpi powerbutton support

[Avi K. <av...@qu...>]

Avi Kivity wrote:
> Guido Guenther wrote:
>> On Tue, Jan 08, 2008 at 10:50:00AM +0200, Avi Kivity wrote:
>>  
>>> Yes, you can press the VM's power button (system_powerdown in the 
>>> monitor) and it will invoke the OS shutdown sequence.
>>>
>>> Only worked on Linux in my testing though.
>>>     
>> It works for me _when_ I pass --no-kvm-irqchip. So it seems Windows
>> ignores the MADT entries or something.
>>   
>
> -no-kvm-irqchip implies ignoring the polarity, since qemu doesn't 
> implement ioapic polarity.
>
> I think we need to go back to active low pci irqs, and to have 
> no-ioapic working we need to insert an inverter between the pci irq 
> links and the pic.  I base this on the following:
>
> - piix doesn't contain an ioapic, so the actual lines must be active low
> - the piix pic elcr is documented as active-high for level-triggered irqs
>

Okay, this is likely right as I was able to shutdown both Windows and 
Linux with the following:

- backout the madt interrupt source override changes
- declare all interrupts as active low
- negate pci irq interrupts in qemu just before forwarding to kvm (so 
they become active low)
- negate pci irq interrupts in the kernel just before forwarding to the 
pic (so that they become active high, but leaving the ioapic interrupts 
active low)

There's quite a bit of work before that can be committed, though.  I 
want to generalize interrupt routing in the kernel, and we need tons of 
backward compatibility in the dsdt so that older hosts can still function.

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