[PATCH] system_powerdown via acpi power button

[PATCH] system_powerdown via acpi power button

[Guido Guenther]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

acpi sci polarity

[Guido Guenther]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: acpi sci polarity

[Avi Kivity]

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.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: acpi sci polarity

[Guido Guenther]

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 <agx-wGvLLbajjwFAfugRpC6u6w@public.gmane.org>
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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: acpi sci polarity

[Avi Kivity]

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 <agx-wGvLLbajjwFAfugRpC6u6w@public.gmane.org>
> 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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: acpi sci polarity

[Guido Guenther]

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 <agx-wGvLLbajjwFAfugRpC6u6w@public.gmane.org>
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);
     }


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[PATCH] add acpi powerbutton support

[Guido Guenther]

(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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Jun Koi]

On 1/7/08, Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> 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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

Jun Koi wrote:
> On 1/7/08, Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> 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


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Alexey Eremenko]

[-- Attachment #1.1: Type: text/plain, Size: 151 bytes --]

Hi,

As far as I know, Qemu/KVM already had ACPI power button; It can be used via Qemu Monitor: "system_shutdown" command.

-Alexey "Technologov"

[-- Attachment #1.2: Type: text/html, Size: 580 bytes --]

[-- Attachment #2: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

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

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

On Mon, Jan 14, 2008 at 04:41:23AM -0800, Alexey Eremenko wrote:
> As far as I know, Qemu/KVM already had ACPI power button; It can be used via Qemu Monitor: "system_shutdown" command.
Before 46b1a7377b55a3b6317b18fff64f1a80de7d3120 system_powerdown was a
"do { } while(0);" on i386 and the FF pwrbtn wasn't enabled in the FADT.
 -- Guido

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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

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


[-- Attachment #2: 0001-kvm-bios-pci-interrupts-are-active-low.patch --]
[-- Type: text/x-patch, Size: 3737 bytes --]

>From 736880c1e550f84778e0cd13ae7dd27988dee902 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Mon, 14 Jan 2008 17:34:11 +0200
Subject: [PATCH] kvm: bios: pci interrupts are active low

Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 bios/acpi-dsdt.dsl |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
index df255ce..92fd126 100755
--- a/bios/acpi-dsdt.dsl
+++ b/bios/acpi-dsdt.dsl
@@ -382,7 +382,7 @@ DefinitionBlock (
                 Name(_HID, EISAID("PNP0C0F"))     // PCI interrupt link
                 Name(_UID, 1)
                 Name(_PRS, ResourceTemplate(){
-                    Interrupt (, Level, ActiveHigh, Shared)
+                    Interrupt (, Level, ActiveLow, Shared)
                         { 5, 9, 10, 11 }
                 })
                 Method (_STA, 0, NotSerialized)
@@ -402,7 +402,7 @@ DefinitionBlock (
                 {
                     Name (PRR0, ResourceTemplate ()
                     {
-                        Interrupt (, Level, ActiveHigh, Shared)
+                        Interrupt (, Level, ActiveLow, Shared)
                             {1}
                     })
                     CreateDWordField (PRR0, 0x05, TMP)
@@ -427,7 +427,7 @@ DefinitionBlock (
                 Name(_HID, EISAID("PNP0C0F"))     // PCI interrupt link
                 Name(_UID, 2)
                 Name(_PRS, ResourceTemplate(){
-                    Interrupt (, Level, ActiveHigh, Shared)
+                    Interrupt (, Level, ActiveLow, Shared)
                         { 5, 9, 10, 11 }
                 })
                 Method (_STA, 0, NotSerialized)
@@ -447,7 +447,7 @@ DefinitionBlock (
                 {
                     Name (PRR0, ResourceTemplate ()
                     {
-                        Interrupt (, Level, ActiveHigh, Shared)
+                        Interrupt (, Level, ActiveLow, Shared)
                             {1}
                     })
                     CreateDWordField (PRR0, 0x05, TMP)
@@ -472,7 +472,7 @@ DefinitionBlock (
                 Name(_HID, EISAID("PNP0C0F"))     // PCI interrupt link
                 Name(_UID, 3)
                 Name(_PRS, ResourceTemplate(){
-                    Interrupt (, Level, ActiveHigh, Shared)
+                    Interrupt (, Level, ActiveLow, Shared)
                         { 5, 9, 10, 11 }
                 })
                 Method (_STA, 0, NotSerialized)
@@ -492,7 +492,7 @@ DefinitionBlock (
                 {
                     Name (PRR0, ResourceTemplate ()
                     {
-                        Interrupt (, Level, ActiveHigh, Shared)
+                        Interrupt (, Level, ActiveLow, Shared)
                             {1}
                     })
                     CreateDWordField (PRR0, 0x05, TMP)
@@ -517,7 +517,7 @@ DefinitionBlock (
                 Name(_HID, EISAID("PNP0C0F"))     // PCI interrupt link
                 Name(_UID, 4)
                 Name(_PRS, ResourceTemplate(){
-                    Interrupt (, Level, ActiveHigh, Shared)
+                    Interrupt (, Level, ActiveLow, Shared)
                         { 5, 9, 10, 11 }
                 })
                 Method (_STA, 0, NotSerialized)
@@ -537,7 +537,7 @@ DefinitionBlock (
                 {
                     Name (PRR0, ResourceTemplate ()
                     {
-                        Interrupt (, Level, ActiveHigh, Shared)
+                        Interrupt (, Level, ActiveLow, Shared)
                             {1}
                     })
                     CreateDWordField (PRR0, 0x05, TMP)
-- 
1.5.3.7


[-- Attachment #3: 0002-kvm-bios-don-t-advertise-the-pci-interrupts-as-act.patch --]
[-- Type: text/x-patch, Size: 976 bytes --]

>From 96ecf8e23154baa8020c58316cc013b8fa28689c Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Mon, 14 Jan 2008 17:47:35 +0200
Subject: [PATCH] kvm: bios: don't advertise the pci interrupts as active high

Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 bios/rombios32.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/bios/rombios32.c b/bios/rombios32.c
index 967c119..ebfbb24 100755
--- a/bios/rombios32.c
+++ b/bios/rombios32.c
@@ -1369,7 +1369,7 @@ void acpi_bios_init(void)
 
         intsrcovr = (struct madt_intsrcovr*)(io_apic + 1);
         for ( i = 0; i < 16; i++ ) {
-            if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
+            if ( 0 && (PCI_ISA_IRQ_MASK & (1U << i) )) {
                 memset(intsrcovr, 0, sizeof(*intsrcovr));
                 intsrcovr->type   = APIC_XRUPT_OVERRIDE;
                 intsrcovr->length = sizeof(*intsrcovr);
-- 
1.5.3.7


[-- Attachment #4: 0003-kvm-qemu-invert-pci-interrupts-so-they-are-active.patch --]
[-- Type: text/x-patch, Size: 823 bytes --]

>From 5a5564a0998f42c5394090167d0dbf1e111dde10 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Mon, 14 Jan 2008 17:48:51 +0200
Subject: [PATCH] kvm: qemu: invert pci interrupts so they are active low

qemu treats them as active high, but they really are active low.

Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 qemu/qemu-kvm.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index dd84686..33db263 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -742,6 +742,8 @@ int kvm_get_phys_ram_page_bitmap(unsigned char *bitmap)
 
 int kvm_set_irq(int irq, int level)
 {
+    if ((1 << irq) & 0xe20)
+	level ^= 1;
     return kvm_set_irq_level(kvm_context, irq, level);
 }
 
-- 
1.5.3.7


[-- Attachment #5: 0004-KVM-Hack-pci-pic-irqs-to-be-inverted-relative-to-io.patch --]
[-- Type: text/x-patch, Size: 1293 bytes --]

>From 1d578b022cfc4973ae3da3b3c639ab41f7a84f28 Mon Sep 17 00:00:00 2001
From: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
Date: Mon, 14 Jan 2008 17:29:38 +0200
Subject: [PATCH] KVM: Hack pci pic irqs to be inverted relative to ioapic irqs

PCI irqs are active low, but the pic wants them active high.

Signed-off-by: Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
---
 arch/x86/kvm/x86.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77d57ff..ffce026 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1434,16 +1434,21 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		break;
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
+		int isa_irq_level;
 
 		r = -EFAULT;
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
 			mutex_lock(&kvm->lock);
-			if (irq_event.irq < 16)
+			if (irq_event.irq < 16) {
+				isa_irq_level = irq_event.level;
+				if ((1 << irq_event.irq) & 0xe20)
+					isa_irq_level ^= 1;
 				kvm_pic_set_irq(pic_irqchip(kvm),
 					irq_event.irq,
-					irq_event.level);
+					isa_irq_level);
+			}
 			kvm_ioapic_set_irq(kvm->arch.vioapic,
 					irq_event.irq,
 					irq_event.level);
-- 
1.5.3.7


[-- Attachment #6: Type: text/plain, Size: 278 bytes --]

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #7: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

Avi Kivity wrote:
>>
>> 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:
>

No, it's wrong.

I found a piix4 acpi spec update that says that the SCI interrupt is 
hardwired to IRQ9 and is active high.  Linux knows about this and 
ignores the pci irq config for this function.  With the following changes

- change acpi sci interrupt to be hardwired to irq9
- remove irq 9 from acpi dsdt (since qemu can't mix isa irqs and pci 
irqs correctly)
- change the madt interrupt source override structure size to 10 (it was 
12, which caused Windows to ignore it)
- advertise the sci interrupt as tied to irq 9 in the fadt (instead of 
consulting the false pci config)

both Windows and Linux are happy.  I was even able to drop the forced 
pci irq enabling hack.

I'll push those changes tomorrow morning.  They are especially important 
with the acpi-based cpu and device hotplug that is brewing.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

On Sat, Jan 19, 2008 at 12:20:56AM +0200, Avi Kivity wrote:
> I found a piix4 acpi spec update that says that the SCI interrupt is 
> hardwired to IRQ9 and is active high.  Linux knows about this and ignores 
> the pci irq config for this function.  With the following changes
Aahh, that's why Xen hardwires the SCI to 9. I actually tried to do the
same last week but got no IRQs delivered after switching to fixed IRQ 9.
I was missing 2ae9165b4181a024060d4b4e5bfb0b21b993a00.
Cheers,
 -- Guido

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 2208 bytes --]

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 
> 
> virsh shutdown <vm>
> 
> 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
===================================================================
--- 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 = incoming;
 }
 
+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)
         }
     }
 
+    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);
 
Index: kvm-userspace/qemu/sysemu.h
===================================================================
--- 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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

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

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] add acpi powerbutton support

[Guido Guenther]

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



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Jan Kiszka]

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;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Jan Kiszka]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Avi Kivity]

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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Gerd Hoffmann]

  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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] add acpi powerbutton support

[Jan Kiszka]

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Jan Kiszka]

Guido Guenther wrote:
> +#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

This code should also care about !pm_state, ie. non-PCI systems.

Jan

---
 qemu/hw/acpi.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: kvm-userspace/qemu/hw/acpi.c
===================================================================
--- kvm-userspace.orig/qemu/hw/acpi.c
+++ kvm-userspace/qemu/hw/acpi.c
@@ -72,7 +72,7 @@ typedef struct PIIX4PMState {
 #define SMBHSTDAT1 0x06
 #define SMBBLKDAT 0x07

-PIIX4PMState *pm_state;
+PIIX4PMState *pm_state = NULL;

 static uint32_t get_pmtmr(PIIX4PMState *s)
 {
@@ -527,6 +527,8 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int
 #if defined(TARGET_I386)
 void qemu_system_powerdown(void)
 {
+    if (!pm_state)
+        exit(0);
     if(pm_state->pmen & PWRBTN_EN) {
         pm_state->pmsts |= PWRBTN_EN;
 	pm_update_sci(pm_state);

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Guido Guenther]

[-- Attachment #1.1: Type: text/plain, Size: 546 bytes --]

On Tue, Jan 22, 2008 at 11:11:00AM +0100, Jan Kiszka wrote:
>  #if defined(TARGET_I386)
>  void qemu_system_powerdown(void)
>  {
> +    if (!pm_state)
> +        exit(0);
>      if(pm_state->pmen & PWRBTN_EN) {
>          pm_state->pmsts |= PWRBTN_EN;
>  	pm_update_sci(pm_state);
This totally defeats the "clean shutdown" purpose of system_powerdown.
Think of it like: 
    system_powerdown: hint the system to shutdown cleanly
    SIGTERM: kill the vm
This is how things are wired up in libvirt at the moment.
Cheers,
 -- Guido

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

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

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] system_powerdown via acpi power button

[Jan Kiszka]

Guido Guenther wrote:
> On Tue, Jan 22, 2008 at 11:11:00AM +0100, Jan Kiszka wrote:
>>  #if defined(TARGET_I386)
>>  void qemu_system_powerdown(void)
>>  {
>> +    if (!pm_state)
>> +        exit(0);
>>      if(pm_state->pmen & PWRBTN_EN) {
>>          pm_state->pmsts |= PWRBTN_EN;
>>  	pm_update_sci(pm_state);
> This totally defeats the "clean shutdown" purpose of system_powerdown.
> Think of it like: 
>     system_powerdown: hint the system to shutdown cleanly
>     SIGTERM: kill the vm
> This is how things are wired up in libvirt at the moment.

So how to signal a "clean shutdown" to the guest if there is no channel
for this?

My idea was to go the PC way: If the OS is not handling ACPI, hitting
power normally switches the PC off - IIRC.

Jan

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

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Guido Guenther]

On Tue, Jan 22, 2008 at 01:12:12PM +0100, Jan Kiszka wrote:
> Guido Guenther wrote:
> > On Tue, Jan 22, 2008 at 11:11:00AM +0100, Jan Kiszka wrote:
> >>  #if defined(TARGET_I386)
> >>  void qemu_system_powerdown(void)
> >>  {
> >> +    if (!pm_state)
> >> +        exit(0);
> >>      if(pm_state->pmen & PWRBTN_EN) {
> >>          pm_state->pmsts |= PWRBTN_EN;
> >>  	pm_update_sci(pm_state);
> > This totally defeats the "clean shutdown" purpose of system_powerdown.
> > Think of it like: 
> >     system_powerdown: hint the system to shutdown cleanly
> >     SIGTERM: kill the vm
> > This is how things are wired up in libvirt at the moment.
> 
> So how to signal a "clean shutdown" to the guest if there is no channel
> for this?
It isn't. It's about what system_powerdown should mean. IMHO it should
be "signal the guest to shut down cleanly". 

If the guest can't do that, it simply can't. You can then SIGTERM the
guest after a timeout. Note that even an acpi system might not be able
to shut down since it hangs on a network unmount or it ignores the ACPI
interrupt via kernel commandline or whatever. You can't ever expect a
system to shutdown cleanly, so you need the SIGTERM failsave anyway.
Have a look at e.g. domainShutdown() vs. domainDestroy() in libvirt.
Cheers,
 -- Guido

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Avi Kivity]

Guido Guenther wrote:
> If the guest can't do that, it simply can't. You can then SIGTERM the
> guest after a timeout. Note that even an acpi system might not be able
> to shut down since it hangs on a network unmount or it ignores the ACPI
> interrupt via kernel commandline or whatever. You can't ever expect a
> system to shutdown cleanly, so you need the SIGTERM failsave anyway.
> Have a look at e.g. domainShutdown() vs. domainDestroy() in libvirt.
>   

Agree, but should try a "quit" monitor command first.  Signals are racy, 
like anything that deals with pids (qemu dies, another process is 
fork()ed with the same pid, libvirt kills it).

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Gerd Hoffmann]

Avi Kivity wrote:
> Agree, but should try a "quit" monitor command first.  Signals are racy, 
> like anything that deals with pids (qemu dies, another process is 
> fork()ed with the same pid, libvirt kills it).

There is no race in that specific case because qemu is started by
libvirtd.  libvirtd can savely kill qemu as long as it hasn't collected
the exit status via waitpid().  While the qemu zombie hangs around the
pid will not be reused.

cheers,
  Gerd

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] system_powerdown via acpi power button

[Avi Kivity]

Gerd Hoffmann wrote:
> Avi Kivity wrote:
>   
>> Agree, but should try a "quit" monitor command first.  Signals are racy, 
>> like anything that deals with pids (qemu dies, another process is 
>> fork()ed with the same pid, libvirt kills it).
>>     
>
> There is no race in that specific case because qemu is started by
> libvirtd.  libvirtd can savely kill qemu as long as it hasn't collected
> the exit status via waitpid().  While the qemu zombie hangs around the
> pid will not be reused.
>
>   

Ah good.  It does mean that the we can't recover from an untimely 
libvirtd death, but I'd consider it a reasonable tradeoff.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/