Re: [Xen-devel] [PATCH v4 01/16] tools/libxl: Add an unified configuration option for ACPI

[Julien Grall <julien.grall@xxxxxxx>]

Hi Shannon,

On 16/08/16 11:24, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
>
> Since the existing configuration option "u.hvm.acpi" is x86 specific and
> we want to reuse it on ARM as well, add a unified option "acpi" for
> x86 and ARM, and for ARM it's disabled by default.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_create.c  | 9 ++++++++-
>  tools/libxl/libxl_dm.c      | 6 ++++--
>  tools/libxl/libxl_types.idl | 4 ++++
>  tools/libxl/xl_cmdimpl.c    | 2 +-
>  4 files changed, 17 insertions(+), 4 deletions(-)
I think you need to update docs/man/xl.cfg.pod.5.in to mention the 
difference between x86 and ARM.
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 08822e3..3043b1f 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -215,6 +215,12 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>      if (!b_info->event_channels)
>          b_info->event_channels = 1023;
>
> +#if defined(__arm__) || defined(__aarch64__)
> +    libxl_defbool_setdefault(&b_info->acpi, false);
> +#else
> +    libxl_defbool_setdefault(&b_info->acpi, true);
> +#endif
> +
>      switch (b_info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>          if (b_info->shadow_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -454,7 +460,8 @@ int libxl__domain_build(libxl__gc *gc,
>          localents = libxl__calloc(gc, 9, sizeof(char *));
>          i = 0;
>          localents[i++] = "platform/acpi";
> -        localents[i++] = libxl_defbool_val(info->u.hvm.acpi) ? "1" : "0";
> +        localents[i++] = (libxl_defbool_val(info->acpi) &&
> +                         libxl_defbool_val(info->u.hvm.acpi)) ? "1" : "0";
It feels a bit weird to handle the backward compatibility directly in 
the code. It means that you have to do it everywhere hvm.acpi is used.
Can't you handle the backward compatibility directly in 
libxl__domain_build_info_setdefault?
>          localents[i++] = "platform/acpi_s3";
>          localents[i++] = libxl_defbool_val(info->u.hvm.acpi_s3) ? "1" : "0";
>          localents[i++] = "platform/acpi_s4";
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index de16a59..12e4084 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -583,7 +583,8 @@ static int libxl__build_device_model_args_old(libxl__gc *gc,
>          if (b_info->u.hvm.soundhw) {
>              flexarray_vappend(dm_args, "-soundhw", b_info->u.hvm.soundhw, 
> NULL);
>          }
> -        if (libxl_defbool_val(b_info->u.hvm.acpi)) {
> +        if (libxl_defbool_val(b_info->acpi) &&
> +            libxl_defbool_val(b_info->u.hvm.acpi)) {
>              flexarray_append(dm_args, "-acpi");
>          }
>          if (b_info->max_vcpus > 1) {
> @@ -1204,7 +1205,8 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>          if (b_info->u.hvm.soundhw) {
>              flexarray_vappend(dm_args, "-soundhw", b_info->u.hvm.soundhw, 
> NULL);
>          }
> -        if (!libxl_defbool_val(b_info->u.hvm.acpi)) {
> +        if (!(libxl_defbool_val(b_info->acpi) &&
> +             libxl_defbool_val(b_info->u.hvm.acpi))) {
>              flexarray_append(dm_args, "-no-acpi");
>          }
>          if (b_info->max_vcpus > 1) {
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 98bfc3a..a02446f 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -494,11 +494,15 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      # Note that the partial device tree should avoid to use the phandle
>      # 65000 which is reserved by the toolstack.
>      ("device_tree",      string),
> +    ("acpi",             libxl_defbool),
>      ("u", KeyedUnion(None, libxl_domain_type, "type",
>                  [("hvm", Struct(None, [("firmware",         string),
>                                         ("bios",             libxl_bios_type),
>                                         ("pae",              libxl_defbool),
>                                         ("apic",             libxl_defbool),
> +                                       # The following acpi field is 
> deprecated.
> +                                       # Please use the unified acpi field 
> above
> +                                       # which works for both x86 and ARM.
>                                         ("acpi",             libxl_defbool),
>                                         ("acpi_s3",          libxl_defbool),
>                                         ("acpi_s4",          libxl_defbool),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1d06598..be17702 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1547,6 +1547,7 @@ static void parse_config_data(const char *config_source,
>      b_info->cmdline = parse_cmdline(config);
>
>      xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
> +    xlu_cfg_get_defbool(config, "acpi", &b_info->acpi, 0);
>
>      switch(b_info->type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
> @@ -1576,7 +1577,6 @@ static void parse_config_data(const char *config_source,
>
>          xlu_cfg_get_defbool(config, "pae", &b_info->u.hvm.pae, 0);
>          xlu_cfg_get_defbool(config, "apic", &b_info->u.hvm.apic, 0);
> -        xlu_cfg_get_defbool(config, "acpi", &b_info->u.hvm.acpi, 0);
>          xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
>          xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
>          xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel