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

[Hari Limaye <hari.limaye@xxxxxxx>]

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.'

> 
> 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`?

> 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)

> +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.

> +
> +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)
>  {

Many thanks,

Hari