Re: [PATCH v2 03/17] libxl/arm: Add a configuration option for ARM DomU ACPI

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Jun 27, 2016 at 11:40:32AM +0100, Julien Grall wrote:
> Hi Shannon,
> 
> On 23/06/16 15:34, Shannon Zhao wrote:
> >On 2016年06月23日 21:39, Stefano Stabellini wrote:
> >>On Thu, 23 Jun 2016, Shannon Zhao wrote:
> >>>>From: Shannon Zhao <shannon.zhao@linaro.org>
> >>>>
> >>>>Add a configuration option for ARM DomU so that user can deicde to use
> >>>>ACPI or not. This option is defaultly false.
> >>>>
> >>>>Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >>>>---
> >>>>  tools/libxl/libxl_arm.c       | 3 +++
> >>>>  tools/libxl/libxl_types.idl   | 1 +
> >>>>  tools/libxl/xl_cmdimpl.c      | 4 ++++
> >>>>  xen/include/public/arch-arm.h | 1 +
> >>>>  4 files changed, 9 insertions(+)
> >>>>
> >>>>diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >>>>index 8f15d9b..cc5a717 100644
> >>>>--- a/tools/libxl/libxl_arm.c
> >>>>+++ b/tools/libxl/libxl_arm.c
> >>>>@@ -77,6 +77,9 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >>>>          return ERROR_FAIL;
> >>>>      }
> >>>>
> >>>>+    xc_config->acpi = libxl_defbool_val(d_config->b_info.arch_arm.acpi)
> >>>>+                      ? true : false;
> >>>>+
> >>>>      return 0;
> >>>>  }
> >>>>
> >>>>diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >>>>index ef614be..426b868 100644
> >>>>--- a/tools/libxl/libxl_types.idl
> >>>>+++ b/tools/libxl/libxl_types.idl
> >>>>@@ -560,6 +560,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>>>
> >>>>
> >>>>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> >>>>+                               ("acpi", libxl_defbool),
> >>>>                                ])),
> >>>>
> >>>>      ], dir=DIR_IN
> >>>>diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> >>>>index 6459eec..0634ffa 100644
> >>>>--- a/tools/libxl/xl_cmdimpl.c
> >>>>+++ b/tools/libxl/xl_cmdimpl.c
> >>>>@@ -2506,6 +2506,10 @@ skip_usbdev:
> >>>>          }
> >>>>       }
> >>>>
> >>>>+    if (xlu_cfg_get_defbool(config, "acpi", &b_info->arch_arm.acpi, 0)) {
> >>>>+        libxl_defbool_set(&b_info->arch_arm.acpi, 0);
> >>>>+    }
> >>We cannot share the existing code to parse the acpi paramter because
> >>that is saved in b_info->u.hvm.acpi, right?
> >Yes.
> >>It's a pity. I wonder if we
> >>could refactor the existing code so that we can actually share the acpi
> >>parameter between x86 and arm.
> >>
> >I have no idea about this since I'm not familiar with this. But is there
> >any downsides of current way? Because for x86, it will use
> >b_info->u.hvm.acpi and for ARM it will use b_info->arch_arm.acpi. I
> >think they don't conflict even though we store it at two places.
> 
> Yes, there is a downside.  Toolstack, such as libvirt, would need to have
> separate code for x86 and ARM in order to enable/disable ACPI.
> 
> I would introduce a new generic acpi parameters, deprecate
> b_info->u.hvm.acpi. Ian, Stefano, Wei, any opinions?
> 

Yeah, we can deprecate that field. But we need to take care to not break
users of the old field.

Wei.

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