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

[Stefano Stabellini <sstabellini@kernel.org>]

[-- 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.