| Message ID | 1480100781-2915-1-git-send-email-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
On 25/11/16 19:06, Julien Grall wrote: > The tools (such as mk_dsdt) can be cross-built when it may not be > desirable to build them on the target. > > The commit c4ac1077 "libxl/arm: Generate static ACPI DSDT table" > introduced support of ARM64 in mk_dsdt but also break cross-building > tools because the ACPI tables are not correct. > > While mk_dsdt should generate ACPI table for the target architecture, it > currently generates the one for the host. This is because the source > code contains reference to the host architecture (__aarch64__, > __x86_64__, __i386__) when it should be the target architecture. > > Replace all __aarch64__, __x86_64__, __i386__ by the corresponding > CONFIG_*. > > Also expose the CONFIG_* to the source code as the currently only > exposed to the Makefile. > > Reported-by: Andrii Anisov <andrii.anisov@gmail.com> > Suggested-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one tweak, > @@ -27,6 +27,11 @@ DSDT_FILES ?= $(C_SRC-y) > C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) > H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) > > +MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 > +MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 > + > +MKDSDT_CFLAGS = $(MKDSDT_CFLAGS-y) := to avoid excessive re-evaluation. > + > # Suffix for temporary files. > # > # We will also use this suffix to workaround a bug in older iasl >
>>> On 25.11.16 at 20:06, <julien.grall@arm.com> wrote: > --- a/tools/libacpi/Makefile > +++ b/tools/libacpi/Makefile > @@ -27,6 +27,11 @@ DSDT_FILES ?= $(C_SRC-y) > C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) > H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) > > +MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 > +MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 > + > +MKDSDT_CFLAGS = $(MKDSDT_CFLAGS-y) On top of what Andrew has said, I don't think this variable is really needed at this point anyway. Simply use the -y tagged one below right away. > --- a/tools/libacpi/mk_dsdt.c > +++ b/tools/libacpi/mk_dsdt.c > @@ -17,9 +17,9 @@ > #include <getopt.h> > #include <stdlib.h> > #include <stdbool.h> > -#if defined(__i386__) || defined(__x86_64__) > +#ifdef CONFIG_X86 > #include <xen/hvm/hvm_info_table.h> > -#elif defined(__aarch64__) > +#elif CONFIG_ARM_64 #elif defined() Also, for thing to look consistent, please also use #if defined() in favor of #ifdef then (unless, like in a few cases further down) there's no such "else". Jan
On Nov 26, 2016, at 4:06 AM, Julien Grall wrote: Hi Julien and Xen folks, > The tools (such as mk_dsdt) can be cross-built when it may not be > desirable to build them on the target. > > The commit c4ac1077 "libxl/arm: Generate static ACPI DSDT table" > introduced support of ARM64 in mk_dsdt but also break cross-building > tools because the ACPI tables are not correct. > > While mk_dsdt should generate ACPI table for the target architecture, it > currently generates the one for the host. This is because the source > code contains reference to the host architecture (__aarch64__, > __x86_64__, __i386__) when it should be the target architecture. > > Replace all __aarch64__, __x86_64__, __i386__ by the corresponding > CONFIG_*. > > Also expose the CONFIG_* to the source code as the currently only > exposed to the Makefile. > > Reported-by: Andrii Anisov <andrii.anisov@gmail.com> > Suggested-by: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > This was reported on the ML recently (see [1]) and affects only Xen > 4.8. Without this patch, cross-building the tools will not work. > > I think this patch is quite important for embedded users where they > tend to cross-build the rootfs (for instance using yocto). I've experienced the same build breakage when using Yocto. This patch resolves the problem in my case. Thanks a lot! Best Regards Jungseok Lee
Hi Jan, On 28/11/16 08:10, Jan Beulich wrote: >>>> On 25.11.16 at 20:06, <julien.grall@arm.com> wrote: >> --- a/tools/libacpi/Makefile >> +++ b/tools/libacpi/Makefile >> @@ -27,6 +27,11 @@ DSDT_FILES ?= $(C_SRC-y) >> C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) >> H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) >> >> +MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 >> +MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 >> + >> +MKDSDT_CFLAGS = $(MKDSDT_CFLAGS-y) > > On top of what Andrew has said, I don't think this variable is really > needed at this point anyway. Simply use the -y tagged one below > right away. I will do that. > >> --- a/tools/libacpi/mk_dsdt.c >> +++ b/tools/libacpi/mk_dsdt.c >> @@ -17,9 +17,9 @@ >> #include <getopt.h> >> #include <stdlib.h> >> #include <stdbool.h> >> -#if defined(__i386__) || defined(__x86_64__) >> +#ifdef CONFIG_X86 >> #include <xen/hvm/hvm_info_table.h> >> -#elif defined(__aarch64__) >> +#elif CONFIG_ARM_64 > > #elif defined() > > Also, for thing to look consistent, please also use #if defined() > in favor of #ifdef then (unless, like in a few cases further down) > there's no such "else". Good point. I will send a new version shortly. Cheers,
Hi Andrew, On 25/11/16 19:12, Andrew Cooper wrote: > On 25/11/16 19:06, Julien Grall wrote: >> The tools (such as mk_dsdt) can be cross-built when it may not be >> desirable to build them on the target. >> >> The commit c4ac1077 "libxl/arm: Generate static ACPI DSDT table" >> introduced support of ARM64 in mk_dsdt but also break cross-building >> tools because the ACPI tables are not correct. >> >> While mk_dsdt should generate ACPI table for the target architecture, it >> currently generates the one for the host. This is because the source >> code contains reference to the host architecture (__aarch64__, >> __x86_64__, __i386__) when it should be the target architecture. >> >> Replace all __aarch64__, __x86_64__, __i386__ by the corresponding >> CONFIG_*. >> >> Also expose the CONFIG_* to the source code as the currently only >> exposed to the Makefile. >> >> Reported-by: Andrii Anisov <andrii.anisov@gmail.com> >> Suggested-by: Wei Liu <wei.liu2@citrix.com> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one tweak, Jan asked few changes in the patch. Can I keep the reviewed-by on the next version? > >> @@ -27,6 +27,11 @@ DSDT_FILES ?= $(C_SRC-y) >> C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) >> H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) >> >> +MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 >> +MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 >> + >> +MKDSDT_CFLAGS = $(MKDSDT_CFLAGS-y) > > := to avoid excessive re-evaluation. Jan suggested to use directly MKDSDT_CFLAGS-y directly. So I will drop this statement. > >> + >> # Suffix for temporary files. >> # >> # We will also use this suffix to workaround a bug in older iasl >> > Cheers,
On 28/11/16 13:00, Julien Grall wrote: > Hi Andrew, > > On 25/11/16 19:12, Andrew Cooper wrote: >> On 25/11/16 19:06, Julien Grall wrote: >>> The tools (such as mk_dsdt) can be cross-built when it may not be >>> desirable to build them on the target. >>> >>> The commit c4ac1077 "libxl/arm: Generate static ACPI DSDT table" >>> introduced support of ARM64 in mk_dsdt but also break cross-building >>> tools because the ACPI tables are not correct. >>> >>> While mk_dsdt should generate ACPI table for the target >>> architecture, it >>> currently generates the one for the host. This is because the source >>> code contains reference to the host architecture (__aarch64__, >>> __x86_64__, __i386__) when it should be the target architecture. >>> >>> Replace all __aarch64__, __x86_64__, __i386__ by the corresponding >>> CONFIG_*. >>> >>> Also expose the CONFIG_* to the source code as the currently only >>> exposed to the Makefile. >>> >>> Reported-by: Andrii Anisov <andrii.anisov@gmail.com> >>> Suggested-by: Wei Liu <wei.liu2@citrix.com> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one tweak, > > Jan asked few changes in the patch. Can I keep the reviewed-by on the > next version? Yes. Fine by me. ~Andrew
diff --git a/tools/libacpi/Makefile b/tools/libacpi/Makefile index ccc32c9..4560c0e 100644 --- a/tools/libacpi/Makefile +++ b/tools/libacpi/Makefile @@ -27,6 +27,11 @@ DSDT_FILES ?= $(C_SRC-y) C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, $(DSDT_FILES)) H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h) +MKDSDT_CFLAGS-$(CONFIG_ARM_64) = -DCONFIG_ARM_64 +MKDSDT_CFLAGS-$(CONFIG_X86) = -DCONFIG_X86 + +MKDSDT_CFLAGS = $(MKDSDT_CFLAGS-y) + # Suffix for temporary files. # # We will also use this suffix to workaround a bug in older iasl @@ -44,7 +49,7 @@ $(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl rm -f $(addprefix $(ACPI_BUILD_DIR)/, $*.aml $*.hex) $(MK_DSDT): mk_dsdt.c - $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c + $(HOSTCC) $(HOSTCFLAGS) $(MKDSDT_CFLAGS) $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -o $@ mk_dsdt.c $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl $(MK_DSDT) # Remove last bracket diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c index 16320a9..fa3c1e9 100644 --- a/tools/libacpi/mk_dsdt.c +++ b/tools/libacpi/mk_dsdt.c @@ -17,9 +17,9 @@ #include <getopt.h> #include <stdlib.h> #include <stdbool.h> -#if defined(__i386__) || defined(__x86_64__) +#ifdef CONFIG_X86 #include <xen/hvm/hvm_info_table.h> -#elif defined(__aarch64__) +#elif CONFIG_ARM_64 #include <xen/arch-arm.h> #endif @@ -111,9 +111,9 @@ int main(int argc, char **argv) unsigned int slot, dev, intx, link, cpu, max_cpus; dm_version dm_version = QEMU_XEN_TRADITIONAL; -#if defined(__i386__) || defined(__x86_64__) +#ifdef CONFIG_X86 max_cpus = HVM_MAX_VCPUS; -#elif defined(__aarch64__) +#elif CONFIG_ARM_64 max_cpus = GUEST_MAX_VCPUS; #endif @@ -169,7 +169,7 @@ int main(int argc, char **argv) /**** Processor start ****/ push_block("Scope", "\\_SB"); -#if defined(__i386__) || defined(__x86_64__) +#ifdef CONFIG_X86 /* MADT checksum */ stmt("OperationRegion", "MSUM, SystemMemory, \\_SB.MSUA, 1"); push_block("Field", "MSUM, ByteAcc, NoLock, Preserve"); @@ -193,7 +193,7 @@ int main(int argc, char **argv) stmt("Name", "_HID, \"ACPI0007\""); stmt("Name", "_UID, %d", cpu); -#if defined(__aarch64__) +#ifdef CONFIG_ARM_64 pop_block(); continue; #endif @@ -235,7 +235,7 @@ int main(int argc, char **argv) pop_block(); } -#if defined(__aarch64__) +#ifdef CONFIG_ARM_64 pop_block(); /**** Processor end ****/ pop_block();
The tools (such as mk_dsdt) can be cross-built when it may not be desirable to build them on the target. The commit c4ac1077 "libxl/arm: Generate static ACPI DSDT table" introduced support of ARM64 in mk_dsdt but also break cross-building tools because the ACPI tables are not correct. While mk_dsdt should generate ACPI table for the target architecture, it currently generates the one for the host. This is because the source code contains reference to the host architecture (__aarch64__, __x86_64__, __i386__) when it should be the target architecture. Replace all __aarch64__, __x86_64__, __i386__ by the corresponding CONFIG_*. Also expose the CONFIG_* to the source code as the currently only exposed to the Makefile. Reported-by: Andrii Anisov <andrii.anisov@gmail.com> Suggested-by: Wei Liu <wei.liu2@citrix.com> Signed-off-by: Julien Grall <julien.grall@arm.com> --- This was reported on the ML recently (see [1]) and affects only Xen 4.8. Without this patch, cross-building the tools will not work. I think this patch is quite important for embedded users where they tend to cross-build the rootfs (for instance using yocto). The patch is fairly simple, exposing CONFIG_* to the source code and replacing all reference to the host architecture to the corresponding target architecture. It could be easy to test all the configuration. I diffed the generated dsdt and it is the same before and after the patches when built natively. I did try cross-build, Andrii could you give a try? [1] https://lists.xenproject.org/archives/html/xen-devel/2016-11/msg01903.html --- tools/libacpi/Makefile | 7 ++++++- tools/libacpi/mk_dsdt.c | 14 +++++++------- 2 files changed, 13 insertions(+), 8 deletions(-)