[Xen-devel,for-4.8] tools/libacpi: Fix compilation when cross building the tools

Message ID 1480100781-2915-1-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall Nov. 25, 2016, 7:06 p.m.
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(-)

Comments

Andrew Cooper Nov. 25, 2016, 7:12 p.m. | #1
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
>
Jan Beulich Nov. 28, 2016, 8:10 a.m. | #2
>>> 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
Jungseok Lee Nov. 28, 2016, 12:53 p.m. | #3
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
Julien Grall Nov. 28, 2016, 12:53 p.m. | #4
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,
Julien Grall Nov. 28, 2016, 1 p.m. | #5
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,
Andrew Cooper Nov. 28, 2016, 1:01 p.m. | #6
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

Patch hide | download patch | download mbox

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();