| Message ID | alpine.DEB.2.10.1610171530120.16441@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 17, 2016 at 04:36:18PM -0700, Stefano Stabellini wrote: > On Mon, 17 Oct 2016, Wei Liu wrote: > > On Mon, Oct 17, 2016 at 03:57:06PM +0100, Steve Capper wrote: > > > On Mon, Oct 17, 2016 at 11:47:00AM +0100, Wei Liu wrote: > > > > On Fri, Oct 14, 2016 at 06:02:30PM +0100, Wei Liu wrote: > > > > > The arm64 build for libacpi was broken due to two reasons: > > > > > [ ... ] > > > In file included from /usr/include/linux/types.h:4:0, > > > from /usr/include/aarch64-linux-gnu/asm/sigcontext.h:19, > > > from /usr/include/aarch64-linux-gnu/bits/sigcontext.h:27, > > > from /usr/include/signal.h:332, > > > from libxl_internal.h:30, > > > from libxl_arm.h:17, > > > from libxl_arm_acpi.c:19: > > > ../../xen/include/asm/types.h:54:13: error: conflicting types for 'register_t' > > > typedef u64 register_t; > > > ^ > > > > Now that I think about this, we indeed had similar error in the past. > > > > But I'm curious why I succeeded. > > I confirm that your patch (344da4f3ad6c4f76ef4efd530f4b1cc6901d6ff9) > fixes the dsdt_anycpu_arm.c build issue. > > This error is due to the libxl build picking up > "../../xen/include/asm/types.h" for > > #include <asm/types.h> > > which is wrong (it should be /usr/include/asm/types.h). > I did some digging and it depends on the build order: > > * build tools/ before xen/ --> works > * build xen/ before tools/ --> does not work > > The reason for this is that building Xen creates the xen/include/arm > link, which causes ../../xen/include/asm/types.h from being chosen > first, because of -I../../xen/include/ in libxl/Makefile. > > This is dangerous and wrong. > > --- > ARM64: fix libxl build, do not include ../../xen/include > > Do not include ../../xen/include/ to build libxl_arm_acpi.c: header > files clashing against default headers under /usr/include are present in > that directory. > > Link only $(XEN_ROOT)/xen/include/acpi under tools/include instead. > > Build tested on ARM64 and x86_64. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > diff --git a/tools/include/Makefile b/tools/include/Makefile > index dec8b3d..d95d837 100644 > --- a/tools/include/Makefile > +++ b/tools/include/Makefile > @@ -20,6 +20,7 @@ xen/.dir: > ln -sf ../xen-sys/$(XEN_OS) xen/sys > ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ > ln -s ../xen-foreign xen/foreign > + ln -s $(XEN_ROOT)/xen/include/acpi acpi > touch $@ > > # Not xen/xsm as that clashes with link to > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index c4e4117..dac19ac 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -96,7 +96,7 @@ dsdt_anycpu_arm.c: > $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) > > libxl_arm_acpi.o: libxl_arm_acpi.c > - $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > + $(CC) -c $(CFLAGS) -o $@ libxl_arm_acpi.c > else > LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o > endif Thanks Stefano, It hadn't occurred to me to add the acpi symlink :-). I can confirm that this fix works for me. ( I think we just delete the libxl_arm_acpi.o build rule and rely on implicit make? ) Cheers,
On Mon, Oct 17, 2016 at 04:36:18PM -0700, Stefano Stabellini wrote: > On Mon, 17 Oct 2016, Wei Liu wrote: > > On Mon, Oct 17, 2016 at 03:57:06PM +0100, Steve Capper wrote: > > > On Mon, Oct 17, 2016 at 11:47:00AM +0100, Wei Liu wrote: > > > > On Fri, Oct 14, 2016 at 06:02:30PM +0100, Wei Liu wrote: > > > > > The arm64 build for libacpi was broken due to two reasons: > > > > > > > > > > 1. ACPI_BUILD_DIR was appended twice to dsdt_anycpu_arm.c. > > > > > 2. The inclusion of firmware/Rules.mk overrided XEN_TARGET_ARCH, which > > > > > made CONFIG_ARM disappear. > > > > > > > > > > Fix those by: > > > > > > > > > > 1. Correctly generate full path for dsdt_anaycpu_arm.c. > > > > > 2. Include tools/Rules.mk instead, because libacpi/Makefile doesn't rely > > > > > on settings in firmware/Rules.mk. > > > > > > > > > > While at it, use CONFIG_ARM_64 instead of CONFIG_ARM as it is more > > > > > accurate. > > > > > > > > > > Reported-by: Julien Grall <julien.grall@arm.com> > > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > > > > --- > > > > > Cc: Julien Grall <julien.grall@arm.com> > > > > > Cc: Wei Chen <wei.chen@arm.com> > > > > > Cc: Steve Capper <steve.capper@arm.com> > > > > > Cc: Jan Beulich <JBeulich@suse.com> > > > > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > > Cc: Shannon Zhao <zhaoshenglong@huawei.com> > > > > > Cc: Stefano Stebellini <sstabellini@kernel.org> > > > > > > > > > > Please check if CONFIG_ARM_64 is correct -- IIRC ACPI is only relevant > > > > > on arm64. > > > > > > > > > > Would appreciate any build test report from ARM people. > > > > > > > > I set up a chroot environment this morning and built arm64 Xen. It > > > > worked. > > > > > > > > Since Jan and Julien are both away, I've taken the liberty of applying > > > > this patch with both my RM hat and tools maintainer hat on. > > > > > > > > I have also applied patch #3 since it is rather trivial. > > > > > > > > I will let Jan decide whether patch #2 is necessary. > > > > > > > > > > Thanks Wei, > > > I am trying to verify this patch, but I think I am running into another > > > issue with the libxl acpi support patches. > > > > > > Essentially I'm getting namespace clashes with the following: > > > * nonnull > > > * noreturn > > > * register_t > > > > > > I think this is due to the following logic in the libxl/Makefile: > > > libxl_arm_acpi.o: libxl_arm_acpi.c > > > $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > > > > > > When compiling libxl_arm_acpi.c, /usr/include/linux/types.h tries to pull > > > in xen/include/xen/types.h (instead of /usr/include/asm/types.h), which > > > then eventually pulls in xen/include/xen/compiler.h which redefines key > > > information. > > > > > > Which rootfs are you chrooting into for the testing? > > > (I've experienced build issues on Debian Jessie and Ubuntu Xenial running > > > in a Docker container). > > > > > > > qemu-debootstrap, sid, arm64. > > > > I saw similar errors. But they went away after I `git clean -fffddddxx` > > the tree. > > > > The fact that they went away somehow and Julien succeeded in building on > > variant of this patch made me think it was due to some issues in my > > environment. > > > > > Cheers, > > > -- > > > Steve > > > > > > I build Xen via: > > > $ git clean -f -d -x > > > $ ./configure --with-xenstored=xenstored --with-system-qemu=/usr/local/bin/qemu-system-aarch64 > > > $ make > > > > > > My builds finish like this: > > > > > > gcc -c -O1 -fno-omit-frame-pointer -DBUILD_ID -g -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O0 -g3 -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF .libxl_arm_acpi.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -fno-optimize-sibling-calls -Werror -Wno-format-zero-length -Wmissing-declarations -Wno-declaration-after-statement -Wformat-nonliteral -I. -fPIC -pthread -I/home/steven/xen/tools/libxl/../../tools/libs/toollog/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/libs/evtchn/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/libxc/include -I/home/steven/xen/tools/libxl/../../tools/libs/toollog/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/libs/foreignmemory/include -I/home/steven/xen/tool > s/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/include -D__XEN_TOOLS__ -I/home/steven/xen/tools/libxl/../../tools/libxc/include -I/home/steven/xen/tools/libxl/../../tools/libs/evtchn/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/libs/foreignmemory/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/xenstore/include -I/home/steven/xen/tools/libxl/../../tools/include -I/home/steven/xen/tools/libxl/../../tools/blktap2/control -I/home/steven/xen/tools/libxl/../../tools/blktap2/include -I/home/steven/xen/tools/libxl/../../tools/include -Wshadow -include /home/steven/xen/tools/libxl/../../tools/config.h -I../../xen/include/ -o libxl_arm_acpi.o libxl_arm_acpi.c > > > In file included from /usr/include/linux/types.h:4:0, > > > from /usr/include/aarch64-linux-gnu/asm/sigcontext.h:19, > > > from /usr/include/aarch64-linux-gnu/bits/sigcontext.h:27, > > > from /usr/include/signal.h:332, > > > from libxl_internal.h:30, > > > from libxl_arm.h:17, > > > from libxl_arm_acpi.c:19: > > > ../../xen/include/asm/types.h:54:13: error: conflicting types for 'register_t' > > > typedef u64 register_t; > > > ^ > > > > Now that I think about this, we indeed had similar error in the past. > > > > But I'm curious why I succeeded. > > I confirm that your patch (344da4f3ad6c4f76ef4efd530f4b1cc6901d6ff9) > fixes the dsdt_anycpu_arm.c build issue. > > This error is due to the libxl build picking up > "../../xen/include/asm/types.h" for > > #include <asm/types.h> > > which is wrong (it should be /usr/include/asm/types.h). > I did some digging and it depends on the build order: > > * build tools/ before xen/ --> works > * build xen/ before tools/ --> does not work > > The reason for this is that building Xen creates the xen/include/arm > link, which causes ../../xen/include/asm/types.h from being chosen > first, because of -I../../xen/include/ in libxl/Makefile. > Ah, thanks for digging into this. > This is dangerous and wrong. Yes, I agree. This should be fixed. > > --- > ARM64: fix libxl build, do not include ../../xen/include > > Do not include ../../xen/include/ to build libxl_arm_acpi.c: header > files clashing against default headers under /usr/include are present in > that directory. > > Link only $(XEN_ROOT)/xen/include/acpi under tools/include instead. > > Build tested on ARM64 and x86_64. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > Acked-by: Wei Liu <wei.liu2@citrix.com> Thanks for the patch. This is indeed how it should have been done in the first place. Sorry for not paying enough attention to this while reviewing. > diff --git a/tools/include/Makefile b/tools/include/Makefile > index dec8b3d..d95d837 100644 > --- a/tools/include/Makefile > +++ b/tools/include/Makefile > @@ -20,6 +20,7 @@ xen/.dir: > ln -sf ../xen-sys/$(XEN_OS) xen/sys > ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ > ln -s ../xen-foreign xen/foreign > + ln -s $(XEN_ROOT)/xen/include/acpi acpi > touch $@ > > # Not xen/xsm as that clashes with link to > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index c4e4117..dac19ac 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -96,7 +96,7 @@ dsdt_anycpu_arm.c: > $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) > > libxl_arm_acpi.o: libxl_arm_acpi.c > - $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > + $(CC) -c $(CFLAGS) -o $@ libxl_arm_acpi.c > else > LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o > endif
On Tue, Oct 18, 2016 at 08:52:24AM +0100, Steve Capper wrote: > On Mon, Oct 17, 2016 at 04:36:18PM -0700, Stefano Stabellini wrote: > > On Mon, 17 Oct 2016, Wei Liu wrote: > > > On Mon, Oct 17, 2016 at 03:57:06PM +0100, Steve Capper wrote: > > > > On Mon, Oct 17, 2016 at 11:47:00AM +0100, Wei Liu wrote: > > > > > On Fri, Oct 14, 2016 at 06:02:30PM +0100, Wei Liu wrote: > > > > > > The arm64 build for libacpi was broken due to two reasons: > > > > > > > > [ ... ] > > > > > In file included from /usr/include/linux/types.h:4:0, > > > > from /usr/include/aarch64-linux-gnu/asm/sigcontext.h:19, > > > > from /usr/include/aarch64-linux-gnu/bits/sigcontext.h:27, > > > > from /usr/include/signal.h:332, > > > > from libxl_internal.h:30, > > > > from libxl_arm.h:17, > > > > from libxl_arm_acpi.c:19: > > > > ../../xen/include/asm/types.h:54:13: error: conflicting types for 'register_t' > > > > typedef u64 register_t; > > > > ^ > > > > > > Now that I think about this, we indeed had similar error in the past. > > > > > > But I'm curious why I succeeded. > > > > I confirm that your patch (344da4f3ad6c4f76ef4efd530f4b1cc6901d6ff9) > > fixes the dsdt_anycpu_arm.c build issue. > > > > This error is due to the libxl build picking up > > "../../xen/include/asm/types.h" for > > > > #include <asm/types.h> > > > > which is wrong (it should be /usr/include/asm/types.h). > > I did some digging and it depends on the build order: > > > > * build tools/ before xen/ --> works > > * build xen/ before tools/ --> does not work > > > > The reason for this is that building Xen creates the xen/include/arm > > link, which causes ../../xen/include/asm/types.h from being chosen > > first, because of -I../../xen/include/ in libxl/Makefile. > > > > This is dangerous and wrong. > > > > --- > > ARM64: fix libxl build, do not include ../../xen/include > > > > Do not include ../../xen/include/ to build libxl_arm_acpi.c: header > > files clashing against default headers under /usr/include are present in > > that directory. > > > > Link only $(XEN_ROOT)/xen/include/acpi under tools/include instead. > > > > Build tested on ARM64 and x86_64. > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > diff --git a/tools/include/Makefile b/tools/include/Makefile > > index dec8b3d..d95d837 100644 > > --- a/tools/include/Makefile > > +++ b/tools/include/Makefile > > @@ -20,6 +20,7 @@ xen/.dir: > > ln -sf ../xen-sys/$(XEN_OS) xen/sys > > ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ > > ln -s ../xen-foreign xen/foreign > > + ln -s $(XEN_ROOT)/xen/include/acpi acpi > > touch $@ > > > > # Not xen/xsm as that clashes with link to > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > > index c4e4117..dac19ac 100644 > > --- a/tools/libxl/Makefile > > +++ b/tools/libxl/Makefile > > @@ -96,7 +96,7 @@ dsdt_anycpu_arm.c: > > $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) > > > > libxl_arm_acpi.o: libxl_arm_acpi.c > > - $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > > + $(CC) -c $(CFLAGS) -o $@ libxl_arm_acpi.c > > else > > LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o > > endif > > Thanks Stefano, > It hadn't occurred to me to add the acpi symlink :-). > > I can confirm that this fix works for me. > Right. I will turn this into a Tested-by tag. Thanks for testing. > ( I think we just delete the libxl_arm_acpi.o build rule and rely on > implicit make? ) > Yes, that can be done. Patch is welcome. :-) Wei. > Cheers, > -- > Steve
On Tue, Oct 18, 2016 at 12:21:02PM +0100, Wei Liu wrote: [...] > > > --- > > > ARM64: fix libxl build, do not include ../../xen/include > > > > > > Do not include ../../xen/include/ to build libxl_arm_acpi.c: header > > > files clashing against default headers under /usr/include are present in > > > that directory. > > > > > > Link only $(XEN_ROOT)/xen/include/acpi under tools/include instead. > > > > > > Build tested on ARM64 and x86_64. > > > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > > > diff --git a/tools/include/Makefile b/tools/include/Makefile > > > index dec8b3d..d95d837 100644 > > > --- a/tools/include/Makefile > > > +++ b/tools/include/Makefile > > > @@ -20,6 +20,7 @@ xen/.dir: > > > ln -sf ../xen-sys/$(XEN_OS) xen/sys > > > ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ > > > ln -s ../xen-foreign xen/foreign > > > + ln -s $(XEN_ROOT)/xen/include/acpi acpi > > > touch $@ > > > > > > # Not xen/xsm as that clashes with link to > > > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > > > index c4e4117..dac19ac 100644 > > > --- a/tools/libxl/Makefile > > > +++ b/tools/libxl/Makefile > > > @@ -96,7 +96,7 @@ dsdt_anycpu_arm.c: > > > $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) > > > > > > libxl_arm_acpi.o: libxl_arm_acpi.c > > > - $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c > > > + $(CC) -c $(CFLAGS) -o $@ libxl_arm_acpi.c > > > else > > > LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o > > > endif > > > > Thanks Stefano, > > It hadn't occurred to me to add the acpi symlink :-). > > > > I can confirm that this fix works for me. > > > > Right. I will turn this into a Tested-by tag. Thanks for testing. > > > ( I think we just delete the libxl_arm_acpi.o build rule and rely on > > implicit make? ) > > > > Yes, that can be done. Patch is welcome. :-) > I don't know why, but my arm64 chroot is broken at the moment (hangs from time to time, got TCG fatal error). I might submit a patch to change that if I manage to fix my arm64 chroot. Feel free to submit a patch to delete the explicit rule. In the mean time, I've applied this patch as-is so that it passes push-gate sooner. Wei. > Wei. > > > Cheers, > > -- > > Steve
diff --git a/tools/include/Makefile b/tools/include/Makefile index dec8b3d..d95d837 100644 --- a/tools/include/Makefile +++ b/tools/include/Makefile @@ -20,6 +20,7 @@ xen/.dir: ln -sf ../xen-sys/$(XEN_OS) xen/sys ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h elfstructs.h) xen/libelf/ ln -s ../xen-foreign xen/foreign + ln -s $(XEN_ROOT)/xen/include/acpi acpi touch $@ # Not xen/xsm as that clashes with link to diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index c4e4117..dac19ac 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -96,7 +96,7 @@ dsdt_anycpu_arm.c: $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(CURDIR) libxl_arm_acpi.o: libxl_arm_acpi.c - $(CC) -c $(CFLAGS) -I../../xen/include/ -o $@ libxl_arm_acpi.c + $(CC) -c $(CFLAGS) -o $@ libxl_arm_acpi.c else LIBXL_OBJS-$(CONFIG_ARM) += libxl_arm_no_acpi.o endif