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

[Shannon Zhao <zhaoshenglong@xxxxxxxxxx>]

On 2016/7/15 16:00, Shannon Zhao wrote:
> 
> 
> On 2016/7/13 18:03, Julien Grall wrote:
>>
>>
>> On 13/07/2016 10:48, Shannon Zhao wrote:
>>>
>>>
>>> On 2016/7/13 17:20, Julien Grall wrote:
>>>> On 13/07/2016 08:54, Shannon Zhao wrote:
>>>>> On 2016/7/12 19:33, Wei Liu wrote:
>>>>>> On Tue, Jul 12, 2016 at 10:22:39AM +0100, Julien Grall wrote:
>>>>>> [...]
>>>>>>>>> Yeah, we can deprecate that field. But we need to take care to not
>>>>>>>>> break
>>>>>>>>> users of the old field.
>>>>>>>> Ok, what name would you suggest?
>>>>>>>
>>>>>>> I would suggest b_info->u.acpi
>>>>>>>
>>>>>>
>>>>>> b_info->acpi would be more appropriate.
>>>>>>
>>>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>>>> index ef614be..a57823d 100644
>>>>>> --- a/tools/libxl/libxl_types.idl
>>>>>> +++ b/tools/libxl/libxl_types.idl
>>>>>> @@ -494,11 +494,16 @@ 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),
>>>>>>
>>>>>>
>>>>>> And then:
>>>>>>
>>>>>> 1. modify xl to set the new field.
>>>>>> 2. modify libxl to handle compatibility: user of the old field should
>>>>>>    continue to work.
>>>>>>
>>>>> I found that the default value of acpi is true on x86. But we decided
>>>>> before it's should be false by default on ARM. How to deal with this?
>>>>> Julien, Stefano, can we make acpi true by default?
>>>>
>>>> I already said that I am not in favor of making ACPI true by default at
>>>> least for ARM 32-bit guest.
>>>>
>>>> ARM 32-bit guest will not use ACPI, if we decide to enable it by
>>>> default, we will require the user to install iasl for nothing.
>>>>
>>>> IHMO, ACPI should be disabled by default for any ARM guests. Libxl can
>>>> take this decision easily.
>>> I know but here we want to unify the acpi option for x86 and ARM while
>>> on x86 it's true by default. What I want to ask is that how to
>>> distinguish x86 and ARM in libxl__domain_build_info_setdefault(), so we
>>> can assign acpi with different default value for x86 and ARM.
>>
>> By using #ifdef in the code?
> Maybe this could not work since CONFIG_ARM can not be accessed in libxl
> in current codes. I'm not sure why it can't work. Wei, do you have any
> suggestion?
> 
And is it ok to use
#if defined(__arm__) || defined(__aarch64__)
?

Thanks,
-- 
Shannon


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