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

[Julien Grall <julien.grall@xxxxxxx>]

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