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

[Julien Grall <julien.grall@arm.com>]

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?

Regards,

-- 
Julien Grall

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