[Buildroot] [PATCH 2/2] xen: new package

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Dear Maxime Ripard,

On Thu, 25 Jun 2015 16:04:56 +0200, Maxime Ripard wrote:
> This package allows to compile both the Xen hypervisor image, and the Xen
> tool stack intended to be used in the priviledged guest (aka dom0).
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Completely side question: do you know if there is a Qemu configuration
that emulates the ARM and/or x86 virtualization features that would
allow to run this Xen stuff ? It could be useful for quick testing
purposes (but isn't of course mandatory at all to get this patch
merged).

> diff --git a/package/xen/0001-xencommons-create-log-directory-before-using-it.patch b/package/xen/0001-xencommons-create-log-directory-before-using-it.patch
> new file mode 100644
> index 000000000000..365cdc5b5a9b
> --- /dev/null
> +++ b/package/xen/0001-xencommons-create-log-directory-before-using-it.patch
> @@ -0,0 +1,31 @@
> +From a7d1fde9a14ca0cf9c3f7c7950a6f9ea89ff58a6 Mon Sep 17 00:00:00 2001
> +From: Maxime Ripard <maxime.ripard@free-electrons.com>
> +Date: Thu, 25 Jun 2015 15:47:42 +0200
> +Subject: [PATCH] xencommons: create log directory before using it
> +
> +In the case where /var/log is volatile, for example when using a tmpfs, the
> +/var/log/xen directory will not be found on the system, and every attempt
> +to log something to one of the files in that directory will fail.
> +
> +Create it in the xencommons init script
> +
> +Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> +---
> + tools/hotplug/Linux/init.d/xencommons.in | 1 +
> + 1 file changed, 1 insertion(+)
> +
> +diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
> +index a1095c29ce0f..89210a02120a 100644
> +--- a/tools/hotplug/Linux/init.d/xencommons.in
> ++++ b/tools/hotplug/Linux/init.d/xencommons.in
> +@@ -61,6 +61,7 @@ do_start () {
> + 
> + 	mkdir -p ${XEN_RUN_DIR}
> + 	mkdir -p ${XEN_LOCK_DIR}
> ++	mkdir -p /var/log/xen
> + 
> + 	if ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1`
> + 	then
> +-- 
> +2.4.3
> +

Has this patch been submitted upstream?

> diff --git a/package/xen/Config.in b/package/xen/Config.in
> new file mode 100644
> index 000000000000..33eb11c50a8e
> --- /dev/null
> +++ b/package/xen/Config.in
> @@ -0,0 +1,30 @@
> +config BR2_PACKAGE_XEN
> +	bool "xen"
> +	depends on BR2_arm || BR2_arm64 || \
> +		BR2_i386 || BR2_x86_64
> +	depends on BR2_PACKAGE_LIBAIO_ARCH_SUPPORTS
> +	depends on !BR2_STATIC_LIBS # dtc (libfdt)
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # libglib2
> +	depends on BR2_USE_WCHAR # libglib2, util-linux

You need a comment to match these 'depends on' toolchain features.

> +	select BR2_PACKAGE_DTC
> +	select BR2_PACKAGE_LIBAIO
> +	select BR2_PACKAGE_LIBGLIB2
> +	select BR2_PACKAGE_NCURSES
> +	select BR2_PACKAGE_OPENSSL
> +	select BR2_PACKAGE_PIXMAN
> +	select BR2_PACKAGE_UTIL_LINUX
> +	select BR2_PACKAGE_UTIL_LINUX_LIBUUID
> +	select BR2_PACKAGE_YAJL

Are all these dependencies needed when you don't install the Xen tools?

> +	help
> +	  This builds the Xen hypervisor and toolstack
> +
> +	  http://www.xenproject.org/
> +
> +if BR2_PACKAGE_XEN
> +
> +config BR2_PACKAGE_XEN_HYPERVISOR
> +	bool "Build the Xen hypervisor"
> +
> +config BR2_PACKAGE_XEN_TOOLS
> +	bool "Build the Xen tools"
> +endif

Empty newline missing after "endif".

Also, if neither the hypervisor nor the tools are enabled, what gets
built? Nothing? If so, maybe you want:

	select BR2_PACKAGE_XEN_HYPERVISOR if !BR2_PACKAGE_XEN_TOOLS

in the top-level option.

> diff --git a/package/xen/xen.mk b/package/xen/xen.mk
> new file mode 100644
> index 000000000000..fee9fdc12cdf
> --- /dev/null
> +++ b/package/xen/xen.mk
> @@ -0,0 +1,49 @@
> +################################################################################
> +#
> +# Xen
> +#
> +################################################################################
> +
> +XEN_VERSION = 4.5.0
> +XEN_SITE = http://bits.xensource.com/oss-xen/release/$(XEN_VERSION)

Missing LICENSE + LICENSE_FILES.

> +XEN_INSTALL_IMAGES = YES
> +
> +XEN_DEPENDENCIES += dtc libaio libglib2 ncurses openssl pixman util-linux yajl
> +
> +XEN_MAKE_ENV = \
> +	XEN_TARGET_ARCH=arm32 \

Hard-coded arm32 doesn't seem right.

> +	CROSS_COMPILE=$(TARGET_CROSS) \
> +	CXXFLAGS="$(TARGET_CXXFLAGS) -D_FILE_OFFSET_BITS=64" \
> +	CFLAGS="$(TARGET_CFLAGS) -D_FILE_OFFSET_BITS=64" \

Why do you pass -D_FILE_OFFSET_BITS=64 ? It is already in
TARGET_CFLAGS / TARGET_CXXFLAGS.

> +	PKG_CONFIG=$(PKG_CONFIG_HOST_BINARY)
> +
> +XEN_INSTALL_TARGET_OPTS = DESTDIR=$(TARGET_DIR)

This seems weird, you're losing the "install" target.

> +
> +ifeq ($(BR2_PACKAGE_XEN_HYPERVISOR),y)
> +XEN_MAKE_OPTS += dist-xen
> +
> +define XEN_INSTALL_IMAGES_CMDS
> +	cp $(@D)/xen/xen $(BINARIES_DIR)
> +endef
> +else
> +XEN_CONF_OPTS += --disable-xen
> +endif
> +
> +XEN_CONF_OPTS += --disable-ocamltools

We generally put the common options towards the top of the file, before
all the conditional stuff. And the += is unneeded then, it can be just
a '='.

> +ifeq ($(BR2_PACKAGE_XEN_TOOLS),y)
> +XEN_MAKE_OPTS += dist-tools
> +XEN_INSTALL_TARGET_OPTS += install-tools

Ah, so here it comes. So maybe instead you want to do
XEN_INSTALL_TARGET = NO by default, and do XEN_INSTALL_TARGET_OPTS =
DESTDIR=$(TARGET_DIR) install-tools when BR2_PACKAGE_XEN_TOOLS=y.

> +define XEN_RENAME_INIT_SCRIPTS
> +	mv $(TARGET_DIR)/etc/init.d/xencommons $(TARGET_DIR)/etc/init.d/S50xencommons
> +	mv $(TARGET_DIR)/etc/init.d/xen-watchdog $(TARGET_DIR)/etc/init.d/S50xen-watchdog
> +	mv $(TARGET_DIR)/etc/init.d/xendomains $(TARGET_DIR)/etc/init.d/S60xendomains

Don't know if it's better or not:

	(cd $(TARGET_DIR)/etc/init.d; \
		mv xencommons S50xencommons; \
		mv ... ; \
		mv ...)

> +endef
> +else
> +XEN_CONF_OPTS += --disable-tools
> +endif
> +
> +XEN_POST_INSTALL_TARGET_HOOKS += XEN_RENAME_INIT_SCRIPTS

Should be part of the ifeq BR2_PACKAGE_XEN_TOOLS=y.

Also, here it doesn't build, because it cannot find libyajl due to the
following failure:

configure:8342: /home/thomas/projets/buildroot/output/host/usr/bin/arm-linux-gcc -o conftest -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64   -Os   -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FI
LE_OFFSET_BITS=64     conftest.c -lyajl  -lcrypto -laio  -lutil >&5
/home/thomas/projets/buildroot/output/host/usr/arm-buildroot-linux-uclibcgnueabi/sysroot/usr/lib/libcrypto.so: warning: gethostbyname is obsolescent, use getnameinfo() instead.
/home/thomas/projets/buildroot/output/host/usr/arm-buildroot-linux-uclibcgnueabi/sysroot/usr/lib/libyajl.so: undefined reference to `__isnan'
/home/thomas/projets/buildroot/output/host/usr/arm-buildroot-linux-uclibcgnueabi/sysroot/usr/lib/libyajl.so: undefined reference to `__isinf'

You need to change:

AC_CHECK_LIB([yajl], [yajl_alloc], [],
    [AC_MSG_ERROR([Could not find yajl])])

to:

AC_CHECK_LIB([yajl], [yajl_alloc], [],
    [AC_MSG_ERROR([Could not find yajl])], [m])

So that it links with the math library. I used the following defconfig
to produce the problem:

BR2_arm=y
BR2_TOOLCHAIN_EXTERNAL=y
BR2_TOOLCHAIN_EXTERNAL_CUSTOM=y
BR2_TOOLCHAIN_EXTERNAL_DOWNLOAD=y
BR2_TOOLCHAIN_EXTERNAL_URL="http://autobuild.buildroot.org/toolchains/tarballs/br-arm-full-2015.05.tar.bz2"
BR2_TOOLCHAIN_EXTERNAL_HEADERS_4_0=y
BR2_TOOLCHAIN_EXTERNAL_LOCALE=y
# BR2_TOOLCHAIN_EXTERNAL_HAS_THREADS_DEBUG is not set
BR2_TOOLCHAIN_EXTERNAL_INET_RPC=y
BR2_TOOLCHAIN_EXTERNAL_CXX=y
BR2_INIT_NONE=y
BR2_SYSTEM_BIN_SH_NONE=y
# BR2_PACKAGE_BUSYBOX is not set
BR2_PACKAGE_NGINX=y
BR2_PACKAGE_XEN=y
BR2_PACKAGE_XEN_HYPERVISOR=y
BR2_PACKAGE_XEN_TOOLS=y
# BR2_TARGET_ROOTFS_TAR is not set

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com