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

2016-07-19 Thread Ian Jackson 
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v2 03/17] libxl/arm: Add a 
configuration option for ARM DomU ACPI"):
...
> > >>> 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?

We normally try to deal with this kind of thing by separating the
arch-specific code into separate files, which are compiled as needed.

Maybe libxl__arch_domain_prepare_config is the right place ?

> > > 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.

I definitely don't want open-coded alternations like this.  If an
#ifdef is needed, a single feature macro should be (if necessary
invented) and tested.

But as I say I think this can probably be done with libxl_arch.h,
libxl_arm.c, libxl_x86.c, etc.

Thanks,
Ian.

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

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

2016-07-19 Thread Wei Liu 
On Mon, Jul 18, 2016 at 12:40:43PM -0700, Stefano Stabellini wrote:
[...]
> > #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.

Yes, I think that's the correct ifdefs to use.

Wei.

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

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

2016-07-18 Thread Stefano Stabellini 
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@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-07-15 Thread Shannon Zhao 


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@lists.xen.org
https://lists.xen.org/xen-devel

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

2016-07-15 Thread Shannon Zhao 


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?

Thanks,
-- 
Shannon


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

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

2016-07-13 Thread Julien Grall 



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?

Regards,

--
Julien Grall

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

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

2016-07-13 Thread Shannon Zhao 


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.

Thanks,
-- 
Shannon


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

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

2016-07-13 Thread Julien Grall 

Hi Shannon,

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.


Regards,

--
Julien Grall

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

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

2016-07-13 Thread Shannon Zhao 


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?

Thanks,
-- 
Shannon


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

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

2016-07-12 Thread Shannon Zhao 
On 2016年07月12日 22:33, Wei Liu wrote:
> On Tue, Jul 12, 2016 at 10:17:20PM +0800, Shannon Zhao wrote:
>> > On 2016年07月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 know this is a bit terse. Feel free to ask questions if you have any
>>> > > doubt.
>> > I'm not sure I understand correctly. While xl is always matching libxl,
>> > so can we just let xl set the new field and libxl to use the new field?
>> > To users, they will still use the configure option "acpi".
>> > 
> We need to distinguish between the library to control Xen (libxl) and
> the user of that library (xl). Xl is just one of the possibly users of
> libxl. For example, libvirt uses libxl APIs without involving xl at all,
> hence my second point.
Oh, I see. Thanks for the clarification.

-- 
Shannon

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

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

2016-07-12 Thread Wei Liu 
On Tue, Jul 12, 2016 at 10:17:20PM +0800, Shannon Zhao wrote:
> On 2016年07月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 know this is a bit terse. Feel free to ask questions if you have any
> > doubt.
> I'm not sure I understand correctly. While xl is always matching libxl,
> so can we just let xl set the new field and libxl to use the new field?
> To users, they will still use the configure option "acpi".
> 

We need to distinguish between the library to control Xen (libxl) and
the user of that library (xl). Xl is just one of the possibly users of
libxl. For example, libvirt uses libxl APIs without involving xl at all,
hence my second point.

Wei.

> Thanks,
> -- 
> Shannon

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

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

2016-07-12 Thread Shannon Zhao 
On 2016年07月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 know this is a bit terse. Feel free to ask questions if you have any
> doubt.
I'm not sure I understand correctly. While xl is always matching libxl,
so can we just let xl set the new field and libxl to use the new field?
To users, they will still use the configure option "acpi".

Thanks,
-- 
Shannon

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

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

2016-07-12 Thread Wei Liu 
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 know this is a bit terse. Feel free to ask questions if you have any
doubt.

Wei.

> Regards,
> 
> -- 
> Julien Grall

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

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

2016-07-12 Thread Julien Grall 

Hi Shannon,

On 12/07/16 04:40, Shannon Zhao wrote:



On 2016/7/7 23:30, Wei Liu wrote:

On Mon, Jun 27, 2016 at 11:40:32AM +0100, Julien Grall wrote:

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 
+if (xlu_cfg_get_defbool(config, "acpi", _info->arch_arm.acpi, 0)) {
+libxl_defbool_set(_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.

Ok, what name would you suggest?


I would suggest b_info->u.acpi

Regards,

--
Julien Grall

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

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

2016-07-11 Thread Shannon Zhao 


On 2016/7/7 23:30, Wei Liu wrote:
> 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 
>> > 
>> > 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 
>> > ---
>> >   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", 
>> > _info->arch_arm.acpi, 0)) {
>> > +libxl_defbool_set(_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.
Ok, what name would you suggest?

Thanks,
-- 
Shannon


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

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

2016-07-07 Thread Wei Liu 
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 
> 
> 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 
> ---
>   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", _info->arch_arm.acpi, 0)) {
> +libxl_defbool_set(_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

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

2016-06-27 Thread Julien Grall 

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 

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 
---
  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", _info->arch_arm.acpi, 0)) {
+libxl_defbool_set(_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

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

2016-06-23 Thread Julien Grall 

Hi Shannon,

On 23/06/16 04:16, Shannon Zhao wrote:

From: Shannon Zhao 

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 
---
  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),


If we are going towards a new field, you will need add a new define for 
advertising external toolstack of the presence of arch_arm.acpi in 
libxl.h (see LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION for instance).



])),

  ], 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", _info->arch_arm.acpi, 0)) {
+libxl_defbool_set(_info->arch_arm.acpi, 0);
+}
+
  xlu_cfg_destroy(config);
  }

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 870bc3b..05e4a58 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -306,6 +306,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
  struct xen_arch_domainconfig {
  /* IN/OUT */
  uint8_t gic_version;
+uint8_t acpi;


This structure is used to pass information to the hypervisor. I do not 
think the hypervisor has to know the domain is using ACPI.



  /* IN */
  uint32_t nr_spis;
  /*



Regards,

--
Julien Grall

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

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

2016-06-23 Thread Shannon Zhao 
On 2016年06月23日 21:39, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Shannon Zhao wrote:
>> > From: Shannon Zhao 
>> > 
>> > 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 
>> > ---
>> >  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", _info->arch_arm.acpi, 0)) {
>> > +libxl_defbool_set(_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.

Or could we add some codes to check the arch and decide where to store it?

Thanks,
-- 
Shannon

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

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

2016-06-23 Thread Stefano Stabellini 
On Thu, 23 Jun 2016, Shannon Zhao wrote:
> From: Shannon Zhao 
> 
> 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 
> ---
>  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", _info->arch_arm.acpi, 0)) {
> +libxl_defbool_set(_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? 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.


>  xlu_cfg_destroy(config);
>  }
>  
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 870bc3b..05e4a58 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -306,6 +306,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>  struct xen_arch_domainconfig {
>  /* IN/OUT */
>  uint8_t gic_version;
> +uint8_t acpi;
>  /* IN */
>  uint32_t nr_spis;
>  /*
> -- 
> 2.0.4
> 
> 

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

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

2016-06-22 Thread Shannon Zhao 
From: Shannon Zhao 

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 
---
 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", _info->arch_arm.acpi, 0)) {
+libxl_defbool_set(_info->arch_arm.acpi, 0);
+}
+
 xlu_cfg_destroy(config);
 }
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 870bc3b..05e4a58 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -306,6 +306,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 struct xen_arch_domainconfig {
 /* IN/OUT */
 uint8_t gic_version;
+uint8_t acpi;
 /* IN */
 uint32_t nr_spis;
 /*
-- 
2.0.4



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