Thread: Re: [kvm-devel] [PATCH] add acpi powerbutton support (Page 2)

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

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"

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



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.




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



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



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 |=3D PWRBTN_EN;
>  	pm_update_sci(pm_state);
This totally defeats the "clean shutdown" purpose of system_powerdown.
Think of it like:=20
    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

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



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



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.




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



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.



