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 <[email protected]> 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.
