[PATCH] libxl: create control/sysrq xenstore node

[PATCH] libxl: create control/sysrq xenstore node

[Vitaly Kuznetsov]

'xl sysrq' command doesn't work with modern Linux guests with the following
message in guest's log:

 xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq

xenstore trace confirms:

 IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
 OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )

The problem seems to be in the fact that we don't pre-create control/sysrq
xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
read-only. As we want to allow guests to clean 'control/sysrq' after the
requested action is performed, we need to make this node writable.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
- Linux code will need to be modified too. With this patch we get something
like

 sysrq: SysRq : Emergency Sync
 xen:manage: Error -34 reading sysrq code in control/sysrq
 Emergency Sync complete

This new ERANGE fault happens because after we clean control/sysrq node
xenstore watch in the guest is re-asserted but the value is empty ("") so
xenbus_scanf() fails "%c" parsing.

It seems the feature is long broken.
---
 tools/libxl/libxl_create.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index b7b44e280b..d60f952f73 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -692,6 +692,9 @@ retry_transaction:
                         GCSPRINTF("%s/control/feature-s4", dom_path),
                         rwperm, ARRAY_SIZE(rwperm));
     }
+    libxl__xs_mknod(gc, t,
+                    GCSPRINTF("%s/control/sysrq", dom_path),
+                    rwperm, ARRAY_SIZE(rwperm));
     libxl__xs_mknod(gc, t,
                     GCSPRINTF("%s/device/suspend/event-channel", dom_path),
                     rwperm, ARRAY_SIZE(rwperm));
-- 
2.14.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Wei Liu]

On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
> 'xl sysrq' command doesn't work with modern Linux guests with the following
> message in guest's log:
> 
>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
> 
> xenstore trace confirms:
> 
>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
> 
> The problem seems to be in the fact that we don't pre-create control/sysrq
> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
> read-only. As we want to allow guests to clean 'control/sysrq' after the
> requested action is performed, we need to make this node writable.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Hmm... this node isn't documented.

Can you point me to the code in Linux?

> ---
> - Linux code will need to be modified too. With this patch we get something
> like
> 
>  sysrq: SysRq : Emergency Sync
>  xen:manage: Error -34 reading sysrq code in control/sysrq
>  Emergency Sync complete
> 
> This new ERANGE fault happens because after we clean control/sysrq node
> xenstore watch in the guest is re-asserted but the value is empty ("") so
> xenbus_scanf() fails "%c" parsing.
> 
> It seems the feature is long broken.
> ---
>  tools/libxl/libxl_create.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index b7b44e280b..d60f952f73 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -692,6 +692,9 @@ retry_transaction:
>                          GCSPRINTF("%s/control/feature-s4", dom_path),
>                          rwperm, ARRAY_SIZE(rwperm));
>      }
> +    libxl__xs_mknod(gc, t,
> +                    GCSPRINTF("%s/control/sysrq", dom_path),
> +                    rwperm, ARRAY_SIZE(rwperm));
>      libxl__xs_mknod(gc, t,
>                      GCSPRINTF("%s/device/suspend/event-channel", dom_path),
>                      rwperm, ARRAY_SIZE(rwperm));
> -- 
> 2.14.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Vitaly Kuznetsov]

Wei Liu <wei.liu2@citrix.com> writes:

> On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
>> 'xl sysrq' command doesn't work with modern Linux guests with the following
>> message in guest's log:
>> 
>>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
>> 
>> xenstore trace confirms:
>> 
>>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
>>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
>> 
>> The problem seems to be in the fact that we don't pre-create control/sysrq
>> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
>> read-only. As we want to allow guests to clean 'control/sysrq' after the
>> requested action is performed, we need to make this node writable.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Hmm... this node isn't documented.
>

But libxl already writes it, see libxl_send_sysrq().

> Can you point me to the code in Linux?
>

Sure,

drivers/xen/manage.c:

static void sysrq_handler(struct xenbus_watch *watch, const char *path,
			  const char *token)
{
	char sysrq_key = '\0';
	struct xenbus_transaction xbt;
	int err;

 again:
	err = xenbus_transaction_start(&xbt);
	if (err)
		return;
	err = xenbus_scanf(xbt, "control", "sysrq", "%c", &sysrq_key);
	if (err < 0) {
		/*
		 * The Xenstore watch fires directly after registering it and
		 * after a suspend/resume cycle. So ENOENT is no error but
		 * might happen in those cases.
		 */
		if (err != -ENOENT)
			pr_err("Error %d reading sysrq code in control/sysrq\n",
			       err);
		xenbus_transaction_end(xbt, 1);
		return;
	}

	if (sysrq_key != '\0') {
		err = xenbus_printf(xbt, "control", "sysrq", "%c", '\0');
		if (err) {
			pr_err("%s: Error %d writing sysrq in control/sysrq\n",
			       __func__, err);
			xenbus_transaction_end(xbt, 1);
			return;
		}
	}

	err = xenbus_transaction_end(xbt, 0);
	if (err == -EAGAIN)
		goto again;

	if (sysrq_key != '\0')
		handle_sysrq(sysrq_key);
}

static struct xenbus_watch sysrq_watch = {
	.node = "control/sysrq",
	.callback = sysrq_handler
};


-- 
  Vitaly

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Wei Liu]

Also CC Linux maintainers.

On Tue, Sep 04, 2018 at 07:27:31PM +0200, Vitaly Kuznetsov wrote:
> Wei Liu <wei.liu2@citrix.com> writes:
> 
> > On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
> >> 'xl sysrq' command doesn't work with modern Linux guests with the following
> >> message in guest's log:
> >> 
> >>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
> >> 
> >> xenstore trace confirms:
> >> 
> >>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
> >>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
> >> 
> >> The problem seems to be in the fact that we don't pre-create control/sysrq
> >> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
> >> read-only. As we want to allow guests to clean 'control/sysrq' after the
> >> requested action is performed, we need to make this node writable.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >
> > Hmm... this node isn't documented.
> >
> 
> But libxl already writes it, see libxl_send_sysrq().

I think your patch is fine.

What I wanted to do is to document this node. :p

> 
> > Can you point me to the code in Linux?
> >
> 
> Sure,
> 
> drivers/xen/manage.c:

Alright, it appears DomU only expects a single character to be written.

I have written the following diff, do you think it is sensible? I
believe this is Linux only, but I have CC Roger for correction.


diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 60c8b3fbe5..33d281915c 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -410,6 +410,14 @@ by udev ("0") or will be run by the toolstack directly ("1").
 
 ### Platform Feature and Control Paths
 
+#### ~/control/sysrq = (""|COMMAND) [w]
+
+This is the PV SysRq control node. A toolstack can write a single character
+representing a magic SysRq key understood by the Linux kernel.  The guest
+acknowledges a request by writing the empty string back to the command node.
+
+This protocol is Linux only.
+
 #### ~/control/shutdown = (""|COMMAND) [w]
 
 This is the PV shutdown control node. A toolstack can write various


Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Wei Liu]

On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
> 'xl sysrq' command doesn't work with modern Linux guests with the following
> message in guest's log:
> 
>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
> 
> xenstore trace confirms:
> 
>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
> 
> The problem seems to be in the fact that we don't pre-create control/sysrq
> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
> read-only. As we want to allow guests to clean 'control/sysrq' after the
> requested action is performed, we need to make this node writable.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Roger Pau Monné]

On Wed, Sep 05, 2018 at 09:22:03AM +0100, Wei Liu wrote:
> Also CC Linux maintainers.
> 
> On Tue, Sep 04, 2018 at 07:27:31PM +0200, Vitaly Kuznetsov wrote:
> > Wei Liu <wei.liu2@citrix.com> writes:
> > 
> > > On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
> > >> 'xl sysrq' command doesn't work with modern Linux guests with the following
> > >> message in guest's log:
> > >> 
> > >>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
> > >> 
> > >> xenstore trace confirms:
> > >> 
> > >>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
> > >>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
> > >> 
> > >> The problem seems to be in the fact that we don't pre-create control/sysrq
> > >> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
> > >> read-only. As we want to allow guests to clean 'control/sysrq' after the
> > >> requested action is performed, we need to make this node writable.
> > >> 
> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > >
> > > Hmm... this node isn't documented.
> > >
> > 
> > But libxl already writes it, see libxl_send_sysrq().
> 
> I think your patch is fine.
> 
> What I wanted to do is to document this node. :p
> 
> > 
> > > Can you point me to the code in Linux?
> > >
> > 
> > Sure,
> > 
> > drivers/xen/manage.c:
> 
> Alright, it appears DomU only expects a single character to be written.
> 
> I have written the following diff, do you think it is sensible? I
> believe this is Linux only, but I have CC Roger for correction.
> 
> 
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 60c8b3fbe5..33d281915c 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -410,6 +410,14 @@ by udev ("0") or will be run by the toolstack directly ("1").
>  
>  ### Platform Feature and Control Paths
>  
> +#### ~/control/sysrq = (""|COMMAND) [w]
> +
> +This is the PV SysRq control node. A toolstack can write a single character
> +representing a magic SysRq key understood by the Linux kernel.  The guest
> +acknowledges a request by writing the empty string back to the command node.
> +
> +This protocol is Linux only.
> +
>  #### ~/control/shutdown = (""|COMMAND) [w]
>  
>  This is the PV shutdown control node. A toolstack can write various

LGTM. I haven't implemented this on FreeBSD, and I very much doubt
NetBSD uses it at all. FreeBSD doesn't have such thing as sysrq keys,
there's only a key sequence used in order to break into the debugger from
the console AFAIK.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: create control/sysrq xenstore node

[Vitaly Kuznetsov]

Wei Liu <wei.liu2@citrix.com> writes:

> Also CC Linux maintainers.
>
> On Tue, Sep 04, 2018 at 07:27:31PM +0200, Vitaly Kuznetsov wrote:
>> Wei Liu <wei.liu2@citrix.com> writes:
>> 
>> > On Tue, Sep 04, 2018 at 01:39:29PM +0200, Vitaly Kuznetsov wrote:
>> >> 'xl sysrq' command doesn't work with modern Linux guests with the following
>> >> message in guest's log:
>> >> 
>> >>  xen:manage: sysrq_handler: Error -13 writing sysrq in control/sysrq
>> >> 
>> >> xenstore trace confirms:
>> >> 
>> >>  IN 0x24bd9a0 20180904 04:36:32 WRITE (control/sysrq )
>> >>  OUT 0x24bd9a0 20180904 04:36:32 ERROR (EACCES )
>> >> 
>> >> The problem seems to be in the fact that we don't pre-create control/sysrq
>> >> xenstore node and libxl_send_sysrq() doing libxl__xs_printf() creates it as
>> >> read-only. As we want to allow guests to clean 'control/sysrq' after the
>> >> requested action is performed, we need to make this node writable.
>> >> 
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >
>> > Hmm... this node isn't documented.
>> >
>> 
>> But libxl already writes it, see libxl_send_sysrq().
>
> I think your patch is fine.
>
> What I wanted to do is to document this node. :p
>
>> 
>> > Can you point me to the code in Linux?
>> >
>> 
>> Sure,
>> 
>> drivers/xen/manage.c:
>
> Alright, it appears DomU only expects a single character to be written.
>
> I have written the following diff, do you think it is sensible? I
> believe this is Linux only, but I have CC Roger for correction.
>
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 60c8b3fbe5..33d281915c 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -410,6 +410,14 @@ by udev ("0") or will be run by the toolstack directly ("1").
>
>  ### Platform Feature and Control Paths
>
> +#### ~/control/sysrq = (""|COMMAND) [w]
> +
> +This is the PV SysRq control node. A toolstack can write a single character
> +representing a magic SysRq key understood by the Linux kernel.  The guest
> +acknowledges a request by writing the empty string back to the command node.
> +
> +This protocol is Linux only.
> +
>  #### ~/control/shutdown = (""|COMMAND) [w]
>

Makes perfect sense to me and sounds correct, thanks!

-- 
  Vitaly

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel