| Message ID | 1466651824-6964-4-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
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. Or could we add some codes to check the arch and decide where to store it? Thanks,
Hi Shannon, On 23/06/16 04:16, 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), 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", &b_info->arch_arm.acpi, 0)) { > + libxl_defbool_set(&b_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,
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,
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 <shannon.zhao@linaro.org> >>>>>>>>>>>> + 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. > Ok, what name would you suggest? I would suggest b_info->u.acpi Regards,
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,
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.
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,
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,
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); + } + 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; /*