Re: [PATCH v6 2/4] tools/xl: Allow compilation of 'xl resume' command on Arm

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Anthony,

On Thu, Jul 24, 2025 at 5:01 PM Anthony PERARD <anthony@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Jul 24, 2025 at 12:40:57PM +0300, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > The "xl resume" command was previously excluded from Arm builds because
> > system suspend/resume (e.g., SYSTEM_SUSPEND via vPSCI) was not
> > implemented. On x86, this command is used for ACPI S3 suspend/resume.
> >
> > This change enables compilation of `xl resume` on Arm regardless of the
> > underlying implementation status, making the tool available for testing
> > and future feature support. The relevant libxl infrastructure and handler
> > functions are already present and usable.
> >
> > The macro `LIBXL_HAVE_NO_SUSPEND_RESUME` has been renamed to
> > `LIBXL_HAVE_NO_SUSPEND` to better reflect the updated semantics.
> >
> > Note: This does not imply full system suspend/resume support on Arm.
> >       The `xl suspend` command still does not work on Arm platforms.
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> > Changes in v6:
> > - Renamed macro from LIBXL_HAVE_NO_SUSPEND_RESUME to LIBXL_HAVE_NO_SUSPEND
> >   to better reflect the scope of this change
> > - Applied cosmetic changes based on review feedback
> > ---
> >  tools/include/libxl.h     |  5 ++---
> >  tools/xl/xl.h             | 10 +++++-----
> >  tools/xl/xl_cmdtable.c    |  8 ++++----
> >  tools/xl/xl_migrate.c     |  4 ++--
> >  tools/xl/xl_saverestore.c |  4 ++--
> >  tools/xl/xl_vmcontrol.c   | 14 +++++++-------
> >  6 files changed, 22 insertions(+), 23 deletions(-)
> >
> > diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> > index d6b6e5d2dd..632264912a 100644
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
> > @@ -1128,17 +1128,16 @@ typedef struct libxl__ctx libxl_ctx;
> >  #define LIBXL_HAVE_SIGCHLD_SHARING 1
> >
> >  /*
> > - * LIBXL_HAVE_NO_SUSPEND_RESUME
> > + * LIBXL_HAVE_NO_SUSPEND
> >   *
> >   * Is this is defined then the platform has no support for saving,
> >   * restoring or migrating a domain. In this case the related functions
> >   * should be expected to return failure. That is:
> >   *  - libxl_domain_suspend
> > - *  - libxl_domain_resume
> >   *  - libxl_domain_remus_start
> >   */
> >  #if defined(__arm__) || defined(__aarch64__)
> > -#define LIBXL_HAVE_NO_SUSPEND_RESUME 1
> > +#define LIBXL_HAVE_NO_SUSPEND 1
> >  #endif
>
> I'm sorry, if you remove LIBXL_HAVE_NO_SUSPEND_RESUME, you have to
> implement all the function listed. I'm pretty sure `libvirt` isn't going
> to build (on arm) if you remove that macro... Actually, libvirt is going
> to build, it's going to expect migration to work, and probably allow to
> try to migrate Arm VMs instead of bailing out early.
>
> I wonder what this LIBXL_HAVE_NO_SUSPEND_RESUME is for, since you don't
> make any changes to libxl (tools/libs/light), but only to program that
> make use of it.
>
> Looking at 3ac3817762d1 ("xl: suppress suspend/resume functions on platforms 
> which do not support it.")
>     
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=3ac3817762d1a8b39fa45998ec8c40cabfcfc802
> it seems the real purpose was just an hint that migrate/suspend/saving
> aren't going to work on that platform.
>
> Looks like `xl resume` is a fairly new command which only makes use if
> libxl_domain_resume outside of migration, but the macro
> LIBXL_HAVE_NO_SUSPEND_RESUME was mostly a hint that migration doesn't
> work.
>
> So I think moving the `xl resume` command out of
> LIBXL_HAVE_NO_SUSPEND_RESUME would be good enough for this patch,
> without touching libxl.h.

Got it. I'll revert the patch to the previous version.
Thanks for the review and clarification -- much appreciated!

>
> Cheers,
>
> --
> Anthony PERARD

Best regards,
Mykola