Xen devel - May 2013 - Early ACPI events prevent subsequent ACPI functionality on xen 4.3 + HVM domU

This is problem 3 of 3 problems we are having with live migration and/or ACPI on
Xen-4.3 and Xen-4.2.

Any help would be appreciated.

Detailed description of problem:

We are using Xen-4.3-rc1 with dom0 running Ubuntu Precise and 3.5.0-23-generic
kernel, and domU running Ubuntu Precise (12.04) cloud images running
3.2.0-39-virtual. We are using the xl.conf below on qemu-upstream-dm and HVM and
two identical sending and receiving machines (hardware and software)

If a dom-U HVM server is started with ACPI enabled, and an ACPI event (such as
shutdown or reboot) is sent to domU before the BIOS and/or operating system in
the dom-U has initialised ACPI, then all subsequent ACPI events are ignored.

This is replicable every time.

How to replicate:
1. Take a machines running xen-4.3-rc1 version of Xen on Ubuntu Precise with
3.5.0-23-generic kernel.
2. Use the xl.conf below as a configuration file.
3. Create a VM using Ubuntu Precise and 3.5.0-23 generic.
4. Start the VM
5. Immediately (before boot is completed) run ''xl shutdown'' or
''xl reboot''. As ACPI is not initialised, this will have no
effect.
6. Wait until domU has completed booting.
7. Run ''xl shutdown'' or ''xl reboot'' again.

Expected results:

At step 7, domU will shutdown or reboot as appropriate

Actual results:

domU does not shutdown or reboot and it is necessary to ''xl
des'' the domU to stop it.

Notes:

On xen-4.2, a similar thing happens.

--xl.conf--

builder=''hvm''
memory = 512
name = "416-vm"
vcpus=1
disk = [ ''tap:qcow2:/root/diana.qcow2,xvda,w'' ]
vif = [''mac=00:16:3f:1d:6a:c0, bridge=defaultbr'']
sdl=0
opengl=1
vnc=1
vnclisten="0.0.0.0"
vncdisplay=0
vncunused=0
vncpasswd=''p''
stdvga=0
serial=''pty''

CCing the guys who know about qemu.

On Tue, 2013-05-14 at 14:13 +0100, Diana Crisan wrote:
> This is problem 3 of 3 problems we are having with live migration
> and/or ACPI on Xen-4.3 and Xen-4.2.
> 
> Any help would be appreciated.
> 
> Detailed description of problem:
> 
> We are using Xen-4.3-rc1 with dom0 running Ubuntu Precise and
> 3.5.0-23-generic kernel, and domU running Ubuntu Precise (12.04) cloud
> images running 3.2.0-39-virtual. We are using the xl.conf below on
> qemu-upstream-dm and HVM and two identical sending and receiving
> machines (hardware and software)
> 
> If a dom-U HVM server is started with ACPI enabled, and an ACPI event
> (such as shutdown or reboot) is sent to domU before the BIOS and/or
> operating system in the dom-U has initialised ACPI, then all
> subsequent ACPI events are ignored.
I suppose something must be getting latched and since no one was around
at the time to clear it no further events are possible. Not sure if this
is a qemu or a guest issue though.
> 
> This is replicable every time.
> 
> How to replicate:
> 1. Take a machines running xen-4.3-rc1 version of Xen on Ubuntu Precise
with 3.5.0-23-generic kernel.
> 2. Use the xl.conf below as a configuration file.
> 3. Create a VM using Ubuntu Precise and 3.5.0-23 generic.
> 4. Start the VM
> 5. Immediately (before boot is completed) run ''xl
shutdown'' or ''xl reboot''. As ACPI is not initialised,
this will have no effect.
> 6. Wait until domU has completed booting.
> 7. Run ''xl shutdown'' or ''xl reboot''
again.
> 
> Expected results:
> 
> At step 7, domU will shutdown or reboot as appropriate
> 
> Actual results:
> 
> domU does not shutdown or reboot and it is necessary to ''xl
des'' the domU to stop it.
> 
> Notes:
> 
> On xen-4.2, a similar thing happens.
> 
> --xl.conf--
> 
> builder=''hvm''
> memory = 512
> name = "416-vm"
> vcpus=1
> disk = [ ''tap:qcow2:/root/diana.qcow2,xvda,w'' ]
> vif = [''mac=00:16:3f:1d:6a:c0, bridge=defaultbr'']
> sdl=0
> opengl=1
> vnc=1
> vnclisten="0.0.0.0"
> vncdisplay=0
> vncunused=0
> vncpasswd=''p''
> stdvga=0
> serial=''pty''
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian,

--On 17 May 2013 18:35:51 +0100 Ian Campbell <ian.campbell@citrix.com> 
wrote:
>> We are using Xen-4.3-rc1 with dom0 running Ubuntu Precise and
>> 3.5.0-23-generic kernel, and domU running Ubuntu Precise (12.04) cloud
>> images running 3.2.0-39-virtual. We are using the xl.conf below on
>> qemu-upstream-dm and HVM and two identical sending and receiving
>> machines (hardware and software)
>>
>> If a dom-U HVM server is started with ACPI enabled, and an ACPI event
>> (such as shutdown or reboot) is sent to domU before the BIOS and/or
>> operating system in the dom-U has initialised ACPI, then all
>> subsequent ACPI events are ignored.
>
> I suppose something must be getting latched and since no one was around
> at the time to clear it no further events are possible. Not sure if this
> is a qemu or a guest issue though.
I can confirm it does not happen with KVM with the same guest, and it
happens with multiple Linux guests on Xen. Our test suite triggers it
every time.

It''s a pain because if someone starts a server by accident and
immediately
goes to shut it down, they have to do an ungraceful destroy rather than
a clean shutdown.

-- 
Alex Bligh

On Sat, May 18, 2013 at 10:55 AM, Alex Bligh <alex@alex.org.uk>
wrote:
> Ian,
>
>
> --On 17 May 2013 18:35:51 +0100 Ian Campbell
<ian.campbell@citrix.com>
> wrote:
>
>>> We are using Xen-4.3-rc1 with dom0 running Ubuntu Precise and
>>> 3.5.0-23-generic kernel, and domU running Ubuntu Precise (12.04)
cloud
>>> images running 3.2.0-39-virtual. We are using the xl.conf below on
>>> qemu-upstream-dm and HVM and two identical sending and receiving
>>> machines (hardware and software)
>>>
>>> If a dom-U HVM server is started with ACPI enabled, and an ACPI
event
>>> (such as shutdown or reboot) is sent to domU before the BIOS and/or
>>> operating system in the dom-U has initialised ACPI, then all
>>> subsequent ACPI events are ignored.
>>
>>
>> I suppose something must be getting latched and since no one was around
>> at the time to clear it no further events are possible. Not sure if
this
>> is a qemu or a guest issue though.
>
>
> I can confirm it does not happen with KVM with the same guest, and it
> happens with multiple Linux guests on Xen. Our test suite triggers it
> every time.
>
> It''s a pain because if someone starts a server by accident and
immediately
> goes to shut it down, they have to do an ungraceful destroy rather than
> a clean shutdown.
So this appears to be an xl toolstack thing.  I managed to reproduce
your results using "xl shutdown -F [domain]"; but if you then do
"xl
trigger [domian] power", the domain shuts down as normal.

That at least should give you a work-around so you don''t have to do
"xl destroy".

I''ll take a look and see if I can figure out what''s going
on...

 -George

On Tue, May 21, 2013 at 2:39 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Sat, May 18, 2013 at 10:55 AM, Alex Bligh <alex@alex.org.uk>
wrote:
>> Ian,
>>
>>
>> --On 17 May 2013 18:35:51 +0100 Ian Campbell
<ian.campbell@citrix.com>
>> wrote:
>>
>>>> We are using Xen-4.3-rc1 with dom0 running Ubuntu Precise and
>>>> 3.5.0-23-generic kernel, and domU running Ubuntu Precise
(12.04) cloud
>>>> images running 3.2.0-39-virtual. We are using the xl.conf below
on
>>>> qemu-upstream-dm and HVM and two identical sending and
receiving
>>>> machines (hardware and software)
>>>>
>>>> If a dom-U HVM server is started with ACPI enabled, and an ACPI
event
>>>> (such as shutdown or reboot) is sent to domU before the BIOS
and/or
>>>> operating system in the dom-U has initialised ACPI, then all
>>>> subsequent ACPI events are ignored.
>>>
>>>
>>> I suppose something must be getting latched and since no one was
around
>>> at the time to clear it no further events are possible. Not sure if
this
>>> is a qemu or a guest issue though.
>>
>>
>> I can confirm it does not happen with KVM with the same guest, and it
>> happens with multiple Linux guests on Xen. Our test suite triggers it
>> every time.
>>
>> It''s a pain because if someone starts a server by accident and
immediately
>> goes to shut it down, they have to do an ungraceful destroy rather than
>> a clean shutdown.
>
> So this appears to be an xl toolstack thing.  I managed to reproduce
> your results using "xl shutdown -F [domain]"; but if you then do
"xl
> trigger [domian] power", the domain shuts down as normal.
>
> That at least should give you a work-around so you don''t have to
do
> "xl destroy".
>
> I''ll take a look and see if I can figure out what''s going
on...
Actually, this has nothing to do with ACPI at all.

"xl shutdown" will *always* attempt to issue a PV shutdown command
first.  If it thinks there is no PV drivers present, *and* the
''-F''
option is added, then it will also attempt to send an acpi power
event.

So a plain "xl shutdown" maps to:
if( pv_drivers_available(domain) )
  write_xenstore_node();

The PV drivers in domU should have watches on the xenstore node
written to cause the shutdown.  But apparently there''s a window
between the time pv_drivers_available() returns succeeds and the time
this watch is set up; and if the node is written between these two
times, then the notification gets dropped.

xl seems to use HVM_PARAM_CALLBACK_IRQ being non-zero as an indicator.

Dave and Paul, does this race exist in XenServer with the Citrix PV
drivers?  How does xe detect whether pv drivers are available for an
HVM domain? And will the Citrix PV drivers check for the existence of
shutdown commands before they start a watch?

 -George

On Tue, 2013-05-21 at 15:16 +0100, George Dunlap wrote:
> And will the Citrix PV drivers check for the existence of
> shutdown commands before they start a watch? 
Watches fire immediately when you register them, to allow this race to
be easily avoided without really having to think about it.

Ian.

On 05/21/2013 03:20 PM, Ian Campbell wrote:
> On Tue, 2013-05-21 at 15:16 +0100, George Dunlap wrote:
>> And will the Citrix PV drivers check for the existence of
>> shutdown commands before they start a watch?
>
> Watches fire immediately when you register them, to allow this race to
> be easily avoided without really having to think about it.
Well then that watch mechanism is broken, at least for values written 
before some point in time:

# xenstore-ls -f /local/domain/10
[...]
/local/domain/10/control/shutdown = "poweroff"
[...]

But the domain doesn''t shut down.

Konrad, sorry to keep sending stuff your way, but I''m not sure who else
to bring in for this one.  The original thread is here:

http://www.gossamer-threads.com/lists/xen/devel/282467

But the problem actually has nothing to do with ACPI -- it''s the PV 
shutdown command that is getting lost after the driver registers 
HVM_PARAM_CALLBACK_IRQ but before something else comes up that starts 
watching ../control/shutdown.

  -George

On Tue, 2013-05-21 at 15:34 +0100, George Dunlap wrote:
> On 05/21/2013 03:20 PM, Ian Campbell wrote:
> > On Tue, 2013-05-21 at 15:16 +0100, George Dunlap wrote:
> >> And will the Citrix PV drivers check for the existence of
> >> shutdown commands before they start a watch?
> >
> > Watches fire immediately when you register them, to allow this race to
> > be easily avoided without really having to think about it.
> 
> Well then that watch mechanism is broken, 
The firing happens in xenstored, and that certainly works everywhere
else, or all sorts of things would be broken.

That''s not to say there isn''t a problem with e.g. the kernels
watch
event handling causing the watch which is fired to not get as far as the
appropriate handler.
> at least for values written 
> before some point in time:
> 
> # xenstore-ls -f /local/domain/10
> [...]
> /local/domain/10/control/shutdown = "poweroff"
> [...]
> 
> But the domain doesn''t shut down.
> 
> Konrad, sorry to keep sending stuff your way, but I''m not sure who
else
> to bring in for this one.  The original thread is here:
> 
> http://www.gossamer-threads.com/lists/xen/devel/282467
> 
> But the problem actually has nothing to do with ACPI -- it''s the
PV
> shutdown command that is getting lost after the driver registers 
> HVM_PARAM_CALLBACK_IRQ but before something else comes up that starts 
> watching ../control/shutdown.
> 
>   -George

George,

--On 21 May 2013 14:39:55 +0100 George Dunlap
<George.Dunlap@eu.citrix.com>
wrote:
> So this appears to be an xl toolstack thing.  I managed to reproduce
> your results using "xl shutdown -F [domain]"; but if you then do
"xl
> trigger [domian] power", the domain shuts down as normal.
OK. But doesn''t the power thing yank the power rather than send a
clean shutdown?

We are using libxl here (admittedly having looked carefully at
the xl code for guidance) and get the same problem.

-- 
Alex Bligh

--On 21 May 2013 15:34:34 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
> But the problem actually has nothing to do with ACPI -- it''s the
PV
> shutdown command that is getting lost after the driver registers
> HVM_PARAM_CALLBACK_IRQ but before something else comes up that starts
> watching ../control/shutdown.
I''m puzzled as to why you say that as the problem does not appear
without ACPI in the xl.conf file.

-- 
Alex Bligh

On 05/21/2013 04:16 PM, Alex Bligh wrote:
> George,
>
> --On 21 May 2013 14:39:55 +0100 George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>
>> So this appears to be an xl toolstack thing.  I managed to reproduce
>> your results using "xl shutdown -F [domain]"; but if you then
do "xl
>> trigger [domian] power", the domain shuts down as normal.
>
> OK. But doesn''t the power thing yank the power rather than send a
> clean shutdown?
No -- if you push the button just once on most modern hardware it will 
send an ACPI "poweroff" event that the OS handles gracefully. 
That''s
what gets sent when you do "xl trigger [domain] power".  If the OS 
ignores it (either on real hardware or virtual hardware) nothing 
happens.  On real hardware you have to then hold down the button for 5 
seconds for a hard-shutdown, with xl you have to do "xl destroy".
> We are using libxl here (admittedly having looked carefully at
> the xl code for guidance) and get the same problem.
The relevant xl code is here:

     rc=libxl_domain_shutdown(ctx, domid);
     if (rc == ERROR_NOPARAVIRT) {
         if (fallback_trigger) {
             fprintf(stderr, "PV control interface not available:"
                     " sending ACPI power button event.\n");
             rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
         } else {
             fprintf(stderr, "PV control interface not available:"
                     " external graceful shutdown not possible.\n");
             fprintf(stderr, "Use \"-F\" to fallback to ACPI
power
event.\n");
         }
     }

It looks like libxl_domain_shutdown() will only use the PV shutdown 
method.  If that method is not available, then it will call 
libxl_send_trigger().

What does your code do?  Does it call libxl_domain_shutdown(), or 
libxl_send_trigger() (or one then the other, like xl)?

  -George

On 05/21/2013 04:17 PM, Alex Bligh wrote:
>
>
> --On 21 May 2013 15:34:34 +0100 George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
>
>> But the problem actually has nothing to do with ACPI -- it''s
the PV
>> shutdown command that is getting lost after the driver registers
>> HVM_PARAM_CALLBACK_IRQ but before something else comes up that starts
>> watching ../control/shutdown.
>
> I''m puzzled as to why you say that as the problem does not appear
> without ACPI in the xl.conf file.
I can still get the same effect -- the only difference for me seems to 
be that the "window" in which I can send a shutdown with no effect it
is
shorter.  Maybe ACPI causes the extra few seconds at boot which makes it 
more likely you''re going to hit it?

  -George

On 05/21/2013 04:36 PM, George Dunlap wrote:
> On 05/21/2013 04:17 PM, Alex Bligh wrote:
>>
>>
>> --On 21 May 2013 15:34:34 +0100 George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>>
>>> But the problem actually has nothing to do with ACPI --
it''s the PV
>>> shutdown command that is getting lost after the driver registers
>>> HVM_PARAM_CALLBACK_IRQ but before something else comes up that
starts
>>> watching ../control/shutdown.
>>
>> I''m puzzled as to why you say that as the problem does not
appear
>> without ACPI in the xl.conf file.
>
> I can still get the same effect -- the only difference for me seems to
> be that the "window" in which I can send a shutdown with no
effect it is
> shorter.  Maybe ACPI causes the extra few seconds at boot which makes it
> more likely you''re going to hit it?
If you can''t trigger it manually, try something like this:

xl create dom.cfg ; while ! xl shutdown dom ; do sleep 1 ; done

Before PARAM_CALLBACK_IRQ is set, "xl shutdown dom" will fail, so this
will keep trying every 1 second until it succeeds.  Since this is most 
likely within 1 second of PARAM_CALLBACK_IRQ being set, it should be 
within the window where the watch isn''t set yet (or whatever it is that
makes domU responsive to the PV shutdown commands).

It works reliably for me with acpi=0.

  -George

George,

--On 21 May 2013 16:23:00 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
> On 05/21/2013 04:16 PM, Alex Bligh wrote:
>> George,
>>
>> --On 21 May 2013 14:39:55 +0100 George Dunlap
>> <George.Dunlap@eu.citrix.com> wrote:
>>
>>> So this appears to be an xl toolstack thing.  I managed to
reproduce
>>> your results using "xl shutdown -F [domain]"; but if you
then do "xl
>>> trigger [domian] power", the domain shuts down as normal.
>>
>> OK. But doesn''t the power thing yank the power rather than
send a
>> clean shutdown?
>
> No -- if you push the button just once on most modern hardware it will
> send an ACPI "poweroff" event that the OS handles gracefully. 
That''s
> what gets sent when you do "xl trigger [domain] power".  If the
OS
> ignores it (either on real hardware or virtual hardware) nothing happens.
> On real hardware you have to then hold down the button for 5 seconds for
> a hard-shutdown, with xl you have to do "xl destroy".
OK, great. I am guessing the reason why ''xl shutdown''
doesn''t do
that is to cope with (a) non-HVM domains, and (b) old fashioned
HVM domains with PV support but not ACPI support. Correct?
>> We are using libxl here (admittedly having looked carefully at
>> the xl code for guidance) and get the same problem.
>
> The relevant xl code is here:
>
>      rc=libxl_domain_shutdown(ctx, domid);
>      if (rc == ERROR_NOPARAVIRT) {
>          if (fallback_trigger) {
>              fprintf(stderr, "PV control interface not
available:"
>                      " sending ACPI power button event.\n");
>              rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER, 0);
>          } else {
>              fprintf(stderr, "PV control interface not
available:"
>                      " external graceful shutdown not
possible.\n");
>              fprintf(stderr, "Use \"-F\" to fallback to ACPI
power
> event.\n");
>          }
>      }
>
> It looks like libxl_domain_shutdown() will only use the PV shutdown
> method.  If that method is not available, then it will call
> libxl_send_trigger().
OK. Well if that''s the case, why is it not working? i.e. how
is using ''xl trigger power'' a workaround? As our testing was
on xl (as well as libxl). Perhaps libxl_domain_shutdown
is returning an error other than ERROR_NOPARAVIRT or no error
at all?

The major problem for us isn''t that the initial domain
shutdown doesn''t work (that''s just a bit sad), it''s
that it blocks
subsequent shutdowns from working.
> What does your code do?  Does it call libxl_domain_shutdown(), or
> libxl_send_trigger() (or one then the other, like xl)?
When Diana reported the bug, she was using xl (and thus the
code you are using above).

Our libxl code currently just calls libxl_domain_shutdown.
Apparently we didn''t look too closely at xl :-)

Does xl trigger reset do the same thing (i.e. a graceful shutdown)?
If not we''d also (I think) have to recode reboots to do a power trigger
then a restart.

-- 
Alex Bligh

On 05/21/2013 04:59 PM, Alex Bligh wrote:
> George,
>
> --On 21 May 2013 16:23:00 +0100 George Dunlap
> <george.dunlap@eu.citrix.com> wrote:
>
>> On 05/21/2013 04:16 PM, Alex Bligh wrote:
>>> George,
>>>
>>> --On 21 May 2013 14:39:55 +0100 George Dunlap
>>> <George.Dunlap@eu.citrix.com> wrote:
>>>
>>>> So this appears to be an xl toolstack thing.  I managed to
reproduce
>>>> your results using "xl shutdown -F [domain]"; but if
you then do "xl
>>>> trigger [domian] power", the domain shuts down as normal.
>>>
>>> OK. But doesn''t the power thing yank the power rather than
send a
>>> clean shutdown?
>>
>> No -- if you push the button just once on most modern hardware it will
>> send an ACPI "poweroff" event that the OS handles gracefully.
That''s
>> what gets sent when you do "xl trigger [domain] power".  If
the OS
>> ignores it (either on real hardware or virtual hardware) nothing
happens.
>> On real hardware you have to then hold down the button for 5 seconds
for
>> a hard-shutdown, with xl you have to do "xl destroy".
>
> OK, great. I am guessing the reason why ''xl shutdown''
doesn''t do
> that is to cope with (a) non-HVM domains, and (b) old fashioned
> HVM domains with PV support but not ACPI support. Correct?
>
>>> We are using libxl here (admittedly having looked carefully at
>>> the xl code for guidance) and get the same problem.
>>
>> The relevant xl code is here:
>>
>>      rc=libxl_domain_shutdown(ctx, domid);
>>      if (rc == ERROR_NOPARAVIRT) {
>>          if (fallback_trigger) {
>>              fprintf(stderr, "PV control interface not
available:"
>>                      " sending ACPI power button event.\n");
>>              rc = libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_POWER,
0);
>>          } else {
>>              fprintf(stderr, "PV control interface not
available:"
>>                      " external graceful shutdown not
possible.\n");
>>              fprintf(stderr, "Use \"-F\" to fallback to
ACPI power
>> event.\n");
>>          }
>>      }
>>
>> It looks like libxl_domain_shutdown() will only use the PV shutdown
>> method.  If that method is not available, then it will call
>> libxl_send_trigger().
>
> OK. Well if that''s the case, why is it not working? i.e. how
> is using ''xl trigger power'' a workaround? As our testing
was
> on xl (as well as libxl). Perhaps libxl_domain_shutdown
> is returning an error other than ERROR_NOPARAVIRT or no error
> at all?
xl/libxl is working as designed. xl tried libxl_domain_shutdown() first. 
  libxl saw that there were PV drivers available, so it sent the PV 
shutdown signal over xenstore and returned success.  libxl and xl have 
no way of knowing that the signal was never received, so it never falls 
back to ACPI.

"xl trigger power" is a work-around because when you detect that a
guest
is stuck, you should be able to issue an ACPI shutdown and get a clean 
shutdown, even when the PV shutdown path is stuck.

(In fact, as a short-term solution, you might consider just replacing 
"xl shutdown $d" with "xl trigger $d power" in your control
framework.)
> Does xl trigger reset do the same thing (i.e. a graceful shutdown)?
> If not we''d also (I think) have to recode reboots to do a power
trigger
> then a restart.
Hmm, this is what I get:

# xl trigger h0 reset
libxl: error: libxl.c:4639:libxl_send_trigger: Send trigger
''reset''
failed: Function not implemented

YMMV...

  -George

George,

--On 21 May 2013 16:51:29 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
> If you can''t trigger it manually, try something like this:
>
> xl create dom.cfg ; while ! xl shutdown dom ; do sleep 1 ; done
Diana (who is actually doing the testing) says I may be wrong about it
working with ACPI. She also says that ''xl trigger power'' also
do not work
reliably even when the machine is fully booted :-/ She''s going to file
a
proper bug report.

-- 
Alex Bligh

George,
> xl/libxl is working as designed. xl tried libxl_domain_shutdown() first.
> libxl saw that there were PV drivers available, so it sent the PV
> shutdown signal over xenstore and returned success.  libxl and xl have no
> way of knowing that the signal was never received, so it never falls back
> to ACPI.
>
> "xl trigger power" is a work-around because when you detect that
a guest
> is stuck, you should be able to issue an ACPI shutdown and get a clean
> shutdown, even when the PV shutdown path is stuck.
>
> (In fact, as a short-term solution, you might consider just replacing
"xl
> shutdown $d" with "xl trigger $d power" in your control
framework.)
Yup. I think this is the best option, save for the fact that ''xl
trigger
power'' itself appears unreliable. We apparently need to send it
multiple
times. Diana will send more details.
>> Does xl trigger reset do the same thing (i.e. a graceful shutdown)?
>> If not we''d also (I think) have to recode reboots to do a
power trigger
>> then a restart.
>
> Hmm, this is what I get:
>
># xl trigger h0 reset
> libxl: error: libxl.c:4639:libxl_send_trigger: Send trigger
''reset''
> failed: Function not implemented
>
> YMMV...
Indeed. So presumably ''xl trigger power'' (+/- repeats to get
it
to work), wait until the domain is dead, then power it back up
again.

-- 
Alex Bligh

On Tue, May 21, 2013 at 04:51:29PM +0100, George Dunlap
wrote:
> On 05/21/2013 04:36 PM, George Dunlap wrote:
> >On 05/21/2013 04:17 PM, Alex Bligh wrote:
> >>
> >>
> >>--On 21 May 2013 15:34:34 +0100 George Dunlap
> >><george.dunlap@eu.citrix.com> wrote:
> >>
> >>>But the problem actually has nothing to do with ACPI --
it''s the PV
> >>>shutdown command that is getting lost after the driver
registers
> >>>HVM_PARAM_CALLBACK_IRQ but before something else comes up that
starts
> >>>watching ../control/shutdown.
> >>
> >>I''m puzzled as to why you say that as the problem does not
appear
> >>without ACPI in the xl.conf file.
> >
> >I can still get the same effect -- the only difference for me seems to
> >be that the "window" in which I can send a shutdown with no
effect it is
> >shorter.  Maybe ACPI causes the extra few seconds at boot which makes
it
> >more likely you''re going to hit it?
> 
> If you can''t trigger it manually, try something like this:
> 
> xl create dom.cfg ; while ! xl shutdown dom ; do sleep 1 ; done
> 
> Before PARAM_CALLBACK_IRQ is set, "xl shutdown dom" will fail, so
> this will keep trying every 1 second until it succeeds.  Since this
> is most likely within 1 second of PARAM_CALLBACK_IRQ being set, it
> should be within the window where the watch isn''t set yet (or
> whatever it is that makes domU responsive to the PV shutdown
> commands).
> 
> It works reliably for me with acpi=0.
And with ''acpi=1'' ?

From the Linux side, there are three paths that can cause a shutdown.
Internally the user can invoke poweroff, which will end up calling the
shutdown procecedure.

PV path - which works on both PVHVM and PV. This is triggered by
watch firring on control/shutdown. Thought interestingly if you wrote
the initial value of "poweroff" there, the kernel would not read it
unless
a trigger followed. A change can be done in there to read the value
at bootup and see if "poweroff" is present.

And the emulated path. I think this is ACPI S5, but I would have to
dig a bit in the QEMU to see if that is how it does it. This is the
same path that Windows or any HVM guests would shutdown.

For the emulated path I am not entirely sure how the ACPI path works 
when a guest hasn''t initialized its ACPI machinery. I would think the
ACPI SCI would be triggered, but it might not be since the ACPI AML
has no way of running (as the OS hasn''t even started reading it). In
which case the emulated path ought to use a fallback, whatever that is.
But I am not sure if there is a fallback except "yanking the power" -
which
I think is what normal machines do if you hold the power off button for
more than 3 seconds.

George,

On 21/05/13 17:09, George Dunlap wrote:
> On 05/21/2013 04:59 PM, Alex Bligh wrote:
>> George,
>>
>> --On 21 May 2013 16:23:00 +0100 George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>>
>>> On 05/21/2013 04:16 PM, Alex Bligh wrote:
>>>> George,
>>>>
>>>> --On 21 May 2013 14:39:55 +0100 George Dunlap
>>>> <George.Dunlap@eu.citrix.com> wrote:
>>>>
>>>>> So this appears to be an xl toolstack thing.  I managed to
reproduce
>>>>> your results using "xl shutdown -F [domain]"; but
if you then do "xl
>>>>> trigger [domian] power", the domain shuts down as
normal.
>>>>
>>>> OK. But doesn''t the power thing yank the power rather
than send a
>>>> clean shutdown?
>>>
>>> No -- if you push the button just once on most modern hardware it
will
>>> send an ACPI "poweroff" event that the OS handles
gracefully. That''s
>>> what gets sent when you do "xl trigger [domain] power". 
If the OS
>>> ignores it (either on real hardware or virtual hardware) nothing 
>>> happens.
>>> On real hardware you have to then hold down the button for 5
seconds
>>> for
>>> a hard-shutdown, with xl you have to do "xl destroy".
>>
>> OK, great. I am guessing the reason why ''xl shutdown''
doesn''t do
>> that is to cope with (a) non-HVM domains, and (b) old fashioned
>> HVM domains with PV support but not ACPI support. Correct?
>>
>>>> We are using libxl here (admittedly having looked carefully at
>>>> the xl code for guidance) and get the same problem.
>>>
>>> The relevant xl code is here:
>>>
>>>      rc=libxl_domain_shutdown(ctx, domid);
>>>      if (rc == ERROR_NOPARAVIRT) {
>>>          if (fallback_trigger) {
>>>              fprintf(stderr, "PV control interface not
available:"
>>>                      " sending ACPI power button
event.\n");
>>>              rc = libxl_send_trigger(ctx, domid, 
>>> LIBXL_TRIGGER_POWER, 0);
>>>          } else {
>>>              fprintf(stderr, "PV control interface not
available:"
>>>                      " external graceful shutdown not
possible.\n");
>>>              fprintf(stderr, "Use \"-F\" to fallback
to ACPI power
>>> event.\n");
>>>          }
>>>      }
>>>
>>> It looks like libxl_domain_shutdown() will only use the PV shutdown
>>> method.  If that method is not available, then it will call
>>> libxl_send_trigger().
>>
>> OK. Well if that''s the case, why is it not working? i.e. how
>> is using ''xl trigger power'' a workaround? As our
testing was
>> on xl (as well as libxl). Perhaps libxl_domain_shutdown
>> is returning an error other than ERROR_NOPARAVIRT or no error
>> at all?
>
> xl/libxl is working as designed. xl tried libxl_domain_shutdown() 
> first.  libxl saw that there were PV drivers available, so it sent the 
> PV shutdown signal over xenstore and returned success.  libxl and xl 
> have no way of knowing that the signal was never received, so it never 
> falls back to ACPI.
>
> "xl trigger power" is a work-around because when you detect that
a
> guest is stuck, you should be able to issue an ACPI shutdown and get a 
> clean shutdown, even when the PV shutdown path is stuck.
>
> (In fact, as a short-term solution, you might consider just replacing 
> "xl shutdown $d" with "xl trigger $d power" in your
control framework.)
>
I am having problems replicating the short-term solution reliably on our 
end, even with xl/libxl. It seems that issuing an "xl trigger dom
power"
does not make the domain shutdown, even after a 10 minute wait. 
Subsequent ones tend to do the same and after issuing quite a few I 
managed to get the domU to power off.

This happens even if I do not issue the shutdown early in the boot 
process, but rather issue directly an xl trigger dom power.
>> Does xl trigger reset do the same thing (i.e. a graceful shutdown)?
>> If not we''d also (I think) have to recode reboots to do a
power trigger
>> then a restart.
>
> Hmm, this is what I get:
>
> # xl trigger h0 reset
> libxl: error: libxl.c:4639:libxl_send_trigger: Send trigger
''reset''
> failed: Function not implemented
>
> YMMV...
>
>  -George
--
Diana Crisan

Hi,
> On Tue, 2013-05-21 at 15:34 +0100, George Dunlap wrote:
> > On 05/21/2013 03:20 PM, Ian Campbell wrote:
> > > On Tue, 2013-05-21 at 15:16 +0100, George Dunlap wrote:
> > >> And will the Citrix PV drivers check for the existence of
shutdown
> > >> commands before they start a watch?
> > >
> > > Watches fire immediately when you register them, to allow this
race
> > > to be easily avoided without really having to think about it.
> >
> > Well then that watch mechanism is broken,
> 
> The firing happens in xenstored, and that certainly works everywhere else,
> or all sorts of things would be broken.
IIRC the watch event should appear in the xenstore (access?) log. I agree that
it''s very unlikely that xenstore is broken.
 
> That''s not to say there isn''t a problem with e.g. the
kernels watch event
> handling causing the watch which is fired to not get as far as the
appropriate
> handler.
For a long time our test cases have had a ''sleep 10'' after a
''xe vm-start'' and before a ''xe vm-shutdown''.
IIRC when I last looked into this (years ago) it was claimed that the PV kernel
was trying to signal init to change runlevel, but it was too soon and the
event/signal/whatever  was dropped. Recently to speed up the tests
we''ve started switching to a custom miniOS kernel (based on Mirage)
which doesn''t have this problem. However it would be great to finally
fix this race...

Cheers,
Dave

Tuesday, May 21, 2013, 6:09:33 PM, you wrote:
> On 05/21/2013 04:59 PM, Alex Bligh wrote:
>> George,
>>
>> --On 21 May 2013 16:23:00 +0100 George Dunlap
>> <george.dunlap@eu.citrix.com> wrote:
>>
>>> On 05/21/2013 04:16 PM, Alex Bligh wrote:
>>>> George,
>>>>
>>>> --On 21 May 2013 14:39:55 +0100 George Dunlap
>>>> <George.Dunlap@eu.citrix.com> wrote:
>>>>
>>>>> So this appears to be an xl toolstack thing.  I managed to
reproduce
>>>>> your results using "xl shutdown -F [domain]"; but
if you then do "xl
>>>>> trigger [domian] power", the domain shuts down as
normal.
>>>>
>>>> OK. But doesn''t the power thing yank the power rather
than send a
>>>> clean shutdown?
>>>
>>> No -- if you push the button just once on most modern hardware it
will
>>> send an ACPI "poweroff" event that the OS handles
gracefully.  That''s
>>> what gets sent when you do "xl trigger [domain] power". 
If the OS
>>> ignores it (either on real hardware or virtual hardware) nothing
happens.
>>> On real hardware you have to then hold down the button for 5
seconds for
>>> a hard-shutdown, with xl you have to do "xl destroy".
>>
>> OK, great. I am guessing the reason why ''xl shutdown''
doesn''t do
>> that is to cope with (a) non-HVM domains, and (b) old fashioned
>> HVM domains with PV support but not ACPI support. Correct?
>>
>>>> We are using libxl here (admittedly having looked carefully at
>>>> the xl code for guidance) and get the same problem.
>>>
>>> The relevant xl code is here:
>>>
>>>      rc=libxl_domain_shutdown(ctx, domid);
>>>      if (rc == ERROR_NOPARAVIRT) {
>>>          if (fallback_trigger) {
>>>              fprintf(stderr, "PV control interface not
available:"
>>>                      " sending ACPI power button
event.\n");
>>>              rc = libxl_send_trigger(ctx, domid,
LIBXL_TRIGGER_POWER, 0);
>>>          } else {
>>>              fprintf(stderr, "PV control interface not
available:"
>>>                      " external graceful shutdown not
possible.\n");
>>>              fprintf(stderr, "Use \"-F\" to fallback
to ACPI power
>>> event.\n");
>>>          }
>>>      }
>>>
>>> It looks like libxl_domain_shutdown() will only use the PV shutdown
>>> method.  If that method is not available, then it will call
>>> libxl_send_trigger().
>>
>> OK. Well if that''s the case, why is it not working? i.e. how
>> is using ''xl trigger power'' a workaround? As our
testing was
>> on xl (as well as libxl). Perhaps libxl_domain_shutdown
>> is returning an error other than ERROR_NOPARAVIRT or no error
>> at all?
> xl/libxl is working as designed. xl tried libxl_domain_shutdown() first. 
>   libxl saw that there were PV drivers available, so it sent the PV 
> shutdown signal over xenstore and returned success.  libxl and xl have 
> no way of knowing that the signal was never received, so it never falls 
> back to ACPI.
At a sidenote ...

This reminds me that there still is the issue that a HVM domain without PV
drivers doesn''t shutdown properly with the standard init.d scripts.
It was discussed last year with IanC and IanJ, to make the fallback to acpi
automatical (at present it isn''t it just prints a warning)
IanJ was in favor, IanC wasn''t convinced that it wouldn''t hurt
in some cases. At some point the discussion resulted in a stalemate.
(thread is here:
http://lists.xen.org/archives/html/xen-devel/2012-10/msg01973.html )

There are a few options:
- always invoking acpi method if pv method fails in libxl
- add -F to the shutdown line in /etc/(sysconfig|default)/xendomains, so
automatically invoking the shutdown is only done from the shutdown script.
- adding a option to the domains cfg (f.e. shutdown="shutdown_acpi"
instead of shutdown="shutdown")

This issue also causes the "guest-stop fail   never pass" results in
the auto test system.

--
Sander

> "xl trigger power" is a work-around because when you detect that
a guest
> is stuck, you should be able to issue an ACPI shutdown and get a clean 
> shutdown, even when the PV shutdown path is stuck.
> (In fact, as a short-term solution, you might consider just replacing 
> "xl shutdown $d" with "xl trigger $d power" in your
control framework.)
>> Does xl trigger reset do the same thing (i.e. a graceful shutdown)?
>> If not we''d also (I think) have to recode reboots to do a
power trigger
>> then a restart.
> Hmm, this is what I get:
> # xl trigger h0 reset
> libxl: error: libxl.c:4639:libxl_send_trigger: Send trigger
''reset''
> failed: Function not implemented
> YMMV...
>   -George

Konrad,

--On 21 May 2013 12:45:31 -0400 Konrad Rzeszutek Wilk 
<konrad.wilk@oracle.com> wrote:
> For the emulated path I am not entirely sure how the ACPI path works
> when a guest hasn''t initialized its ACPI machinery. I would think
the
> ACPI SCI would be triggered, but it might not be since the ACPI AML
> has no way of running (as the OS hasn''t even started reading it).
In
> which case the emulated path ought to use a fallback, whatever that is.
> But I am not sure if there is a fallback except "yanking the
power" -
> which I think is what normal machines do if you hold the power off button
> for more than 3 seconds.
The problem isn''t that a shutdown doesn''t work if there are no
drivers
for it. The problem is that if you issue a shutdown when there are
no drivers, then later issue a shutdown when there are drivers, it
still doesn''t work.

Somewhat to my surprise we are using the PV/PVHVM route not the ACPI
route (libxl_domain_shutdown / xl shutdown) so the subject line would
appear to be inaccurate.

The second problem is that ACPI shutdown appears not to work reliably
either (whenever issued).

-- 
Alex Bligh

On Tue, May 21, 2013 at 06:48:16PM +0100, Alex Bligh
wrote:
> Konrad,
> 
> --On 21 May 2013 12:45:31 -0400 Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> 
> >For the emulated path I am not entirely sure how the ACPI path works
> >when a guest hasn''t initialized its ACPI machinery. I would
think the
> >ACPI SCI would be triggered, but it might not be since the ACPI AML
> >has no way of running (as the OS hasn''t even started reading
it). In
> >which case the emulated path ought to use a fallback, whatever that is.
> >But I am not sure if there is a fallback except "yanking the
power" -
> >which I think is what normal machines do if you hold the power off
button
> >for more than 3 seconds.
> 
> The problem isn''t that a shutdown doesn''t work if there
are no drivers
> for it. The problem is that if you issue a shutdown when there are
> no drivers, then later issue a shutdown when there are drivers, it
> still doesn''t work.
OK, but we should be able to fix this in Linux by doing something along
these lines (not compile tested):


diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 412b96c..34f967f 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -194,7 +194,6 @@ static void do_reboot(void)
 	shutting_down = SHUTDOWN_POWEROFF; /* ? */
 	ctrl_alt_del();
 }
-
 static void shutdown_handler(struct xenbus_watch *watch,
 			     const char **vec, unsigned int len)
 {
@@ -252,6 +251,10 @@ static void shutdown_handler(struct xenbus_watch *watch,
 	kfree(str);
 }
 
+static void check_shutdown_handler(void)
+{
+	 shutdown_handler(NULL, NULL, 0);
+}
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handler(struct xenbus_watch *watch, const char **vec,
 			  unsigned int len)
@@ -310,7 +313,7 @@ static int setup_shutdown_watcher(void)
 		return err;
 	}
 #endif
-
+	check_shutdown_handler();
 	return 0;
 }
 
> 
> Somewhat to my surprise we are using the PV/PVHVM route not the ACPI
> route (libxl_domain_shutdown / xl shutdown) so the subject line would
> appear to be inaccurate.
> 
> The second problem is that ACPI shutdown appears not to work reliably
> either (whenever issued).
Right and that one would require some more investigation as I think it 
works as it would work on baremetal. Meaning if you press the
''shutdown''
button and only BIOS is running it will ignore you. This would
be similar to the ''xl trigger'' call that hopefully invokes the
ACPI signal.

You have to hold it for 5 seconds or so for the BIOS to do anything
about it.

Konrad,

--On 21 May 2013 15:33:44 -0400 Konrad Rzeszutek Wilk 
<konrad.wilk@oracle.com> wrote:
> OK, but we should be able to fix this in Linux by doing something along
> these lines (not compile tested):
Yep. Not much use on guests with current OS''s though :-(
>> Somewhat to my surprise we are using the PV/PVHVM route not the ACPI
>> route (libxl_domain_shutdown / xl shutdown) so the subject line would
>> appear to be inaccurate.
>>
>> The second problem is that ACPI shutdown appears not to work reliably
>> either (whenever issued).
>
> Right and that one would require some more investigation as I think it
> works as it would work on baremetal.
Diana thinks it does not - i.e. when you have a fully booted running Ubuntu
OS, does not reliably cause shutdown (as per Diana''s testing), whether
or not you send an a trigger during early boot. Pressing the hardware
button does one would presume.
> Meaning if you press the ''shutdown''
> button and only BIOS is running it will ignore you. This would
> be similar to the ''xl trigger'' call that hopefully
invokes the ACPI
> signal.
>
> You have to hold it for 5 seconds or so for the BIOS to do anything
> about it.
I''m fine with ACPI shutdown not working before ACPI''s been
initialised,
in BIOS or whatever, so long as it works it has been booted!

-- 
Alex Bligh

On Tue, 2013-05-21 at 17:51 +0100, Dave Scott wrote:
> For a long time our test cases have had a ''sleep 10''
after a ''xe
> vm-start'' and before a ''xe vm-shutdown''. IIRC
when I last looked into
> this (years ago) it was claimed that the PV kernel was trying to
> signal init to change runlevel, but it was too soon and the
> event/signal/whatever  was dropped. Recently to speed up the tests
> we''ve started switching to a custom miniOS kernel (based on
Mirage)
> which doesn''t have this problem. However it would be great to
finally
> fix this race...
Oh yes, this is true -- there is an issue with the guest internal (so ~native)
shutdown stuff not working if you signal it too early. i.e. you
are confusing /sbin/init and not anything Xen specific.

I expect that the way to determine this would be to check if the
xenstore key was "" or "something". If it is blank then the
guest has
acknowledged the request and the fault is /sbin/init otherwise it is a
Xen specific issue, probably internal to the kernel though IMHO.

Ian.

On Tue, May 21, 2013 at 6:48 PM, Alex Bligh <alex@alex.org.uk>
wrote:
> The problem isn''t that a shutdown doesn''t work if there
are no drivers
> for it. The problem is that if you issue a shutdown when there are
> no drivers, then later issue a shutdown when there are drivers, it
> still doesn''t work.
Just to be clear, the issue is if you issue a shutdown when the
drivers are partially initialized, but before the whole system has
come up.  If you issue a shutdown command in BIOS or GRUB, for
example, the toolstack returns an error and doesn''t cause the
"wedge".

(From the other thread of the discussion, it''s not 100% clear whether
the problem is that the kernel driver isn''t getting the shutdown
signal, or whether it has sent the signal but it''s too early for the
rest of the kernel to heed it.)
> The second problem is that ACPI shutdown appears not to work reliably
> either (whenever issued).
I''ll have another look today, but it seemed fairly reliable to me.
Just to double-check -- you are issuing the ACPI commands after the VM
has come all the way up, right?  Or are you issuing them early during
boot, like you were for the xl shutdown commands?

The other thing is that I''m using Wheezy, not Ubuntu, which may cause
a difference.

 -George

> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 412b96c..34f967f 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -194,7 +194,6 @@ static void do_reboot(void)
>  	shutting_down = SHUTDOWN_POWEROFF; /* ? */
>  	ctrl_alt_del();
>  }
> -
>  static void shutdown_handler(struct xenbus_watch *watch,
>  			     const char **vec, unsigned int len)
>  {
> @@ -252,6 +251,10 @@ static void shutdown_handler(struct xenbus_watch
*watch,
>  	kfree(str);
>  }
>  
> +static void check_shutdown_handler(void)
> +{
> +	 shutdown_handler(NULL, NULL, 0);
> +}
>  #ifdef CONFIG_MAGIC_SYSRQ
>  static void sysrq_handler(struct xenbus_watch *watch, const char **vec,
>  			  unsigned int len)
> @@ -310,7 +313,7 @@ static int setup_shutdown_watcher(void)
>  		return err;
>  	}
>  #endif
> -
> +	check_shutdown_handler();
If this is necessary then there is a bug in the kernel''s watch
handling,
for which this is just a workaround, since the call to
register_xenbus_watch should have triggered an initial watch.

--On 22 May 2013 10:21:44 +0100 George Dunlap
<George.Dunlap@eu.citrix.com>
wrote:
>> The second problem is that ACPI shutdown appears not to work reliably
>> either (whenever issued).
>
> I''ll have another look today, but it seemed fairly reliable to me.
> Just to double-check -- you are issuing the ACPI commands after the VM
> has come all the way up, right?  Or are you issuing them early during
> boot, like you were for the xl shutdown commands?
I believe Diana has tried both. Diana?
> The other thing is that I''m using Wheezy, not Ubuntu, which may
cause
> a difference.
Possibly. I guess Diana could try that too :-)

-- 
Alex Bligh

>--On 22 May 2013 10:21:44 +0100 George Dunlap
<George.Dunlap@eu.citrix.com>
>wrote:
>>> The second problem is that ACPI shutdown appears not to work
reliably
>>> either (whenever issued).
>>
>> I''ll have another look today, but it seemed fairly reliable to
me.
>> Just to double-check -- you are issuing the ACPI commands after the VM
>> has come all the way up, right?  Or are you issuing them early during
>> boot, like you were for the xl shutdown commands?
>
>I believe Diana has tried both. Diana?
I have tested both cases with Ubuntu.
Sending the trigger is reliable if you wait for boot to fully complete. 
However, if I issue it during boot it does not get executed. Any subsequent
triggers do not get executed as well until one is sent when the vm has fully
booted.

>
>> The other thing is that I''m using Wheezy, not Ubuntu, which
may cause
>> a difference.
>
>Possibly. I guess Diana could try that too :-)
>-- 
>Alex Bligh
-- 
Diana Crisan

On 22/05/13 11:45, Diana Crisan wrote:
>> --On 22 May 2013 10:21:44 +0100 George Dunlap
<George.Dunlap@eu.citrix.com>
>> wrote:
>>>> The second problem is that ACPI shutdown appears not to work
reliably
>>>> either (whenever issued).
>>> I''ll have another look today, but it seemed fairly
reliable to me.
>>> Just to double-check -- you are issuing the ACPI commands after the
VM
>>> has come all the way up, right?  Or are you issuing them early
during
>>> boot, like you were for the xl shutdown commands?
>> I believe Diana has tried both. Diana?
> I have tested both cases with Ubuntu.
> Sending the trigger is reliable if you wait for boot to fully complete.
> However, if I issue it during boot it does not get executed. Any subsequent
triggers do not get executed as well until one is sent when the vm has fully
booted.
Right -- so what I hear you saying is, "ACPI commands issued before the 
OS is paying attention are ignored."  I think that''s expected
behavior.

I can see that "Shut this vm down as soon as possible" is a useful
thing
to have. The problem at the moment guest OSes can ignore signals sent 
too early, it doesn''t really seem within the scope of libxl to work 
around that.

You could hold off issuing shutdown commands until the guest responds to 
a ping; here is a rune I use for that in my test harness:

ping -c 1 -i 5 -q -w ${timeout} ${host}

This will ping ${host} every 5 seconds until it receives 1 ping back, 
failing with an error after ${timeout} seconds.

Alternately, you could find another xenstore key that indicates that the 
guest is ready to receive a shutdown command; or you could add a line to 
/etc/rc.local to write a xenstore key once the system is done booting.

  -George

George,

--On 22 May 2013 11:55:03 +0100 George Dunlap
<george.dunlap@eu.citrix.com>
wrote:
>> I have tested both cases with Ubuntu.
>> Sending the trigger is reliable if you wait for boot to fully complete.
>> However, if I issue it during boot it does not get executed. Any
>> subsequent triggers do not get executed as well until one is sent when
>> the vm has fully booted.
>
> Right -- so what I hear you saying is, "ACPI commands issued before
the
> OS is paying attention are ignored."  I think that''s expected
behavior.
>
> I can see that "Shut this vm down as soon as possible" is a
useful thing
> to have. The problem at the moment guest OSes can ignore signals sent too
> early, it doesn''t really seem within the scope of libxl to work
around
> that.
I (now) think xl trigger power is working as well as could be expected.
xl shutdown has a problem (in that an early use of this
breaks later use), but we can avoid that by using xl trigger power
(or rather the libxl equivalent). So we have a workaround for this one.

FWIW our workaround actually will be send xl trigger power every
second until the machine disappears, as there is in general no IP
connectivity between hypervisor and guest in our environment. (Just
in case anyone is reading the archive and wants a hint).

So that leaves our one (hurray!) outstanding issue with xen 4.2 (and
4.3) Diana''s first problem - the stuck clock after migration.

-- 
Alex Bligh

On 22/05/13 12:16, Alex Bligh wrote:
> George,
>
> --On 22 May 2013 11:55:03 +0100 George Dunlap 
> <george.dunlap@eu.citrix.com> wrote:
>
>>> I have tested both cases with Ubuntu.
>>> Sending the trigger is reliable if you wait for boot to fully
complete.
>>> However, if I issue it during boot it does not get executed. Any
>>> subsequent triggers do not get executed as well until one is sent
when
>>> the vm has fully booted.
>>
>> Right -- so what I hear you saying is, "ACPI commands issued
before the
>> OS is paying attention are ignored."  I think that''s
expected behavior.
>>
>> I can see that "Shut this vm down as soon as possible" is a
useful thing
>> to have. The problem at the moment guest OSes can ignore signals sent 
>> too
>> early, it doesn''t really seem within the scope of libxl to
work around
>> that.
>
> I (now) think xl trigger power is working as well as could be expected.
> xl shutdown has a problem (in that an early use of this
> breaks later use), but we can avoid that by using xl trigger power
> (or rather the libxl equivalent). So we have a workaround for this one.
OK -- this will still be on our list of bugs to track, but as it''s 
probably a linux issue, it may take some time to filter back into 
distributions.
> FWIW our workaround actually will be send xl trigger power every
> second until the machine disappears, as there is in general no IP
> connectivity between hypervisor and guest in our environment. (Just
> in case anyone is reading the archive and wants a hint).
Ah right -- that makes sense.  Poking it until it shuts off it a bit 
blunt, but should be effective. :-)

  -George

On Wed, May 22, 2013 at 12:50:50PM +0100, George Dunlap
wrote:
> On 22/05/13 12:16, Alex Bligh wrote:
> >George,
> >
> >--On 22 May 2013 11:55:03 +0100 George Dunlap
> ><george.dunlap@eu.citrix.com> wrote:
> >
> >>>I have tested both cases with Ubuntu.
> >>>Sending the trigger is reliable if you wait for boot to fully
complete.
> >>>However, if I issue it during boot it does not get executed.
Any
> >>>subsequent triggers do not get executed as well until one is
sent when
> >>>the vm has fully booted.
> >>
> >>Right -- so what I hear you saying is, "ACPI commands issued
before the
> >>OS is paying attention are ignored."  I think that''s
expected behavior.
> >>
> >>I can see that "Shut this vm down as soon as possible" is
a useful thing
> >>to have. The problem at the moment guest OSes can ignore signals
> >>sent too
> >>early, it doesn''t really seem within the scope of libxl to
work around
> >>that.
> >
> >I (now) think xl trigger power is working as well as could be expected.
> >xl shutdown has a problem (in that an early use of this
> >breaks later use), but we can avoid that by using xl trigger power
> >(or rather the libxl equivalent). So we have a workaround for this one.
> 
> OK -- this will still be on our list of bugs to track, but as it''s
> probably a linux issue, it may take some time to filter back into
> distributions.
There is an bug somewhere. I did this:

#xl create /pv.cfg && xl pause latest && xl shutdown latest
#xenstore-ls | grep poweroff
    shutdown = "poweroff"
#xl unpause latest
#xl console latest
...

and the guest continues on as if the change never happend and won''t
shutdown.

On 21/05/13 18:31, Sander Eikelenboom wrote:
> Tuesday, May 21, 2013, 6:09:33 PM, you wrote:
>
>> On 05/21/2013 04:59 PM, Alex Bligh wrote:
>>> George,
>>>
>>> --On 21 May 2013 16:23:00 +0100 George Dunlap
>>> <george.dunlap@eu.citrix.com> wrote:
>>>
>>>> On 05/21/2013 04:16 PM, Alex Bligh wrote:
>>>>> George,
>>>>>
>>>>> --On 21 May 2013 14:39:55 +0100 George Dunlap
>>>>> <George.Dunlap@eu.citrix.com> wrote:
>>>>>
>>>>>> So this appears to be an xl toolstack thing.  I managed
to reproduce
>>>>>> your results using "xl shutdown -F [domain]";
but if you then do "xl
>>>>>> trigger [domian] power", the domain shuts down as
normal.
>>>>> OK. But doesn''t the power thing yank the power
rather than send a
>>>>> clean shutdown?
>>>> No -- if you push the button just once on most modern hardware
it will
>>>> send an ACPI "poweroff" event that the OS handles
gracefully.  That''s
>>>> what gets sent when you do "xl trigger [domain]
power".  If the OS
>>>> ignores it (either on real hardware or virtual hardware)
nothing happens.
>>>> On real hardware you have to then hold down the button for 5
seconds for
>>>> a hard-shutdown, with xl you have to do "xl destroy".
>>> OK, great. I am guessing the reason why ''xl
shutdown'' doesn''t do
>>> that is to cope with (a) non-HVM domains, and (b) old fashioned
>>> HVM domains with PV support but not ACPI support. Correct?
>>>
>>>>> We are using libxl here (admittedly having looked carefully
at
>>>>> the xl code for guidance) and get the same problem.
>>>> The relevant xl code is here:
>>>>
>>>>       rc=libxl_domain_shutdown(ctx, domid);
>>>>       if (rc == ERROR_NOPARAVIRT) {
>>>>           if (fallback_trigger) {
>>>>               fprintf(stderr, "PV control interface not
available:"
>>>>                       " sending ACPI power button
event.\n");
>>>>               rc = libxl_send_trigger(ctx, domid,
LIBXL_TRIGGER_POWER, 0);
>>>>           } else {
>>>>               fprintf(stderr, "PV control interface not
available:"
>>>>                       " external graceful shutdown not
possible.\n");
>>>>               fprintf(stderr, "Use \"-F\" to
fallback to ACPI power
>>>> event.\n");
>>>>           }
>>>>       }
>>>>
>>>> It looks like libxl_domain_shutdown() will only use the PV
shutdown
>>>> method.  If that method is not available, then it will call
>>>> libxl_send_trigger().
>>> OK. Well if that''s the case, why is it not working? i.e.
how
>>> is using ''xl trigger power'' a workaround? As our
testing was
>>> on xl (as well as libxl). Perhaps libxl_domain_shutdown
>>> is returning an error other than ERROR_NOPARAVIRT or no error
>>> at all?
>> xl/libxl is working as designed. xl tried libxl_domain_shutdown()
first.
>>    libxl saw that there were PV drivers available, so it sent the PV
>> shutdown signal over xenstore and returned success.  libxl and xl have
>> no way of knowing that the signal was never received, so it never falls
>> back to ACPI.
> At a sidenote ...
>
> This reminds me that there still is the issue that a HVM domain without PV
drivers doesn''t shutdown properly with the standard init.d scripts.
BTW, if you want this to get attention, you should probably either reply 
to that thread, or start a new thread.  Starting a new topic on an 
unrelated thread is generally frowned upon. :-)

  -George

On Wed, May 22, 2013 at 10:43:01AM -0400, Konrad Rzeszutek Wilk
wrote:
> On Wed, May 22, 2013 at 12:50:50PM +0100, George Dunlap wrote:
> > On 22/05/13 12:16, Alex Bligh wrote:
> > >George,
> > >
> > >--On 22 May 2013 11:55:03 +0100 George Dunlap
> > ><george.dunlap@eu.citrix.com> wrote:
> > >
> > >>>I have tested both cases with Ubuntu.
> > >>>Sending the trigger is reliable if you wait for boot to
fully complete.
> > >>>However, if I issue it during boot it does not get
executed. Any
> > >>>subsequent triggers do not get executed as well until one
is sent when
> > >>>the vm has fully booted.
> > >>
> > >>Right -- so what I hear you saying is, "ACPI commands
issued before the
> > >>OS is paying attention are ignored."  I think
that''s expected behavior.
> > >>
> > >>I can see that "Shut this vm down as soon as
possible" is a useful thing
> > >>to have. The problem at the moment guest OSes can ignore
signals
> > >>sent too
> > >>early, it doesn''t really seem within the scope of
libxl to work around
> > >>that.
> > >
> > >I (now) think xl trigger power is working as well as could be
expected.
> > >xl shutdown has a problem (in that an early use of this
> > >breaks later use), but we can avoid that by using xl trigger power
> > >(or rather the libxl equivalent). So we have a workaround for this
one.
> > 
> > OK -- this will still be on our list of bugs to track, but as
it''s
> > probably a linux issue, it may take some time to filter back into
> > distributions.
> 
> There is an bug somewhere. I did this:
> 
> #xl create /pv.cfg && xl pause latest && xl shutdown latest
> #xenstore-ls | grep poweroff
>     shutdown = "poweroff"
> #xl unpause latest
> #xl console latest
> ...
> 
> and the guest continues on as if the change never happend and
won''t
> shutdown.
This patch should fix it.
From f53c1f691f726a9d72ad11be56f84389c3f3d7b0 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Wed, 6 Nov 2013 10:57:56 -0500
Subject: [PATCH] xen/manage: Poweroff forcefully if user-space is not yet up.

The user can launch the guest in this sequence:

xl create -p /vm.cfg	[launch, but pause it]
xl shutdown latest	[sets control/shutdown=poweroff]
xl unpause latest
xl console latest	[and see that the guest has completlty
ignored the shutdown request]

In reality the guest hasn''t ignored it. It registers a watch
and gets a notification that there is value. It then calls
the shutdown_handler which ends up calling orderly_shutdown.

Unfortunately that is so early in the bootup that there
are no user-space. Which means that the orderly_shutdown fails.
But since the force flag was set to false it continues on without
reporting.

We check if the system is still in the booting stage and if so
enable the force option (which will shutdown in early bootup
process). If in normal running case we don''t force it.

Fixes-Bug: http://bugs.xenproject.org/xen/bug/6
Reported-by:  Alex Bligh <alex@alex.org.uk>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 624e8dc..fe1c0a6 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -185,7 +185,7 @@ struct shutdown_handler {
 static void do_poweroff(void)
 {
 	shutting_down = SHUTDOWN_POWEROFF;
-	orderly_poweroff(false);
+	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
 }
 
 static void do_reboot(void)
-- 
1.8.3.1

On Wed, 2013-11-06 at 11:05 -0500, Konrad Rzeszutek Wilk
wrote:
> From f53c1f691f726a9d72ad11be56f84389c3f3d7b0 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Wed, 6 Nov 2013 10:57:56 -0500
> Subject: [PATCH] xen/manage: Poweroff forcefully if user-space is not yet
up.
> 
> The user can launch the guest in this sequence:
> 
> xl create -p /vm.cfg	[launch, but pause it]
> xl shutdown latest	[sets control/shutdown=poweroff]
> xl unpause latest
> xl console latest	[and see that the guest has completlty
"completely"
> ignored the shutdown request]
> 
> In reality the guest hasn''t ignored it. It registers a watch
> and gets a notification that there is value. It then calls
> the shutdown_handler which ends up calling orderly_shutdown.
> 
> Unfortunately that is so early in the bootup that there
> are no user-space. Which means that the orderly_shutdown fails.
> But since the force flag was set to false it continues on without
> reporting.
> 
> We check if the system is still in the booting stage and if so
> enable the force option (which will shutdown in early bootup
> process). If in normal running case we don''t force it.
> 
> Fixes-Bug: http://bugs.xenproject.org/xen/bug/6
> Reported-by:  Alex Bligh <alex@alex.org.uk>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/xen/manage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 624e8dc..fe1c0a6 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -185,7 +185,7 @@ struct shutdown_handler {
>  static void do_poweroff(void)
>  {
>  	shutting_down = SHUTDOWN_POWEROFF;
> -	orderly_poweroff(false);
> +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
Does this DTRT for systems in SYSTEM_{HALTED,POWEROFF}? I suppose under
those circumstances forcing is the desired action, insn''t it

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

>>> On 06.11.13 at 17:05, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -185,7 +185,7 @@ struct shutdown_handler {
>  static void do_poweroff(void)
>  {
>  	shutting_down = SHUTDOWN_POWEROFF;
> -	orderly_poweroff(false);
> +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
What''s the ?: operator good for here?

Jan

On Wed, Nov 06, 2013 at 04:14:20PM +0000, Ian Campbell
wrote:
> On Wed, 2013-11-06 at 11:05 -0500, Konrad Rzeszutek Wilk wrote:
> > From f53c1f691f726a9d72ad11be56f84389c3f3d7b0 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Wed, 6 Nov 2013 10:57:56 -0500
> > Subject: [PATCH] xen/manage: Poweroff forcefully if user-space is not
yet up.
> > 
> > The user can launch the guest in this sequence:
> > 
> > xl create -p /vm.cfg	[launch, but pause it]
> > xl shutdown latest	[sets control/shutdown=poweroff]
> > xl unpause latest
> > xl console latest	[and see that the guest has completlty
> 
> "completely"
> 
> > ignored the shutdown request]
> > 
> > In reality the guest hasn''t ignored it. It registers a watch
> > and gets a notification that there is value. It then calls
> > the shutdown_handler which ends up calling orderly_shutdown.
> > 
> > Unfortunately that is so early in the bootup that there
> > are no user-space. Which means that the orderly_shutdown fails.
> > But since the force flag was set to false it continues on without
> > reporting.
> > 
> > We check if the system is still in the booting stage and if so
> > enable the force option (which will shutdown in early bootup
> > process). If in normal running case we don''t force it.
> > 
> > Fixes-Bug: http://bugs.xenproject.org/xen/bug/6
> > Reported-by:  Alex Bligh <alex@alex.org.uk>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/xen/manage.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index 624e8dc..fe1c0a6 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -185,7 +185,7 @@ struct shutdown_handler {
> >  static void do_poweroff(void)
> >  {
> >  	shutting_down = SHUTDOWN_POWEROFF;
> > -	orderly_poweroff(false);
> > +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
> 
> Does this DTRT for systems in SYSTEM_{HALTED,POWEROFF}? I suppose under
> those circumstances forcing is the desired action, insn''t it
Yes. And there is also a guard (shutting_down) that gets set on
drivers/xen/manage.c so that the ''do_poweroff'' will only get
called
once. Which would guard against us powering off and then
receiving another ''xl shutdown'' when the the system_state is
in
HALTED or POWEROFF.

But I hadn''t tested the case where the user does
''poweroff''
and at the same time the system admin does ''xl shutdown''.

Depending on the race, the state will be SYSTEM_RUNNING or
SYSTEM_POWER_OFF. If SYSTEM_RUNNING we just end up making
a duplicate call to ''poweroff'' (while it is running).

That will fail or execute (And if executed then it will be
stuck in the reboot_mutex mutex). But nobody will care b/c the
machine is in poweroff sequence.

If the state is SYSTEM_POWER_OFF then we end up making
a duplicate call to kernel_power_off. There is no locking
there so we walk in the same steps as what ''poweroff''
has been doing.

This code in kernel/reboot.c doesn''t look that safe when it
comes to a user-invoked ''poweroff'' operation and a kernel
''orderly_poweroff'' operation.

Perhaps what we should do is just:

	if (system_state == SYSTEM_BOOTING)
		orderly_poweroff(true);
	else if (system_state == SYSTEM_RUNNING)
		orderly_poweroff(false);
	else
		printk("Shutdown in progress. Ignoring xl shutdown");

But then ''system_state'' is not guarded by a spinlock or such.
Thought
it is guarded by the xenwatch mutex.

Perhaps to be extra safe we should add ourselves to the
register_reboot_notifier like so (not compile tested)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index fe1c0a6..fb43db6 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -36,7 +36,8 @@ enum shutdown_state {
 	 SHUTDOWN_HALT = 4,
 };
 
-/* Ignore multiple shutdown requests. */
+/* Ignore multiple shutdown requests. Our mutex for this is that
+ * shutdown handler is called with a mutex from xenwatch. */
 static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
 
 struct suspend_info {
@@ -185,7 +186,12 @@ struct shutdown_handler {
 static void do_poweroff(void)
 {
 	shutting_down = SHUTDOWN_POWEROFF;
-	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
+	if (system_state == SYSTEM_RUNNING)
+		orderly_poweroff(false);
+	else if (system_state == SYSTEM_BOOTING)
+		orderly_poweroff(true);
+	else
+		printk(KERN_WARNING "Ignorning shutdown request as poweroff in
progress.\n");
 }
 
 static void do_reboot(void)
@@ -250,7 +256,29 @@ static void shutdown_handler(struct xenbus_watch *watch,
 
 	kfree(str);
 }
+/*
+ * This function is called when the system is being rebooted.
+ */
+static int
+xxen_system_reboot(struct notifier_block *nb, unsigned long event, void
*unused)
+{
+	switch (event) {
+	case SYS_RESTART:
+	case SYS_HALT:
+	case SYS_POWER_OFF:
+	default:
+		mutex_lock(&xenwatch_mutex);
+		shutting_down = SHUTDOWN_POWEROFF;
+		mutex_unlock(&xenwatch_mutex);
+		break;
+	}
+
+	return NOTIFY_DONE;
+}
 
+static struct notifier_block xen_shutdown_notifier = {
+        .notifier_call = xen_system_reboot,
+};
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handler(struct xenbus_watch *watch, const char **vec,
 			  unsigned int len)
@@ -308,7 +336,11 @@ static int setup_shutdown_watcher(void)
 		return err;
 	}
 #endif
-
+	err = register_reboot_notifier(&xen_shutdown_notifier);
+	if (err) {
+		pr_warn("Failed to register shutdown notifier\n");
+		return err;
+	}
 	return 0;
 }
 
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index b6d5fff..ac25752 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -122,7 +122,7 @@ static DEFINE_SPINLOCK(watch_events_lock);
  * carrying out work.
  */
 static pid_t xenwatch_pid;
-static DEFINE_MUTEX(xenwatch_mutex);
+DEFINE_MUTEX(xenwatch_mutex);
 static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq);
 
 static int get_error(const char *errorstring)
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index 40abaf6..57b3370 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -125,7 +125,7 @@ struct xenbus_transaction
 {
 	u32 id;
 };
-
+extern struct mutex xenwatch_mutex;
 /* Nil transaction ID. */
 #define XBT_NIL ((struct xenbus_transaction) { 0 })

> > @@ -185,7 +185,7 @@ struct shutdown_handler {
> > >  static void do_poweroff(void)
> > >  {
> > >  	shutting_down = SHUTDOWN_POWEROFF;
> > > -	orderly_poweroff(false);
> > > +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true :
false);
> > 
> > Does this DTRT for systems in SYSTEM_{HALTED,POWEROFF}? I suppose
under
> > those circumstances forcing is the desired action, insn''t it
> 
> Yes. And there is also a guard (shutting_down) that gets set on
> drivers/xen/manage.c so that the ''do_poweroff'' will only
get called
> once. Which would guard against us powering off and then
> receiving another ''xl shutdown'' when the the system_state
is in
> HALTED or POWEROFF.
> 
> But I hadn''t tested the case where the user does
''poweroff''
> and at the same time the system admin does ''xl shutdown''.
> 
> Depending on the race, the state will be SYSTEM_RUNNING or
> SYSTEM_POWER_OFF. If SYSTEM_RUNNING we just end up making
> a duplicate call to ''poweroff'' (while it is running).
> 
> That will fail or execute (And if executed then it will be
> stuck in the reboot_mutex mutex). But nobody will care b/c the
> machine is in poweroff sequence.
Right.
> If the state is SYSTEM_POWER_OFF then we end up making
> a duplicate call to kernel_power_off. There is no locking
> there so we walk in the same steps as what ''poweroff''
> has been doing.
> 
> This code in kernel/reboot.c doesn''t look that safe when it
> comes to a user-invoked ''poweroff'' operation and a kernel
> ''orderly_poweroff'' operation.
Yes.
> Perhaps what we should do is just:
> 
> 	if (system_state == SYSTEM_BOOTING)
> 		orderly_poweroff(true);
> 	else if (system_state == SYSTEM_RUNNING)
> 		orderly_poweroff(false);
> 	else
> 		printk("Shutdown in progress. Ignoring xl shutdown");
(nb: switch() ;-)). I would also avoiding saying xl since it may not be
true. "Ignoring Xen toolstack shutdown" or something
> But then ''system_state'' is not guarded by a spinlock or
such. Thought
> it is guarded by the xenwatch mutex.
system_state is a core global though, so it must surely also be touched
outside of xen code and therefore outside of xenwatch mutex.

maybe you meant s/system_state/shutting_down/?
> Perhaps to be extra safe we should add ourselves to the
> register_reboot_notifier like so (not compile tested)
I think this only makes sense if you did mean
s/system_state/shutting_down/ above, so I''ll assume that to be the
case.

It''s a shame this has to expose the watch mutex outside of the core xs
code. Perhaps the core code could add the notifier itself and in turn
call a manage.c notification function with the lock already held?
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index fe1c0a6..fb43db6 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -36,7 +36,8 @@ enum shutdown_state {
>  	 SHUTDOWN_HALT = 4,
>  };
>  
> -/* Ignore multiple shutdown requests. */
> +/* Ignore multiple shutdown requests. Our mutex for this is that
> + * shutdown handler is called with a mutex from xenwatch. */
>  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
>  
>  struct suspend_info {
> @@ -185,7 +186,12 @@ struct shutdown_handler {
>  static void do_poweroff(void)
>  {
>  	shutting_down = SHUTDOWN_POWEROFF;
> -	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
> +	if (system_state == SYSTEM_RUNNING)
> +		orderly_poweroff(false);
> +	else if (system_state == SYSTEM_BOOTING)
> +		orderly_poweroff(true);
> +	else
> +		printk(KERN_WARNING "Ignorning shutdown request as poweroff in
progress.\n");
>  }
>  
>  static void do_reboot(void)
> @@ -250,7 +256,29 @@ static void shutdown_handler(struct xenbus_watch
*watch,
>  
>  	kfree(str);
>  }
> +/*
> + * This function is called when the system is being rebooted.
> + */
> +static int
> +xxen_system_reboot(struct notifier_block *nb, unsigned long event, void
*unused)
> +{
> +	switch (event) {
> +	case SYS_RESTART:
> +	case SYS_HALT:
> +	case SYS_POWER_OFF:
> +	default:
> +		mutex_lock(&xenwatch_mutex);
> +		shutting_down = SHUTDOWN_POWEROFF;
> +		mutex_unlock(&xenwatch_mutex);
> +		break;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
>  
> +static struct notifier_block xen_shutdown_notifier = {
> +        .notifier_call = xen_system_reboot,
> +};
>  #ifdef CONFIG_MAGIC_SYSRQ
>  static void sysrq_handler(struct xenbus_watch *watch, const char **vec,
>  			  unsigned int len)
> @@ -308,7 +336,11 @@ static int setup_shutdown_watcher(void)
>  		return err;
>  	}
>  #endif
> -
> +	err = register_reboot_notifier(&xen_shutdown_notifier);
> +	if (err) {
> +		pr_warn("Failed to register shutdown notifier\n");
> +		return err;
> +	}
>  	return 0;
>  }
>  
> diff --git a/drivers/xen/xenbus/xenbus_xs.c
b/drivers/xen/xenbus/xenbus_xs.c
> index b6d5fff..ac25752 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -122,7 +122,7 @@ static DEFINE_SPINLOCK(watch_events_lock);
>   * carrying out work.
>   */
>  static pid_t xenwatch_pid;
> -static DEFINE_MUTEX(xenwatch_mutex);
> +DEFINE_MUTEX(xenwatch_mutex);
>  static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq);
>  
>  static int get_error(const char *errorstring)
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 40abaf6..57b3370 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -125,7 +125,7 @@ struct xenbus_transaction
>  {
>  	u32 id;
>  };
> -
> +extern struct mutex xenwatch_mutex;
>  /* Nil transaction ID. */
>  #define XBT_NIL ((struct xenbus_transaction) { 0 })
>  
>

On Thu, Nov 07, 2013 at 11:24:45AM +0000, Ian Campbell
wrote:
> > > @@ -185,7 +185,7 @@ struct shutdown_handler {
> > > >  static void do_poweroff(void)
> > > >  {
> > > >  	shutting_down = SHUTDOWN_POWEROFF;
> > > > -	orderly_poweroff(false);
> > > > +	orderly_poweroff(system_state != SYSTEM_RUNNING ? true :
false);
> > > 
> > > Does this DTRT for systems in SYSTEM_{HALTED,POWEROFF}? I suppose
under
> > > those circumstances forcing is the desired action,
insn''t it
> > 
> > Yes. And there is also a guard (shutting_down) that gets set on
> > drivers/xen/manage.c so that the ''do_poweroff'' will
only get called
> > once. Which would guard against us powering off and then
> > receiving another ''xl shutdown'' when the the
system_state is in
> > HALTED or POWEROFF.
> > 
> > But I hadn''t tested the case where the user does
''poweroff''
> > and at the same time the system admin does ''xl
shutdown''.
> > 
> > Depending on the race, the state will be SYSTEM_RUNNING or
> > SYSTEM_POWER_OFF. If SYSTEM_RUNNING we just end up making
> > a duplicate call to ''poweroff'' (while it is
running).
> > 
> > That will fail or execute (And if executed then it will be
> > stuck in the reboot_mutex mutex). But nobody will care b/c the
> > machine is in poweroff sequence.
> 
> Right.
> 
> > If the state is SYSTEM_POWER_OFF then we end up making
> > a duplicate call to kernel_power_off. There is no locking
> > there so we walk in the same steps as what
''poweroff''
> > has been doing.
> > 
> > This code in kernel/reboot.c doesn''t look that safe when it
> > comes to a user-invoked ''poweroff'' operation and a
kernel
> > ''orderly_poweroff'' operation.
> 
> Yes.
> 
> > Perhaps what we should do is just:
> > 
> > 	if (system_state == SYSTEM_BOOTING)
> > 		orderly_poweroff(true);
> > 	else if (system_state == SYSTEM_RUNNING)
> > 		orderly_poweroff(false);
> > 	else
> > 		printk("Shutdown in progress. Ignoring xl shutdown");
> 
> (nb: switch() ;-)). I would also avoiding saying xl since it may not be
> true. "Ignoring Xen toolstack shutdown" or something
> 
> > But then ''system_state'' is not guarded by a spinlock
or such. Thought
> > it is guarded by the xenwatch mutex.
> 
> system_state is a core global though, so it must surely also be touched
> outside of xen code and therefore outside of xenwatch mutex.
> 
> maybe you meant s/system_state/shutting_down/?
I think I meant shutting_down. Which is a guard variable we use
to guard against calling the orderly_poweroff multiple times.

But then in my mind the system_state and shutting_down melted
in one.

I blame the sleep deprevation on that.
> 
> > Perhaps to be extra safe we should add ourselves to the
> > register_reboot_notifier like so (not compile tested)
> 
> I think this only makes sense if you did mean
> s/system_state/shutting_down/ above, so I''ll assume that to be the
case.
> 
> It''s a shame this has to expose the watch mutex outside of the
core xs
> code. Perhaps the core code could add the notifier itself and in turn
> call a manage.c notification function with the lock already held?
We could also make the ''shutting_down'' be an atomic. That way
it will always have the correct value and we don''t have to depend on
mutexes.

And then we won''t go in the orderly_shutdown when a
''poweroff''
has been done from user-space. Problem solved :-)

We will still need the notifier naturally (in the manage.c
code).
> 
> > 
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index fe1c0a6..fb43db6 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -36,7 +36,8 @@ enum shutdown_state {
> >  	 SHUTDOWN_HALT = 4,
> >  };
> >  
> > -/* Ignore multiple shutdown requests. */
> > +/* Ignore multiple shutdown requests. Our mutex for this is that
> > + * shutdown handler is called with a mutex from xenwatch. */
> >  static enum shutdown_state shutting_down = SHUTDOWN_INVALID;
> >  
> >  struct suspend_info {
> > @@ -185,7 +186,12 @@ struct shutdown_handler {
> >  static void do_poweroff(void)
> >  {
> >  	shutting_down = SHUTDOWN_POWEROFF;
> > -	orderly_poweroff(system_state != SYSTEM_RUNNING ? true : false);
> > +	if (system_state == SYSTEM_RUNNING)
> > +		orderly_poweroff(false);
> > +	else if (system_state == SYSTEM_BOOTING)
> > +		orderly_poweroff(true);
> > +	else
> > +		printk(KERN_WARNING "Ignorning shutdown request as poweroff in
progress.\n");
> >  }
> >  
> >  static void do_reboot(void)
> > @@ -250,7 +256,29 @@ static void shutdown_handler(struct xenbus_watch
*watch,
> >  
> >  	kfree(str);
> >  }
> > +/*
> > + * This function is called when the system is being rebooted.
> > + */
> > +static int
> > +xxen_system_reboot(struct notifier_block *nb, unsigned long event,
void *unused)
> > +{
> > +	switch (event) {
> > +	case SYS_RESTART:
> > +	case SYS_HALT:
> > +	case SYS_POWER_OFF:
> > +	default:
> > +		mutex_lock(&xenwatch_mutex);
> > +		shutting_down = SHUTDOWN_POWEROFF;
> > +		mutex_unlock(&xenwatch_mutex);
> > +		break;
> > +	}
> > +
> > +	return NOTIFY_DONE;
> > +}
> >  
> > +static struct notifier_block xen_shutdown_notifier = {
> > +        .notifier_call = xen_system_reboot,
> > +};
> >  #ifdef CONFIG_MAGIC_SYSRQ
> >  static void sysrq_handler(struct xenbus_watch *watch, const char
**vec,
> >  			  unsigned int len)
> > @@ -308,7 +336,11 @@ static int setup_shutdown_watcher(void)
> >  		return err;
> >  	}
> >  #endif
> > -
> > +	err = register_reboot_notifier(&xen_shutdown_notifier);
> > +	if (err) {
> > +		pr_warn("Failed to register shutdown notifier\n");
> > +		return err;
> > +	}
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/xen/xenbus/xenbus_xs.c
b/drivers/xen/xenbus/xenbus_xs.c
> > index b6d5fff..ac25752 100644
> > --- a/drivers/xen/xenbus/xenbus_xs.c
> > +++ b/drivers/xen/xenbus/xenbus_xs.c
> > @@ -122,7 +122,7 @@ static DEFINE_SPINLOCK(watch_events_lock);
> >   * carrying out work.
> >   */
> >  static pid_t xenwatch_pid;
> > -static DEFINE_MUTEX(xenwatch_mutex);
> > +DEFINE_MUTEX(xenwatch_mutex);
> >  static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq);
> >  
> >  static int get_error(const char *errorstring)
> > diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> > index 40abaf6..57b3370 100644
> > --- a/include/xen/xenbus.h
> > +++ b/include/xen/xenbus.h
> > @@ -125,7 +125,7 @@ struct xenbus_transaction
> >  {
> >  	u32 id;
> >  };
> > -
> > +extern struct mutex xenwatch_mutex;
> >  /* Nil transaction ID. */
> >  #define XBT_NIL ((struct xenbus_transaction) { 0 })
> >  
> > 
> 
>