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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxx> 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.