Skip to contentSkip to conversation

EFI fixes

GIT PULL Merged Ingo Molnar 9 participants 13 messages Jun 28, 2020 – Jul 10, 2020

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

Linus,

Please pull the latest efi/urgent git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-2020-06-28

# HEAD: 2a55280a3675203496d302463b941834228b9875 efi/libstub: arm: Print CPU boot mode and MMU state at boot

- Fix build regression on v4.8 and older
- Robustness fix for TPM log parsing code
- kobject refcount fix for the ESRT parsing code
- Two efivarfs fixes to make it behave more like an ordinary file system
- Style fixup for zero length arrays
- Fix a regression in path separator handling in the initrd loader
- Fix a missing prototype warning
- Add some kerneldoc headers for newly introduced stub routines
- Allow support for SSDT overrides via EFI variables to be disabled
- Report CPU mode and MMU state upon entry for 32-bit ARM
- Use the correct stack pointer alignment when entering from mixed mode

Thanks,

Ingo

------------------>
Ard Biesheuvel (3):
efi/libstub: Fix missing-prototype warning for skip_spaces() efi/libstub: arm: Omit arch specific config table matching array on arm64 efi/libstub: arm: Print CPU boot mode and MMU state at boot

Arvind Sankar (2):
efi/x86: Fix build with gcc 4
efi/x86: Setup stack correctly for efi_pe_entry

Fabian Vogt (1):
efi/tpm: Verify event log header before parsing

Gustavo A. R. Silva (1):
efi: Replace zero-length array and use struct_size() helper

Heinrich Schuchardt (1):
efi/libstub: Descriptions for stub helper functions

Peter Jones (1):
efi: Make it possible to disable efivar_ssdt entirely

Philipp Fent (1):
efi/libstub: Fix path separator regression

Qiushi Wu (1):
efi/esrt: Fix reference count leak in esre_create_sysfs_entry.

Tony Luck (2):
efivarfs: Update inode modification time for successful writes
efivarfs: Don't return -EINTR when rate-limiting reads

 arch/arm/include/asm/efi.h                     |  7 +++
 arch/x86/boot/compressed/head_64.S             | 11 +++-
 drivers/firmware/efi/Kconfig                   | 11 ++++
 drivers/firmware/efi/arm-init.c                | 40 +++++++++++--
 drivers/firmware/efi/efi.c                     |  5 +-
 drivers/firmware/efi/esrt.c                    |  2 +-
 drivers/firmware/efi/libstub/Makefile          |  3 +-
 drivers/firmware/efi/libstub/arm32-stub.c      | 54 +++++++++++++++++-
 drivers/firmware/efi/libstub/efi-stub-helper.c | 78 ++++++++++++++++++++++----
 drivers/firmware/efi/libstub/efi-stub.c        |  3 +
 drivers/firmware/efi/libstub/efistub.h         | 12 +++-
 drivers/firmware/efi/libstub/file.c            | 16 ++++--
 drivers/firmware/efi/libstub/skip_spaces.c     |  1 +
 fs/efivarfs/file.c                             |  7 +--
 include/linux/efi.h                            |  8 +--
 include/linux/tpm_eventlog.h                   | 14 ++++-
 16 files changed, 235 insertions(+), 37 deletions(-)
arch/arm/include/asm/efi.h +7 -0
index 84dc0ba822f5..5dcf3c6011b7 100644
@@ -87,4 +87,11 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
 	return dram_base + SZ_512M;
 }
 
+struct efi_arm_entry_state {
+	u32	cpsr_before_ebs;
+	u32	sctlr_before_ebs;
+	u32	cpsr_after_ebs;
+	u32	sctlr_after_ebs;
+};
+
 #endif /* _ASM_ARM_EFI_H */
arch/x86/boot/compressed/head_64.S +10 -1
index e821a7d7d5c4..97d37f0a34f5 100644
@@ -213,7 +213,6 @@ SYM_FUNC_START(startup_32)
 	 * We place all of the values on our mini stack so lret can
 	 * used to perform that far jump.
 	 */
-	pushl	$__KERNEL_CS
 	leal	startup_64(%ebp), %eax
 #ifdef CONFIG_EFI_MIXED
 	movl	efi32_boot_args(%ebp), %edi
@@ -224,11 +223,20 @@ SYM_FUNC_START(startup_32)
 	movl	efi32_boot_args+8(%ebp), %edx	// saved bootparams pointer
 	cmpl	$0, %edx
 	jnz	1f
+	/*
+	 * efi_pe_entry uses MS calling convention, which requires 32 bytes of
+	 * shadow space on the stack even if all arguments are passed in
+	 * registers. We also need an additional 8 bytes for the space that
+	 * would be occupied by the return address, and this also results in
+	 * the correct stack alignment for entry.
+	 */
+	subl	$40, %esp
 	leal	efi_pe_entry(%ebp), %eax
 	movl	%edi, %ecx			// MS calling convention
 	movl	%esi, %edx
 1:
 #endif
+	pushl	$__KERNEL_CS
 	pushl	%eax
 
 	/* Enter paged protected Mode, activating Long Mode */
@@ -784,6 +792,7 @@ SYM_DATA_LOCAL(boot_heap,	.fill BOOT_HEAP_SIZE, 1, 0)
 
 SYM_DATA_START_LOCAL(boot_stack)
 	.fill BOOT_STACK_SIZE, 1, 0
+	.balign 16
 SYM_DATA_END_LABEL(boot_stack, SYM_L_LOCAL, boot_stack_end)
 
 /*
drivers/firmware/efi/Kconfig +11 -0
index e6fc022bc87e..3939699e62fe 100644
@@ -278,3 +278,14 @@ config EFI_EARLYCON
 	depends on SERIAL_EARLYCON && !ARM && !IA64
 	select FONT_SUPPORT
 	select ARCH_USE_MEMREMAP_PROT
+
+config EFI_CUSTOM_SSDT_OVERLAYS
+	bool "Load custom ACPI SSDT overlay from an EFI variable"
+	depends on EFI_VARS && ACPI
+	default ACPI_TABLE_UPGRADE
+	help
+	  Allow loading of an ACPI SSDT overlay from an EFI variable specified
+	  by a kernel command line option.
+
+	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
+	  information.
drivers/firmware/efi/arm-init.c +36 -4
index c697e70ca7e7..71c445d20258 100644
@@ -52,9 +52,11 @@ static phys_addr_t __init efi_to_phys(unsigned long addr)
 }
 
 static __initdata unsigned long screen_info_table = EFI_INVALID_TABLE_ADDR;
+static __initdata unsigned long cpu_state_table = EFI_INVALID_TABLE_ADDR;
 
 static const efi_config_table_type_t arch_tables[] __initconst = {
 	{LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID, &screen_info_table},
+	{LINUX_EFI_ARM_CPU_STATE_TABLE_GUID, &cpu_state_table},
 	{}
 };
 
@@ -62,7 +64,8 @@ static void __init init_screen_info(void)
 {
 	struct screen_info *si;
 
-	if (screen_info_table != EFI_INVALID_TABLE_ADDR) {
+	if (IS_ENABLED(CONFIG_ARM) &&
+	    screen_info_table != EFI_INVALID_TABLE_ADDR) {
 		si = early_memremap_ro(screen_info_table, sizeof(*si));
 		if (!si) {
 			pr_err("Could not map screen_info config table\n");
@@ -116,7 +119,8 @@ static int __init uefi_init(u64 efi_system_table)
 		goto out;
 	}
 	retval = efi_config_parse_tables(config_tables, systab->nr_tables,
-					 arch_tables);
+					 IS_ENABLED(CONFIG_ARM) ? arch_tables
+								: NULL);
 
 	early_memunmap(config_tables, table_size);
 out:
@@ -238,9 +242,37 @@ void __init efi_init(void)
 
 	init_screen_info();
 
+#ifdef CONFIG_ARM
 	/* ARM does not permit early mappings to persist across paging_init() */
-	if (IS_ENABLED(CONFIG_ARM))
-		efi_memmap_unmap();
+	efi_memmap_unmap();
+
+	if (cpu_state_table != EFI_INVALID_TABLE_ADDR) {
+		struct efi_arm_entry_state *state;
+		bool dump_state = true;
+
+		state = early_memremap_ro(cpu_state_table,
+					  sizeof(struct efi_arm_entry_state));
+		if (state == NULL) {
+			pr_warn("Unable to map CPU entry state table.\n");
+			return;
+		}
+
+		if ((state->sctlr_before_ebs & 1) == 0)
+			pr_warn(FW_BUG "EFI stub was entered with MMU and Dcache disabled, please fix your firmware!\n");
+		else if ((state->sctlr_after_ebs & 1) == 0)
+			pr_warn(FW_BUG "ExitBootServices() returned with MMU and Dcache disabled, please fix your firmware!\n");
+		else
+			dump_state = false;
+
+		if (dump_state || efi_enabled(EFI_DBG)) {
+			pr_info("CPSR at EFI stub entry        : 0x%08x\n", state->cpsr_before_ebs);
+			pr_info("SCTLR at EFI stub entry       : 0x%08x\n", state->sctlr_before_ebs);
+			pr_info("CPSR after ExitBootServices() : 0x%08x\n", state->cpsr_after_ebs);
+			pr_info("SCTLR after ExitBootServices(): 0x%08x\n", state->sctlr_after_ebs);
+		}
+		early_memunmap(state, sizeof(struct efi_arm_entry_state));
+	}
+#endif
 }
 
 static bool efifb_overlaps_pci_range(const struct of_pci_range *range)
drivers/firmware/efi/efi.c +3 -2
index 7f1657b6c30d..5114cae4ec97 100644
@@ -189,7 +189,7 @@ static void generic_ops_unregister(void)
 	efivars_unregister(&generic_efivars);
 }
 
-#if IS_ENABLED(CONFIG_ACPI)
+#ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
 #define EFIVAR_SSDT_NAME_MAX	16
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
 static int __init efivar_ssdt_setup(char *str)
@@ -622,7 +622,8 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 			rsv = (void *)(p + prsv % PAGE_SIZE);
 
 			/* reserve the entry itself */
-			memblock_reserve(prsv, EFI_MEMRESERVE_SIZE(rsv->size));
+			memblock_reserve(prsv,
+					 struct_size(rsv, entry, rsv->size));
 
 			for (i = 0; i < atomic_read(&rsv->count); i++) {
 				memblock_reserve(rsv->entry[i].base,
drivers/firmware/efi/esrt.c +1 -1
index e3d692696583..d5915272141f 100644
@@ -181,7 +181,7 @@ static int esre_create_sysfs_entry(void *esre, int entry_num)
 		rc = kobject_init_and_add(&entry->kobj, &esre1_ktype, NULL,
 					  "entry%d", entry_num);
 		if (rc) {
-			kfree(entry);
+			kobject_put(&entry->kobj);
 			return rc;
 		}
 	}
drivers/firmware/efi/libstub/Makefile +2 -1
index 75daaf20374e..4cce372edaf4 100644
@@ -6,7 +6,8 @@
 # enabled, even if doing so doesn't break the build.
 #
 cflags-$(CONFIG_X86_32)		:= -march=i386
-cflags-$(CONFIG_X86_64)		:= -mcmodel=small
+cflags-$(CONFIG_X86_64)		:= -mcmodel=small \
+				   $(call cc-option,-maccumulate-outgoing-args)
 cflags-$(CONFIG_X86)		+= -m$(BITS) -D__KERNEL__ \
 				   -fPIC -fno-strict-aliasing -mno-red-zone \
 				   -mno-mmx -mno-sse -fshort-wchar \
drivers/firmware/efi/libstub/arm32-stub.c +53 -1

72 lines not on this page · raw message

drivers/firmware/efi/libstub/efi-stub-helper.c +67 -11

146 lines not on this page · raw message

drivers/firmware/efi/libstub/efi-stub.c +3 -0
index e97370bdfdb0..3318ec3f8e5b 100644
@@ -329,6 +329,9 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 	if (status != EFI_SUCCESS)
 		goto fail_free_initrd;
 
+	if (IS_ENABLED(CONFIG_ARM))
+		efi_handle_post_ebs_state();
+
 	efi_enter_kernel(image_addr, fdt_addr, fdt_totalsize((void *)fdt_addr));
 	/* not reached */
 
drivers/firmware/efi/libstub/efistub.h +10 -2
index bcd8c0a785f0..2c9d42264c29 100644
@@ -157,8 +157,14 @@ typedef void (__efiapi *efi_event_notify_t)(efi_event_t, void *);
 #define EFI_EVT_NOTIFY_WAIT	0x00000100U
 #define EFI_EVT_NOTIFY_SIGNAL	0x00000200U
 
-/*
- * boottime->wait_for_event takes an array of events as input.
+/**
+ * efi_set_event_at() - add event to events array
+ *
+ * @events:	array of UEFI events
+ * @ids:	index where to put the event in the array
+ * @event:	event to add to the aray
+ *
+ * boottime->wait_for_event() takes an array of events as input.
  * Provide a helper to set it up correctly for mixed mode.
  */
 static inline
@@ -771,4 +777,6 @@ efi_status_t efi_load_initrd(efi_loaded_image_t *image,
 			     unsigned long soft_limit,
 			     unsigned long hard_limit);
 
+void efi_handle_post_ebs_state(void);
+
 #endif
drivers/firmware/efi/libstub/file.c +12 -4
index 2005e33b33d5..630caa6b1f4c 100644
@@ -102,12 +102,20 @@ static int find_file_option(const efi_char16_t *cmdline, int cmdline_len,
 	if (!found)
 		return 0;
 
+	/* Skip any leading slashes */
+	while (cmdline[i] == L'/' || cmdline[i] == L'\\')
+		i++;
+
 	while (--result_len > 0 && i < cmdline_len) {
-		if (cmdline[i] == L'\0' ||
-		    cmdline[i] == L'\n' ||
-		    cmdline[i] == L' ')
+		efi_char16_t c = cmdline[i++];
+
+		if (c == L'\0' || c == L'\n' || c == L' ')
 			break;
-		*result++ = cmdline[i++];
+		else if (c == L'/')
+			/* Replace UNIX dir separators with EFI standard ones */
+			*result++ = L'\\';
+		else
+			*result++ = c;
 	}
 	*result = L'\0';
 	return i;
drivers/firmware/efi/libstub/skip_spaces.c +1 -0
index a700b3c7f7d0..159fb4e456c6 100644
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/ctype.h>
+#include <linux/string.h>
 #include <linux/types.h>
 
 char *skip_spaces(const char *str)
fs/efivarfs/file.c +3 -4
index e9e27a271af0..feaa5e182b7b 100644
@@ -51,6 +51,7 @@ static ssize_t efivarfs_file_write(struct file *file,
 	} else {
 		inode_lock(inode);
 		i_size_write(inode, datasize + sizeof(attributes));
+		inode->i_mtime = current_time(inode);
 		inode_unlock(inode);
 	}
 
@@ -72,10 +73,8 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
 	ssize_t size = 0;
 	int err;
 
-	while (!__ratelimit(&file->f_cred->user->ratelimit)) {
-		if (!msleep_interruptible(50))
-			return -EINTR;
-	}
+	while (!__ratelimit(&file->f_cred->user->ratelimit))
+		msleep(50);
 
 	err = efivar_entry_size(var, &datasize);
 
include/linux/efi.h +3 -5
index 2c6495f72f79..bb35f3305e55 100644
@@ -350,6 +350,7 @@ void efi_native_runtime_setup(void);
  * associated with ConOut
  */
 #define LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID	EFI_GUID(0xe03fc20a, 0x85dc, 0x406e,  0xb9, 0x0e, 0x4a, 0xb5, 0x02, 0x37, 0x1d, 0x95)
+#define LINUX_EFI_ARM_CPU_STATE_TABLE_GUID	EFI_GUID(0xef79e4aa, 0x3c3d, 0x4989,  0xb9, 0x02, 0x07, 0xa9, 0x43, 0xe5, 0x50, 0xd2)
 #define LINUX_EFI_LOADER_ENTRY_GUID		EFI_GUID(0x4a67b082, 0x0a4c, 0x41cf,  0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f)
 #define LINUX_EFI_RANDOM_SEED_TABLE_GUID	EFI_GUID(0x1ce1e5bc, 0x7ceb, 0x42f2,  0x81, 0xe5, 0x8a, 0xad, 0xf1, 0x80, 0xf5, 0x7b)
 #define LINUX_EFI_TPM_EVENT_LOG_GUID		EFI_GUID(0xb7799cb0, 0xeca2, 0x4943,  0x96, 0x67, 0x1f, 0xae, 0x07, 0xb7, 0x47, 0xfa)
@@ -1236,14 +1237,11 @@ struct linux_efi_memreserve {
 	struct {
 		phys_addr_t	base;
 		phys_addr_t	size;
-	} entry[0];
+	} entry[];
 };
 
-#define EFI_MEMRESERVE_SIZE(count) (sizeof(struct linux_efi_memreserve) + \
-	(count) * sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
-
 #define EFI_MEMRESERVE_COUNT(size) (((size) - sizeof(struct linux_efi_memreserve)) \
-	/ sizeof(((struct linux_efi_memreserve *)0)->entry[0]))
+	/ sizeof_field(struct linux_efi_memreserve, entry[0]))
 
 void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size);
 
include/linux/tpm_eventlog.h +13 -1
index 4f8c90c93c29..64356b199e94 100644
@@ -81,6 +81,8 @@ struct tcg_efi_specid_event_algs {
 	u16 digest_size;
 } __packed;
 
+#define TCG_SPECID_SIG "Spec ID Event03"
+
 struct tcg_efi_specid_event_head {
 	u8 signature[16];
 	u32 platform_class;
@@ -171,6 +173,7 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 	int i;
 	int j;
 	u32 count, event_type;
+	const u8 zero_digest[sizeof(event_header->digest)] = {0};
 
 	marker = event;
 	marker_start = marker;
@@ -198,10 +201,19 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 	count = READ_ONCE(event->count);
 	event_type = READ_ONCE(event->event_type);
 
+	/* Verify that it's the log header */
+	if (event_header->pcr_idx != 0 ||
+	    event_header->event_type != NO_ACTION ||
+	    memcmp(event_header->digest, zero_digest, sizeof(zero_digest))) {
+		size = 0;
+		goto out;
+	}
+
 	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
 
 	/* Check if event is malformed. */
-	if (count > efispecid->num_algs) {
+	if (memcmp(efispecid->signature, TCG_SPECID_SIG,
+		   sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
 		size = 0;
 		goto out;
 	}
#1 lore raw
Quoted 2 lines · Ingo Molnar

- Fix build regression on v4.8 and older

Interesting.

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

So if we just say that we only care about gcc-4.9 upwards, it frees us to clean up some (really) old constructions where we've been using macros with sizeof and/or typeof, and make the code potentially rather more readable and often more compact.

Yeah, I know we _just_ made the minimum compiler version be 4.8, but I do get the feeling that we should just have bitten the bullet and gone all the way to 4.9.

Arnd, what was the reason for 4.8 support? I'm assuming there's some sad unfortunate distro that still uses that ancient compiler..

Ok, ok, 4.9 isn't exactly new either (4.9.0 released May 2014, and final 4.9 release was 2016), but 4.9 really from a feature angle is a much saner thing than 4.8.

Afaik, the main "interesting" part of gcc-4.8 was that it was when gcc switched over to be built as C++. That's perhaps a huge milestone for gcc itself, but not necessarily for the users..

Arnd? You're the one who tends to keep track of these things..

Linus

maintainer
#2 lore raw
Quoted 10 lines · Linus Torvalds
Quoted 2 lines · Ingo Molnar

- Fix build regression on v4.8 and older

Interesting.

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

+1

Quoted 9 lines

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

So if we just say that we only care about gcc-4.9 upwards, it frees us to clean up some (really) old constructions where we've been using macros with sizeof and/or typeof, and make the code potentially rather more readable and often more compact.

Quoted 7 lines

Yeah, I know we _just_ made the minimum compiler version be 4.8, but I do get the feeling that we should just have bitten the bullet and gone all the way to 4.9.

Arnd, what was the reason for 4.8 support? I'm assuming there's some sad unfortunate distro that still uses that ancient compiler..

Arnd may know more, but I know one of the reasons 4.8 is significant is because RHEL 7 uses it, which will go EOL in 2024.

Quoted 11 lines

Ok, ok, 4.9 isn't exactly new either (4.9.0 released May 2014, and final 4.9 release was 2016), but 4.9 really from a feature angle is a much saner thing than 4.8.

Afaik, the main "interesting" part of gcc-4.8 was that it was when gcc switched over to be built as C++. That's perhaps a huge milestone for gcc itself, but not necessarily for the users..

Arnd? You're the one who tends to keep track of these things..

Linus

maintainer
#3 lore raw
Quoted 2 lines · Ard Biesheuvel

Arnd may know more, but I know one of the reasons 4.8 is significant is because RHEL 7 uses it, which will go EOL in 2024.

Yeah, I always question the relevance of this argument because distros:

* backport fixes for the compiler so gcc-4.8 in the distro is not always upstream gcc-4.8

* they build the distro kernel with that compiler and former is something old.

Yeah, it can happen that some backported patch has trouble building with the distro compiler but that is kinda seldom.

IMHO, of course.

Signature · 4 lines
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
#4 lore raw
In reply to Linus Torvalds
Quoted 4 lines · Linus Torvalds
Quoted 2 lines · Ingo Molnar

- Fix build regression on v4.8 and older

Quoted 8 lines

Interesting.

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

+1

Anybody for nay, or should we just do this?

#5 lore raw
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

+1

Anybody for nay, or should we just do this?

I'll just do it. Let's see if anybody screams with a good reason. I hate the whole "support old compilers", it ends up not only making for complex code, it tends to cause these unnecessary kinds of "guys, we tested this really well, but that crazy compiler had a very particular odd issue, and it wasn't in any test box.

Linus

#6 lore raw
Quoted 11 lines · Linus Torvalds
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

+1

Quoted 7 lines
Quoted 1 line

Anybody for nay, or should we just do this?

I'll just do it. Let's see if anybody screams with a good reason. I hate the whole "support old compilers", it ends up not only making for complex code, it tends to cause these unnecessary kinds of "guys, we tested this really well, but that crazy compiler had a very particular odd issue, and it wasn't in any test box.

Excellent, thanks!

#7 lore raw
In reply to Linus Torvalds
Quoted 12 lines · Linus Torvalds
Quoted 10 lines · Peter Zijlstra
Quoted 7 lines

It's perhaps yet another reason to just skip gcc-4.8 too, since apparently 4.9 works.

gcc-4.9 really has a lot of advantages. It's where (I think) gcc basically supports all C11 things, including _Generic() but also __auto_type.

+1

Quoted 7 lines
Quoted 1 line

Anybody for nay, or should we just do this?

I'll just do it. Let's see if anybody screams with a good reason. I hate the whole "support old compilers", it ends up not only making for complex code, it tends to cause these unnecessary kinds of "guys, we tested this really well, but that crazy compiler had a very particular odd issue, and it wasn't in any test box.

Cool, thanks for changing it, this is clearly a better suited compiler version. Aside from the added C11 features, this is also where a lot of the optimizations changed, so code generation is more predictable if we don't need to worry about gcc-4.8.

On the flip side, gcc-4.8 was used by old enterprise distros that are still supported (SUSE 12, RHEL 7), whereas gcc-4.9 was only shipped in Debian Jessie and Android releases that are both EOL now (Android never moved beyond a buggy gcc-4.9 prerelease but now uses clang for everything).

I don't see any technical reasons to go even further, but if something does come up, the users of these Long-term supported distros would be most impacted by a change:

gcc-4.9: Used in Debian 8 (Jessie), EOL June 2020
gcc-5: Used in Ubuntu 16.04 (Xenial, Mint 18, ...), EOL April 2021
gcc-6: Used in Debian 9 (Stretch), EOL 2022
gcc-7: Used in SLES 15, Ubuntu 18.04 (Bionic, Mint 19, ...)
gcc-8: Used in RHEL-8 (centos, oracle, ...), OpenWRT

The most interesting version to require in the future would be gcc-7, which IIRC is the point at which we can just use -std=gnu99 or -std=gnu11 instead of -std=gnu89 without running into the problem with compound literals[1].

Arnd

[1] https://patchwork.kernel.org/patch/11195831/

#8 lore raw
Quoted 4 lines · Arnd Bergmann

The most interesting version to require in the future would be gcc-7, which IIRC is the point at which we can just use -std=gnu99 or -std=gnu11 instead of -std=gnu89 without running into the problem with compound literals[1].

It is gcc-5, not gcc-7. This commit:

https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d303aeafa9b4

Signature · 1 line
 Kirill A. Shutemov
#9 lore raw
Quoted 10 lines · Kirill A. Shutemov
Quoted 4 lines · Arnd Bergmann

The most interesting version to require in the future would be gcc-7, which IIRC is the point at which we can just use -std=gnu99 or -std=gnu11 instead of -std=gnu89 without running into the problem with compound literals[1].

It is gcc-5, not gcc-7. This commit:

https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d303aeafa9b4

Ok, glad I was wrong here. I confirmed that with gcc-5 or higher I can build a kernel with -std=gnu11 or -std=gnu99 instead of -std=gnu89, but gcc-4.9.4 fails with anything other than gnu89.

I forgot why we care though -- is there any behavior of gnu11 that we prefer over the gnu99 behavior, or is it just going with the times because it's the right thing to do? All the interesting features of c11 seem to also be available as extensions in gcc-4.9's gnu89, though I could not find a definite list of the differences.

Arnd

#10 lore raw
Quoted 16 lines · Arnd Bergmann
Quoted 10 lines · Kirill A. Shutemov
Quoted 4 lines · Arnd Bergmann

The most interesting version to require in the future would be gcc-7, which IIRC is the point at which we can just use -std=gnu99 or -std=gnu11 instead of -std=gnu89 without running into the problem with compound literals[1].

It is gcc-5, not gcc-7. This commit:

https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d303aeafa9b4

Ok, glad I was wrong here. I confirmed that with gcc-5 or higher I can build a kernel with -std=gnu11 or -std=gnu99 instead of -std=gnu89, but gcc-4.9.4 fails with anything other than gnu89.

Quoted 6 lines

I forgot why we care though -- is there any behavior of gnu11 that we prefer over the gnu99 behavior, or is it just going with the times because it's the right thing to do? All the interesting features of c11 seem to also be available as extensions in gcc-4.9's gnu89, though I could not find a definite list of the differences.

Last time (llist_entry_safe() thread) it came up due to local variables in loops feature that is not available for gnu89. Both gnu99 and gnu11 is fine. Maybe we should leave it to default for the GCC (gnu17/gnu18 in gcc-10).

Signature · 1 line
 Kirill A. Shutemov
#11 lore raw
Quoted 11 lines · Kirill A. Shutemov
Quoted 6 lines · Arnd Bergmann

I forgot why we care though -- is there any behavior of gnu11 that we prefer over the gnu99 behavior, or is it just going with the times because it's the right thing to do? All the interesting features of c11 seem to also be available as extensions in gcc-4.9's gnu89, though I could not find a definite list of the differences.

Last time (llist_entry_safe() thread) it came up due to local variables in loops feature that is not available for gnu89. Both gnu99 and gnu11 is fine.

Same for anonymous structs/unions. I used to have a use case for that in struct efi_dev_path in include/linux/efi.h, but Ard refactored it in a gnu89-compatible way for v5.7 with db8952e7094f.

(BTW, revisiting that commit I think it should have been broken into smaller pieces, in particular the efi_get_device_by_path() argument and #ifdef change should have gone into a separate commit.)

Thanks,

Lukas

merge bot Merged
#12 lore raw
In reply to Ingo Molnar
The pull request you sent on Sun, 28 Jun 2020 20:26:01 +0200:

The pull request you sent on Sun, 28 Jun 2020 20:26:01 +0200:

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-2020-06-28

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/bc53f67d247a38d43e081faa7e63690a1279f5c7

Thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker