[PATCH 0/4] build: corrections to .init.o generation logic

[PATCH 0/4] build: corrections to .init.o generation logic

[Jan Beulich]

Initially I merely noticed the regression addressed by the 1st patch,
but looking more closely revealed further deficiencies. After having
moved the FIXME in patch 3 I couldn't resist and address that issue
at least partly (patch 4), seeing that three and a half years have
passed and nothing was done to improve the situation.

1: build: work around bash issue
2: build: correctly report non-empty section sizes upon .o -> .init.o conversion
3: build: also check for empty .bss.* in .o -> .init.o conversion
4: EFI: free unused boot mem in at least some cases

Jan

[PATCH 1/4] build: work around bash issue

[Jan Beulich]

Older bash fails to honor "set -e" for certain built-in commands
("while" here), despite the command's status correctly bein non-zero.
The subsequent objcopy invocation now being separated by a semicolon
results in no failure. Insert an explicit "exit" (replacing ; by &&
ought to be another possible workaround).

Fixes: e321576f4047 ("xen/build: start using if_changed")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I've done a pretty light-weight audit of possible further instances of
this issue, but didn't find any.

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -193,7 +193,7 @@ define cmd_obj_init_o
             echo "Error: size of $<:$$name is 0x$$sz" >&2; \
             exit $$(expr $$idx + 1);; \
         esac; \
-    done; \
+    done || exit $$?; \
     $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@
 endef
 

Re: [PATCH 1/4] build: work around bash issue

[Julien Grall]

Hi Jan,

On 06/08/2020 10:04, Jan Beulich wrote:
> Older bash fails to honor "set -e" for certain built-in commands

"Older" is pretty vague. May I ask the exact version you run into the issue?

> ("while" here), despite the command's status correctly bein non-zero.
> The subsequent objcopy invocation now being separated by a semicolon
> results in no failure. Insert an explicit "exit" (replacing ; by &&
> ought to be another possible workaround).
> 
> Fixes: e321576f4047 ("xen/build: start using if_changed")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I've done a pretty light-weight audit of possible further instances of
> this issue, but didn't find any.
> 
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -193,7 +193,7 @@ define cmd_obj_init_o
>               echo "Error: size of $<:$$name is 0x$$sz" >&2; \
>               exit $$(expr $$idx + 1);; \
>           esac; \
> -    done; \
> +    done || exit $$?; \
>       $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section .$(s)=.init.$(s)) $< $@
>   endef
>   
> 

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] build: work around bash issue

[Jan Beulich]

On 06.08.2020 11:07, Julien Grall wrote:
> On 06/08/2020 10:04, Jan Beulich wrote:
>> Older bash fails to honor "set -e" for certain built-in commands
> 
> "Older" is pretty vague. May I ask the exact version you run into the issue?

If I knew in what version the issue got fixed, I'd have specified
that version. I've observed it with 3.2.57(2).

Jan

Re: [PATCH 1/4] build: work around bash issue

[Julien Grall]

On 06/08/2020 10:14, Jan Beulich wrote:
> On 06.08.2020 11:07, Julien Grall wrote:
>> On 06/08/2020 10:04, Jan Beulich wrote:
>>> Older bash fails to honor "set -e" for certain built-in commands
>>
>> "Older" is pretty vague. May I ask the exact version you run into the issue?
> 
> If I knew in what version the issue got fixed, I'd have specified
> that version.

Right, but this is not what I asked. What I requested is that you 
provide the version you observe the issue.

This doesn't mean other versions are not affected, but at least it give 
us a clue where this can happen.

> I've observed it with 3.2.57(2).

Thank you. Please mention it in the commit message.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] build: work around bash issue

[Jan Beulich]

On 06.08.2020 11:20, Julien Grall wrote:
> On 06/08/2020 10:14, Jan Beulich wrote:
>> I've observed it with 3.2.57(2).
> 
> Thank you. Please mention it in the commit message.

Well, added. If I had seen any use of the precise version, I would
have done so right away. Would you mind educating me how knowing
the precise version is useful, without knowing up to which version
the issue exists?

Jan

Re: [PATCH 1/4] build: work around bash issue

[Julien Grall]

On 06/08/2020 10:25, Jan Beulich wrote:
> On 06.08.2020 11:20, Julien Grall wrote:
>> On 06/08/2020 10:14, Jan Beulich wrote:
>>> I've observed it with 3.2.57(2).
>>
>> Thank you. Please mention it in the commit message.
> 
> Well, added. If I had seen any use of the precise version, I would
> have done so right away. Would you mind educating me how knowing
> the precise version is useful, without knowing up to which version
> the issue exists?

Indeed, this wouldn't tell us whether other bash versions are affected.

However, as a reviewer, I need to be able to verify what you wrote is 
correct and that your patch is the correct way to solve it.

So providing the bash version where you observed the issue will at least 
fulfill this part.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/4] build: work around bash issue

[Andrew Cooper]

On 06/08/2020 10:14, Jan Beulich wrote:
> On 06.08.2020 11:07, Julien Grall wrote:
>> On 06/08/2020 10:04, Jan Beulich wrote:
>>> Older bash fails to honor "set -e" for certain built-in commands
>> "Older" is pretty vague. May I ask the exact version you run into the issue?
> If I knew in what version the issue got fixed, I'd have specified
> that version. I've observed it with 3.2.57(2).

Its still very useful information for the commit message.

I tend to phrase something like this as "Older versions of bash (at
least, 3.2.57(2))" which gives a clear indication that the problem was
observed with the specified version, without suggesting that this is a
boundary of when the issue was introduced/fixed.

With this adjusted, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
to avoid a second posting.

~Andrew

[PATCH 2/4] build: correctly report non-empty section sizes upon .o -> .init.o conversion

[Jan Beulich]

The originally used sed expression converted not just multiple leading
zeroes (as intended), but also trailing ones, rendering the error
message somewhat confusing. Collapse zeroes in just the one place where
we need them collapsed, and leave objdump's output as is for all other
purposes.

Fixes: 48115d14743e ("Move more kernel decompression bits to .init.* sections")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -185,11 +185,11 @@ cmd_cc_o_S = $(CC) $(a_flags) -c $< -o $
 
 quiet_cmd_obj_init_o = INIT_O  $@
 define cmd_obj_init_o
-    $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz rest; do \
+    $(OBJDUMP) -h $< | while read idx name sz rest; do \
         case "$$name" in \
         .*.local) ;; \
         .text|.text.*|.data|.data.*|.bss) \
-            test $$sz != 0 || continue; \
+            test $$(echo $$sz | sed 's,00*,0,') != 0 || continue; \
             echo "Error: size of $<:$$name is 0x$$sz" >&2; \
             exit $$(expr $$idx + 1);; \
         esac; \

Re: [PATCH 2/4] build: correctly report non-empty section sizes upon .o -> .init.o conversion

[Andrew Cooper]

On 06/08/2020 10:05, Jan Beulich wrote:
> The originally used sed expression converted not just multiple leading
> zeroes (as intended), but also trailing ones, rendering the error
> message somewhat confusing. Collapse zeroes in just the one place where
> we need them collapsed, and leave objdump's output as is for all other
> purposes.
>
> Fixes: 48115d14743e ("Move more kernel decompression bits to .init.* sections")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Jan Beulich]

We're gaining such sections, and like .text.* and .data.* they shouldn't
be present in objects subject to automatic to-init conversion. Oddly
enough for quite some time we did have an instance breaking this rule,
which gets fixed at this occasion, by breaking out the EFI boot
allocator functions into its own translation unit.

Fixes: c5b9805bc1f7 ("efi: create new early memory allocator")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This likely has a (weak) dependency on "x86/EFI: sanitize build logic"
sent several weeks ago, due to the new source file added, as explicit
dependencies upon the individual objects in x86/Makefile go away there.

--- a/xen/Makefile
+++ b/xen/Makefile
@@ -355,7 +355,7 @@ $(TARGET): delete-unfresh-files
 	$(MAKE) -C tools
 	$(MAKE) -f $(BASEDIR)/Rules.mk include/xen/compile.h
 	[ -e include/asm ] || ln -sf asm-$(TARGET_ARCH) include/asm
-	[ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c runtime.c compat.c efi.h;\
+	[ -e arch/$(TARGET_ARCH)/efi ] && for f in $$(cd common/efi; echo *.[ch]); \
 		do test -r arch/$(TARGET_ARCH)/efi/$$f || \
 		   ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \
 		done; \
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -188,7 +188,7 @@ define cmd_obj_init_o
     $(OBJDUMP) -h $< | while read idx name sz rest; do \
         case "$$name" in \
         .*.local) ;; \
-        .text|.text.*|.data|.data.*|.bss) \
+        .text|.text.*|.data|.data.*|.bss|.bss.*) \
             test $$(echo $$sz | sed 's,00*,0,') != 0 || continue; \
             echo "Error: size of $<:$$name is 0x$$sz" >&2; \
             exit $$(expr $$idx + 1);; \
--- a/xen/arch/arm/efi/Makefile
+++ b/xen/arch/arm/efi/Makefile
@@ -1,4 +1,4 @@
 CFLAGS-y += -fshort-wchar
 
-obj-y +=  boot.init.o runtime.o
+obj-y +=  boot.init.o ebmalloc.o runtime.o
 obj-$(CONFIG_ACPI) +=  efi-dom0.init.o
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -8,7 +8,7 @@ cmd_objcopy_o_ihex = $(OBJCOPY) -I ihex
 
 boot.init.o: buildid.o
 
-EFIOBJ := boot.init.o compat.o runtime.o
+EFIOBJ := boot.init.o ebmalloc.o compat.o runtime.o
 
 $(call cc-option-add,cflags-stack-boundary,CC,-mpreferred-stack-boundary=4)
 $(EFIOBJ): CFLAGS-stack-boundary := $(cflags-stack-boundary)
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -112,7 +112,6 @@ static CHAR16 *FormatDec(UINT64 Val, CHA
 static CHAR16 *FormatHex(UINT64 Val, UINTN Width, CHAR16 *Buffer);
 static void  DisplayUint(UINT64 Val, INTN Width);
 static CHAR16 *wstrcpy(CHAR16 *d, const CHAR16 *s);
-static void noreturn blexit(const CHAR16 *str);
 static void PrintErrMesg(const CHAR16 *mesg, EFI_STATUS ErrCode);
 static char *get_value(const struct file *cfg, const char *section,
                               const char *item);
@@ -155,56 +154,6 @@ static CHAR16 __initdata newline[] = L"\
 #define PrintStr(s) StdOut->OutputString(StdOut, s)
 #define PrintErr(s) StdErr->OutputString(StdErr, s)
 
-#ifdef CONFIG_ARM
-/*
- * TODO: Enable EFI boot allocator on ARM.
- * This code can be common for x86 and ARM.
- * Things TODO on ARM before enabling ebmalloc:
- *   - estimate required EBMALLOC_SIZE value,
- *   - where (in which section) ebmalloc_mem[] should live; if in
- *     .bss.page_aligned, as it is right now, then whole BSS zeroing
- *     have to be disabled in xen/arch/arm/arm64/head.S; though BSS
- *     should be initialized somehow before use of variables living there,
- *   - use ebmalloc() in ARM/common EFI boot code,
- *   - call free_ebmalloc_unused_mem() somewhere in init code.
- */
-#define EBMALLOC_SIZE	MB(0)
-#else
-#define EBMALLOC_SIZE	MB(1)
-#endif
-
-static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
-    ebmalloc_mem[EBMALLOC_SIZE];
-static unsigned long __initdata ebmalloc_allocated;
-
-/* EFI boot allocator. */
-static void __init __maybe_unused *ebmalloc(size_t size)
-{
-    void *ptr = ebmalloc_mem + ebmalloc_allocated;
-
-    ebmalloc_allocated += ROUNDUP(size, sizeof(void *));
-
-    if ( ebmalloc_allocated > sizeof(ebmalloc_mem) )
-        blexit(L"Out of static memory\r\n");
-
-    return ptr;
-}
-
-static void __init __maybe_unused free_ebmalloc_unused_mem(void)
-{
-#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for dom0. */
-    unsigned long start, end;
-
-    start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
-    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
-
-    destroy_xen_mappings(start, end);
-    init_xenheap_pages(__pa(start), __pa(end));
-
-    printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
-#endif
-}
-
 /*
  * Include architecture specific implementation here, which references the
  * static globals defined above.
@@ -321,7 +270,7 @@ static bool __init match_guid(const EFI_
            !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4));
 }
 
-static void __init noreturn blexit(const CHAR16 *str)
+void __init noreturn blexit(const CHAR16 *str)
 {
     if ( str )
         PrintStr((CHAR16 *)str);
--- /dev/null
+++ b/xen/common/efi/ebmalloc.c
@@ -0,0 +1,52 @@
+#include "efi.h"
+#include <xen/init.h>
+
+#ifdef CONFIG_ARM
+/*
+ * TODO: Enable EFI boot allocator on ARM.
+ * This code can be common for x86 and ARM.
+ * Things TODO on ARM before enabling ebmalloc:
+ *   - estimate required EBMALLOC_SIZE value,
+ *   - where (in which section) ebmalloc_mem[] should live; if in
+ *     .bss.page_aligned, as it is right now, then whole BSS zeroing
+ *     have to be disabled in xen/arch/arm/arm64/head.S; though BSS
+ *     should be initialized somehow before use of variables living there,
+ *   - use ebmalloc() in ARM/common EFI boot code,
+ *   - call free_ebmalloc_unused_mem() somewhere in init code.
+ */
+#define EBMALLOC_SIZE	MB(0)
+#else
+#define EBMALLOC_SIZE	MB(1)
+#endif
+
+static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+    ebmalloc_mem[EBMALLOC_SIZE];
+static unsigned long __initdata ebmalloc_allocated;
+
+/* EFI boot allocator. */
+void __init *ebmalloc(size_t size)
+{
+    void *ptr = ebmalloc_mem + ebmalloc_allocated;
+
+    ebmalloc_allocated += ROUNDUP(size, sizeof(void *));
+
+    if ( ebmalloc_allocated > sizeof(ebmalloc_mem) )
+        blexit(L"Out of static memory\r\n");
+
+    return ptr;
+}
+
+void __init free_ebmalloc_unused_mem(void)
+{
+#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for dom0. */
+    unsigned long start, end;
+
+    start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
+    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
+
+    destroy_xen_mappings(start, end);
+    init_xenheap_pages(__pa(start), __pa(end));
+
+    printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
+#endif
+}
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -40,4 +40,10 @@ extern UINT64 efi_boot_max_var_store_siz
 extern UINT64 efi_apple_properties_addr;
 extern UINTN efi_apple_properties_len;
 
+void noreturn blexit(const CHAR16 *str);
+
 const CHAR16 *wmemchr(const CHAR16 *s, CHAR16 c, UINTN n);
+
+/* EFI boot allocator. */
+void *ebmalloc(size_t size);
+void free_ebmalloc_unused_mem(void);

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Andrew Cooper]

On 06/08/2020 10:05, Jan Beulich wrote:
> We're gaining such sections, and like .text.* and .data.* they shouldn't
> be present in objects subject to automatic to-init conversion. Oddly
> enough for quite some time we did have an instance breaking this rule,
> which gets fixed at this occasion, by breaking out the EFI boot
> allocator functions into its own translation unit.
>
> Fixes: c5b9805bc1f7 ("efi: create new early memory allocator")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> This likely has a (weak) dependency on "x86/EFI: sanitize build logic"
> sent several weeks ago, due to the new source file added, as explicit
> dependencies upon the individual objects in x86/Makefile go away there.
>
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -355,7 +355,7 @@ $(TARGET): delete-unfresh-files
>  	$(MAKE) -C tools
>  	$(MAKE) -f $(BASEDIR)/Rules.mk include/xen/compile.h
>  	[ -e include/asm ] || ln -sf asm-$(TARGET_ARCH) include/asm
> -	[ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c runtime.c compat.c efi.h;\
> +	[ -e arch/$(TARGET_ARCH)/efi ] && for f in $$(cd common/efi; echo *.[ch]); \
>  		do test -r arch/$(TARGET_ARCH)/efi/$$f || \
>  		   ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \
>  		done; \

Maybe not for this patch, but we need to start removing this (and other)
symlinking in the tree for proper out-of-tree builds to work.

AFAICT, this logic predates both Kconfig and x86's blur into having EFI
support in xen.gz.

Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
properly in Kconfig, and gathering all the objects normally, rather than
bodging all of common/efi/ through arch/efi/ ?

~Andrew

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Jan Beulich]

On 06.08.2020 18:16, Andrew Cooper wrote:
> On 06/08/2020 10:05, Jan Beulich wrote:
>> We're gaining such sections, and like .text.* and .data.* they shouldn't
>> be present in objects subject to automatic to-init conversion. Oddly
>> enough for quite some time we did have an instance breaking this rule,
>> which gets fixed at this occasion, by breaking out the EFI boot
>> allocator functions into its own translation unit.
>>
>> Fixes: c5b9805bc1f7 ("efi: create new early memory allocator")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> This likely has a (weak) dependency on "x86/EFI: sanitize build logic"
>> sent several weeks ago, due to the new source file added, as explicit
>> dependencies upon the individual objects in x86/Makefile go away there.
>>
>> --- a/xen/Makefile
>> +++ b/xen/Makefile
>> @@ -355,7 +355,7 @@ $(TARGET): delete-unfresh-files
>>  	$(MAKE) -C tools
>>  	$(MAKE) -f $(BASEDIR)/Rules.mk include/xen/compile.h
>>  	[ -e include/asm ] || ln -sf asm-$(TARGET_ARCH) include/asm
>> -	[ -e arch/$(TARGET_ARCH)/efi ] && for f in boot.c runtime.c compat.c efi.h;\
>> +	[ -e arch/$(TARGET_ARCH)/efi ] && for f in $$(cd common/efi; echo *.[ch]); \
>>  		do test -r arch/$(TARGET_ARCH)/efi/$$f || \
>>  		   ln -nsf ../../../common/efi/$$f arch/$(TARGET_ARCH)/efi/; \
>>  		done; \
> 
> Maybe not for this patch, but we need to start removing this (and other)
> symlinking in the tree for proper out-of-tree builds to work.

Yes, but indeed not right here.

> AFAICT, this logic predates both Kconfig and x86's blur into having EFI
> support in xen.gz.

Yes, it was a result of making parts of that code also usable by Arm64.

> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
> properly in Kconfig, and gathering all the objects normally, rather than
> bodging all of common/efi/ through arch/efi/ ?

_If_ we settle on Kconfig to be allowed to check compiler (and linker)
features, then yes. This continues to be a pending topic though, so
the switch can't be made like this at this point in time. (It could be
made a Kconfig item now - which, when enabled, implies the assertion
that a capable tool chain is in use.)

Jan

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Andrew Cooper]

On 07/08/2020 11:56, Jan Beulich wrote:
> On 06.08.2020 18:16, Andrew Cooper wrote:
>> On 06/08/2020 10:05, Jan Beulich wrote:
>> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
>> properly in Kconfig, and gathering all the objects normally, rather than
>> bodging all of common/efi/ through arch/efi/ ?
> _If_ we settle on Kconfig to be allowed to check compiler (and linker)
> features, then yes. This continues to be a pending topic though, so
> the switch can't be made like this at this point in time. (It could be
> made a Kconfig item now - which, when enabled, implies the assertion
> that a capable tool chain is in use.)

I am still of the opinion that nothing needs discussing, but you are
obviously not.

Please raise this as a topic and lets discuss it, because it has a
meaningful impacting on a large number of pending series.

~Andrew

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Jan Beulich]

On 07.08.2020 17:12, Andrew Cooper wrote:
> On 07/08/2020 11:56, Jan Beulich wrote:
>> On 06.08.2020 18:16, Andrew Cooper wrote:
>>> On 06/08/2020 10:05, Jan Beulich wrote:
>>> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
>>> properly in Kconfig, and gathering all the objects normally, rather than
>>> bodging all of common/efi/ through arch/efi/ ?
>> _If_ we settle on Kconfig to be allowed to check compiler (and linker)
>> features, then yes. This continues to be a pending topic though, so
>> the switch can't be made like this at this point in time. (It could be
>> made a Kconfig item now - which, when enabled, implies the assertion
>> that a capable tool chain is in use.)
> 
> I am still of the opinion that nothing needs discussing, but you are
> obviously not.
> 
> Please raise this as a topic and lets discuss it, because it has a
> meaningful impacting on a large number of pending series.

Preferably I would have put this on this month's community meeting
agenda, but I'll be ooo next week, so that's not going to help, I'm
afraid. I guess I should put it up in email form when I'm back,
albeit I wasn't thinking it should need to be me to start the
discussion. Instead my view was that such a discussion should (have
been, now after-the-fact) be started by whoever wants to introduce
a new feature. You did say this was discussed in Chicago, but while
I'm pretty sure I was in all relevant sessions, I don't think this
can have been mentioned more than just in passing.

Jan

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Andrew Cooper]

On 07/08/2020 16:40, Jan Beulich wrote:
> On 07.08.2020 17:12, Andrew Cooper wrote:
>> On 07/08/2020 11:56, Jan Beulich wrote:
>>> On 06.08.2020 18:16, Andrew Cooper wrote:
>>>> On 06/08/2020 10:05, Jan Beulich wrote:
>>>> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
>>>> properly in Kconfig, and gathering all the objects normally, rather than
>>>> bodging all of common/efi/ through arch/efi/ ?
>>> _If_ we settle on Kconfig to be allowed to check compiler (and linker)
>>> features, then yes. This continues to be a pending topic though, so
>>> the switch can't be made like this at this point in time. (It could be
>>> made a Kconfig item now - which, when enabled, implies the assertion
>>> that a capable tool chain is in use.)
>> I am still of the opinion that nothing needs discussing, but you are
>> obviously not.
>>
>> Please raise this as a topic and lets discuss it, because it has a
>> meaningful impacting on a large number of pending series.
> Preferably I would have put this on this month's community meeting
> agenda, but I'll be ooo next week, so that's not going to help, I'm
> afraid. I guess I should put it up in email form when I'm back,
> albeit I wasn't thinking it should need to be me to start the
> discussion. Instead my view was that such a discussion should (have
> been, now after-the-fact) be started by whoever wants to introduce
> a new feature.

It would have been better to raise a concern/objectection before you
committed the feature.

It was a very clear intent of upgrading Kconfig and switching to Kbuild,
to clean up the total and chronic mess we call a build system.  It has
been discussed multiple times in person, and on xen-devel, without
apparent objection at the time.

The state of 4.14 and later is that we have the feature, and it is
already in use, with a lot more use expected to continue fixing the
build system.

You are currently blocking work to fix aspects of the build system based
on a dislike of this feature, *and* expecting someone else to justify
why using this feature as intended is ok in the first place.

I do not consider that a reasonable expectation of how to proceed.

If you wish to undo what was a deliberate intention of the
Kconfig/Kbuild work, then it is you who must start the conversation on
why we should revert the improvements.

~Andrew

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Jan Beulich]

On 10.08.2020 19:51, Andrew Cooper wrote:
> On 07/08/2020 16:40, Jan Beulich wrote:
>> On 07.08.2020 17:12, Andrew Cooper wrote:
>>> On 07/08/2020 11:56, Jan Beulich wrote:
>>>> On 06.08.2020 18:16, Andrew Cooper wrote:
>>>>> On 06/08/2020 10:05, Jan Beulich wrote:
>>>>> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
>>>>> properly in Kconfig, and gathering all the objects normally, rather than
>>>>> bodging all of common/efi/ through arch/efi/ ?
>>>> _If_ we settle on Kconfig to be allowed to check compiler (and linker)
>>>> features, then yes. This continues to be a pending topic though, so
>>>> the switch can't be made like this at this point in time. (It could be
>>>> made a Kconfig item now - which, when enabled, implies the assertion
>>>> that a capable tool chain is in use.)
>>> I am still of the opinion that nothing needs discussing, but you are
>>> obviously not.
>>>
>>> Please raise this as a topic and lets discuss it, because it has a
>>> meaningful impacting on a large number of pending series.
>> Preferably I would have put this on this month's community meeting
>> agenda, but I'll be ooo next week, so that's not going to help, I'm
>> afraid. I guess I should put it up in email form when I'm back,
>> albeit I wasn't thinking it should need to be me to start the
>> discussion. Instead my view was that such a discussion should (have
>> been, now after-the-fact) be started by whoever wants to introduce
>> a new feature.
> 
> It would have been better to raise a concern/objectection before you
> committed the feature.

I did, and I committed the whole lot because of not wanting to block
the many improvements over this one aspect I disagree with. Recall
me asking what happens if the compiler (or any part of the tool chain)
gets upgraded (or, possibly worse, downgraded) between two
(incremental) builds?

> It was a very clear intent of upgrading Kconfig and switching to Kbuild,
> to clean up the total and chronic mess we call a build system.  It has
> been discussed multiple times in person, and on xen-devel, without
> apparent objection at the time.

The change to Kbuild was discussed. The use (and, depending on how one
views it, abuse) of Kconfig to determine tool chain capabilities wasn't,
iirc. At least not in a way that it would have been noticeable to me.

> The state of 4.14 and later is that we have the feature, and it is
> already in use, with a lot more use expected to continue fixing the
> build system.

If I'm not mistaken I did make my ack on the first use of the new
behavior (in your CET series) dependent upon a subsequent discussion
(that should have occurred up front), again in an attempt to get
certain things taken care of for 4.14. This was, again iirc, in turn
referring to the earlier ack on Anthony's series, which was given in
a similarly conditional manner. (But I may be mis-remembering.)
Therefore ...

> You are currently blocking work to fix aspects of the build system based
> on a dislike of this feature, *and* expecting someone else to justify
> why using this feature as intended is ok in the first place.

... I'm pretty puzzled: Am I now being told that I shouldn't have
made the compromises, and rather should have blocked things earlier
on? I.e. is my attempt to show reasonable behavior now being turned
back into an argument against me? If so, I can certainly draw the
obvious conclusions from that, for the future.

> I do not consider that a reasonable expectation of how to proceed.
> 
> If you wish to undo what was a deliberate intention of the
> Kconfig/Kbuild work, then it is you who must start the conversation on
> why we should revert the improvements.

If I hadn't voiced my reservations long before, this _may_ indeed be
a valid position to take. But given all that had been said already
before any of this went in, I don't think it is. Anyway - despite me
not thinking it should be me (and hence it not having happened so
far), I intend (as said) to start a discussion. To be honest, I'll
be curious to see how it'll go, both in terms of number of
responses received and in terms of everyone honoring the fact that
it should _not_ matter that the logic in question was already
committed.

Jan

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Andrew Cooper]

On 07/08/2020 16:40, Jan Beulich wrote:
> On 07.08.2020 17:12, Andrew Cooper wrote:
>> On 07/08/2020 11:56, Jan Beulich wrote:
>>> On 06.08.2020 18:16, Andrew Cooper wrote:
>>>> On 06/08/2020 10:05, Jan Beulich wrote:
>>>> Can't we remove all of this by having CONFIG_XEN_PE expressed/selectable
>>>> properly in Kconfig, and gathering all the objects normally, rather than
>>>> bodging all of common/efi/ through arch/efi/ ?
>>> _If_ we settle on Kconfig to be allowed to check compiler (and linker)
>>> features, then yes. This continues to be a pending topic though, so
>>> the switch can't be made like this at this point in time. (It could be
>>> made a Kconfig item now - which, when enabled, implies the assertion
>>> that a capable tool chain is in use.)
>> I am still of the opinion that nothing needs discussing, but you are
>> obviously not.
>>
>> Please raise this as a topic and lets discuss it, because it has a
>> meaningful impacting on a large number of pending series.
> Preferably I would have put this on this month's community meeting
> agenda, but I'll be ooo next week, so that's not going to help, I'm
> afraid. I guess I should put it up in email form when I'm back,
> albeit I wasn't thinking it should need to be me to start the
> discussion. Instead my view was that such a discussion should (have
> been, now after-the-fact) be started by whoever wants to introduce
> a new feature.

It would have been better to raise a concern/objection before you
committed the feature.

It was a very clear intent of upgrading Kconfig and switching to Kbuild,
to clean up the total and chronic mess we call a build system.  It has
been discussed multiple times in person, and on xen-devel, without
apparent objection at the time.

The state of 4.14 and later is that we have the feature, and it is
already in use, with a lot more use expected to continue fixing the
build system.

You are currently blocking work to fix aspects of the build system based
on a dislike of this feature, *and* expecting someone else to justify
why using this feature as intended is ok in the first place.

I do not consider that a reasonable expectation of how to proceed.

If you wish to undo what was a deliberate intention of the
Kconfig/Kbuild work, then it is you who must start the conversation on
why we should revert the improvements.

~Andrew

Re: [PATCH 3/4] build: also check for empty .bss.* in .o -> .init.o conversion

[Julien Grall]

Hi Jan,

On 06/08/2020 10:05, Jan Beulich wrote:
> We're gaining such sections, and like .text.* and .data.* they shouldn't
> be present in objects subject to automatic to-init conversion. Oddly
> enough for quite some time we did have an instance breaking this rule,
> which gets fixed at this occasion, by breaking out the EFI boot
> allocator functions into its own translation unit.
> 
> Fixes: c5b9805bc1f7 ("efi: create new early memory allocator")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the Arm bits:

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

Cheers,

-- 
Julien Grall

[PATCH 4/4] EFI: free unused boot mem in at least some cases

[Jan Beulich]

Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
free ebmalloc area at all") was put in place: Make xen_in_range() aware
of the freed range. This is in particular relevant for EFI-enabled
builds not actually running on EFI, as the entire range will be unused
in this case.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The remaining issue could be addressed too, by making the area 2M in
size and 2M-aligned.

--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -52,6 +52,12 @@ bool efi_enabled(unsigned int feature)
 
 void __init efi_init_memory(void) { }
 
+bool __hwdom_init efi_boot_mem_unused(unsigned long *start, unsigned long *end)
+{
+    *start = *end = (unsigned long)_end;
+    return false;
+}
+
 void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e) { }
 
 bool efi_rs_using_pgtables(void)
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -608,7 +608,7 @@ static void __init kexec_reserve_area(st
 #endif
 }
 
-static inline bool using_2M_mapping(void)
+bool using_2M_mapping(void)
 {
     return !l1_table_offset((unsigned long)__2M_text_end) &&
            !l1_table_offset((unsigned long)__2M_rodata_start) &&
@@ -1970,7 +1970,7 @@ int __hwdom_init xen_in_range(unsigned l
     paddr_t start, end;
     int i;
 
-    enum { region_s3, region_ro, region_rw, nr_regions };
+    enum { region_s3, region_ro, region_rw, region_bss, nr_regions };
     static struct {
         paddr_t s, e;
     } xen_regions[nr_regions] __hwdom_initdata;
@@ -1994,7 +1994,14 @@ int __hwdom_init xen_in_range(unsigned l
         xen_regions[region_ro].e = __pa(&__2M_rodata_end);
         /* hypervisor .data + .bss */
         xen_regions[region_rw].s = __pa(&__2M_rwdata_start);
-        xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
+        if ( efi_boot_mem_unused(&start, &end) )
+        {
+            xen_regions[region_rw].e = __pa(start);
+            xen_regions[region_bss].s = __pa(end);
+            xen_regions[region_bss].e = __pa(&__2M_rwdata_end);
+        }
+        else
+            xen_regions[region_rw].e = __pa(&__2M_rwdata_end);
     }
 
     start = (paddr_t)mfn << PAGE_SHIFT;
--- a/xen/common/efi/ebmalloc.c
+++ b/xen/common/efi/ebmalloc.c
@@ -1,5 +1,9 @@
 #include "efi.h"
 #include <xen/init.h>
+#include <xen/mm.h>
+#ifdef CONFIG_X86
+#include <asm/setup.h>
+#endif
 
 #ifdef CONFIG_ARM
 /*
@@ -21,7 +25,7 @@
 
 static char __section(".bss.page_aligned") __aligned(PAGE_SIZE)
     ebmalloc_mem[EBMALLOC_SIZE];
-static unsigned long __initdata ebmalloc_allocated;
+static unsigned long __read_mostly ebmalloc_allocated;
 
 /* EFI boot allocator. */
 void __init *ebmalloc(size_t size)
@@ -36,17 +40,32 @@ void __init *ebmalloc(size_t size)
     return ptr;
 }
 
+bool __hwdom_init efi_boot_mem_unused(unsigned long *start, unsigned long *end)
+{
+    *start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
+    *end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
+
+    return *start < *end;
+}
+
 void __init free_ebmalloc_unused_mem(void)
 {
-#if 0 /* FIXME: Putting a hole in the BSS breaks the IOMMU mappings for dom0. */
     unsigned long start, end;
 
-    start = (unsigned long)ebmalloc_mem + PAGE_ALIGN(ebmalloc_allocated);
-    end = (unsigned long)ebmalloc_mem + sizeof(ebmalloc_mem);
+#ifdef CONFIG_X86
+    /* FIXME: Putting a hole in .bss would shatter the large page mapping. */
+    if ( using_2M_mapping() )
+    {
+        ebmalloc_allocated = sizeof(ebmalloc_mem);
+        return;
+    }
+#endif
+
+    if ( !efi_boot_mem_unused(&start, &end) )
+        return;
 
     destroy_xen_mappings(start, end);
     init_xenheap_pages(__pa(start), __pa(end));
 
     printk(XENLOG_INFO "Freed %lukB unused BSS memory\n", (end - start) >> 10);
-#endif
 }
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -9,6 +9,8 @@ extern const char __2M_rodata_start[], _
 extern char __2M_init_start[], __2M_init_end[];
 extern char __2M_rwdata_start[], __2M_rwdata_end[];
 
+bool using_2M_mapping(void);
+
 extern unsigned long xenheap_initial_phys_start;
 extern uint64_t boot_tsc_stamp;
 
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -33,6 +33,7 @@ struct compat_pf_efi_runtime_call;
 
 bool efi_enabled(unsigned int feature);
 void efi_init_memory(void);
+bool efi_boot_mem_unused(unsigned long *start, unsigned long *end);
 bool efi_rs_using_pgtables(void);
 unsigned long efi_get_time(void);
 void efi_halt_system(void);

Re: [PATCH 4/4] EFI: free unused boot mem in at least some cases

[Andrew Cooper]

On 06/08/2020 10:06, Jan Beulich wrote:
> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
> free ebmalloc area at all") was put in place: Make xen_in_range() aware
> of the freed range. This is in particular relevant for EFI-enabled
> builds not actually running on EFI, as the entire range will be unused
> in this case.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The remaining issue could be addressed too, by making the area 2M in
> size and 2M-aligned.

This memory range is only used for relocating the (synthesized?)
multiboot strings, is it not?

I'm not actually convinced that this is a sensible tradeoff.

For one, you've broken setup.c's:

    /* This needs to remain in sync with xen_in_range(). */
    reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));

which covers the runtime aspect of what xen_in_range() covers during boot.


I think the better course of action is to go with David Woodhouse's work
to not relocate the trampoline until later on boot (if even necessary),
at which point both of the custom allocators can disappear.

~Andrew

Re: [PATCH 4/4] EFI: free unused boot mem in at least some cases

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Mon, 2020-08-10 at 18:09 +0100, Andrew Cooper wrote:
> I think the better course of action is to go with David Woodhouse's work
> to not relocate the trampoline until later on boot (if even necessary),
> at which point both of the custom allocators can disappear.

I confess I had mostly given up on cleaning up the boot code. But if
you like I can dust those patches off and post them one last time.

It certainly was an overall improvement on the current setup.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH 4/4] EFI: free unused boot mem in at least some cases

[Jan Beulich]

On 10.08.2020 19:09, Andrew Cooper wrote:
> On 06/08/2020 10:06, Jan Beulich wrote:
>> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
>> free ebmalloc area at all") was put in place: Make xen_in_range() aware
>> of the freed range. This is in particular relevant for EFI-enabled
>> builds not actually running on EFI, as the entire range will be unused
>> in this case.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The remaining issue could be addressed too, by making the area 2M in
>> size and 2M-aligned.
> 
> This memory range is only used for relocating the (synthesized?)
> multiboot strings, is it not?

No. Afaict it has nothing to do with multiboot strings. There are
exactly two uses afaics - in place_string() and in
efi_arch_allocate_mmap_buffer(). The former is used to record
command line pieces, e.g. that parsed from the config file, while
the latter is what allocates the memory for the EFI memory map.

> I'm not actually convinced that this is a sensible tradeoff.
> 
> For one, you've broken setup.c's:
> 
>     /* This needs to remain in sync with xen_in_range(). */
>     reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> 
> which covers the runtime aspect of what xen_in_range() covers during boot.

Hmm, I did specifically look at that and thought it wouldn't need
changing. But now that you point it out (again), it looks like I
was wrong.

> I think the better course of action is to go with David Woodhouse's work
> to not relocate the trampoline until later on boot (if even necessary),
> at which point both of the custom allocators can disappear.

Well, in the light of my response above I'd like to express that
I can't see how David's work would make this allocator go away.

Jan

Re: [PATCH 4/4] EFI: free unused boot mem in at least some cases

[Jan Beulich]

On 10.08.2020 19:09, Andrew Cooper wrote:
> On 06/08/2020 10:06, Jan Beulich wrote:
>> Address at least the primary reason why 52bba67f8b87 ("efi/boot: Don't
>> free ebmalloc area at all") was put in place: Make xen_in_range() aware
>> of the freed range. This is in particular relevant for EFI-enabled
>> builds not actually running on EFI, as the entire range will be unused
>> in this case.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The remaining issue could be addressed too, by making the area 2M in
>> size and 2M-aligned.
> 
> This memory range is only used for relocating the (synthesized?)
> multiboot strings, is it not?
> 
> I'm not actually convinced that this is a sensible tradeoff.
> 
> For one, you've broken setup.c's:
> 
>     /* This needs to remain in sync with xen_in_range(). */
>     reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));
> 
> which covers the runtime aspect of what xen_in_range() covers during boot.

I'm afraid this wasn't a good suggestion here (it was still helpful
to notice that tboot.c also needs adjustment): By not reserving the
range here, it'll get freed by end_boot_allocator(), and hence may
not (again) be freed by free_ebmalloc_unused_mem() (kind of putting
its name under question). Immediately up from the quoted place we
also reserve the space where the modules live, which also gets
freed later. I'm having difficulty to see why this particular
aspect needs to remain in sync between the reservation done here
and xen_in_range().

v2 definitely is broken because of me not having noticed this in
time. I'll first try to fix it without reverting to the v1 model,
but I'd prefer to go back to the earlier approach (keeping merely
the other v2 adjustments). Unless of course you see some breakage
from this that I don't see.

Jan