Re: [PATCH] tools/xl: add suspend-to-ram and resume subcommands

[Jason Andryuk <jason.andryuk@xxxxxxx>]

Hi Zithro,

On 2024-04-24 10:03, Jason Andryuk wrote:
> On 2024-02-29 02:00, zithro / Cyril Rébert wrote:
> > The xl command doesn't provide suspend/resume, so add them :
> >    xl suspend-to-ram <Domain>
> >    xl resume <Domain>
> >
> > This patch follows a discussion on XenDevel: when you want the
> > virtualized equivalent of "sleep"-ing a host, it's better to
> > suspend/resume than to pause/unpause a domain.
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Suggested-by: Marek Marczykowski-Górecki 
> > <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> You should include you S-o-B here to certify your patch under the 
> Developer's Certificate of Origin.  You can read what that means in 
> CONTRIBUTING.  tl;dr: You are stating you can make the open source 
> contribution.
I'd like to re-submit this while retaining your authorship.  Are you 
able to provide your S-o-B?
I'll do all the follow ups, but I need your S-o-B for me to make the 
submission.
> I tested this with a PVH and HVM guest.  suspend-to-ram and resume seem 
> to function properly.  The VCPUs stop, but the domain & qemu remain. 
> Resume works - the VCPUs start running again.
> However, the domain destruction seems to hang on poweroff.  The VM 
> transitions to powered off - state shutdown - but the domain and QEMU 
> instance are not cleaned up.
> If I power off without a suspend-to-ram, everything is cleaned up properly.
>
> Have you seen this?  It's not your code, but I guess something with 
> libxl or qemu.
I've found the issue.  xl exits when the domain suspends.  When the 
domain finally shuts down, xl isn't there to perform the remaining cleanup.
> > ---
> > - Tested on v4.17, x86
> > - the function "libxl_domain_resume" is called like libvirt does, so
> >    using a "co-operative resume", but I don't know what it means (:
> > - there may be a problem with the words resume <-> restore, like
> >    for "LIBXL_HAVE_NO_SUSPEND_RESUME"
> > - for the docs, I only slightly adapted a copy/paste from xl pause ...
> > ---
> > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> > index 98f6bd2e76..ba45f89c5a 100644
> > --- a/tools/xl/xl_vmcontrol.c
> > +++ b/tools/xl/xl_vmcontrol.c
> > @@ -42,6 +42,16 @@ static void unpause_domain(uint32_t domid)
> >       libxl_domain_unpause(ctx, domid, NULL);
> >   }
> > +static void suspend_domain_toram(uint32_t domid)
> > +{
> > +    libxl_domain_suspend_only(ctx, domid, NULL);
> > +}
> > +
> > +static void resume_domain(uint32_t domid)
> > +{
> > +    libxl_domain_resume(ctx, domid, 1, NULL);
> > +}
> > +
> I would just inline these functions below.
I see you were following the existing style, so this may remain as you 
wrote it.
> >   static void destroy_domain(uint32_t domid, int force)
> >   {
> >       int rc;
> > @@ -82,6 +92,32 @@ int main_unpause(int argc, char **argv)
> >       return EXIT_SUCCESS;
> >   }
> > +int main_suspendtoram(int argc, char **argv)
> Maybe main_suspend_to_ram to be closer to the command line suspend-to-ram.
I'm thinking we may want to just use "suspend" for the command name and 
all the functions.
Thanks,
Jason