Re: [Xen-devel] [PATCH v2 11/17] libxl/arm: Construct ACPI DSDT table

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/27/2016 06:29 AM, Julien Grall wrote:
> (CC Boris and Doug)
>
> Hi Shannon,
>
> On 27/06/16 07:01, Shannon Zhao wrote:
> > On 2016/6/24 1:03, Julien Grall wrote:
> > > On 23/06/16 04:16, Shannon Zhao wrote:
> > >
> > > [...]
> > >
> > > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> > > > index 264b6ef..5347480 100644
> > > > --- a/tools/libxl/Makefile
> > > > +++ b/tools/libxl/Makefile
> > > > @@ -77,7 +77,29 @@ endif
> > > >
> > > >    LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
> > > >    LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
> > > > libxl_libfdt_compat.o
> > > > -LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o
> > > > +LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_acpi.o libxl_dsdt_anycpu_arm.o
> > > > +
> > > > +vpath iasl $(PATH)
> > > > +libxl_mk_dsdt_arm: libxl_mk_dsdt_arm.c
> > > > +    $(CC) $(CFLAGS) -o $@ libxl_mk_dsdt_arm.c
> > > > +
> > > > +libxl_dsdt_anycpu_arm.asl: libxl_empty_dsdt_arm.asl libxl_mk_dsdt_arm
> > > > +    awk 'NR > 1 {print s} {s=$$0}' $< > $@
> > > > +    ./libxl_mk_dsdt_arm >> $@
> > > > +
> > > > +libxl_dsdt_anycpu_arm.c: %.c: iasl %.asl
> > > > +    iasl -vs -p $* -tc $*.asl
> > > > +    sed -e 's/AmlCode/$*/g' $*.hex >$@
> > > > +    echo "int $*_len=sizeof($*);" >>$@
> > > > +    rm -f $*.aml $*.hex
> > > > +
> > > I don't like the idea to add iasl as a dependency for all ARM 
> > > platforms.
> > > For instance ARMv7 platform will not use ACPI, but we still ask 
> > > users to
> > > install iasl. So I think we should allow the user to opt-in/opt-out for
> > > ACPI.
> > >
> > > Any opinions?
> > I agree. But how to exclude for ARMv7. I notice it only has the option
> > CONFIG_ARM which doesn't distinguish ARM32 and ARM64.
> I am not sure if we plan to introduce Kconfig for tools. If not, you 
> can add an option to the configure to enable/disable ACPI for guest.
> This would be gated by the presence of "iasl".
>
> [...]
>
> > > > diff --git a/tools/libxl/libxl_mk_dsdt_arm.c
> > > > b/tools/libxl/libxl_mk_dsdt_arm.c
> > > > new file mode 100644
> > > > index 0000000..96fadbd
> > > > --- /dev/null
> > > > +++ b/tools/libxl/libxl_mk_dsdt_arm.c
> > > Can we share the code from tools/firmware/acpi/mk_dsdt.c?
> > Yeah, we can share push_block(), pop_block() stmt() and indent() but the
> > main() function is totally different since there are only the processor
> > device objects for ARM DSDT but there are many other things in x86.
> >
> > I think that since Boris will move the codes under
> > tools/firmware/hvmloader/acpi to other place, after that we could see
> > how to share codes then.
> I would prefer if we discuss about it now in order to avoid code 
> duplication (I have CCed Boris).
> For instance we can create a new directory under tools for mk_dsdt.c. 
> The main could be different, although it might be possible to gate ARM 
> options, and the rest of the code would be shared.

So I think we decided earlier to keep ARM and x86 ACPI builders 
separate, at least for now. However, looking at the Makefile and mk_dsdt 
I wonder whether it would make sense to put the builders in the same 
directory (I am currently using tools/libacpi) so that those two files 
can be kept common as much as possible, with the sources being 
different. E.g. something like
tools/libacpi:
    Makefile
    mk_dsdt.c
    acpi_x86.[ch]
    acpi_arm.[ch]
    *asl
    etc.

The objects will be built in tools/libxl (there will be no libacpi.so) 
but the infrastructure and sources will live together.
-boris

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