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

[Stefano Stabellini <stefano@xxxxxxxxxxx>]

On Fri, 15 Jul 2016, Shannon Zhao wrote:
> 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__)
> ?

I am not a Libxl maintainer anymore, but I think that should be OK or at
least it would be a step in the right direction.

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