| Message ID | c99cf808-0710-51b1-c07c-07bf237e22a3@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | build: corrections to .init.o generation logic | expand |
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
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
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
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
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
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
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,
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
--- 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);
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.