On Fri, Jan 12, 2018 at 03:21:13PM -0600, Eric DeVolder wrote:
> When kexec is utilized in a Xen environment, it has an explicit
> run-time dependency on libxenctrl.so. This dependency occurs
> during the configure stage and when building kexec-tools.
>
> When kexec is utilized in a non-Xen environment (either bare
> metal or KVM), the configure and build of kexec-tools omits
> any reference to libxenctrl.so.
[...]
Wow! What a long story! Patch looks quite good. Just a few nitpicks.
If you fix them then you can add Reviewed-by: Daniel Kiper <daniel.kiper@xxxxxxxxxx>
to the next version.
> Signed-off-by: Eric DeVolder <eric.devolder@xxxxxxxxxx>
> ---
> v1: 29nov2017
> - Daniel Kiper suggested Debian's libxen package of libraries,
> but I did not find similar package on most other systems.
>
> v2: 14dec2017
> - Reposted to kexec and xen-devel mailing lists
>
> v3: 12jan2018
> - Incorporated feedback from Daniel Kiper.
> Configure changes for --with-xen=dl, style problems corrected.
> Extensive testing of the new mode.
> ---
> configure.ac | 27 ++++++++++----
> kexec/Makefile | 1 +
> kexec/arch/i386/crashdump-x86.c | 4 +--
> kexec/arch/i386/kexec-x86-common.c | 4 +--
> kexec/crashdump-xen.c | 4 +--
> kexec/kexec-xen.c | 41 ++++++++++++++++++++-
> kexec/kexec-xen.h | 73 ++++++++++++++++++++++++++++++++++++++
> 7 files changed, 138 insertions(+), 16 deletions(-)
> create mode 100644 kexec/kexec-xen.h
>
> diff --git a/configure.ac b/configure.ac
> index 208dc0a..4dab57f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -167,12 +167,27 @@ if test "$with_xen" = yes ; then
> AC_CHECK_HEADER(xenctrl.h,
> [AC_CHECK_LIB(xenctrl, xc_kexec_load, ,
> AC_MSG_NOTICE([Xen support disabled]))])
> - if test "$ac_cv_lib_xenctrl_xc_kexec_load" = yes ; then
> - AC_CHECK_LIB(xenctrl, xc_kexec_status,
> - AC_DEFINE(HAVE_KEXEC_CMD_STATUS, 1,
> - [The kexec_status call is available]),
> - AC_MSG_NOTICE([The kexec_status call is not available]))
> - fi
> +fi
> +
> +dnl Link libxenctrl.so at run-time rather than build-time
> +if test "$with_xen" = dl ; then
> + AC_CHECK_HEADER(dlfcn.h,
> + [AC_CHECK_LIB(dl, dlopen, ,
> + AC_MSG_ERROR([Dynamic library linking not available]))],
> + AC_MSG_ERROR([Dynamic library linking not available]))
I think that this error message should be like this:
Dynamic library linking header not available
> + AC_DEFINE(CONFIG_LIBXENCTRL_DL, 1, [Define to 1 to link libxenctrl.so at run-time rather than build-time])
> + AC_CHECK_HEADER(xenctrl.h,
> + [AC_CHECK_LIB(xenctrl, xc_kexec_load,
> + AC_DEFINE(HAVE_LIBXENCTRL, 1, ), # required define, and prevent -lxenctrl
> + AC_MSG_NOTICE([Xen support disabled]))])
> +fi
> +
> +dnl Check for the Xen kexec_status hypercall - reachable from --with-xen=yes|dl
> +if test "$ac_cv_lib_xenctrl_xc_kexec_load" = yes ; then
> + AC_CHECK_LIB(xenctrl, xc_kexec_status,
> + AC_DEFINE(HAVE_KEXEC_CMD_STATUS, 1,
> + [The kexec_status call is available]),
> + AC_MSG_NOTICE([The kexec_status call is not available]))
> fi
>
> dnl ---Sanity checks
> diff --git a/kexec/Makefile b/kexec/Makefile
> index 2b4fb3d..8871731 100644
> --- a/kexec/Makefile
> +++ b/kexec/Makefile
> @@ -36,6 +36,7 @@ dist += kexec/Makefile \
> kexec/kexec-elf-boot.h \
> kexec/kexec-elf.h kexec/kexec-sha256.h \
> kexec/kexec-zlib.h kexec/kexec-lzma.h \
> + kexec/kexec-xen.h \
Could you fix backslash alignment? Maybe in separate patch. It can be
part of this series.
> kexec/kexec-syscall.h kexec/kexec.h kexec/kexec.8
>
> dist += kexec/proc_iomem.c
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index 69a063a..a948d9f 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -44,9 +44,7 @@
> #include "kexec-x86.h"
> #include "crashdump-x86.h"
>
Please remove this blank line...
> -#ifdef HAVE_LIBXENCTRL
> -#include <xenctrl.h>
> -#endif /* HAVE_LIBXENCTRL */
> +#include "../../kexec-xen.h"
>
...and this... Same things below... This was done to separate
conditional include from unconditional ones. So, blank lines
after your patch are no longer needed.
> #include "x86-linux-setup.h"
>
> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
> index be03618..b44c8b7 100644
> --- a/kexec/arch/i386/kexec-x86-common.c
> +++ b/kexec/arch/i386/kexec-x86-common.c
> @@ -40,9 +40,7 @@
> #include "../../crashdump.h"
> #include "kexec-x86.h"
>
Ditto...
> -#ifdef HAVE_LIBXENCTRL
> -#include <xenctrl.h>
> -#endif /* HAVE_LIBXENCTRL */
> +#include "../../kexec-xen.h"
>
But leave this one...
> /* Used below but not present in (older?) xenctrl.h */
> #ifndef E820_PMEM
> diff --git a/kexec/crashdump-xen.c b/kexec/crashdump-xen.c
> index 60594f6..2e4cbdc 100644
> --- a/kexec/crashdump-xen.c
> +++ b/kexec/crashdump-xen.c
> @@ -18,9 +18,7 @@
>
> #include "config.h"
>
Ditto...
> -#ifdef HAVE_LIBXENCTRL
> -#include <xenctrl.h>
> -#endif
> +#include "kexec-xen.h"
>
But leave this one... And same rules below...
> struct crash_note_info {
> unsigned long base;
> diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
> index 2b448d3..8ec3398 100644
> --- a/kexec/kexec-xen.c
> +++ b/kexec/kexec-xen.c
> @@ -10,10 +10,49 @@
> #include "config.h"
>
> #ifdef HAVE_LIBXENCTRL
> -#include <xenctrl.h>
> +#include "kexec-xen.h"
>
> #include "crashdump.h"
>
> +#ifdef CONFIG_LIBXENCTRL_DL
> +void *xc_dlhandle;
> +xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
> +xc_interface *__xc_interface_open(xentoollog_logger *logger,
> + xentoollog_logger *dombuild_logger,
> + unsigned open_flags)
> +{
> + xc_interface *xch = NULL;
> +
> + if (!xc_dlhandle)
> + xc_dlhandle = dlopen("libxenctrl.so", RTLD_NOW | RTLD_NODELETE);
> +
> + if (xc_dlhandle) {
> + typedef xc_interface *(*func_t)(xentoollog_logger *logger,
> + xentoollog_logger *dombuild_logger,
> + unsigned open_flags);
> + func_t func = (func_t)dlsym(xc_dlhandle, "xc_interface_open");
I know your opinion about that definitions but I am not happy with them here.
However, if maintainer is OK with them I will not insist on changing that. Just
please add one extra line between definitions above and function call below.
> + xch = func(logger, dombuild_logger, open_flags);
> + }
> +
> + return xch;
> +}
> +
> +int __xc_interface_close(xc_interface *xch)
> +{
> + int rc = -1;
> +
> + if (xc_dlhandle) {
> + typedef int (*func_t)(xc_interface *xch);
> + func_t func = (func_t)dlsym(xc_dlhandle, "xc_interface_close");
Same as above... Please add empty line here...
> + rc = func(xch);
> +
...and I think that you can drop this one here...
> + xc_dlhandle = NULL;
> + }
> +
> + return rc;
> +}
> +#endif /* CONFIG_LIBXENCTRL_DL */
> +
> int xen_kexec_load(struct kexec_info *info)
> {
> uint32_t nr_segments = info->nr_segments;
> diff --git a/kexec/kexec-xen.h b/kexec/kexec-xen.h
> new file mode 100644
> index 0000000..50e2b3d
> --- /dev/null
> +++ b/kexec/kexec-xen.h
> @@ -0,0 +1,73 @@
> +#ifndef KEXEC_XEN_H
> +#define KEXEC_XEN_H
> +
> +#ifdef HAVE_LIBXENCTRL
I would add empty line here...
> +#include <xenctrl.h>
> +
> +#ifdef CONFIG_LIBXENCTRL_DL
...and here...
> +#include <dlfcn.h>
> +
> +/* The handle from dlopen(), needed by dlsym(), dlclose() */
> +extern void *xc_dlhandle;
> +
> +/* Wrappers around xc_interface_open/close() to insert dlopen/dlclose() */
> +xc_interface *__xc_interface_open(xentoollog_logger *logger,
> + xentoollog_logger *dombuild_logger,
> + unsigned open_flags);
> +int __xc_interface_close(xc_interface *xch);
> +
> +/* GCC expression statements for evaluating dlsym() */
> +#define __xc_call(dtype, name, args...) \
> +( \
> + { dtype value; \
> + typedef dtype (*func_t)(xc_interface *, ...); \
> + func_t func = dlsym(xc_dlhandle, #name); \
> + value = func(args); \
> + value; } \
Please use one tab instead of spaces here.
> +)
> +#define __xc_data(dtype, name) \
> +( \
> + { dtype *value = (dtype *)dlsym(xc_dlhandle, #name); \
> + value; } \
It looks that this code can fit into one line. Could you change that?
And one tab instead of spaces...
> +)
> +
> +/* The wrappers around utilized xenctrl.h functions */
> +#define xc_interface_open(a, b, c) \
> + __xc_interface_open(a, b, c)
I would put 2 tabs instead of spaces here and below...
> +#define xc_interface_close(a) \
> + __xc_interface_close(a)
> +#define xc_version(args...) \
> + __xc_call(int, xc_version, args)
> +#define xc_get_max_cpus(args...) \
> + __xc_call(int, xc_get_max_cpus, args)
> +#define xc_get_machine_memory_map(args...) \
> + __xc_call(int, xc_get_machine_memory_map, args)
> +#define xc_kexec_get_range(args...) \
> + __xc_call(int, xc_kexec_get_range, args)
> +#define xc_kexec_load(args...) \
> + __xc_call(int, xc_kexec_load, args)
> +#define xc_kexec_unload(args...) \
> + __xc_call(int, xc_kexec_unload, args)
> +#define xc_kexec_status(args...) \
> + __xc_call(int, xc_kexec_status, args)
> +#define xc_kexec_exec(args...) \
> + __xc_call(int, xc_kexec_exec, args)
> +#define xc_hypercall_buffer_array_create(args...) \
> + __xc_call(xc_hypercall_buffer_array_t *, xc_hypercall_buffer_array_create, args)
> +#define xc__hypercall_buffer_alloc_pages(args...) \
> + __xc_call(void *, xc__hypercall_buffer_alloc_pages, args)
> +#define xc__hypercall_buffer_free_pages(args...) \
> + __xc_call(void , xc__hypercall_buffer_free_pages, args)
> +#define xc__hypercall_buffer_array_alloc(args...) \
> + __xc_call(void *, xc__hypercall_buffer_array_alloc, args)
> +#define xc__hypercall_buffer_array_get(args...) \
> + __xc_call(void *, xc__hypercall_buffer_array_get, args)
> +#define xc_hypercall_buffer_array_destroy(args...) \
> + __xc_call(void *, xc_hypercall_buffer_array_destroy, args)
> +
> +#endif /* CONFIG_LIBXENCTRL_DL */
> +
You can drop this empty line...
> +#endif /* HAVE_LIBXENCTRL */
> +
...and leave this one...
> +#endif /* KEXEC_XEN_H */
> +
...and drop this one...
Thanks,
Daniel
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
