| Message ID | 1466651824-6964-12-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
Hi, On 23/06/16 15:50, Stefano Stabellini wrote: > On Thu, 23 Jun 2016, Shannon Zhao wrote: >> diff --git a/tools/libxl/libxl_empty_dsdt_arm.asl b/tools/libxl/libxl_empty_dsdt_arm.asl >> new file mode 100644 >> index 0000000..005fa6a >> --- /dev/null >> +++ b/tools/libxl/libxl_empty_dsdt_arm.asl >> @@ -0,0 +1,22 @@ >> +/****************************************************************************** >> + * DSDT for Xen ARM DomU >> + * >> + * Copyright (c) 2004, Intel Corporation. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program; If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +DefinitionBlock ("DSDT.aml", "DSDT", 3, "XenARM", "Xen DSDT", 1) >> +{ >> + >> +} > > Why do we need C code to generate the "static" asl? Can't we just > manually writing the asl code here and get rid of libxl_mk_dsdt_arm.c? Whilst I agree that manually writing the asl code sounds more appealing, we need to write one node per processor. So currently this would be 128 nodes and this will likely increase in the future. Generating the asl has the advantage to be able to add new property in the processor node easily without having to modify one by one all the nodes. Regards,
Hi Shannon, 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? > +iasl: > + @echo > + @echo "ACPI ASL compiler (iasl) is needed" > + @echo "Download and install Intel ACPI CA from" > + @echo "http://acpica.org/downloads/" > + @echo > + @exit 1 It is really a pain to discover the dependency in the middle of a build. The presence of iasl should be done by the configure. > > libxl_arm_acpi.o: libxl_arm_acpi.c > $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c > index 353d774..45fc354 100644 > --- a/tools/libxl/libxl_arm_acpi.c > +++ b/tools/libxl/libxl_arm_acpi.c > @@ -54,6 +54,9 @@ enum { > NUMS, > }; > > +extern unsigned char libxl_dsdt_anycpu_arm[]; > +extern int libxl_dsdt_anycpu_arm_len; Not sure this is the right place to mention it, but I don't find the actual declaration. Both variables should be const and _hidden. Also, the *_len should be at least const int. > + > struct acpitable { > void *table; > size_t size; > @@ -256,6 +259,17 @@ static void make_acpi_fadt(libxl__gc *gc, struct xc_dom_image *dom) > dom->acpitable_size += ROUNDUP(acpitables[FADT].size, 3); > } > > +static void make_acpi_dsdt(libxl__gc *gc, struct xc_dom_image *dom) > +{ > + acpitables[DSDT].table = libxl__zalloc(gc, libxl_dsdt_anycpu_arm_len); > + memcpy(acpitables[DSDT].table, libxl_dsdt_anycpu_arm, > + libxl_dsdt_anycpu_arm_len); > + > + acpitables[DSDT].size = libxl_dsdt_anycpu_arm_len; > + /* Align to 64bit. */ > + dom->acpitable_size += ROUNDUP(acpitables[DSDT].size, 3); > +} > + > int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, > libxl__domain_build_state *state, > struct xc_dom_image *dom) > @@ -284,6 +298,7 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, > return rc; > > make_acpi_fadt(gc, dom); > + make_acpi_dsdt(gc, dom); > > return 0; > } > diff --git a/tools/libxl/libxl_arm_acpi.h b/tools/libxl/libxl_arm_acpi.h > index 9b58de6..b0fd9ce 100644 > --- a/tools/libxl/libxl_arm_acpi.h > +++ b/tools/libxl/libxl_arm_acpi.h > @@ -19,6 +19,8 @@ > > #include <xc_dom.h> > > +#define DOMU_MAX_VCPUS 128 > + I would rather define the maximum number of VCPUS in public/arch_arm.h to avoid defining the current number of vCPUs supported in multiple places. > int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, > libxl__domain_build_state *state, > struct xc_dom_image *dom); > diff --git a/tools/libxl/libxl_empty_dsdt_arm.asl b/tools/libxl/libxl_empty_dsdt_arm.asl > new file mode 100644 > index 0000000..005fa6a > --- /dev/null > +++ b/tools/libxl/libxl_empty_dsdt_arm.asl > @@ -0,0 +1,22 @@ > +/****************************************************************************** > + * DSDT for Xen ARM DomU > + * > + * Copyright (c) 2004, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program; If not, see <http://www.gnu.org/licenses/>. > + */ > + > +DefinitionBlock ("DSDT.aml", "DSDT", 3, "XenARM", "Xen DSDT", 1) > +{ > + > +} > 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? > @@ -0,0 +1,94 @@ > +#include <stdio.h> > +#include <stdarg.h> > +#include <stdint.h> > +#include <string.h> > +#include <stdlib.h> > +#include "libxl_arm_acpi.h" > + > +static unsigned int indent_level; > + > +static void indent(void) > +{ > + unsigned int i; > + for ( i = 0; i < indent_level; i++ ) > + printf(" "); > +} > + > +static __attribute__((format(printf, 2, 3))) > +void _stmt(const char *name, const char *fmt, ...) > +{ > + va_list args; > + > + indent(); > + printf("%s", name); > + > + if ( !fmt ) > + return; > + > + printf(" ( "); > + va_start(args, fmt); > + vprintf(fmt, args); > + va_end(args); > + printf(" )"); > +} > + > +#define stmt(n, f, a...) \ > + do { \ > + _stmt(n, f , ## a ); \ > + printf("\n"); \ > + } while (0) > + > +#define push_block(n, f, a...) \ > + do { \ > + _stmt(n, f , ## a ); \ > + printf(" {\n"); \ > + indent_level++; \ > + } while (0) > + > +static void pop_block(void) > +{ > + indent_level--; > + indent(); > + printf("}\n"); > +} > + > +int main(int argc, char **argv) > +{ > + unsigned int cpu, max_cpus = DOMU_MAX_VCPUS; > + > + /**** DSDT DefinitionBlock start ****/ > + /* (we append to existing DSDT definition block) */ > + indent_level++; > + > + /**** Processor start ****/ > + push_block("Scope", "\\_SB"); > + > + /* Define processor objects and control methods. */ > + for ( cpu = 0; cpu < max_cpus; cpu++) > + { > + push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu); > + > + stmt("Name", "_HID, \"ACPI0007\""); > + stmt("Name", "_UID, %d", cpu); > + > + pop_block(); > + } > + > + pop_block(); > + /**** Processor end ****/ > + > + pop_block(); > + /**** DSDT DefinitionBlock end ****/ > + > + return 0; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > Regards,
(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. Regards,
Hi Shannon, On 01/07/16 08:58, Shannon Zhao wrote: > On 2016/6/30 2:58, Boris Ostrovsky wrote: >> git://oss.oracle.com/git/bostrovs/xen.git:acpi_v1_wip > Hi Boris, > > Thanks a lot! > While I found there is a compiling problem of the last patch. > > undefined reference to `libxl__dom_load_acpi' > collect2: error: ld returned 1 exit status > > I think you should define the corresponding function for ARM. And this function should contain arch in the name to show it is arch-specific. However, I am not sure why libxl__arch_domain_{init,finalize}_hw_description were not re-used. > > Julien, Stefano, > > Looks like Boris is going to use libxl__dom_load_acpi to load ACPI > tables, do we need to use it for ARM as well or load the tables in > xc_dom_build_image()? I gave a quick look to the code and it is very similar to ARM for the allocation of the memory and copying the ACPI tables. So we may want to share some bits here. I have the feeling it would be better to do it in libxc where all the guest memory is handled. But I will let the tools maintainers decide where this should be done. Regards,
Hi Boris, On 01/07/16 15:42, Boris Ostrovsky wrote: > On 07/01/2016 06:18 AM, Julien Grall wrote: >> On 01/07/16 08:58, Shannon Zhao wrote: >>> On 2016/6/30 2:58, Boris Ostrovsky wrote: >>>> git://oss.oracle.com/git/bostrovs/xen.git:acpi_v1_wip [...] >>> Looks like Boris is going to use libxl__dom_load_acpi to load ACPI >>> tables, do we need to use it for ARM as well or load the tables in >>> xc_dom_build_image()? >> >> I gave a quick look to the code and it is very similar to ARM for the >> allocation of the memory and copying the ACPI tables. So we may want >> to share some bits here. >> >> I have the feeling it would be better to do it in libxc where all the >> guest memory is handled. But I will let the tools maintainers decide >> where this should be done. > > Haven't we decided that libxl is the better place? The discussion was related to building the ACPI tables. I consider loading the blob into memory as distinct step. Currently, the memory layout and loading blobs into the memory is handled by libxc (see xc_dom_build_image). It would be rather strange to load the ACPI tables in libxl. For what is worth, on ARM the device tree is built by libxl and loaded by libxc. Regards,
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 + +iasl: + @echo + @echo "ACPI ASL compiler (iasl) is needed" + @echo "Download and install Intel ACPI CA from" + @echo "http://acpica.org/downloads/" + @echo + @exit 1 libxl_arm_acpi.o: libxl_arm_acpi.c $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index 353d774..45fc354 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -54,6 +54,9 @@ enum { NUMS, }; +extern unsigned char libxl_dsdt_anycpu_arm[]; +extern int libxl_dsdt_anycpu_arm_len; + struct acpitable { void *table; size_t size; @@ -256,6 +259,17 @@ static void make_acpi_fadt(libxl__gc *gc, struct xc_dom_image *dom) dom->acpitable_size += ROUNDUP(acpitables[FADT].size, 3); } +static void make_acpi_dsdt(libxl__gc *gc, struct xc_dom_image *dom) +{ + acpitables[DSDT].table = libxl__zalloc(gc, libxl_dsdt_anycpu_arm_len); + memcpy(acpitables[DSDT].table, libxl_dsdt_anycpu_arm, + libxl_dsdt_anycpu_arm_len); + + acpitables[DSDT].size = libxl_dsdt_anycpu_arm_len; + /* Align to 64bit. */ + dom->acpitable_size += ROUNDUP(acpitables[DSDT].size, 3); +} + int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state, struct xc_dom_image *dom) @@ -284,6 +298,7 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, return rc; make_acpi_fadt(gc, dom); + make_acpi_dsdt(gc, dom); return 0; } diff --git a/tools/libxl/libxl_arm_acpi.h b/tools/libxl/libxl_arm_acpi.h index 9b58de6..b0fd9ce 100644 --- a/tools/libxl/libxl_arm_acpi.h +++ b/tools/libxl/libxl_arm_acpi.h @@ -19,6 +19,8 @@ #include <xc_dom.h> +#define DOMU_MAX_VCPUS 128 + int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, libxl__domain_build_state *state, struct xc_dom_image *dom); diff --git a/tools/libxl/libxl_empty_dsdt_arm.asl b/tools/libxl/libxl_empty_dsdt_arm.asl new file mode 100644 index 0000000..005fa6a --- /dev/null +++ b/tools/libxl/libxl_empty_dsdt_arm.asl @@ -0,0 +1,22 @@ +/****************************************************************************** + * DSDT for Xen ARM DomU + * + * Copyright (c) 2004, Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program; If not, see <http://www.gnu.org/licenses/>. + */ + +DefinitionBlock ("DSDT.aml", "DSDT", 3, "XenARM", "Xen DSDT", 1) +{ + +} 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 @@ -0,0 +1,94 @@ +#include <stdio.h> +#include <stdarg.h> +#include <stdint.h> +#include <string.h> +#include <stdlib.h> +#include "libxl_arm_acpi.h" + +static unsigned int indent_level; + +static void indent(void) +{ + unsigned int i; + for ( i = 0; i < indent_level; i++ ) + printf(" "); +} + +static __attribute__((format(printf, 2, 3))) +void _stmt(const char *name, const char *fmt, ...) +{ + va_list args; + + indent(); + printf("%s", name); + + if ( !fmt ) + return; + + printf(" ( "); + va_start(args, fmt); + vprintf(fmt, args); + va_end(args); + printf(" )"); +} + +#define stmt(n, f, a...) \ + do { \ + _stmt(n, f , ## a ); \ + printf("\n"); \ + } while (0) + +#define push_block(n, f, a...) \ + do { \ + _stmt(n, f , ## a ); \ + printf(" {\n"); \ + indent_level++; \ + } while (0) + +static void pop_block(void) +{ + indent_level--; + indent(); + printf("}\n"); +} + +int main(int argc, char **argv) +{ + unsigned int cpu, max_cpus = DOMU_MAX_VCPUS; + + /**** DSDT DefinitionBlock start ****/ + /* (we append to existing DSDT definition block) */ + indent_level++; + + /**** Processor start ****/ + push_block("Scope", "\\_SB"); + + /* Define processor objects and control methods. */ + for ( cpu = 0; cpu < max_cpus; cpu++) + { + push_block("Processor", "PR%02X, %d, 0x0000b010, 0x06", cpu, cpu); + + stmt("Name", "_HID, \"ACPI0007\""); + stmt("Name", "_UID, %d", cpu); + + pop_block(); + } + + pop_block(); + /**** Processor end ****/ + + pop_block(); + /**** DSDT DefinitionBlock end ****/ + + return 0; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */