Re: [PATCH v5 2/4] tools/xl: allow resume command compilation for arm

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Hari,

On Thu, Jul 3, 2025 at 12:36 PM Hari Limaye <hari.limaye@xxxxxxx> wrote:
>
> Hi Mykola,
>
> > On Fri, Jun 27, 2025 at 01:51:31PM +0000, 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, the 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 use in
> > testing or for future support. The libxl infrastructure and handler
> > functions are already present and usable.
> >
> > Note: This does not imply full system suspend/resume support on ARM.
> >       "xl suspend" command still not work for arm platforms.
>
> NIT: Last sentence should perhaps be: 'The "xl suspend" command still
> does not work on Arm platforms.'

Thank you for pointing that out — I’ll reword it.

>
> >
> > Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> > ---
> >  tools/include/libxl.h   |  1 -
> >  tools/xl/xl.h           |  2 +-
> >  tools/xl/xl_cmdtable.c  |  2 +-
> >  tools/xl/xl_vmcontrol.c | 12 ++++++------
> >  4 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> > index a8704e0268..0fda8bb616 100644
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
> > @@ -1134,7 +1134,6 @@ typedef struct libxl__ctx libxl_ctx;
> >   * 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__)
>
> The Macro being documented above, and defined below, this ^^^ section of
> the diff, is called `LIBXL_HAVE_NO_SUSPEND_RESUME`. Now it no longer
> indicates lack of support for libxl_domain_resume is it better renamed
> something like `LIBXL_HAVE_NO_SUSPEND`?

Sure, that makes sense — I’ll rename it to LIBXL_HAVE_NO_SUSPEND to
better reflect its purpose.

>
> > diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> > index 45745f0dbb..5b0a481456 100644
> > --- a/tools/xl/xl.h
> > +++ b/tools/xl/xl.h
> > @@ -130,8 +130,8 @@ int main_migrate_receive(int argc, char **argv);
> >  int main_save(int argc, char **argv);
> >  int main_migrate(int argc, char **argv);
> >  int main_suspend(int argc, char **argv);
> > -int main_resume(int argc, char **argv);
> >  #endif
>
> NIT: Could take the opportunity to add comment after `#endif`?
> + #endif /* LIBXL_HAVE_NO_SUSPEND_RESUME */
> (Or LIBXL_HAVE_NO_SUSPEND if renamed)

Sure — I’ll add a comment after the #endif, matching the updated macro name.

>
> > +int main_resume(int argc, char **argv);
> >  int main_dump_core(int argc, char **argv);
> >  int main_pause(int argc, char **argv);
> >  int main_unpause(int argc, char **argv);
> > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> > index 06a0039718..4f662a4189 100644
> > --- a/tools/xl/xl_cmdtable.c
> > +++ b/tools/xl/xl_cmdtable.c
> > @@ -198,12 +198,12 @@ const struct cmd_spec cmd_table[] = {
> >        "Suspend a domain to RAM",
> >        "<Domain>",
> >      },
> > +#endif
>
> NIT: Same as above.
>
> >      { "resume",
> >        &main_resume, 0, 1,
> >        "Resume a domain from RAM",
> >        "<Domain>",
> >      },
> > -#endif
> >      { "dump-core",
> >        &main_dump_core, 0, 1,
> >        "Core dump a domain",
> > diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> > index c813732838..ebacde5482 100644
> > --- a/tools/xl/xl_vmcontrol.c
> > +++ b/tools/xl/xl_vmcontrol.c
> > @@ -38,11 +38,6 @@ static void suspend_domain(uint32_t domid)
> >      libxl_domain_suspend_only(ctx, domid, NULL);
> >  }
> >
> > -static void resume_domain(uint32_t domid)
> > -{
> > -    libxl_domain_resume(ctx, domid, 1, NULL);
> > -}
> > -
> >  int main_suspend(int argc, char **argv)
> >  {
> >      int opt;
> > @@ -55,6 +50,12 @@ int main_suspend(int argc, char **argv)
> >
> >      return EXIT_SUCCESS;
> >  }
> > +#endif
>
> NIT: Same as above.

okay

>
> > +
> > +static void resume_domain(uint32_t domid)
> > +{
> > +    libxl_domain_resume(ctx, domid, 1, NULL);
> > +}
> >
> >  int main_resume(int argc, char **argv)
> >  {
> > @@ -68,7 +69,6 @@ int main_resume(int argc, char **argv)
> >
> >      return EXIT_SUCCESS;
> >  }
> > -#endif
> >
> >  static void pause_domain(uint32_t domid)
> >  {

Thank you for reviewing this patch.

Best regards,
Mykola


>
> Many thanks,
>
> Hari