Xen devel - Jul 2011 - Re: [PATCH 2/6] xen/hvm kexec: unregister shutdown+sysrq watches during reboot

>>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de> wrote:
> Unregister the shutdown and sysrq watch during kexec.  The watches can
> not be re-registered in the kexec kernel because they are still seen as
> busy by xenstore.
This and subsequent patches don''t look right to me from a conceptual
pov: If the kexec attempt is due to a crash, the dying kernel should be
doing as little as possible, and the new kernel should really do the
cleanup. The more logic gets added to the shutdown path of the old
kernel, the more likely it''ll become that the kexec attempt will fail.

If this requires changes outside the kernel (e.g. state reset helpers
in hypervisor or tools) - so be it.

Jan
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/manage.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> Index: linux-3.0/drivers/xen/manage.c
> ==================================================================> ---
linux-3.0.orig/drivers/xen/manage.c
> +++ linux-3.0/drivers/xen/manage.c
> @@ -320,6 +320,18 @@ static int shutdown_event(struct notifie
>  return NOTIFY_DONE;
>  }
>  
> +static void xenbus_disable_shutdown_watcher(void)
> +{
> +unregister_xenbus_watch(&shutdown_watch);
> +#ifdef CONFIG_MAGIC_SYSRQ
> +unregister_xenbus_watch(&sysrq_watch);
> +#endif
> +}
> +
> +static struct syscore_ops xenbus_watcher_syscore_ops = {
> +.shutdown = xenbus_disable_shutdown_watcher,
> +};
> +
>  int xen_setup_shutdown_event(void)
>  {
>  static struct notifier_block xenstore_notifier = {
> @@ -329,6 +341,7 @@ int xen_setup_shutdown_event(void)
>  if (!xen_domain())
>  return -ENODEV;
>  register_xenstore_notifier(&xenstore_notifier);
> +register_syscore_ops(&xenbus_watcher_syscore_ops);
>  
>  return 0;
>  }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Jul 28, Jan Beulich wrote:
> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de>
wrote:
> > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > not be re-registered in the kexec kernel because they are still seen
as
> > busy by xenstore.
> 
> This and subsequent patches don''t look right to me from a
conceptual
> pov: If the kexec attempt is due to a crash, the dying kernel should be
> doing as little as possible, and the new kernel should really do the
> cleanup. The more logic gets added to the shutdown path of the old
> kernel, the more likely it''ll become that the kexec attempt will
fail.
kexec is about reboot, kdump is about crash handling. Both use different
code paths.
The kexec code path is like a reboot without going through the firmware.
The kdump kernel runs in its own memory range, so memory corruption does
not appear to happen (with the sles11sp1 kernel + my kdump patch).
> 
> If this requires changes outside the kernel (e.g. state reset helpers
> in hypervisor or tools) - so be it.
Are you suggesting that there have to be ways for a domU to query the
state of its registered watches and shut them all down during very early
boot? And what about the event/irq handling? There is currently no way
to check what virq is bound to what port, other than looping through all
possible ports and see if one matches the requested virq.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-07-28 at 01:25 -0400, Olaf Hering wrote:
> On Thu, Jul 28, Jan Beulich wrote:
> 
> > >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de>
wrote:
> > > Unregister the shutdown and sysrq watch during kexec.  The
watches can
> > > not be re-registered in the kexec kernel because they are still
seen as
> > > busy by xenstore.
> > 
> > This and subsequent patches don''t look right to me from a
conceptual
> > pov: If the kexec attempt is due to a crash, the dying kernel should
be
> > doing as little as possible, and the new kernel should really do the
> > cleanup. The more logic gets added to the shutdown path of the old
> > kernel, the more likely it''ll become that the kexec attempt
will fail.
> 
> kexec is about reboot, kdump is about crash handling. Both use different
> code paths.
> The kexec code path is like a reboot without going through the firmware.
> The kdump kernel runs in its own memory range, so memory corruption does
> not appear to happen (with the sles11sp1 kernel + my kdump patch).
Getting into the kdump kernel is a kexec like operation though and
shares many of the code paths, doesn''t it?
> > If this requires changes outside the kernel (e.g. state reset helpers
> > in hypervisor or tools) - so be it.
> 
> Are you suggesting that there have to be ways for a domU to query the
> state of its registered watches and shut them all down during very early
> boot?
Perhaps the xenstore protocol could be enhanced with a mechanism to
clear all existing watches? A kernel could call that at start of day.
>  And what about the event/irq handling? There is currently no way
> to check what virq is bound to what port, other than looping through all
> possible ports and see if one matches the requested virq.
There is EVTCHNOP_reset but I''m not sure if it does too much. For
example I''m not sure how the guest could recover event channels which
are setup by the domain builder -- such as the xenstore event channel.

IIRC EVTCHNOP_reset was added to aid with kexec though -- so I must be
missing something.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 28/07/2011 11:52, "Ian Campbell" <Ian.Campbell@citrix.com>
wrote:
>>  And what about the event/irq handling? There is currently no way
>> to check what virq is bound to what port, other than looping through
all
>> possible ports and see if one matches the requested virq.
> 
> There is EVTCHNOP_reset but I''m not sure if it does too much. For
> example I''m not sure how the guest could recover event channels
which
> are setup by the domain builder -- such as the xenstore event channel.
EVTCHNOP_reset is currently only called from the toolstack, which is then
able to re-setup things like the xenstore event channel for the guest.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Jul 28, Ian Campbell wrote:
> Getting into the kdump kernel is a kexec like operation though and
> shares many of the code paths, doesn''t it?
The big difference is that kdump is entered in an unreliable state,
while kexec is a controlled reboot.
> > > If this requires changes outside the kernel (e.g. state reset
helpers
> > > in hypervisor or tools) - so be it.
> > 
> > Are you suggesting that there have to be ways for a domU to query the
> > state of its registered watches and shut them all down during very
early
> > boot?
> 
> Perhaps the xenstore protocol could be enhanced with a mechanism to
> clear all existing watches? A kernel could call that at start of day.
I wonder why xenstore knows that sysrq and shutdown nodes are busy,
while the device, backend and state files can be watched twice.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 28 Jul 2011, Jan Beulich wrote:
> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de>
wrote:
> > Unregister the shutdown and sysrq watch during kexec.  The watches can
> > not be re-registered in the kexec kernel because they are still seen
as
> > busy by xenstore.
> 
> This and subsequent patches don''t look right to me from a
conceptual
> pov: If the kexec attempt is due to a crash, the dying kernel should be
> doing as little as possible, and the new kernel should really do the
> cleanup. The more logic gets added to the shutdown path of the old
> kernel, the more likely it''ll become that the kexec attempt will
fail.
> 
> If this requires changes outside the kernel (e.g. state reset helpers
> in hypervisor or tools) - so be it.
I agree.
--8323329-900038443-1311852992=:12963
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

--8323329-900038443-1311852992=:12963--

On Thu, 2011-07-28 at 07:00 -0400, Keir Fraser wrote:
> On 28/07/2011 11:52, "Ian Campbell"
<Ian.Campbell@citrix.com> wrote:
> 
> >>  And what about the event/irq handling? There is currently no way
> >> to check what virq is bound to what port, other than looping
through all
> >> possible ports and see if one matches the requested virq.
> > 
> > There is EVTCHNOP_reset but I''m not sure if it does too much.
For
> > example I''m not sure how the guest could recover event
channels which
> > are setup by the domain builder -- such as the xenstore event channel.
> 
> EVTCHNOP_reset is currently only called from the toolstack, which is then
> able to re-setup things like the xenstore event channel for the guest.
Oh, right. And toolstack isn''t involved in kexec so that''s not
terribly
helpful.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-07-28 at 07:02 -0400, Olaf Hering wrote:
> On Thu, Jul 28, Ian Campbell wrote:
> 
> > Getting into the kdump kernel is a kexec like operation though and
> > shares many of the code paths, doesn''t it?
> 
> The big difference is that kdump is entered in an unreliable state,
> while kexec is a controlled reboot.
Sure, but by handling the former you also solve the later, while the
opposite is not true.
> > > > If this requires changes outside the kernel (e.g. state
reset helpers
> > > > in hypervisor or tools) - so be it.
> > > 
> > > Are you suggesting that there have to be ways for a domU to query
the
> > > state of its registered watches and shut them all down during
very early
> > > boot?
> > 
> > Perhaps the xenstore protocol could be enhanced with a mechanism to
> > clear all existing watches? A kernel could call that at start of day.
> 
> I wonder why xenstore knows that sysrq and shutdown nodes are busy,
> while the device, backend and state files can be watched twice.
Do the precise path''s watched differ subtly?

Oh, I see, xenstored only calls a watch a duplicate if both the path
_and_ the token match. In the case of backend paths the token is a
reference to the dynamically allocated per-device data structure. In the
sysrq and shutdown case the token is a static global variable -- I
suppose you are kexec''ing an identical kernel? I expect that if you
kexec''d a subtly different kernel where these datastructures ended up
at
a different address you wouldn''t get the EEXIST.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Jul 28, Ian Campbell wrote:
> > Are you suggesting that there have to be ways for a domU to query the
> > state of its registered watches and shut them all down during very
early
> > boot?
> 
> Perhaps the xenstore protocol could be enhanced with a mechanism to
> clear all existing watches? A kernel could call that at start of day.
I poked around in the xenstore sources, there is appearently nothing
that can be used to shutdown all. Or would calling XS_RELEASE do the trick?

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de> wrote:
> On Thu, Jul 28, Ian Campbell wrote:
> 
>>> Are you suggesting that there have to be ways for a domU to query
the
>>> state of its registered watches and shut them all down during very
early
>>> boot?
>> 
>> Perhaps the xenstore protocol could be enhanced with a mechanism to
>> clear all existing watches? A kernel could call that at start of day.
> 
> I poked around in the xenstore sources, there is appearently nothing
> that can be used to shutdown all. Or would calling XS_RELEASE do the trick?
XS_INTRODUCE

 K.
> Olaf
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 28.07.11 at 07:25, Olaf Hering <olaf@aepfle.de> wrote:
> On Thu, Jul 28, Jan Beulich wrote:
> 
>> >>> On 26.07.11 at 13:52, Olaf Hering <olaf@aepfle.de>
wrote:
>> > Unregister the shutdown and sysrq watch during kexec.  The watches
can
>> > not be re-registered in the kexec kernel because they are still
seen as
>> > busy by xenstore.
>> 
>> This and subsequent patches don''t look right to me from a
conceptual
>> pov: If the kexec attempt is due to a crash, the dying kernel should be
>> doing as little as possible, and the new kernel should really do the
>> cleanup. The more logic gets added to the shutdown path of the old
>> kernel, the more likely it''ll become that the kexec attempt
will fail.
> 
> kexec is about reboot, kdump is about crash handling. Both use different
> code paths.
> The kexec code path is like a reboot without going through the firmware.
Oh, I implied it would be kdump.

But then again xenstore state shouldn''t matter wrt the purpose of
the secondary kernel.
> The kdump kernel runs in its own memory range, so memory corruption does
> not appear to happen (with the sles11sp1 kernel + my kdump patch).
It''s also not clear to me what corruption there is - this would seem to
imply that there should be information on certain addresses that were
used in the old kernel to get passed to the new kernel, or get used to
access guest memory from outside the guest. All of which sounds
wrong.

Jan
>> If this requires changes outside the kernel (e.g. state reset helpers
>> in hypervisor or tools) - so be it.
> 
> Are you suggesting that there have to be ways for a domU to query the
> state of its registered watches and shut them all down during very early
> boot? And what about the event/irq handling? There is currently no way
> to check what virq is bound to what port, other than looping through all
> possible ports and see if one matches the requested virq.
> 
> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Jul 28, Keir Fraser wrote:
> On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de> wrote:
> 
> > On Thu, Jul 28, Ian Campbell wrote:
> > 
> >>> Are you suggesting that there have to be ways for a domU to
query the
> >>> state of its registered watches and shut them all down during
very early
> >>> boot?
> >> 
> >> Perhaps the xenstore protocol could be enhanced with a mechanism
to
> >> clear all existing watches? A kernel could call that at start of
day.
> > 
> > I poked around in the xenstore sources, there is appearently nothing
> > that can be used to shutdown all. Or would calling XS_RELEASE do the
trick?
> 
> XS_INTRODUCE
Unfortunately do_introduce() is not preprared for that.

On enter, conn->id contains the domain_id, so it errors out early.
Later it compares domain->conn != conn and does not enter the correct path.

If I remove both checks the kexec appears to work ok.
Is it save to remove both checks?

The only issue I havent figured out:
How to get the domid from within the kernel?

Olaf

diff -r 42edf1481c57 tools/xenstore/xenstored_domain.c
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -326,7 +326,7 @@ void do_introduce(struct connection *con
 		return;
 	}
 
-	if (conn->id != 0 || !conn->can_write) {
+	if (!conn->can_write) {
 		send_error(conn, EACCES);
 		return;
 	}
@@ -365,7 +365,7 @@ void do_introduce(struct connection *con
 		talloc_steal(domain->conn, domain);
 
 		fire_watches(NULL, "@introduceDomain", false);
-	} else if ((domain->mfn == mfn) && (domain->conn != conn)) {
+	} else if (domain->mfn == mfn) {
 		/* Use XS_INTRODUCE for recreating the xenbus event-channel. */
 		if (domain->port)
 			xc_evtchn_unbind(xce_handle, domain->port);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 28/07/2011 20:50, "Olaf Hering" <olaf@aepfle.de> wrote:
> On Thu, Jul 28, Keir Fraser wrote:
> 
>> On 28/07/2011 15:07, "Olaf Hering" <olaf@aepfle.de>
wrote:
>> 
>>> On Thu, Jul 28, Ian Campbell wrote:
>>> 
>>>>> Are you suggesting that there have to be ways for a domU to
query the
>>>>> state of its registered watches and shut them all down
during very early
>>>>> boot?
>>>> 
>>>> Perhaps the xenstore protocol could be enhanced with a
mechanism to
>>>> clear all existing watches? A kernel could call that at start
of day.
>>> 
>>> I poked around in the xenstore sources, there is appearently
nothing
>>> that can be used to shutdown all. Or would calling XS_RELEASE do
the trick?
>> 
>> XS_INTRODUCE
> 
> Unfortunately do_introduce() is not preprared for that.
> 
> On enter, conn->id contains the domain_id, so it errors out early.
> Later it compares domain->conn != conn and does not enter the correct
path.
Oh of course you want to do it from *inside* the guest...
> If I remove both checks the kexec appears to work ok.
> Is it save to remove both checks?
Hmm, no. Attached patch would be safer, give it a try.

And note that it is *dangerous* to reset the domain xenstore connection if
there could be any other concurrent activity on the connection that could
confuse the guest kernel. So doing the reset during kernel bringup might be
safest, there you can do it in the kernel before the whole xenbus subsystem
is fully up.

 -- Keir
> The only issue I havent figured out:
> How to get the domid from within the kernel?
> 
> Olaf
> 
> diff -r 42edf1481c57 tools/xenstore/xenstored_domain.c
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -326,7 +326,7 @@ void do_introduce(struct connection *con
> return;
> }
>  
> - if (conn->id != 0 || !conn->can_write) {
> + if (!conn->can_write) {
> send_error(conn, EACCES);
> return;
> }
> @@ -365,7 +365,7 @@ void do_introduce(struct connection *con
> talloc_steal(domain->conn, domain);
>  
> fire_watches(NULL, "@introduceDomain", false);
> - } else if ((domain->mfn == mfn) && (domain->conn != conn))
{
> + } else if (domain->mfn == mfn) {
> /* Use XS_INTRODUCE for recreating the xenbus event-channel. */
> if (domain->port)
> xc_evtchn_unbind(xce_handle, domain->port);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Jul 28, Keir Fraser wrote:
> > Unfortunately do_introduce() is not preprared for that.
> > 
> > On enter, conn->id contains the domain_id, so it errors out early.
> > Later it compares domain->conn != conn and does not enter the
correct path.
> 
> Oh of course you want to do it from *inside* the guest...
> 
> > If I remove both checks the kexec appears to work ok.
> > Is it save to remove both checks?
> 
> Hmm, no. Attached patch would be safer, give it a try.
> 
> And note that it is *dangerous* to reset the domain xenstore connection if
> there could be any other concurrent activity on the connection that could
> confuse the guest kernel. So doing the reset during kernel bringup might be
> safest, there you can do it in the kernel before the whole xenbus subsystem
> is fully up.
I sent a different xenstored patch which takes DOMID_SELF into account.

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/08/2011 06:01, "Olaf Hering" <olaf@aepfle.de> wrote:
>> Hmm, no. Attached patch would be safer, give it a try.
>> 
>> And note that it is *dangerous* to reset the domain xenstore connection
if
>> there could be any other concurrent activity on the connection that
could
>> confuse the guest kernel. So doing the reset during kernel bringup
might be
>> safest, there you can do it in the kernel before the whole xenbus
subsystem
>> is fully up.
> 
> I sent a different xenstored patch which takes DOMID_SELF into account.
Saw it, I''ll give it a think over. I think it looks okay, I wonder
whether
DOMID_SELF handling should be pushed into find_domain_by_domid, but
it''s a
minor detail really. I certainly support the approach. Hopefully we can get
someone to similarly modify ocaml/xenstored too.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel