[PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Jan Beulich]

In a few cases we link in library-like functions when they're not
actually needed. While we could use Kconfig options for each one
of them, I think the better approach for such generic code is to
build it always (thus making sure a build issue can't be introduced
for these in any however exotic configuration) and then put it into
an archive, for the linker to pick up as needed. The series here
presents a first few tiny steps towards such a goal.

Note that we can't use thin archives yet, due to our tool chain
(binutils) baseline being too low.

Further almost immediate steps I'd like to take if the approach
meets no opposition are
- split and move the rest of common/lib.c,
- split and move common/string.c, dropping the need for all the
  __HAVE_ARCH_* (implying possible per-arch archives then need to
  be specified ahead of lib/lib.a on the linker command lines),
- move common/libelf/ and common/libfdt/.

v3 has a new 1st patch and some review feedback addressed. See
individual patches.

1: xen: fix build when $(obj-y) consists of just blanks
2: lib: collect library files in an archive
3: lib: move list sorting code
4: lib: move parse_size_and_unit()
5: lib: move init_constructors()
6: lib: move rbtree code
7: lib: move bsearch code
8: lib: move sort code

Jan

[PATCH v3 1/8] xen: fix build when $(obj-y) consists of just blanks

[Jan Beulich]

This case can occur when combining empty lists

obj-y :=
...
obj-y += $(empty)

or

obj-y := $(empty) $(empty)

where (only) blanks would accumulate. This was only a latent issue until
now, but would become an active issue for Arm once lib/ gets populated
with all respective objects going into the to be introduced lib.a.

Also address a related issue at this occasion: When an empty built_in.o
gets created, .built_in.o.d will have its dependencies recorded. If, on
a subsequent incremental build, an actual constituent of built_in.o
appeared, the $(filter-out ) would leave these recorded dependencies in
place. But of course the linker won't know what to do with C header
files. (The apparent alternative of avoiding to pass $(c_flags) or
$(a_flags) would not be reliable afaict, as among these flags there may
be some affecting information conveyed via the object file to the
linker. The linker, finding inconsistent flags across object files, may
then error out.) Using just $(obj-y) won't work either: It breaks when
the same object file is listed more than once.

Reported-by: Julien Grall <julien@xen.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/Rules.mk | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 333e19bec343..d5e5eb33de39 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -130,13 +130,13 @@ c_flags += $(CFLAGS-y)
 a_flags += $(CFLAGS-y) $(AFLAGS-y)
 
 built_in.o: $(obj-y) $(extra-y)
-ifeq ($(obj-y),)
+ifeq ($(strip $(obj-y)),)
 	$(CC) $(c_flags) -c -x c /dev/null -o $@
 else
 ifeq ($(CONFIG_LTO),y)
-	$(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^)
+	$(LD_LTO) -r -o $@ $(filter $(obj-y),$^)
 else
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
+	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter $(obj-y),$^)
 endif
 endif
 
@@ -145,10 +145,10 @@ targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
 targets += $(MAKECMDGOALS)
 
 built_in_bin.o: $(obj-bin-y) $(extra-y)
-ifeq ($(obj-bin-y),)
+ifeq ($(strip $(obj-bin-y)),)
 	$(CC) $(a_flags) -c -x assembler /dev/null -o $@
 else
-	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
+	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter $(obj-bin-y),$^)
 endif
 
 # Force execution of pattern rules (for which PHONY cannot be directly used).
-- 
2.22.0

[PATCH v3 2/8] lib: collect library files in an archive

[Jan Beulich]

In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
just to avoid bloating binaries when only some arch-es and/or
configurations need generic library routines, combine objects under lib/
into an archive, which the linker then can pick the necessary objects
out of.

Note that we can't use thin archives just yet, until we've raised the
minimum required binutils version suitably.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/Rules.mk          | 29 +++++++++++++++++++++++++----
 xen/arch/arm/Makefile |  6 +++---
 xen/arch/x86/Makefile |  8 ++++----
 xen/lib/Makefile      |  3 ++-
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index d5e5eb33de39..aba6ca2a90f5 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -41,12 +41,16 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
 ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
 
+ALL_LIBS-y               := $(BASEDIR)/lib/lib.a
+
 # Initialise some variables
+lib-y :=
 targets :=
 CFLAGS-y :=
 AFLAGS-y :=
 
 ALL_OBJS := $(ALL_OBJS-y)
+ALL_LIBS := $(ALL_LIBS-y)
 
 SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
                                             $(foreach w,1 2 4, \
@@ -60,7 +64,14 @@ include Makefile
 # ---------------------------------------------------------------------------
 
 quiet_cmd_ld = LD      $@
-cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
+cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
+               --start-group $(filter %.a,$(real-prereqs)) --end-group
+
+# Archive
+# ---------------------------------------------------------------------------
+
+quiet_cmd_ar = AR      $@
+cmd_ar = rm -f $@; $(AR) cPrs $@ $(real-prereqs)
 
 # Objcopy
 # ---------------------------------------------------------------------------
@@ -86,6 +97,10 @@ obj-y    := $(patsubst %/, %/built_in.o, $(obj-y))
 # tell kbuild to descend
 subdir-obj-y := $(filter %/built_in.o, $(obj-y))
 
+# Libraries are always collected in one lib file.
+# Filter out objects already built-in
+lib-y := $(filter-out $(obj-y), $(sort $(lib-y)))
+
 $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS_ONLY
 
 ifeq ($(CONFIG_COVERAGE),y)
@@ -129,7 +144,7 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
 c_flags += $(CFLAGS-y)
 a_flags += $(CFLAGS-y) $(AFLAGS-y)
 
-built_in.o: $(obj-y) $(extra-y)
+built_in.o: $(obj-y) $(if $(strip $(lib-y)),lib.a) $(extra-y)
 ifeq ($(strip $(obj-y)),)
 	$(CC) $(c_flags) -c -x c /dev/null -o $@
 else
@@ -140,8 +155,14 @@ else
 endif
 endif
 
+lib.a: $(lib-y) FORCE
+	$(call if_changed,ar)
+
 targets += built_in.o
-targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
+ifneq ($(strip $(lib-y)),)
+targets += lib.a
+endif
+targets += $(filter-out $(subdir-obj-y), $(obj-y) $(lib-y)) $(extra-y)
 targets += $(MAKECMDGOALS)
 
 built_in_bin.o: $(obj-bin-y) $(extra-y)
@@ -155,7 +176,7 @@ endif
 PHONY += FORCE
 FORCE:
 
-%/built_in.o: FORCE
+%/built_in.o %/lib.a: FORCE
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o
 
 %/built_in_bin.o: FORCE
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 296c5e68bbc3..612a83b315c8 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -90,14 +90,14 @@ endif
 
 ifeq ($(CONFIG_LTO),y)
 # Gather all LTO objects together
-prelink_lto.o: $(ALL_OBJS)
-	$(LD_LTO) -r -o $@ $^
+prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
+	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
 	$(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) FORCE
+prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
 	$(call if_changed,ld)
 endif
 
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 9b368632fb43..8f2180485b2b 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -132,8 +132,8 @@ EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
 
 ifeq ($(CONFIG_LTO),y)
 # Gather all LTO objects together
-prelink_lto.o: $(ALL_OBJS)
-	$(LD_LTO) -r -o $@ $^
+prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
+	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
 
 # Link it with all the binary objects
 prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
@@ -142,10 +142,10 @@ prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $
 prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
 	$(call if_changed,ld)
 else
-prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
+prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE
 	$(call if_changed,ld)
 
-prelink-efi.o: $(ALL_OBJS) FORCE
+prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
 	$(call if_changed,ld)
 endif
 
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 53b1da025e0d..b8814361d63e 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,2 +1,3 @@
-obj-y += ctype.o
 obj-$(CONFIG_X86) += x86/
+
+lib-y += ctype.o

[PATCH v3 3/8] lib: move list sorting code

[Jan Beulich]

Build the source file always, as by putting it into an archive it still
won't be linked into final binaries when not needed. This way possible
build breakage will be easier to notice, and it's more consistent with
us unconditionally building other library kind of code (e.g. sort() or
bsearch()).

While moving the source file, take the opportunity and drop the
pointless EXPORT_SYMBOL() and an unnecessary #include.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/arm/Kconfig                        | 4 +---
 xen/common/Kconfig                          | 3 ---
 xen/common/Makefile                         | 1 -
 xen/lib/Makefile                            | 1 +
 xen/{common/list_sort.c => lib/list-sort.c} | 2 --
 5 files changed, 2 insertions(+), 9 deletions(-)
 rename xen/{common/list_sort.c => lib/list-sort.c} (98%)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index f5b1bcda0323..38b6c31ba5dd 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -56,9 +56,7 @@ config HVM
         def_bool y
 
 config NEW_VGIC
-	bool
-	prompt "Use new VGIC implementation"
-	select NEEDS_LIST_SORT
+	bool "Use new VGIC implementation"
 	---help---
 
 	This is an alternative implementation of the ARM GIC interrupt
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 3e2cf2508899..0661328a99e7 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -66,9 +66,6 @@ config MEM_ACCESS
 config NEEDS_LIBELF
 	bool
 
-config NEEDS_LIST_SORT
-	bool
-
 menu "Speculative hardening"
 
 config SPECULATIVE_HARDEN_ARRAY
diff --git a/xen/common/Makefile b/xen/common/Makefile
index d109f279a490..332e7d667cec 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -21,7 +21,6 @@ obj-y += keyhandler.o
 obj-$(CONFIG_KEXEC) += kexec.o
 obj-$(CONFIG_KEXEC) += kimage.o
 obj-y += lib.o
-obj-$(CONFIG_NEEDS_LIST_SORT) += list_sort.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
 obj-$(CONFIG_MEM_ACCESS) += mem_access.o
 obj-y += memory.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index b8814361d63e..764f3624b5f9 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_X86) += x86/
 
 lib-y += ctype.o
+lib-y += list-sort.o
diff --git a/xen/common/list_sort.c b/xen/lib/list-sort.c
similarity index 98%
rename from xen/common/list_sort.c
rename to xen/lib/list-sort.c
index af2b2f6519f1..f8d8bbf28178 100644
--- a/xen/common/list_sort.c
+++ b/xen/lib/list-sort.c
@@ -15,7 +15,6 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/lib.h>
 #include <xen/list.h>
 
 #define MAX_LIST_LENGTH_BITS 20
@@ -154,4 +153,3 @@ void list_sort(void *priv, struct list_head *head,
 
 	merge_and_restore_back_links(priv, cmp, head, part[max_lev], list);
 }
-EXPORT_SYMBOL(list_sort);

[PATCH v3 4/8] lib: move parse_size_and_unit()

[Jan Beulich]

... into its own CU, to build it into an archive.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/lib.c     | 39 ----------------------------------
 xen/lib/Makefile     |  1 +
 xen/lib/parse-size.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 39 deletions(-)
 create mode 100644 xen/lib/parse-size.c

diff --git a/xen/common/lib.c b/xen/common/lib.c
index a224efa8f6e8..6cfa332142a5 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -423,45 +423,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 #endif
 }
 
-unsigned long long parse_size_and_unit(const char *s, const char **ps)
-{
-    unsigned long long ret;
-    const char *s1;
-
-    ret = simple_strtoull(s, &s1, 0);
-
-    switch ( *s1 )
-    {
-    case 'T': case 't':
-        ret <<= 10;
-        /* fallthrough */
-    case 'G': case 'g':
-        ret <<= 10;
-        /* fallthrough */
-    case 'M': case 'm':
-        ret <<= 10;
-        /* fallthrough */
-    case 'K': case 'k':
-        ret <<= 10;
-        /* fallthrough */
-    case 'B': case 'b':
-        s1++;
-        break;
-    case '%':
-        if ( ps )
-            break;
-        /* fallthrough */
-    default:
-        ret <<= 10; /* default to kB */
-        break;
-    }
-
-    if ( ps != NULL )
-        *ps = s1;
-
-    return ret;
-}
-
 typedef void (*ctor_func_t)(void);
 extern const ctor_func_t __ctors_start[], __ctors_end[];
 
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 764f3624b5f9..99f857540c99 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_X86) += x86/
 
 lib-y += ctype.o
 lib-y += list-sort.o
+lib-y += parse-size.o
diff --git a/xen/lib/parse-size.c b/xen/lib/parse-size.c
new file mode 100644
index 000000000000..ec980cadfff3
--- /dev/null
+++ b/xen/lib/parse-size.c
@@ -0,0 +1,50 @@
+#include <xen/lib.h>
+
+unsigned long long parse_size_and_unit(const char *s, const char **ps)
+{
+    unsigned long long ret;
+    const char *s1;
+
+    ret = simple_strtoull(s, &s1, 0);
+
+    switch ( *s1 )
+    {
+    case 'T': case 't':
+        ret <<= 10;
+        /* fallthrough */
+    case 'G': case 'g':
+        ret <<= 10;
+        /* fallthrough */
+    case 'M': case 'm':
+        ret <<= 10;
+        /* fallthrough */
+    case 'K': case 'k':
+        ret <<= 10;
+        /* fallthrough */
+    case 'B': case 'b':
+        s1++;
+        break;
+    case '%':
+        if ( ps )
+            break;
+        /* fallthrough */
+    default:
+        ret <<= 10; /* default to kB */
+        break;
+    }
+
+    if ( ps != NULL )
+        *ps = s1;
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

[PATCH v3 5/8] lib: move init_constructors()

[Jan Beulich]

... into its own CU, for being unrelated to other things in
common/lib.c.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/lib.c | 14 --------------
 xen/lib/Makefile |  1 +
 xen/lib/ctors.c  | 25 +++++++++++++++++++++++++
 3 files changed, 26 insertions(+), 14 deletions(-)
 create mode 100644 xen/lib/ctors.c

diff --git a/xen/common/lib.c b/xen/common/lib.c
index 6cfa332142a5..f5ca179a0af4 100644
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -1,6 +1,5 @@
 #include <xen/lib.h>
 #include <xen/types.h>
-#include <xen/init.h>
 #include <asm/byteorder.h>
 
 /*
@@ -423,19 +422,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 #endif
 }
 
-typedef void (*ctor_func_t)(void);
-extern const ctor_func_t __ctors_start[], __ctors_end[];
-
-void __init init_constructors(void)
-{
-    const ctor_func_t *f;
-    for ( f = __ctors_start; f < __ctors_end; ++f )
-        (*f)();
-
-    /* Putting this here seems as good (or bad) as any other place. */
-    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 99f857540c99..72c72fffecf2 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_X86) += x86/
 
+lib-y += ctors.o
 lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
diff --git a/xen/lib/ctors.c b/xen/lib/ctors.c
new file mode 100644
index 000000000000..5bdc591cd50a
--- /dev/null
+++ b/xen/lib/ctors.c
@@ -0,0 +1,25 @@
+#include <xen/init.h>
+#include <xen/lib.h>
+
+typedef void (*ctor_func_t)(void);
+extern const ctor_func_t __ctors_start[], __ctors_end[];
+
+void __init init_constructors(void)
+{
+    const ctor_func_t *f;
+    for ( f = __ctors_start; f < __ctors_end; ++f )
+        (*f)();
+
+    /* Putting this here seems as good (or bad) as any other place. */
+    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

[PATCH v3 6/8] lib: move rbtree code

[Jan Beulich]

Build this code into an archive, which results in not linking it into
x86 final binaries. This saves about 1.5k of dead code.

While moving the source file, take the opportunity and drop the
pointless EXPORT_SYMBOL() and an instance of trailing whitespace.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/Makefile          | 1 -
 xen/lib/Makefile             | 1 +
 xen/{common => lib}/rbtree.c | 9 +--------
 3 files changed, 2 insertions(+), 9 deletions(-)
 rename xen/{common => lib}/rbtree.c (98%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 332e7d667cec..d65c9fe9cb4e 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -33,7 +33,6 @@ obj-y += preempt.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
-obj-y += rbtree.o
 obj-y += rcupdate.o
 obj-y += rwlock.o
 obj-y += shutdown.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index 72c72fffecf2..b0fe8c72acf5 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -4,3 +4,4 @@ lib-y += ctors.o
 lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
+lib-y += rbtree.o
diff --git a/xen/common/rbtree.c b/xen/lib/rbtree.c
similarity index 98%
rename from xen/common/rbtree.c
rename to xen/lib/rbtree.c
index 9f5498a89d4e..95e045d52461 100644
--- a/xen/common/rbtree.c
+++ b/xen/lib/rbtree.c
@@ -25,7 +25,7 @@
 #include <xen/rbtree.h>
 
 /*
- * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree 
+ * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree
  *
  *  1) A node is either red or black
  *  2) The root is black
@@ -223,7 +223,6 @@ void rb_insert_color(struct rb_node *node, struct rb_root *root)
 		}
 	}
 }
-EXPORT_SYMBOL(rb_insert_color);
 
 static void __rb_erase_color(struct rb_node *parent, struct rb_root *root)
 {
@@ -467,7 +466,6 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
 	if (rebalance)
 		__rb_erase_color(rebalance, root);
 }
-EXPORT_SYMBOL(rb_erase);
 
 /*
  * This function returns the first node (in sort order) of the tree.
@@ -483,7 +481,6 @@ struct rb_node *rb_first(const struct rb_root *root)
 		n = n->rb_left;
 	return n;
 }
-EXPORT_SYMBOL(rb_first);
 
 struct rb_node *rb_last(const struct rb_root *root)
 {
@@ -496,7 +493,6 @@ struct rb_node *rb_last(const struct rb_root *root)
 		n = n->rb_right;
 	return n;
 }
-EXPORT_SYMBOL(rb_last);
 
 struct rb_node *rb_next(const struct rb_node *node)
 {
@@ -528,7 +524,6 @@ struct rb_node *rb_next(const struct rb_node *node)
 
 	return parent;
 }
-EXPORT_SYMBOL(rb_next);
 
 struct rb_node *rb_prev(const struct rb_node *node)
 {
@@ -557,7 +552,6 @@ struct rb_node *rb_prev(const struct rb_node *node)
 
 	return parent;
 }
-EXPORT_SYMBOL(rb_prev);
 
 void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 		     struct rb_root *root)
@@ -574,4 +568,3 @@ void rb_replace_node(struct rb_node *victim, struct rb_node *new,
 	/* Copy the pointers/colour from the victim to the replacement */
 	*new = *victim;
 }
-EXPORT_SYMBOL(rb_replace_node);

[PATCH v3 7/8] lib: move bsearch code

[Jan Beulich]

Convert this code to an inline function (backed by an instance in an
archive in case the compiler decides against inlining), which results
in not having it in x86 final binaries. This saves a little bit of dead
code.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/Makefile        |  1 -
 xen/common/bsearch.c       | 51 --------------------------------------
 xen/include/xen/compiler.h |  1 +
 xen/include/xen/lib.h      | 42 ++++++++++++++++++++++++++++++-
 xen/lib/Makefile           |  1 +
 xen/lib/bsearch.c          | 13 ++++++++++
 6 files changed, 56 insertions(+), 53 deletions(-)
 delete mode 100644 xen/common/bsearch.c
 create mode 100644 xen/lib/bsearch.c

diff --git a/xen/common/Makefile b/xen/common/Makefile
index d65c9fe9cb4e..e8ce23acea67 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -1,6 +1,5 @@
 obj-$(CONFIG_ARGO) += argo.o
 obj-y += bitmap.o
-obj-y += bsearch.o
 obj-$(CONFIG_HYPFS_CONFIG) += config_data.o
 obj-$(CONFIG_CORE_PARKING) += core_parking.o
 obj-y += cpu.o
diff --git a/xen/common/bsearch.c b/xen/common/bsearch.c
deleted file mode 100644
index 7090930aab5c..000000000000
--- a/xen/common/bsearch.c
+++ /dev/null
@@ -1,51 +0,0 @@
-/*
- * A generic implementation of binary search for the Linux kernel
- *
- * Copyright (C) 2008-2009 Ksplice, Inc.
- * Author: Tim Abbott <tabbott@ksplice.com>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; version 2.
- */
-
-#include <xen/lib.h>
-
-/*
- * bsearch - binary search an array of elements
- * @key: pointer to item being searched for
- * @base: pointer to first element to search
- * @num: number of elements
- * @size: size of each element
- * @cmp: pointer to comparison function
- *
- * This function does a binary search on the given array.  The
- * contents of the array should already be in ascending sorted order
- * under the provided comparison function.
- *
- * Note that the key need not have the same type as the elements in
- * the array, e.g. key could be a string and the comparison function
- * could compare the string with the struct's name field.  However, if
- * the key and elements in the array are of the same type, you can use
- * the same comparison function for both sort() and bsearch().
- */
-void *bsearch(const void *key, const void *base, size_t num, size_t size,
-	      int (*cmp)(const void *key, const void *elt))
-{
-	size_t start = 0, end = num;
-	int result;
-
-	while (start < end) {
-		size_t mid = start + (end - start) / 2;
-
-		result = cmp(key, base + mid * size);
-		if (result < 0)
-			end = mid;
-		else if (result > 0)
-			start = mid + 1;
-		else
-			return (void *)base + mid * size;
-	}
-
-	return NULL;
-}
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index c0e0ee9f27be..2b7acdf3b188 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -12,6 +12,7 @@
 
 #define inline        __inline__
 #define always_inline __inline__ __attribute__ ((__always_inline__))
+#define gnu_inline    __inline__ __attribute__ ((__gnu_inline__))
 #define noinline      __attribute__((__noinline__))
 
 #define noreturn      __attribute__((__noreturn__))
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index a9679c913d5c..48429b69b8df 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -204,8 +204,48 @@ void dump_execstate(struct cpu_user_regs *);
 
 void init_constructors(void);
 
+/*
+ * bsearch - binary search an array of elements
+ * @key: pointer to item being searched for
+ * @base: pointer to first element to search
+ * @num: number of elements
+ * @size: size of each element
+ * @cmp: pointer to comparison function
+ *
+ * This function does a binary search on the given array.  The
+ * contents of the array should already be in ascending sorted order
+ * under the provided comparison function.
+ *
+ * Note that the key need not have the same type as the elements in
+ * the array, e.g. key could be a string and the comparison function
+ * could compare the string with the struct's name field.  However, if
+ * the key and elements in the array are of the same type, you can use
+ * the same comparison function for both sort() and bsearch().
+ */
+#ifndef BSEARCH_IMPLEMENTATION
+extern gnu_inline
+#endif
 void *bsearch(const void *key, const void *base, size_t num, size_t size,
-              int (*cmp)(const void *key, const void *elt));
+              int (*cmp)(const void *key, const void *elt))
+{
+    size_t start = 0, end = num;
+    int result;
+
+    while ( start < end )
+    {
+        size_t mid = start + (end - start) / 2;
+
+        result = cmp(key, base + mid * size);
+        if ( result < 0 )
+            end = mid;
+        else if ( result > 0 )
+            start = mid + 1;
+        else
+            return (void *)base + mid * size;
+    }
+
+    return NULL;
+}
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index b0fe8c72acf5..f12dab7a737a 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_X86) += x86/
 
+lib-y += bsearch.o
 lib-y += ctors.o
 lib-y += ctype.o
 lib-y += list-sort.o
diff --git a/xen/lib/bsearch.c b/xen/lib/bsearch.c
new file mode 100644
index 000000000000..149f7feafd1f
--- /dev/null
+++ b/xen/lib/bsearch.c
@@ -0,0 +1,13 @@
+/*
+ * A generic implementation of binary search for the Linux kernel
+ *
+ * Copyright (C) 2008-2009 Ksplice, Inc.
+ * Author: Tim Abbott <tabbott@ksplice.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; version 2.
+ */
+
+#define BSEARCH_IMPLEMENTATION
+#include <xen/lib.h>

[PATCH v3 8/8] lib: move sort code

[Jan Beulich]

Build this code into an archive, partly paralleling bsearch().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/Makefile        | 1 -
 xen/lib/Makefile           | 1 +
 xen/{common => lib}/sort.c | 0
 3 files changed, 1 insertion(+), 1 deletion(-)
 rename xen/{common => lib}/sort.c (100%)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index e8ce23acea67..7a4e652b575e 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -36,7 +36,6 @@ obj-y += rcupdate.o
 obj-y += rwlock.o
 obj-y += shutdown.o
 obj-y += softirq.o
-obj-y += sort.o
 obj-y += smp.o
 obj-y += spinlock.o
 obj-y += stop_machine.o
diff --git a/xen/lib/Makefile b/xen/lib/Makefile
index f12dab7a737a..42cf7a1164ef 100644
--- a/xen/lib/Makefile
+++ b/xen/lib/Makefile
@@ -6,3 +6,4 @@ lib-y += ctype.o
 lib-y += list-sort.o
 lib-y += parse-size.o
 lib-y += rbtree.o
+lib-y += sort.o
diff --git a/xen/common/sort.c b/xen/lib/sort.c
similarity index 100%
rename from xen/common/sort.c
rename to xen/lib/sort.c

Re: [PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Wei Liu]

On Mon, Nov 23, 2020 at 04:16:02PM +0100, Jan Beulich wrote:
[...]
> 
> 1: xen: fix build when $(obj-y) consists of just blanks
> 2: lib: collect library files in an archive
> 3: lib: move list sorting code
> 4: lib: move parse_size_and_unit()
> 5: lib: move init_constructors()
> 6: lib: move rbtree code
> 7: lib: move bsearch code
> 8: lib: move sort code

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Bertrand Marquis]

Hi Jan,

I will review this today, sorry for the delay.

Regards
Bertrand

> On 23 Nov 2020, at 15:16, Jan Beulich <jbeulich@suse.com> wrote:
> 
> In a few cases we link in library-like functions when they're not
> actually needed. While we could use Kconfig options for each one
> of them, I think the better approach for such generic code is to
> build it always (thus making sure a build issue can't be introduced
> for these in any however exotic configuration) and then put it into
> an archive, for the linker to pick up as needed. The series here
> presents a first few tiny steps towards such a goal.
> 
> Note that we can't use thin archives yet, due to our tool chain
> (binutils) baseline being too low.
> 
> Further almost immediate steps I'd like to take if the approach
> meets no opposition are
> - split and move the rest of common/lib.c,
> - split and move common/string.c, dropping the need for all the
>  __HAVE_ARCH_* (implying possible per-arch archives then need to
>  be specified ahead of lib/lib.a on the linker command lines),
> - move common/libelf/ and common/libfdt/.
> 
> v3 has a new 1st patch and some review feedback addressed. See
> individual patches.
> 
> 1: xen: fix build when $(obj-y) consists of just blanks
> 2: lib: collect library files in an archive
> 3: lib: move list sorting code
> 4: lib: move parse_size_and_unit()
> 5: lib: move init_constructors()
> 6: lib: move rbtree code
> 7: lib: move bsearch code
> 8: lib: move sort code
> 
> Jan
> 

Re: [PATCH v3 1/8] xen: fix build when $(obj-y) consists of just blanks

[Bertrand Marquis]

Hi Jan,

> On 23 Nov 2020, at 15:20, Jan Beulich <jbeulich@suse.com> wrote:
> 
> This case can occur when combining empty lists
> 
> obj-y :=
> ...
> obj-y += $(empty)
> 
> or
> 
> obj-y := $(empty) $(empty)
> 
> where (only) blanks would accumulate. This was only a latent issue until
> now, but would become an active issue for Arm once lib/ gets populated
> with all respective objects going into the to be introduced lib.a.
> 
> Also address a related issue at this occasion: When an empty built_in.o
> gets created, .built_in.o.d will have its dependencies recorded. If, on
> a subsequent incremental build, an actual constituent of built_in.o
> appeared, the $(filter-out ) would leave these recorded dependencies in
> place. But of course the linker won't know what to do with C header
> files. (The apparent alternative of avoiding to pass $(c_flags) or
> $(a_flags) would not be reliable afaict, as among these flags there may
> be some affecting information conveyed via the object file to the
> linker. The linker, finding inconsistent flags across object files, may
> then error out.) Using just $(obj-y) won't work either: It breaks when
> the same object file is listed more than once.
> 
> Reported-by: Julien Grall <julien@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/Rules.mk | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index 333e19bec343..d5e5eb33de39 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -130,13 +130,13 @@ c_flags += $(CFLAGS-y)
> a_flags += $(CFLAGS-y) $(AFLAGS-y)
> 
> built_in.o: $(obj-y) $(extra-y)
> -ifeq ($(obj-y),)
> +ifeq ($(strip $(obj-y)),)
> 	$(CC) $(c_flags) -c -x c /dev/null -o $@
> else
> ifeq ($(CONFIG_LTO),y)
> -	$(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^)
> +	$(LD_LTO) -r -o $@ $(filter $(obj-y),$^)
> else
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
> +	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter $(obj-y),$^)
> endif
> endif
> 
> @@ -145,10 +145,10 @@ targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
> targets += $(MAKECMDGOALS)
> 
> built_in_bin.o: $(obj-bin-y) $(extra-y)
> -ifeq ($(obj-bin-y),)
> +ifeq ($(strip $(obj-bin-y)),)
> 	$(CC) $(a_flags) -c -x assembler /dev/null -o $@
> else
> -	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
> +	$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter $(obj-bin-y),$^)
> endif
> 
> # Force execution of pattern rules (for which PHONY cannot be directly used).
> -- 
> 2.22.0
> 
> 
> 

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Bertrand Marquis]

Hi,

> On 23 Nov 2020, at 15:21, Jan Beulich <jbeulich@suse.com> wrote:
> 
> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
> just to avoid bloating binaries when only some arch-es and/or
> configurations need generic library routines, combine objects under lib/
> into an archive, which the linker then can pick the necessary objects
> out of.
> 
> Note that we can't use thin archives just yet, until we've raised the
> minimum required binutils version suitably.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

with one remark...


> ---
> xen/Rules.mk          | 29 +++++++++++++++++++++++++----
> xen/arch/arm/Makefile |  6 +++---
> xen/arch/x86/Makefile |  8 ++++----
> xen/lib/Makefile      |  3 ++-
> 4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index d5e5eb33de39..aba6ca2a90f5 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -41,12 +41,16 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
> ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
> ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
> 
> +ALL_LIBS-y               := $(BASEDIR)/lib/lib.a
> +
> # Initialise some variables
> +lib-y :=
> targets :=
> CFLAGS-y :=
> AFLAGS-y :=
> 
> ALL_OBJS := $(ALL_OBJS-y)
> +ALL_LIBS := $(ALL_LIBS-y)
> 
> SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
>                                             $(foreach w,1 2 4, \
> @@ -60,7 +64,14 @@ include Makefile
> # ---------------------------------------------------------------------------
> 
> quiet_cmd_ld = LD      $@
> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
> +               --start-group $(filter %.a,$(real-prereqs)) --end-group

This might be a good idea to add a comment to explain why the start/end-group
is needed so that someone does not change this back in the future.

Something like: put libraries between start/end group to have unused symbols removed.

Cheers
Bertrand

> +
> +# Archive
> +# ---------------------------------------------------------------------------
> +
> +quiet_cmd_ar = AR      $@
> +cmd_ar = rm -f $@; $(AR) cPrs $@ $(real-prereqs)
> 
> # Objcopy
> # ---------------------------------------------------------------------------
> @@ -86,6 +97,10 @@ obj-y    := $(patsubst %/, %/built_in.o, $(obj-y))
> # tell kbuild to descend
> subdir-obj-y := $(filter %/built_in.o, $(obj-y))
> 
> +# Libraries are always collected in one lib file.
> +# Filter out objects already built-in
> +lib-y := $(filter-out $(obj-y), $(sort $(lib-y)))
> +
> $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS_ONLY
> 
> ifeq ($(CONFIG_COVERAGE),y)
> @@ -129,7 +144,7 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
> c_flags += $(CFLAGS-y)
> a_flags += $(CFLAGS-y) $(AFLAGS-y)
> 
> -built_in.o: $(obj-y) $(extra-y)
> +built_in.o: $(obj-y) $(if $(strip $(lib-y)),lib.a) $(extra-y)
> ifeq ($(strip $(obj-y)),)
> 	$(CC) $(c_flags) -c -x c /dev/null -o $@
> else
> @@ -140,8 +155,14 @@ else
> endif
> endif
> 
> +lib.a: $(lib-y) FORCE
> +	$(call if_changed,ar)
> +
> targets += built_in.o
> -targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
> +ifneq ($(strip $(lib-y)),)
> +targets += lib.a
> +endif
> +targets += $(filter-out $(subdir-obj-y), $(obj-y) $(lib-y)) $(extra-y)
> targets += $(MAKECMDGOALS)
> 
> built_in_bin.o: $(obj-bin-y) $(extra-y)
> @@ -155,7 +176,7 @@ endif
> PHONY += FORCE
> FORCE:
> 
> -%/built_in.o: FORCE
> +%/built_in.o %/lib.a: FORCE
> 	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o
> 
> %/built_in_bin.o: FORCE
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 296c5e68bbc3..612a83b315c8 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -90,14 +90,14 @@ endif
> 
> ifeq ($(CONFIG_LTO),y)
> # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -	$(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
> 
> # Link it with all the binary objects
> prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
> 	$(call if_changed,ld)
> else
> -prelink.o: $(ALL_OBJS) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
> 	$(call if_changed,ld)
> endif
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 9b368632fb43..8f2180485b2b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -132,8 +132,8 @@ EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
> 
> ifeq ($(CONFIG_LTO),y)
> # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -	$(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
> 
> # Link it with all the binary objects
> prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
> @@ -142,10 +142,10 @@ prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $
> prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
> 	$(call if_changed,ld)
> else
> -prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE
> 	$(call if_changed,ld)
> 
> -prelink-efi.o: $(ALL_OBJS) FORCE
> +prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
> 	$(call if_changed,ld)
> endif
> 
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 53b1da025e0d..b8814361d63e 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -1,2 +1,3 @@
> -obj-y += ctype.o
> obj-$(CONFIG_X86) += x86/
> +
> +lib-y += ctype.o
> 
> 

Re: [PATCH v3 3/8] lib: move list sorting code

[Bertrand Marquis]

> On 23 Nov 2020, at 15:21, Jan Beulich <jbeulich@suse.com> wrote:
> 
> Build the source file always, as by putting it into an archive it still
> won't be linked into final binaries when not needed. This way possible
> build breakage will be easier to notice, and it's more consistent with
> us unconditionally building other library kind of code (e.g. sort() or
> bsearch()).
> 
> While moving the source file, take the opportunity and drop the
> pointless EXPORT_SYMBOL() and an unnecessary #include.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/Kconfig                        | 4 +---
> xen/common/Kconfig                          | 3 ---
> xen/common/Makefile                         | 1 -
> xen/lib/Makefile                            | 1 +
> xen/{common/list_sort.c => lib/list-sort.c} | 2 --
> 5 files changed, 2 insertions(+), 9 deletions(-)
> rename xen/{common/list_sort.c => lib/list-sort.c} (98%)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index f5b1bcda0323..38b6c31ba5dd 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -56,9 +56,7 @@ config HVM
>         def_bool y
> 
> config NEW_VGIC
> -	bool
> -	prompt "Use new VGIC implementation"
> -	select NEEDS_LIST_SORT
> +	bool "Use new VGIC implementation"
> 	---help---
> 
> 	This is an alternative implementation of the ARM GIC interrupt
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 3e2cf2508899..0661328a99e7 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -66,9 +66,6 @@ config MEM_ACCESS
> config NEEDS_LIBELF
> 	bool
> 
> -config NEEDS_LIST_SORT
> -	bool
> -
> menu "Speculative hardening"
> 
> config SPECULATIVE_HARDEN_ARRAY
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index d109f279a490..332e7d667cec 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -21,7 +21,6 @@ obj-y += keyhandler.o
> obj-$(CONFIG_KEXEC) += kexec.o
> obj-$(CONFIG_KEXEC) += kimage.o
> obj-y += lib.o
> -obj-$(CONFIG_NEEDS_LIST_SORT) += list_sort.o
> obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
> obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> obj-y += memory.o
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index b8814361d63e..764f3624b5f9 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -1,3 +1,4 @@
> obj-$(CONFIG_X86) += x86/
> 
> lib-y += ctype.o
> +lib-y += list-sort.o
> diff --git a/xen/common/list_sort.c b/xen/lib/list-sort.c
> similarity index 98%
> rename from xen/common/list_sort.c
> rename to xen/lib/list-sort.c
> index af2b2f6519f1..f8d8bbf28178 100644
> --- a/xen/common/list_sort.c
> +++ b/xen/lib/list-sort.c
> @@ -15,7 +15,6 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
> 
> -#include <xen/lib.h>
> #include <xen/list.h>
> 
> #define MAX_LIST_LENGTH_BITS 20
> @@ -154,4 +153,3 @@ void list_sort(void *priv, struct list_head *head,
> 
> 	merge_and_restore_back_links(priv, cmp, head, part[max_lev], list);
> }
> -EXPORT_SYMBOL(list_sort);
> 
> 

Re: [PATCH v3 4/8] lib: move parse_size_and_unit()

[Bertrand Marquis]

> On 23 Nov 2020, at 15:22, Jan Beulich <jbeulich@suse.com> wrote:
> 
> ... into its own CU, to build it into an archive.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/common/lib.c     | 39 ----------------------------------
> xen/lib/Makefile     |  1 +
> xen/lib/parse-size.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 51 insertions(+), 39 deletions(-)
> create mode 100644 xen/lib/parse-size.c
> 
> diff --git a/xen/common/lib.c b/xen/common/lib.c
> index a224efa8f6e8..6cfa332142a5 100644
> --- a/xen/common/lib.c
> +++ b/xen/common/lib.c
> @@ -423,45 +423,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> #endif
> }
> 
> -unsigned long long parse_size_and_unit(const char *s, const char **ps)
> -{
> -    unsigned long long ret;
> -    const char *s1;
> -
> -    ret = simple_strtoull(s, &s1, 0);
> -
> -    switch ( *s1 )
> -    {
> -    case 'T': case 't':
> -        ret <<= 10;
> -        /* fallthrough */
> -    case 'G': case 'g':
> -        ret <<= 10;
> -        /* fallthrough */
> -    case 'M': case 'm':
> -        ret <<= 10;
> -        /* fallthrough */
> -    case 'K': case 'k':
> -        ret <<= 10;
> -        /* fallthrough */
> -    case 'B': case 'b':
> -        s1++;
> -        break;
> -    case '%':
> -        if ( ps )
> -            break;
> -        /* fallthrough */
> -    default:
> -        ret <<= 10; /* default to kB */
> -        break;
> -    }
> -
> -    if ( ps != NULL )
> -        *ps = s1;
> -
> -    return ret;
> -}
> -
> typedef void (*ctor_func_t)(void);
> extern const ctor_func_t __ctors_start[], __ctors_end[];
> 
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 764f3624b5f9..99f857540c99 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_X86) += x86/
> 
> lib-y += ctype.o
> lib-y += list-sort.o
> +lib-y += parse-size.o
> diff --git a/xen/lib/parse-size.c b/xen/lib/parse-size.c
> new file mode 100644
> index 000000000000..ec980cadfff3
> --- /dev/null
> +++ b/xen/lib/parse-size.c
> @@ -0,0 +1,50 @@
> +#include <xen/lib.h>
> +
> +unsigned long long parse_size_and_unit(const char *s, const char **ps)
> +{
> +    unsigned long long ret;
> +    const char *s1;
> +
> +    ret = simple_strtoull(s, &s1, 0);
> +
> +    switch ( *s1 )
> +    {
> +    case 'T': case 't':
> +        ret <<= 10;
> +        /* fallthrough */
> +    case 'G': case 'g':
> +        ret <<= 10;
> +        /* fallthrough */
> +    case 'M': case 'm':
> +        ret <<= 10;
> +        /* fallthrough */
> +    case 'K': case 'k':
> +        ret <<= 10;
> +        /* fallthrough */
> +    case 'B': case 'b':
> +        s1++;
> +        break;
> +    case '%':
> +        if ( ps )
> +            break;
> +        /* fallthrough */
> +    default:
> +        ret <<= 10; /* default to kB */
> +        break;
> +    }
> +
> +    if ( ps != NULL )
> +        *ps = s1;
> +
> +    return ret;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 
> 

Re: [PATCH v3 5/8] lib: move init_constructors()

[Bertrand Marquis]

Hi Jan,


> On 23 Nov 2020, at 15:22, Jan Beulich <jbeulich@suse.com> wrote:
> 
> ... into its own CU, for being unrelated to other things in
> common/lib.c.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/common/lib.c | 14 --------------
> xen/lib/Makefile |  1 +
> xen/lib/ctors.c  | 25 +++++++++++++++++++++++++
> 3 files changed, 26 insertions(+), 14 deletions(-)
> create mode 100644 xen/lib/ctors.c
> 
> diff --git a/xen/common/lib.c b/xen/common/lib.c
> index 6cfa332142a5..f5ca179a0af4 100644
> --- a/xen/common/lib.c
> +++ b/xen/common/lib.c
> @@ -1,6 +1,5 @@
> #include <xen/lib.h>
> #include <xen/types.h>
> -#include <xen/init.h>
> #include <asm/byteorder.h>
> 
> /*
> @@ -423,19 +422,6 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> #endif
> }
> 
> -typedef void (*ctor_func_t)(void);
> -extern const ctor_func_t __ctors_start[], __ctors_end[];
> -
> -void __init init_constructors(void)
> -{
> -    const ctor_func_t *f;
> -    for ( f = __ctors_start; f < __ctors_end; ++f )
> -        (*f)();
> -
> -    /* Putting this here seems as good (or bad) as any other place. */
> -    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
> -}
> -
> /*
>  * Local variables:
>  * mode: C
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 99f857540c99..72c72fffecf2 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -1,5 +1,6 @@
> obj-$(CONFIG_X86) += x86/
> 
> +lib-y += ctors.o
> lib-y += ctype.o
> lib-y += list-sort.o
> lib-y += parse-size.o
> diff --git a/xen/lib/ctors.c b/xen/lib/ctors.c
> new file mode 100644
> index 000000000000..5bdc591cd50a
> --- /dev/null
> +++ b/xen/lib/ctors.c
> @@ -0,0 +1,25 @@
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +
> +typedef void (*ctor_func_t)(void);
> +extern const ctor_func_t __ctors_start[], __ctors_end[];
> +
> +void __init init_constructors(void)
> +{
> +    const ctor_func_t *f;
> +    for ( f = __ctors_start; f < __ctors_end; ++f )
> +        (*f)();
> +
> +    /* Putting this here seems as good (or bad) as any other place. */
> +    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 
> 

Re: [PATCH v3 6/8] lib: move rbtree code

[Bertrand Marquis]

Hi,

> On 23 Nov 2020, at 15:23, Jan Beulich <jbeulich@suse.com> wrote:
> 
> Build this code into an archive, which results in not linking it into
> x86 final binaries. This saves about 1.5k of dead code.
> 
> While moving the source file, take the opportunity and drop the
> pointless EXPORT_SYMBOL() and an instance of trailing whitespace.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/common/Makefile          | 1 -
> xen/lib/Makefile             | 1 +
> xen/{common => lib}/rbtree.c | 9 +--------
> 3 files changed, 2 insertions(+), 9 deletions(-)
> rename xen/{common => lib}/rbtree.c (98%)
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 332e7d667cec..d65c9fe9cb4e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -33,7 +33,6 @@ obj-y += preempt.o
> obj-y += random.o
> obj-y += rangeset.o
> obj-y += radix-tree.o
> -obj-y += rbtree.o
> obj-y += rcupdate.o
> obj-y += rwlock.o
> obj-y += shutdown.o
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 72c72fffecf2..b0fe8c72acf5 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -4,3 +4,4 @@ lib-y += ctors.o
> lib-y += ctype.o
> lib-y += list-sort.o
> lib-y += parse-size.o
> +lib-y += rbtree.o
> diff --git a/xen/common/rbtree.c b/xen/lib/rbtree.c
> similarity index 98%
> rename from xen/common/rbtree.c
> rename to xen/lib/rbtree.c
> index 9f5498a89d4e..95e045d52461 100644
> --- a/xen/common/rbtree.c
> +++ b/xen/lib/rbtree.c
> @@ -25,7 +25,7 @@
> #include <xen/rbtree.h>
> 
> /*
> - * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree 
> + * red-black trees properties:  http://en.wikipedia.org/wiki/Rbtree
>  *
>  *  1) A node is either red or black
>  *  2) The root is black
> @@ -223,7 +223,6 @@ void rb_insert_color(struct rb_node *node, struct rb_root *root)
> 		}
> 	}
> }
> -EXPORT_SYMBOL(rb_insert_color);
> 
> static void __rb_erase_color(struct rb_node *parent, struct rb_root *root)
> {
> @@ -467,7 +466,6 @@ void rb_erase(struct rb_node *node, struct rb_root *root)
> 	if (rebalance)
> 		__rb_erase_color(rebalance, root);
> }
> -EXPORT_SYMBOL(rb_erase);
> 
> /*
>  * This function returns the first node (in sort order) of the tree.
> @@ -483,7 +481,6 @@ struct rb_node *rb_first(const struct rb_root *root)
> 		n = n->rb_left;
> 	return n;
> }
> -EXPORT_SYMBOL(rb_first);
> 
> struct rb_node *rb_last(const struct rb_root *root)
> {
> @@ -496,7 +493,6 @@ struct rb_node *rb_last(const struct rb_root *root)
> 		n = n->rb_right;
> 	return n;
> }
> -EXPORT_SYMBOL(rb_last);
> 
> struct rb_node *rb_next(const struct rb_node *node)
> {
> @@ -528,7 +524,6 @@ struct rb_node *rb_next(const struct rb_node *node)
> 
> 	return parent;
> }
> -EXPORT_SYMBOL(rb_next);
> 
> struct rb_node *rb_prev(const struct rb_node *node)
> {
> @@ -557,7 +552,6 @@ struct rb_node *rb_prev(const struct rb_node *node)
> 
> 	return parent;
> }
> -EXPORT_SYMBOL(rb_prev);
> 
> void rb_replace_node(struct rb_node *victim, struct rb_node *new,
> 		     struct rb_root *root)
> @@ -574,4 +568,3 @@ void rb_replace_node(struct rb_node *victim, struct rb_node *new,
> 	/* Copy the pointers/colour from the victim to the replacement */
> 	*new = *victim;
> }
> -EXPORT_SYMBOL(rb_replace_node);
> 
> 

Re: [PATCH v3 8/8] lib: move sort code

[Bertrand Marquis]

Hi,

> On 23 Nov 2020, at 15:24, Jan Beulich <jbeulich@suse.com> wrote:
> 
> Build this code into an archive, partly paralleling bsearch().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/common/Makefile        | 1 -
> xen/lib/Makefile           | 1 +
> xen/{common => lib}/sort.c | 0
> 3 files changed, 1 insertion(+), 1 deletion(-)
> rename xen/{common => lib}/sort.c (100%)
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index e8ce23acea67..7a4e652b575e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -36,7 +36,6 @@ obj-y += rcupdate.o
> obj-y += rwlock.o
> obj-y += shutdown.o
> obj-y += softirq.o
> -obj-y += sort.o
> obj-y += smp.o
> obj-y += spinlock.o
> obj-y += stop_machine.o
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index f12dab7a737a..42cf7a1164ef 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -6,3 +6,4 @@ lib-y += ctype.o
> lib-y += list-sort.o
> lib-y += parse-size.o
> lib-y += rbtree.o
> +lib-y += sort.o
> diff --git a/xen/common/sort.c b/xen/lib/sort.c
> similarity index 100%
> rename from xen/common/sort.c
> rename to xen/lib/sort.c
> 
> 

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Jan Beulich]

On 09.12.2020 12:37, Bertrand Marquis wrote:
>> On 23 Nov 2020, at 15:21, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
>> just to avoid bloating binaries when only some arch-es and/or
>> configurations need generic library routines, combine objects under lib/
>> into an archive, which the linker then can pick the necessary objects
>> out of.
>>
>> Note that we can't use thin archives just yet, until we've raised the
>> minimum required binutils version suitably.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks.

>> @@ -60,7 +64,14 @@ include Makefile
>> # ---------------------------------------------------------------------------
>>
>> quiet_cmd_ld = LD      $@
>> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
>> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
>> +               --start-group $(filter %.a,$(real-prereqs)) --end-group
> 
> This might be a good idea to add a comment to explain why the start/end-group
> is needed so that someone does not change this back in the future.

Since we're trying to inherit Linux'es build system, I did look
there and iirc there was no comment, so I didn't see a basis for
us to have one.

> Something like: put libraries between start/end group to have unused symbols removed.

Now that's not the reason - why you describe is the default
behavior for archives, and there is something like a "whole
archive" option iirc to change to a mode where all objects
get pulled out. Instead this is a symbol resolution thing
aiui - by default earlier archives can't resolve undefined
symbols first referenced by objects pulled out of later
archives.

Jan

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Bertrand Marquis]

Hi Jan,

> On 9 Dec 2020, at 14:42, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 09.12.2020 12:37, Bertrand Marquis wrote:
>>> On 23 Nov 2020, at 15:21, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
>>> just to avoid bloating binaries when only some arch-es and/or
>>> configurations need generic library routines, combine objects under lib/
>>> into an archive, which the linker then can pick the necessary objects
>>> out of.
>>> 
>>> Note that we can't use thin archives just yet, until we've raised the
>>> minimum required binutils version suitably.
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Thanks.
> 
>>> @@ -60,7 +64,14 @@ include Makefile
>>> # ---------------------------------------------------------------------------
>>> 
>>> quiet_cmd_ld = LD      $@
>>> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
>>> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
>>> +               --start-group $(filter %.a,$(real-prereqs)) --end-group
>> 
>> This might be a good idea to add a comment to explain why the start/end-group
>> is needed so that someone does not change this back in the future.
> 
> Since we're trying to inherit Linux'es build system, I did look
> there and iirc there was no comment, so I didn't see a basis for
> us to have one.
> 
>> Something like: put libraries between start/end group to have unused symbols removed.
> 
> Now that's not the reason - why you describe is the default
> behavior for archives, and there is something like a "whole
> archive" option iirc to change to a mode where all objects
> get pulled out. Instead this is a symbol resolution thing
> aiui - by default earlier archives can't resolve undefined
> symbols first referenced by objects pulled out of later
> archives.

ah yes i remember seeing that.
Maybe just add your last sentence in the commit message.

Cheers
Bertrand

> 
> Jan

Re: [PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Jan Beulich]

On 09.12.2020 12:33, Bertrand Marquis wrote:
> I will review this today, sorry for the delay.

Thanks for the reviews, and no problem at all. Since iirc it was
you who asked on the last community call, I wanted to point out
that despite your reviews and despite Wei's acks the series
still won't be able to go in, because patches 2 and 3 are still
lacking Arm maintainer acks.

Jan

Re: [PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Julien Grall]

On 09/12/2020 14:47, Jan Beulich wrote:
> On 09.12.2020 12:33, Bertrand Marquis wrote:
>> I will review this today, sorry for the delay.
> 
> Thanks for the reviews, and no problem at all. Since iirc it was
> you who asked on the last community call, I wanted to point out
> that despite your reviews and despite Wei's acks the series
> still won't be able to go in, because patches 2 and 3 are still
> lacking Arm maintainer acks.

I am waiting on Anthony's input before giving my ack on patch 2. I am 
not sure whether he is already on holidays.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 0/8] xen: beginnings of moving library-like code into an archive

[Jan Beulich]

On 09.12.2020 15:51, Julien Grall wrote:
> On 09/12/2020 14:47, Jan Beulich wrote:
>> On 09.12.2020 12:33, Bertrand Marquis wrote:
>>> I will review this today, sorry for the delay.
>>
>> Thanks for the reviews, and no problem at all. Since iirc it was
>> you who asked on the last community call, I wanted to point out
>> that despite your reviews and despite Wei's acks the series
>> still won't be able to go in, because patches 2 and 3 are still
>> lacking Arm maintainer acks.
> 
> I am waiting on Anthony's input before giving my ack on patch 2.

Well, okay. I'll continue to wait then. What about patch 3 though?

Jan

Re: [PATCH v3 1/8] xen: fix build when $(obj-y) consists of just blanks

[Anthony PERARD]

On Mon, Nov 23, 2020 at 04:20:52PM +0100, Jan Beulich wrote:
> This case can occur when combining empty lists
> 
> obj-y :=
> ...
> obj-y += $(empty)
> 
> or
> 
> obj-y := $(empty) $(empty)
> 
> where (only) blanks would accumulate. This was only a latent issue until
> now, but would become an active issue for Arm once lib/ gets populated
> with all respective objects going into the to be introduced lib.a.
> 
> Also address a related issue at this occasion: When an empty built_in.o
> gets created, .built_in.o.d will have its dependencies recorded. If, on
> a subsequent incremental build, an actual constituent of built_in.o
> appeared, the $(filter-out ) would leave these recorded dependencies in
> place. But of course the linker won't know what to do with C header
> files. (The apparent alternative of avoiding to pass $(c_flags) or
> $(a_flags) would not be reliable afaict, as among these flags there may
> be some affecting information conveyed via the object file to the
> linker. The linker, finding inconsistent flags across object files, may

How about using $(XEN_CFLAGS) instead of $(c_flags)? That should prevent
CC from generating the .*.o.d files while keeping the relevant flags. I
was planing to do that to avoid the issue, see:
https://lore.kernel.org/xen-devel/20200421161208.2429539-10-anthony.perard@citrix.com

> then error out.) Using just $(obj-y) won't work either: It breaks when
> the same object file is listed more than once.

Do we need to worry about having a object file been listed twice?
Wouldn't that be a mistake?

-- 
Anthony PERARD

Re: [PATCH v3 1/8] xen: fix build when $(obj-y) consists of just blanks

[Jan Beulich]

On 09.12.2020 18:40, Anthony PERARD wrote:
> On Mon, Nov 23, 2020 at 04:20:52PM +0100, Jan Beulich wrote:
>> This case can occur when combining empty lists
>>
>> obj-y :=
>> ...
>> obj-y += $(empty)
>>
>> or
>>
>> obj-y := $(empty) $(empty)
>>
>> where (only) blanks would accumulate. This was only a latent issue until
>> now, but would become an active issue for Arm once lib/ gets populated
>> with all respective objects going into the to be introduced lib.a.
>>
>> Also address a related issue at this occasion: When an empty built_in.o
>> gets created, .built_in.o.d will have its dependencies recorded. If, on
>> a subsequent incremental build, an actual constituent of built_in.o
>> appeared, the $(filter-out ) would leave these recorded dependencies in
>> place. But of course the linker won't know what to do with C header
>> files. (The apparent alternative of avoiding to pass $(c_flags) or
>> $(a_flags) would not be reliable afaict, as among these flags there may
>> be some affecting information conveyed via the object file to the
>> linker. The linker, finding inconsistent flags across object files, may
> 
> How about using $(XEN_CFLAGS) instead of $(c_flags)? That should prevent
> CC from generating the .*.o.d files while keeping the relevant flags.

What does "relevant" cover? For an empty .o it may not be important
right now, but I could see

c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"'
a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS)

include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk

c_flags += $(CFLAGS-y)
a_flags += $(CFLAGS-y) $(AFLAGS-y)

leading to CFLAGS-y / AFLAGS-y which need to be consistent across
_all_ object files (e.g. some recording of ABI used).

> I
> was planing to do that to avoid the issue, see:
> https://lore.kernel.org/xen-devel/20200421161208.2429539-10-anthony.perard@citrix.com
> 
>> then error out.) Using just $(obj-y) won't work either: It breaks when
>> the same object file is listed more than once.
> 
> Do we need to worry about having a object file been listed twice?
> Wouldn't that be a mistake?

No. The list approach (obj-$(CONFIG_xyz) += ...) easily allows for
this to happen. See xen/arch/x86/mm/Makefile for an existing example.

Jan

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Anthony PERARD]

On Mon, Nov 23, 2020 at 04:21:19PM +0100, Jan Beulich wrote:
> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
> just to avoid bloating binaries when only some arch-es and/or
> configurations need generic library routines, combine objects under lib/
> into an archive, which the linker then can pick the necessary objects
> out of.
> 
> Note that we can't use thin archives just yet, until we've raised the
> minimum required binutils version suitably.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/Rules.mk          | 29 +++++++++++++++++++++++++----
>  xen/arch/arm/Makefile |  6 +++---
>  xen/arch/x86/Makefile |  8 ++++----
>  xen/lib/Makefile      |  3 ++-
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index d5e5eb33de39..aba6ca2a90f5 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -60,7 +64,14 @@ include Makefile
>  # ---------------------------------------------------------------------------
>  
>  quiet_cmd_ld = LD      $@
> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
> +               --start-group $(filter %.a,$(real-prereqs)) --end-group

It might be a bit weird to modify the generic LD command for the benefit
of only prelink.o objects but it's probably fine as long as we only use
archives for lib.a. libelf and libfdt will just have --start/end-group
added to there ld command line. So I guess the change is fine.


The rest looks good,
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 1/8] xen: fix build when $(obj-y) consists of just blanks

[Anthony PERARD]

On Thu, Dec 10, 2020 at 11:21:53AM +0100, Jan Beulich wrote:
> On 09.12.2020 18:40, Anthony PERARD wrote:
> > How about using $(XEN_CFLAGS) instead of $(c_flags)? That should prevent
> > CC from generating the .*.o.d files while keeping the relevant flags.
> 
> What does "relevant" cover? For an empty .o it may not be important
> right now, but I could see
> 
> c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"'
> a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS)
> 
> include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
> 
> c_flags += $(CFLAGS-y)
> a_flags += $(CFLAGS-y) $(AFLAGS-y)
> 
> leading to CFLAGS-y / AFLAGS-y which need to be consistent across
> _all_ object files (e.g. some recording of ABI used).
> 
> > Do we need to worry about having a object file been listed twice?
> > Wouldn't that be a mistake?
> 
> No. The list approach (obj-$(CONFIG_xyz) += ...) easily allows for
> this to happen. See xen/arch/x86/mm/Makefile for an existing example.


Sounds good,
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Jan Beulich]

On 10.12.2020 15:47, Anthony PERARD wrote:
> On Mon, Nov 23, 2020 at 04:21:19PM +0100, Jan Beulich wrote:
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -60,7 +64,14 @@ include Makefile
>>  # ---------------------------------------------------------------------------
>>  
>>  quiet_cmd_ld = LD      $@
>> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
>> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
>> +               --start-group $(filter %.a,$(real-prereqs)) --end-group
> 
> It might be a bit weird to modify the generic LD command for the benefit
> of only prelink.o objects but it's probably fine as long as we only use
> archives for lib.a. libelf and libfdt will just have --start/end-group
> added to there ld command line. So I guess the change is fine.

I'm afraid I don't understand what the concern is. Neither libelf
nor libfdt use any %.a right now. Or are you referring to them
merely because it's just them which have got converted to using
$(call if-changed ...), and your remark would eventually apply to
e.g. built_in.o as well? And then further is all you're worried
about the fact that there may be "--start-group  --end-group" on
the command line, i.e. with nothing inbetween? If so, besides
possibly looking a little odd if someone inspected the command
lines closely, what possible issue do you see? (If there is one,
making the addition of both options conditional upon there being
any/multiple %.a in the first place wouldn't be a big problem,
albeit Linux also doesn't care whether ${KBUILD_VMLINUX_LIBS} is
empty.)

> The rest looks good,
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks, but I'd prefer the above clarified.

Jan

Re: [PATCH v3 2/8] lib: collect library files in an archive

[Anthony PERARD]

On Fri, Dec 11, 2020 at 11:00:19AM +0100, Jan Beulich wrote:
> On 10.12.2020 15:47, Anthony PERARD wrote:
> > On Mon, Nov 23, 2020 at 04:21:19PM +0100, Jan Beulich wrote:
> >> --- a/xen/Rules.mk
> >> +++ b/xen/Rules.mk
> >> @@ -60,7 +64,14 @@ include Makefile
> >>  # ---------------------------------------------------------------------------
> >>  
> >>  quiet_cmd_ld = LD      $@
> >> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
> >> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
> >> +               --start-group $(filter %.a,$(real-prereqs)) --end-group
> > 
> > It might be a bit weird to modify the generic LD command for the benefit
> > of only prelink.o objects but it's probably fine as long as we only use
> > archives for lib.a. libelf and libfdt will just have --start/end-group
> > added to there ld command line. So I guess the change is fine.
> 
> I'm afraid I don't understand what the concern is. Neither libelf
> nor libfdt use any %.a right now. Or are you referring to them
> merely because it's just them which have got converted to using
> $(call if-changed ...), and your remark would eventually apply to
> e.g. built_in.o as well? And then further is all you're worried
> about the fact that there may be "--start-group  --end-group" on
> the command line, i.e. with nothing inbetween? If so, besides
> possibly looking a little odd if someone inspected the command
> lines closely, what possible issue do you see? (If there is one,
> making the addition of both options conditional upon there being
> any/multiple %.a in the first place wouldn't be a big problem,
> albeit Linux also doesn't care whether ${KBUILD_VMLINUX_LIBS} is
> empty.)

Well, maybe one day we might want to collect objects in build_in.a archives
rather than build_in.o, like Linux does, but Xen is probably too small
for that to have any impact on the time it takes to build. And if that
happen, we will have to change the linker's command anyway to use
--whole-archive. So the change is fine.

As for "--start-group --end-group", yes it would just look odd, but I don't
think there would by any issue with option added to the command line.

> > The rest looks good,
> > Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Thanks, but I'd prefer the above clarified.
> 
> Jan

-- 
Anthony PERARD

Ping: Arm: [PATCH v3 2/8] lib: collect library files in an archive

[Jan Beulich]

On 23.11.2020 16:21, Jan Beulich wrote:
> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
> just to avoid bloating binaries when only some arch-es and/or
> configurations need generic library routines, combine objects under lib/
> into an archive, which the linker then can pick the necessary objects
> out of.
> 
> Note that we can't use thin archives just yet, until we've raised the
> minimum required binutils version suitably.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/Rules.mk          | 29 +++++++++++++++++++++++++----
>  xen/arch/arm/Makefile |  6 +++---

For this and patch 3 of the series, may I ask for an Arm side ack
(or otherwise)?

Thanks, Jan

>  xen/arch/x86/Makefile |  8 ++++----
>  xen/lib/Makefile      |  3 ++-
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index d5e5eb33de39..aba6ca2a90f5 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -41,12 +41,16 @@ ALL_OBJS-y               += $(BASEDIR)/xsm/built_in.o
>  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
>  ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
>  
> +ALL_LIBS-y               := $(BASEDIR)/lib/lib.a
> +
>  # Initialise some variables
> +lib-y :=
>  targets :=
>  CFLAGS-y :=
>  AFLAGS-y :=
>  
>  ALL_OBJS := $(ALL_OBJS-y)
> +ALL_LIBS := $(ALL_LIBS-y)
>  
>  SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
>                                              $(foreach w,1 2 4, \
> @@ -60,7 +64,14 @@ include Makefile
>  # ---------------------------------------------------------------------------
>  
>  quiet_cmd_ld = LD      $@
> -cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
> +cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %.a,$(real-prereqs)) \
> +               --start-group $(filter %.a,$(real-prereqs)) --end-group
> +
> +# Archive
> +# ---------------------------------------------------------------------------
> +
> +quiet_cmd_ar = AR      $@
> +cmd_ar = rm -f $@; $(AR) cPrs $@ $(real-prereqs)
>  
>  # Objcopy
>  # ---------------------------------------------------------------------------
> @@ -86,6 +97,10 @@ obj-y    := $(patsubst %/, %/built_in.o, $(obj-y))
>  # tell kbuild to descend
>  subdir-obj-y := $(filter %/built_in.o, $(obj-y))
>  
> +# Libraries are always collected in one lib file.
> +# Filter out objects already built-in
> +lib-y := $(filter-out $(obj-y), $(sort $(lib-y)))
> +
>  $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS_ONLY
>  
>  ifeq ($(CONFIG_COVERAGE),y)
> @@ -129,7 +144,7 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk
>  c_flags += $(CFLAGS-y)
>  a_flags += $(CFLAGS-y) $(AFLAGS-y)
>  
> -built_in.o: $(obj-y) $(extra-y)
> +built_in.o: $(obj-y) $(if $(strip $(lib-y)),lib.a) $(extra-y)
>  ifeq ($(strip $(obj-y)),)
>  	$(CC) $(c_flags) -c -x c /dev/null -o $@
>  else
> @@ -140,8 +155,14 @@ else
>  endif
>  endif
>  
> +lib.a: $(lib-y) FORCE
> +	$(call if_changed,ar)
> +
>  targets += built_in.o
> -targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
> +ifneq ($(strip $(lib-y)),)
> +targets += lib.a
> +endif
> +targets += $(filter-out $(subdir-obj-y), $(obj-y) $(lib-y)) $(extra-y)
>  targets += $(MAKECMDGOALS)
>  
>  built_in_bin.o: $(obj-bin-y) $(extra-y)
> @@ -155,7 +176,7 @@ endif
>  PHONY += FORCE
>  FORCE:
>  
> -%/built_in.o: FORCE
> +%/built_in.o %/lib.a: FORCE
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o
>  
>  %/built_in_bin.o: FORCE
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 296c5e68bbc3..612a83b315c8 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -90,14 +90,14 @@ endif
>  
>  ifeq ($(CONFIG_LTO),y)
>  # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -	$(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
>  
>  # Link it with all the binary objects
>  prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
>  	$(call if_changed,ld)
>  else
> -prelink.o: $(ALL_OBJS) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
>  	$(call if_changed,ld)
>  endif
>  
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 9b368632fb43..8f2180485b2b 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -132,8 +132,8 @@ EFI_OBJS-$(XEN_BUILD_EFI) := efi/relocs-dummy.o
>  
>  ifeq ($(CONFIG_LTO),y)
>  # Gather all LTO objects together
> -prelink_lto.o: $(ALL_OBJS)
> -	$(LD_LTO) -r -o $@ $^
> +prelink_lto.o: $(ALL_OBJS) $(ALL_LIBS)
> +	$(LD_LTO) -r -o $@ $(filter-out %.a,$^) --start-group $(filter %.a,$^) --end-group
>  
>  # Link it with all the binary objects
>  prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $(EFI_OBJS-y) FORCE
> @@ -142,10 +142,10 @@ prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o $
>  prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o FORCE
>  	$(call if_changed,ld)
>  else
> -prelink.o: $(ALL_OBJS) $(EFI_OBJS-y) FORCE
> +prelink.o: $(ALL_OBJS) $(ALL_LIBS) $(EFI_OBJS-y) FORCE
>  	$(call if_changed,ld)
>  
> -prelink-efi.o: $(ALL_OBJS) FORCE
> +prelink-efi.o: $(ALL_OBJS) $(ALL_LIBS) FORCE
>  	$(call if_changed,ld)
>  endif
>  
> diff --git a/xen/lib/Makefile b/xen/lib/Makefile
> index 53b1da025e0d..b8814361d63e 100644
> --- a/xen/lib/Makefile
> +++ b/xen/lib/Makefile
> @@ -1,2 +1,3 @@
> -obj-y += ctype.o
>  obj-$(CONFIG_X86) += x86/
> +
> +lib-y += ctype.o
> 
> 

Re: Ping: Arm: [PATCH v3 2/8] lib: collect library files in an archive

[Julien Grall]

Hi Jan,

On 18/12/2020 08:02, Jan Beulich wrote:
> On 23.11.2020 16:21, Jan Beulich wrote:
>> In order to (subsequently) drop odd things like CONFIG_NEEDS_LIST_SORT
>> just to avoid bloating binaries when only some arch-es and/or
>> configurations need generic library routines, combine objects under lib/
>> into an archive, which the linker then can pick the necessary objects
>> out of.
>>
>> Note that we can't use thin archives just yet, until we've raised the
>> minimum required binutils version suitably.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>>   xen/Rules.mk          | 29 +++++++++++++++++++++++++----
>>   xen/arch/arm/Makefile |  6 +++---
> 
> For this and patch 3 of the series, may I ask for an Arm side ack
> (or otherwise)?

For the 2 patches:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall