Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Rich Persaud]

(adding xen-devel & toolstack devs)

On Dec 14, 2020, at 16:12, Jason Andryuk <jandryuk@gmail.com> wrote:
> 
> On Fri, Dec 11, 2020 at 3:56 PM Chris Rogers <crogers122@gmail.com> wrote:
>> 
>> This is a follow up to a request during our roadmapping meeting to clarify the purpose of libxl-fix-reboot.patch on the current version of Xen in OpenXT (4.12).  It's pretty simple.  While the domctl API does define a trigger for reset in xen/include/public/domctl.h:
>> 
> 
>> The call stack looks like this:
>>> libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_RESET, 0);
>>> xc_domain_send_trigger(ctx->xch, domid, XEN_DOMCTL_SENDTRIGGER_RESET, vcupid);
>>> do_domctl()
>>> arch_do_domctl()
>> and reaching the case statement in arch_do_domctl() for XEN_DOMCTL_sendtrigger, with RESET, we get -ENOSYS as illustrated above.
> 
> Thanks, Chris.  It's surprising that xl trigger reset exists, but
> isn't wired through to do anything.  And that reboot has a fallback
> command to something that doesn't work.
> 
> If we have to turn reboot into shutdown + start, it seems like that
> could be done in xenmgr instead of libxl.  Similarly, this may avoid
> the signaling between xenmgr and libxl.

If upstream Xen's libxl cannot support VM reset, can we drop/hide reset support from the OpenXT CLI and UIVM? That would avoid incurring costs for a fake feature with no long-term future. A reset is not the same as shutdown + start.  If reset is not supportable, the user can perform shutdown + reboot manually. Then they would at least be aware of the consequences, e.g. temporary storage snapshots will be deleted and changes lost immediately.

OpenXT derivatives which need reset support can use another Xen toolstack which provides this capability, e.g. the Citrix XenServer xapi ocaml toolstack, for this single function.  Or the old XenClient xenops fork of xapi.

The long-term direction, based on an upstream prototype in Rust, is a low level toolstack daemon that accepts input over an RPC protocol that is stable and versioned, which will drive a stable hypercall ABI for Xen. We can ask for reset support to be prioritized in the Rust prototype, which would then enable testing of OpenXT integration.

Hopefully an upstream Xen LibXL developer will recall why the reset logic isn't yet fully wired up.

Rich

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Stefano Stabellini]

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

On Mon, 14 Dec 2020, Rich Persaud wrote:
> (adding xen-devel & toolstack devs)
> 
> On Dec 14, 2020, at 16:12, Jason Andryuk <jandryuk@gmail.com> wrote:
> > 
> > On Fri, Dec 11, 2020 at 3:56 PM Chris Rogers <crogers122@gmail.com> wrote:
> >> 
> >> This is a follow up to a request during our roadmapping meeting to clarify the purpose of libxl-fix-reboot.patch on the current version of Xen in OpenXT (4.12).  It's pretty simple.  While the domctl API does define a trigger for reset in xen/include/public/domctl.h:
> >> 
> > 
> >> The call stack looks like this:
> >>> libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_RESET, 0);
> >>> xc_domain_send_trigger(ctx->xch, domid, XEN_DOMCTL_SENDTRIGGER_RESET, vcupid);
> >>> do_domctl()
> >>> arch_do_domctl()
> >> and reaching the case statement in arch_do_domctl() for XEN_DOMCTL_sendtrigger, with RESET, we get -ENOSYS as illustrated above.
> > 
> > Thanks, Chris.  It's surprising that xl trigger reset exists, but
> > isn't wired through to do anything.  And that reboot has a fallback
> > command to something that doesn't work.

I miss some of the context of this thread -- let me try to understand
the issue properly.

It looks like HVM reboot doesn't work properly, or is it HVM reset
(in-guest reset)? It looks like it is implemented by calling "xl trigger
reset", which is implemented by libxl_send_trigger. The call chain leads
to a XEN_DOMCTL_sendtrigger domctl with XEN_DOMCTL_SENDTRIGGER_RESET as
a parameter that is not implemented on x86.

That looks like a pretty serious bug :-)


I imagine the reason why it is in that state is that the main way to
reboot would be to call "xl reboot" which is implemented with the PV
protocol "reboot" write to xenstore?  Either way, the bug should be
fixed.

What does your libxl-fix-reboot.patch patch do? Does it add an
implementation of XEN_DOMCTL_SENDTRIGGER_RESET?

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Chris Rogers]

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

Hopefully I can provide a little more context.  Here is a link to the patch:

https://github.com/OpenXT/xenclient-oe/blob/master/recipes-extended/xen/files/libxl-fix-reboot.patch

The patch is a bit mis-named.  It does not implement
XEN_DOMCTL_SENDTRIGGER_RESET.  It's just a workaround to handle the missing
RESET implementation.

Its purpose is to make an HVM guest "reboot" regardless of whether PV tools
have been installed and the xenstore interface is listening or not.  From
the client perspective that OpenXT is concerned with, this is for
ease-of-use for working with HVM guests before PV tools are installed.  To
summarize the flow of the patch:

- User input causes high level toolstack, xenmgr, to do xl reboot <domid>
- libxl hits "PV interface not available", so it tries the fallback ACPI
reset trigger (but that's not implemented in domctl)
- therefore, the patch changes the RESET trigger to POWER trigger, and sets
a 'reboot' flag
- when the xl create process handles the domain_death event for
LIBXL_SHUTDOWN_REASON_POWEROFF, we check for our 'reboot' flag.
- It's set, so we set "action" as if we came from a real restart, which
makes the xl create process take the 'goto start' codepath to rebuild the
domain.

I think we'd like to get rid of this patch, but at the moment I don't have
any code or a design to propose that would implement the
XEN_DOMCTL_SENDTRIGGER_RESET.

On Mon, Dec 14, 2020 at 7:42 PM Stefano Stabellini <sstabellini@kernel.org>
wrote:

> On Mon, 14 Dec 2020, Rich Persaud wrote:
> > (adding xen-devel & toolstack devs)
> >
> > On Dec 14, 2020, at 16:12, Jason Andryuk <jandryuk@gmail.com> wrote:
> > >
> > > On Fri, Dec 11, 2020 at 3:56 PM Chris Rogers <crogers122@gmail.com>
> wrote:
> > >>
> > >> This is a follow up to a request during our roadmapping meeting to
> clarify the purpose of libxl-fix-reboot.patch on the current version of Xen
> in OpenXT (4.12).  It's pretty simple.  While the domctl API does define a
> trigger for reset in xen/include/public/domctl.h:
> > >>
> > >
> > >> The call stack looks like this:
> > >>> libxl_send_trigger(ctx, domid, LIBXL_TRIGGER_RESET, 0);
> > >>> xc_domain_send_trigger(ctx->xch, domid,
> XEN_DOMCTL_SENDTRIGGER_RESET, vcupid);
> > >>> do_domctl()
> > >>> arch_do_domctl()
> > >> and reaching the case statement in arch_do_domctl() for
> XEN_DOMCTL_sendtrigger, with RESET, we get -ENOSYS as illustrated above.
> > >
> > > Thanks, Chris.  It's surprising that xl trigger reset exists, but
> > > isn't wired through to do anything.  And that reboot has a fallback
> > > command to something that doesn't work.
>
> I miss some of the context of this thread -- let me try to understand
> the issue properly.
>
> It looks like HVM reboot doesn't work properly, or is it HVM reset
> (in-guest reset)? It looks like it is implemented by calling "xl trigger
> reset", which is implemented by libxl_send_trigger. The call chain leads
> to a XEN_DOMCTL_sendtrigger domctl with XEN_DOMCTL_SENDTRIGGER_RESET as
> a parameter that is not implemented on x86.
>
> That looks like a pretty serious bug :-)
>
>
> I imagine the reason why it is in that state is that the main way to
> reboot would be to call "xl reboot" which is implemented with the PV
> protocol "reboot" write to xenstore?  Either way, the bug should be
> fixed.
>
> What does your libxl-fix-reboot.patch patch do? Does it add an
> implementation of XEN_DOMCTL_SENDTRIGGER_RESET?

[-- Attachment #2: Type: text/html, Size: 4544 bytes --]

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Jason Andryuk]

On Tue, Dec 15, 2020 at 5:22 PM Chris Rogers <crogers122@gmail.com> wrote:
>
> Hopefully I can provide a little more context.  Here is a link to the patch:
>
> https://github.com/OpenXT/xenclient-oe/blob/master/recipes-extended/xen/files/libxl-fix-reboot.patch
>
> The patch is a bit mis-named.  It does not implement XEN_DOMCTL_SENDTRIGGER_RESET.  It's just a workaround to handle the missing RESET implementation.
>
> Its purpose is to make an HVM guest "reboot" regardless of whether PV tools have been installed and the xenstore interface is listening or not.  From the client perspective that OpenXT is concerned with, this is for ease-of-use for working with HVM guests before PV tools are installed.  To summarize the flow of the patch:
>
> - User input causes high level toolstack, xenmgr, to do xl reboot <domid>
> - libxl hits "PV interface not available", so it tries the fallback ACPI reset trigger (but that's not implemented in domctl)
> - therefore, the patch changes the RESET trigger to POWER trigger, and sets a 'reboot' flag
> - when the xl create process handles the domain_death event for LIBXL_SHUTDOWN_REASON_POWEROFF, we check for our 'reboot' flag.
> - It's set, so we set "action" as if we came from a real restart, which makes the xl create process take the 'goto start' codepath to rebuild the domain.
>
> I think we'd like to get rid of this patch, but at the moment I don't have any code or a design to propose that would implement the XEN_DOMCTL_SENDTRIGGER_RESET.

I'm not sure it's possible to implement one.  Does an ACPI
reset/reboot button exist?  And then you'd have the problem that the
guest needs to be configured to react to the button.

Regards,
Jason

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Andrew Cooper]

On 16/12/2020 14:14, Jason Andryuk wrote:
> On Tue, Dec 15, 2020 at 5:22 PM Chris Rogers <crogers122@gmail.com> wrote:
>> Hopefully I can provide a little more context.  Here is a link to the patch:
>>
>> https://github.com/OpenXT/xenclient-oe/blob/master/recipes-extended/xen/files/libxl-fix-reboot.patch
>>
>> The patch is a bit mis-named.  It does not implement XEN_DOMCTL_SENDTRIGGER_RESET.  It's just a workaround to handle the missing RESET implementation.
>>
>> Its purpose is to make an HVM guest "reboot" regardless of whether PV tools have been installed and the xenstore interface is listening or not.  From the client perspective that OpenXT is concerned with, this is for ease-of-use for working with HVM guests before PV tools are installed.  To summarize the flow of the patch:
>>
>> - User input causes high level toolstack, xenmgr, to do xl reboot <domid>
>> - libxl hits "PV interface not available", so it tries the fallback ACPI reset trigger (but that's not implemented in domctl)
>> - therefore, the patch changes the RESET trigger to POWER trigger, and sets a 'reboot' flag
>> - when the xl create process handles the domain_death event for LIBXL_SHUTDOWN_REASON_POWEROFF, we check for our 'reboot' flag.
>> - It's set, so we set "action" as if we came from a real restart, which makes the xl create process take the 'goto start' codepath to rebuild the domain.
>>
>> I think we'd like to get rid of this patch, but at the moment I don't have any code or a design to propose that would implement the XEN_DOMCTL_SENDTRIGGER_RESET.
> I'm not sure it's possible to implement one.  Does an ACPI
> reset/reboot button exist?  And then you'd have the problem that the
> guest needs to be configured to react to the button.

The ACPI spec has two signals as far as this goes. "the user pressed the
power button" and "the user {pressed the suspend button, closed the
laptop lid}".  Neither are useful for VMs typically, because default OS
settings do the wrong thing.

The mystery to unravel here is why xl is issuing an erroneous hypercall.

It is very unlikely that we will have dropped
XEN_DOMCTL_SENDTRIGGER_RESET from Xen, but I suppose its possible.  It's
definitely weird that we have it in the interface and unimplemented.

It's also possible it was a copy&paste mistake when trying to implement
an interface consistent with `xm trigger`.

It is definitely concerning that we've got a piece of functionality like
this which clearly hasn't seen any testing upstream.

~Andrew

Re: [openxt-dev] Re: Follow up on libxl-fix-reboot.patch

[Stefano Stabellini]

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

On Wed, 16 Dec 2020, Andrew Cooper wrote:
> On 16/12/2020 14:14, Jason Andryuk wrote:
> > On Tue, Dec 15, 2020 at 5:22 PM Chris Rogers <crogers122@gmail.com> wrote:
> >> Hopefully I can provide a little more context.  Here is a link to the patch:
> >>
> >> https://github.com/OpenXT/xenclient-oe/blob/master/recipes-extended/xen/files/libxl-fix-reboot.patch
> >>
> >> The patch is a bit mis-named.  It does not implement XEN_DOMCTL_SENDTRIGGER_RESET.  It's just a workaround to handle the missing RESET implementation.
> >>
> >> Its purpose is to make an HVM guest "reboot" regardless of whether PV tools have been installed and the xenstore interface is listening or not.  From the client perspective that OpenXT is concerned with, this is for ease-of-use for working with HVM guests before PV tools are installed.  To summarize the flow of the patch:
> >>
> >> - User input causes high level toolstack, xenmgr, to do xl reboot <domid>
> >> - libxl hits "PV interface not available", so it tries the fallback ACPI reset trigger (but that's not implemented in domctl)
> >> - therefore, the patch changes the RESET trigger to POWER trigger, and sets a 'reboot' flag
> >> - when the xl create process handles the domain_death event for LIBXL_SHUTDOWN_REASON_POWEROFF, we check for our 'reboot' flag.
> >> - It's set, so we set "action" as if we came from a real restart, which makes the xl create process take the 'goto start' codepath to rebuild the domain.
> >>
> >> I think we'd like to get rid of this patch, but at the moment I don't have any code or a design to propose that would implement the XEN_DOMCTL_SENDTRIGGER_RESET.
> > I'm not sure it's possible to implement one.  Does an ACPI
> > reset/reboot button exist?  And then you'd have the problem that the
> > guest needs to be configured to react to the button.

Looking at the patch, it is difficult to suggest anything better. The
only thing I could think of would be to force shutdown_reason to be
"reboot" from xl_vmcontrol.c:reboot_domain. To do that, we would have to
be careful not to overwrite it in domain_death_xswatch_callback.

I am not sure if it would be actually a lot better.


> The ACPI spec has two signals as far as this goes. "the user pressed the
> power button" and "the user {pressed the suspend button, closed the
> laptop lid}".  Neither are useful for VMs typically, because default OS
> settings do the wrong thing.
> 
> The mystery to unravel here is why xl is issuing an erroneous hypercall.
> 
> It is very unlikely that we will have dropped
> XEN_DOMCTL_SENDTRIGGER_RESET from Xen, but I suppose its possible.  It's
> definitely weird that we have it in the interface and unimplemented.
> 
> It's also possible it was a copy&paste mistake when trying to implement
> an interface consistent with `xm trigger`.
> 
> It is definitely concerning that we've got a piece of functionality like
> this which clearly hasn't seen any testing upstream.

Indeed. I think we should fix this in 4.15, one way or another.