Skip to contentSkip to conversation

EFI changes for v4.6

GIT PULL In review Matt Fleming 9 participants 54 messages Feb 1, 2016 – Feb 11, 2016

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

Folks, please pull the following changes for v4.6. This mostly consists of cleanups but the patches to execute EFI runtime services with interrupts enabled would benefit from a thorough baking in linux-next.

I'm sending this pull nice and early so that we can shake out any bugs before the next merge window.

The following changes since commit 753b11ef8e92a1c1bbe97f2a5ec14bdd1ef2e6fe:

x86/efi: Setup separate EFI page tables in kexec paths (2016-01-21 21:01:34 +0100)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next

for you to fetch changes up to 921c2e93a5d7f14a803663389b9541f14744d048:

x86/efi: Print size in binary units in efi_print_memmap (2016-02-01 20:52:49 +0000)

----------------------------------------------------------------
* Run EFI runtime services with interrupts enabled for the benefit of uniprocessor machines with slow variable stores and because there's no good reason to disable them since we don't invoke runtime services from IRQ code paths - Ard Biesheuvel

* Use the existing to_efivar_entry() accessor function in the efivars code instead of open-coding it - Geliang Tang

* Align the ACPI BGRT driver more closely with how Windows interprets the 'status' field so we can use the BGRT image - Môshe van der Sterre

* checkpatch.pl cleanup of the GUIDs in efi.h which has the added benefit of making them more closely resemble how they're presented in the UEFI specification - Peter Jones

* Refactor the x86 EFI memory map printing code and add improved support for printing Persistent Memory entries - Robert Elliott, Andy Shevchenko

----------------------------------------------------------------
Ard Biesheuvel (7):
efi: Expose non-blocking set_variable() wrapper to efivars
efi: Remove redundant efi_set_variable_nonblocking prototype
efi: runtime-wrappers: Add a nonblocking version of QueryVariableInfo
efi: Add nonblocking option to efi_query_variable_store()
efi: runtime-wrappers: Remove out of date comment regarding in_nmi()
efi: runtime-wrapper: Get rid of the rtc_lock spinlock
efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Geliang Tang (1):
efivars: Use to_efivar_entry

Môshe van der Sterre (1):
x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

Peter Jones (1):
efi: Make checkpatch complain less about efi.h GUID additions

Robert Elliott (4):
x86/efi: Show actual ending addresses in efi_print_memmap
efi: Add NV memory attribute
efi: Add Persistent Memory type name
x86/efi: Print size in binary units in efi_print_memmap

 arch/x86/platform/efi/efi-bgrt.c        |  10 +--
 arch/x86/platform/efi/efi.c             |  25 +++++--
 arch/x86/platform/efi/quirks.c          |  33 ++++++++-
 drivers/firmware/efi/efi.c              |   9 ++-
 drivers/firmware/efi/efivars.c          |   2 +-
 drivers/firmware/efi/runtime-wrappers.c | 115 ++++++++++++--------------------
 drivers/firmware/efi/vars.c             |  16 ++++-
 include/linux/efi.h                     |  85 ++++++++++++++---------
 8 files changed, 173 insertions(+), 122 deletions(-)
#1 lore raw

PATCH 1/14 efi: Expose non-blocking set_variable() wrapper to efivars

From: Ard Biesheuvel <[email protected]>

Commit 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation") implemented a non-blocking alternative for the UEFI SetVariable() invocation performed by efivars, since it may occur in atomic context. However, this version of the function was never exposed via the efivars struct, so the non-blocking versions was not actually callable. Fix that.

Fixes 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation")
---
 drivers/firmware/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)
drivers/firmware/efi/efi.c +1 -0
index e9c458ba9353..0cd8f039602e 100644
@@ -180,6 +180,7 @@ static int generic_ops_register(void)
 {
 	generic_ops.get_variable = efi.get_variable;
 	generic_ops.set_variable = efi.set_variable;
+	generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
 	generic_ops.get_next_variable = efi.get_next_variable;
 	generic_ops.query_variable_store = efi_query_variable_store;
 
Signature · 1 line
2.6.2
#2 lore raw

[tip:efi/core] efi: Expose non-blocking set_variable() wrapper to efivars

efi: Expose non-blocking set_variable() wrapper to efivars
Commit-ID:  9c6672ac9c91f7eb1ec436be1442b8c26d098e55
Gitweb:     http://git.kernel.org/tip/9c6672ac9c91f7eb1ec436be1442b8c26d098e55
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:06:55 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:01 +0100

efi: Expose non-blocking set_variable() wrapper to efivars

Commit 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation") implemented a non-blocking alternative for the UEFI SetVariable() invocation performed by efivars, since it may occur in atomic context. However, this version of the function was never exposed via the efivars struct, so the non-blocking versions was not actually callable. Fix that.

Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Fixes 6d80dba1c9fe ("efi: Provide a non-blocking SetVariable() operation")
---
 drivers/firmware/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)
drivers/firmware/efi/efi.c +1 -0
index 9b815c8..20451c2 100644
@@ -182,6 +182,7 @@ static int generic_ops_register(void)
 {
 	generic_ops.get_variable = efi.get_variable;
 	generic_ops.set_variable = efi.set_variable;
+	generic_ops.set_variable_nonblocking = efi.set_variable_nonblocking;
 	generic_ops.get_next_variable = efi.get_next_variable;
 	generic_ops.query_variable_store = efi_query_variable_store;
 
#3 lore raw

PATCH 2/14 efi: Remove redundant efi_set_variable_nonblocking prototype

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

There is no need for a separate nonblocking prototype definition for the SetVariable() UEFI Runtime Service, since it is identical to the blocking version.

---
 include/linux/efi.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
include/linux/efi.h +2 -6
index 569b5a866bb1..8706e0aabedc 100644
@@ -507,10 +507,6 @@ typedef efi_status_t efi_get_next_variable_t (unsigned long *name_size, efi_char
 typedef efi_status_t efi_set_variable_t (efi_char16_t *name, efi_guid_t *vendor, 
 					 u32 attr, unsigned long data_size,
 					 void *data);
-typedef efi_status_t
-efi_set_variable_nonblocking_t(efi_char16_t *name, efi_guid_t *vendor,
-			       u32 attr, unsigned long data_size, void *data);
-
 typedef efi_status_t efi_get_next_high_mono_count_t (u32 *count);
 typedef void efi_reset_system_t (int reset_type, efi_status_t status,
 				 unsigned long data_size, efi_char16_t *data);
@@ -851,7 +847,7 @@ extern struct efi {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
@@ -1091,7 +1087,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_store_t *query_variable_store;
 };
 
Signature · 1 line
2.6.2
#4 lore raw

[tip:efi/core] efi: Remove redundant efi_set_variable_nonblocking () prototype

efi: Remove redundant efi_set_variable_nonblocking() prototype
Commit-ID:  70d2a3cf2f4ae2e93b7a661842d84c2b5132cee7
Gitweb:     http://git.kernel.org/tip/70d2a3cf2f4ae2e93b7a661842d84c2b5132cee7
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:06:56 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:02 +0100

efi: Remove redundant efi_set_variable_nonblocking() prototype

There is no need for a separate nonblocking prototype definition for the SetVariable() UEFI Runtime Service, since it is identical to the blocking version.

---
 include/linux/efi.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
include/linux/efi.h +2 -6
index 569b5a8..8706e0a 100644
@@ -507,10 +507,6 @@ typedef efi_status_t efi_get_next_variable_t (unsigned long *name_size, efi_char
 typedef efi_status_t efi_set_variable_t (efi_char16_t *name, efi_guid_t *vendor, 
 					 u32 attr, unsigned long data_size,
 					 void *data);
-typedef efi_status_t
-efi_set_variable_nonblocking_t(efi_char16_t *name, efi_guid_t *vendor,
-			       u32 attr, unsigned long data_size, void *data);
-
 typedef efi_status_t efi_get_next_high_mono_count_t (u32 *count);
 typedef void efi_reset_system_t (int reset_type, efi_status_t status,
 				 unsigned long data_size, efi_char16_t *data);
@@ -851,7 +847,7 @@ extern struct efi {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
@@ -1091,7 +1087,7 @@ struct efivar_operations {
 	efi_get_variable_t *get_variable;
 	efi_get_next_variable_t *get_next_variable;
 	efi_set_variable_t *set_variable;
-	efi_set_variable_nonblocking_t *set_variable_nonblocking;
+	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_store_t *query_variable_store;
 };
 
#5 lore raw

PATCH 3/14 efi: runtime-wrappers: Add a nonblocking version of QueryVariableInfo

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

This introduces a new runtime wrapper for the QueryVariableInfo() UEFI Runtime Service, which gives up immediately rather than spins on failure to grab the efi_runtime spinlock.

This is required in the non-blocking path of the efi-pstore code.

---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
 include/linux/efi.h                     |  1 +
 2 files changed, 23 insertions(+)
drivers/firmware/efi/runtime-wrappers.c +22 -0
index 228bbf910461..e9f2867f0d91 100644
@@ -230,6 +230,27 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 	return status;
 }
 
+static efi_status_t
+virt_efi_query_variable_info_nonblocking(u32 attr,
+					 u64 *storage_space,
+					 u64 *remaining_space,
+					 u64 *max_variable_size)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+		return EFI_NOT_READY;
+
+	status = efi_call_virt(query_variable_info, attr, storage_space,
+			       remaining_space, max_variable_size);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	return status;
+}
+
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
 	unsigned long flags;
@@ -300,6 +321,7 @@ void efi_native_runtime_setup(void)
 	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
 	efi.reset_system = virt_efi_reset_system;
 	efi.query_variable_info = virt_efi_query_variable_info;
+	efi.query_variable_info_nonblocking = virt_efi_query_variable_info_nonblocking;
 	efi.update_capsule = virt_efi_update_capsule;
 	efi.query_capsule_caps = virt_efi_query_capsule_caps;
 }
include/linux/efi.h +1 -0
index 8706e0aabedc..ad1e177ba48e 100644
@@ -849,6 +849,7 @@ extern struct efi {
 	efi_set_variable_t *set_variable;
 	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
+	efi_query_variable_info_t *query_variable_info_nonblocking;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
 	efi_get_next_high_mono_count_t *get_next_high_mono_count;
Signature · 1 line
2.6.2
#6 lore raw

[tip:efi/core] efi/runtime-wrappers: Add a nonblocking version of QueryVariableInfo()

efi/runtime-wrappers: Add a nonblocking version of QueryVariableInfo()
Commit-ID:  d3cac1f83c631b9fe5edaebcba49f6989bfff089
Gitweb:     http://git.kernel.org/tip/d3cac1f83c631b9fe5edaebcba49f6989bfff089
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:06:57 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:03 +0100

efi/runtime-wrappers: Add a nonblocking version of QueryVariableInfo()

This introduces a new runtime wrapper for the QueryVariableInfo() UEFI Runtime Service, which gives up immediately rather than spins on failure to grab the efi_runtime spinlock.

This is required in the non-blocking path of the efi-pstore code.

---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
 include/linux/efi.h                     |  1 +
 2 files changed, 23 insertions(+)
drivers/firmware/efi/runtime-wrappers.c +22 -0
index 228bbf9..e9f2867 100644
@@ -230,6 +230,27 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 	return status;
 }
 
+static efi_status_t
+virt_efi_query_variable_info_nonblocking(u32 attr,
+					 u64 *storage_space,
+					 u64 *remaining_space,
+					 u64 *max_variable_size)
+{
+	unsigned long flags;
+	efi_status_t status;
+
+	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
+		return EFI_UNSUPPORTED;
+
+	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+		return EFI_NOT_READY;
+
+	status = efi_call_virt(query_variable_info, attr, storage_space,
+			       remaining_space, max_variable_size);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	return status;
+}
+
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
 	unsigned long flags;
@@ -300,6 +321,7 @@ void efi_native_runtime_setup(void)
 	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
 	efi.reset_system = virt_efi_reset_system;
 	efi.query_variable_info = virt_efi_query_variable_info;
+	efi.query_variable_info_nonblocking = virt_efi_query_variable_info_nonblocking;
 	efi.update_capsule = virt_efi_update_capsule;
 	efi.query_capsule_caps = virt_efi_query_capsule_caps;
 }
include/linux/efi.h +1 -0
index 8706e0a..ad1e177 100644
@@ -849,6 +849,7 @@ extern struct efi {
 	efi_set_variable_t *set_variable;
 	efi_set_variable_t *set_variable_nonblocking;
 	efi_query_variable_info_t *query_variable_info;
+	efi_query_variable_info_t *query_variable_info_nonblocking;
 	efi_update_capsule_t *update_capsule;
 	efi_query_capsule_caps_t *query_capsule_caps;
 	efi_get_next_high_mono_count_t *get_next_high_mono_count;
#7 lore raw

PATCH 4/14 efi: Add nonblocking option to efi_query_variable_store()

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

The function efi_query_variable_store() may be invoked by efivar_entry_set_nonblocking(), which itself takes care to only call a non-blocking version of the SetVariable() runtime wrapper. However, efi_query_variable_store() may call the SetVariable() wrapper directly, as well as the wrapper for QueryVariableInfo(), both of which could deadlock in the same way we are trying to prevent by calling efivar_entry_set_nonblocking() in the first place.

So instead, modify efi_query_variable_store() to use the non-blocking variants of QueryVariableInfo() (and give up rather than free up space if the available space is below EFI_MIN_RESERVE) if invoked with the 'nonblocking' argument set to true.

---
 arch/x86/platform/efi/quirks.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/firmware/efi/vars.c    | 16 ++++++++++++++--
 include/linux/efi.h            | 12 +++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)
arch/x86/platform/efi/quirks.c +32 -1
index 6452070f3025..dc4f7b51abf9 100644
@@ -56,13 +56,41 @@ void efi_delete_dummy_variable(void)
 }
 
 /*
+ * In the nonblocking case we do not attempt to perform garbage
+ * collection if we do not have enough free space. Rather, we do the
+ * bare minimum check and give up immediately if the available space
+ * is below EFI_MIN_RESERVE.
+ *
+ * This function is intended to be small and simple because it is
+ * invoked from crash handler paths.
+ */
+static efi_status_t query_variable_store_nonblocking(u32 attributes,
+						     unsigned long size)
+{
+	efi_status_t status;
+	u64 storage_size, remaining_size, max_size;
+
+	status = efi.query_variable_info_nonblocking(attributes, &storage_size,
+						     &remaining_size,
+						     &max_size);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	if (remaining_size - size < EFI_MIN_RESERVE)
+		return EFI_OUT_OF_RESOURCES;
+
+	return EFI_SUCCESS;
+}
+
+/*
  * Some firmware implementations refuse to boot if there's insufficient space
  * in the variable store. Ensure that we never use more than a safe limit.
  *
  * Return EFI_SUCCESS if it is safe to write 'size' bytes to the variable
  * store.
  */
-efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
+				      bool nonblocking)
 {
 	efi_status_t status;
 	u64 storage_size, remaining_size, max_size;
@@ -70,6 +98,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
 	if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
 		return 0;
 
+	if (nonblocking)
+		return query_variable_store_nonblocking(attributes, size);
+
 	status = efi.query_variable_info(attributes, &storage_size,
 					 &remaining_size, &max_size);
 	if (status != EFI_SUCCESS)
drivers/firmware/efi/vars.c +14 -2
index 70a0fb10517f..d2a49626a335 100644
@@ -234,7 +234,18 @@ check_var_size(u32 attributes, unsigned long size)
 	if (!fops->query_variable_store)
 		return EFI_UNSUPPORTED;
 
-	return fops->query_variable_store(attributes, size);
+	return fops->query_variable_store(attributes, size, false);
+}
+
+static efi_status_t
+check_var_size_nonblocking(u32 attributes, unsigned long size)
+{
+	const struct efivar_operations *fops = __efivars->ops;
+
+	if (!fops->query_variable_store)
+		return EFI_UNSUPPORTED;
+
+	return fops->query_variable_store(attributes, size, true);
 }
 
 static int efi_status_to_err(efi_status_t status)
@@ -615,7 +626,8 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 	if (!spin_trylock_irqsave(&__efivars->lock, flags))
 		return -EBUSY;
 
-	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
+	status = check_var_size_nonblocking(attributes,
+					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
 		spin_unlock_irqrestore(&__efivars->lock, flags);
 		return -ENOSPC;
include/linux/efi.h +9 -3
index ad1e177ba48e..09f1559e7525 100644
@@ -525,7 +525,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
 					      unsigned long count,
 					      u64 *max_size,
 					      int *reset_type);
-typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size);
+typedef efi_status_t efi_query_variable_store_t(u32 attributes,
+						unsigned long size,
+						bool nonblocking);
 
 void efi_native_runtime_setup(void);
 
@@ -881,13 +883,17 @@ extern void efi_enter_virtual_mode (void);	/* switch EFI to virtual mode, if pos
 #ifdef CONFIG_X86
 extern void efi_late_init(void);
 extern void efi_free_boot_services(void);
-extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size);
+extern efi_status_t efi_query_variable_store(u32 attributes,
+					     unsigned long size,
+					     bool nonblocking);
 extern void efi_find_mirror(void);
 #else
 static inline void efi_late_init(void) {}
 static inline void efi_free_boot_services(void) {}
 
-static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+static inline efi_status_t efi_query_variable_store(u32 attributes,
+						    unsigned long size,
+						    bool nonblocking)
 {
 	return EFI_SUCCESS;
 }
Signature · 1 line
2.6.2
#8 lore raw

[tip:efi/core] efi: Add nonblocking option to efi_query_variable_store()

efi: Add nonblocking option to efi_query_variable_store()
Commit-ID:  ca0e30dcaa53a3fcb2dfdf74252d30bc40603eea
Gitweb:     http://git.kernel.org/tip/ca0e30dcaa53a3fcb2dfdf74252d30bc40603eea
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:06:58 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:04 +0100

efi: Add nonblocking option to efi_query_variable_store()

The function efi_query_variable_store() may be invoked by efivar_entry_set_nonblocking(), which itself takes care to only call a non-blocking version of the SetVariable() runtime wrapper. However, efi_query_variable_store() may call the SetVariable() wrapper directly, as well as the wrapper for QueryVariableInfo(), both of which could deadlock in the same way we are trying to prevent by calling efivar_entry_set_nonblocking() in the first place.

So instead, modify efi_query_variable_store() to use the non-blocking variants of QueryVariableInfo() (and give up rather than free up space if the available space is below EFI_MIN_RESERVE) if invoked with the 'nonblocking' argument set to true.

---
 arch/x86/platform/efi/quirks.c | 33 ++++++++++++++++++++++++++++++++-
 drivers/firmware/efi/vars.c    | 16 ++++++++++++++--
 include/linux/efi.h            | 12 +++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)
arch/x86/platform/efi/quirks.c +32 -1
index 4535046..2326bf5 100644
@@ -57,13 +57,41 @@ void efi_delete_dummy_variable(void)
 }
 
 /*
+ * In the nonblocking case we do not attempt to perform garbage
+ * collection if we do not have enough free space. Rather, we do the
+ * bare minimum check and give up immediately if the available space
+ * is below EFI_MIN_RESERVE.
+ *
+ * This function is intended to be small and simple because it is
+ * invoked from crash handler paths.
+ */
+static efi_status_t
+query_variable_store_nonblocking(u32 attributes, unsigned long size)
+{
+	efi_status_t status;
+	u64 storage_size, remaining_size, max_size;
+
+	status = efi.query_variable_info_nonblocking(attributes, &storage_size,
+						     &remaining_size,
+						     &max_size);
+	if (status != EFI_SUCCESS)
+		return status;
+
+	if (remaining_size - size < EFI_MIN_RESERVE)
+		return EFI_OUT_OF_RESOURCES;
+
+	return EFI_SUCCESS;
+}
+
+/*
  * Some firmware implementations refuse to boot if there's insufficient space
  * in the variable store. Ensure that we never use more than a safe limit.
  *
  * Return EFI_SUCCESS if it is safe to write 'size' bytes to the variable
  * store.
  */
-efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
+				      bool nonblocking)
 {
 	efi_status_t status;
 	u64 storage_size, remaining_size, max_size;
@@ -71,6 +99,9 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
 	if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
 		return 0;
 
+	if (nonblocking)
+		return query_variable_store_nonblocking(attributes, size);
+
 	status = efi.query_variable_info(attributes, &storage_size,
 					 &remaining_size, &max_size);
 	if (status != EFI_SUCCESS)
drivers/firmware/efi/vars.c +14 -2
index 70a0fb1..d2a4962 100644
@@ -234,7 +234,18 @@ check_var_size(u32 attributes, unsigned long size)
 	if (!fops->query_variable_store)
 		return EFI_UNSUPPORTED;
 
-	return fops->query_variable_store(attributes, size);
+	return fops->query_variable_store(attributes, size, false);
+}
+
+static efi_status_t
+check_var_size_nonblocking(u32 attributes, unsigned long size)
+{
+	const struct efivar_operations *fops = __efivars->ops;
+
+	if (!fops->query_variable_store)
+		return EFI_UNSUPPORTED;
+
+	return fops->query_variable_store(attributes, size, true);
 }
 
 static int efi_status_to_err(efi_status_t status)
@@ -615,7 +626,8 @@ efivar_entry_set_nonblocking(efi_char16_t *name, efi_guid_t vendor,
 	if (!spin_trylock_irqsave(&__efivars->lock, flags))
 		return -EBUSY;
 
-	status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
+	status = check_var_size_nonblocking(attributes,
+					    size + ucs2_strsize(name, 1024));
 	if (status != EFI_SUCCESS) {
 		spin_unlock_irqrestore(&__efivars->lock, flags);
 		return -ENOSPC;
include/linux/efi.h +9 -3
index ad1e177..09f1559 100644
@@ -525,7 +525,9 @@ typedef efi_status_t efi_query_capsule_caps_t(efi_capsule_header_t **capsules,
 					      unsigned long count,
 					      u64 *max_size,
 					      int *reset_type);
-typedef efi_status_t efi_query_variable_store_t(u32 attributes, unsigned long size);
+typedef efi_status_t efi_query_variable_store_t(u32 attributes,
+						unsigned long size,
+						bool nonblocking);
 
 void efi_native_runtime_setup(void);
 
@@ -881,13 +883,17 @@ extern void efi_enter_virtual_mode (void);	/* switch EFI to virtual mode, if pos
 #ifdef CONFIG_X86
 extern void efi_late_init(void);
 extern void efi_free_boot_services(void);
-extern efi_status_t efi_query_variable_store(u32 attributes, unsigned long size);
+extern efi_status_t efi_query_variable_store(u32 attributes,
+					     unsigned long size,
+					     bool nonblocking);
 extern void efi_find_mirror(void);
 #else
 static inline void efi_late_init(void) {}
 static inline void efi_free_boot_services(void) {}
 
-static inline efi_status_t efi_query_variable_store(u32 attributes, unsigned long size)
+static inline efi_status_t efi_query_variable_store(u32 attributes,
+						    unsigned long size,
+						    bool nonblocking)
 {
 	return EFI_SUCCESS;
 }
#9 lore raw

PATCH 5/14 efi: runtime-wrappers: Remove out of date comment regarding in_nmi()

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

This code is long gone, so remove the comment as well.

---
 drivers/firmware/efi/runtime-wrappers.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +0 -26
index e9f2867f0d91..311f415bff51 100644
@@ -62,32 +62,6 @@
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
 /*
- * Some runtime services calls can be reentrant under NMI, even if the table
- * above says they are not. (source: UEFI Specification v2.4A)
- *
- * Table 32. Functions that may be called after Machine Check, INIT and NMI
- * +----------------------------+------------------------------------------+
- * | Function			| Called after Machine Check, INIT and NMI |
- * +----------------------------+------------------------------------------+
- * | GetTime()			| Yes, even if previously busy.		   |
- * | GetVariable()		| Yes, even if previously busy		   |
- * | GetNextVariableName()	| Yes, even if previously busy		   |
- * | QueryVariableInfo()	| Yes, even if previously busy		   |
- * | SetVariable()		| Yes, even if previously busy		   |
- * | UpdateCapsule()		| Yes, even if previously busy		   |
- * | QueryCapsuleCapabilities()	| Yes, even if previously busy		   |
- * | ResetSystem()		| Yes, even if previously busy		   |
- * +----------------------------+------------------------------------------+
- *
- * In order to prevent deadlocks under NMI, the wrappers for these functions
- * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
- * However, not all of the services listed are reachable through NMI code paths,
- * so the the special handling as suggested by the UEFI spec is only implemented
- * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
- * context through efi_pstore_write().
- */
-
-/*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
  * the EFI specification requires that callers of the time related runtime
  * functions serialize with other CMOS accesses in the kernel, as the EFI time
Signature · 1 line
2.6.2
#10 lore raw

[tip:efi/core] efi/runtime-wrappers: Remove out of date comment regarding in_nmi()

efi/runtime-wrappers: Remove out of date comment regarding in_nmi()
Commit-ID:  774846defceb16dcab2f0215cfc467f7c93f1c26
Gitweb:     http://git.kernel.org/tip/774846defceb16dcab2f0215cfc467f7c93f1c26
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:06:59 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:04 +0100

efi/runtime-wrappers: Remove out of date comment regarding in_nmi()

This code is long gone, so remove the comment as well.

---
 drivers/firmware/efi/runtime-wrappers.c | 26 --------------------------
 1 file changed, 26 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +0 -26
index e9f2867..311f415 100644
@@ -62,32 +62,6 @@
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
 /*
- * Some runtime services calls can be reentrant under NMI, even if the table
- * above says they are not. (source: UEFI Specification v2.4A)
- *
- * Table 32. Functions that may be called after Machine Check, INIT and NMI
- * +----------------------------+------------------------------------------+
- * | Function			| Called after Machine Check, INIT and NMI |
- * +----------------------------+------------------------------------------+
- * | GetTime()			| Yes, even if previously busy.		   |
- * | GetVariable()		| Yes, even if previously busy		   |
- * | GetNextVariableName()	| Yes, even if previously busy		   |
- * | QueryVariableInfo()	| Yes, even if previously busy		   |
- * | SetVariable()		| Yes, even if previously busy		   |
- * | UpdateCapsule()		| Yes, even if previously busy		   |
- * | QueryCapsuleCapabilities()	| Yes, even if previously busy		   |
- * | ResetSystem()		| Yes, even if previously busy		   |
- * +----------------------------+------------------------------------------+
- *
- * In order to prevent deadlocks under NMI, the wrappers for these functions
- * may only grab the efi_runtime_lock or rtc_lock spinlocks if !efi_in_nmi().
- * However, not all of the services listed are reachable through NMI code paths,
- * so the the special handling as suggested by the UEFI spec is only implemented
- * for QueryVariableInfo() and SetVariable(), as these can be reached in NMI
- * context through efi_pstore_write().
- */
-
-/*
  * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
  * the EFI specification requires that callers of the time related runtime
  * functions serialize with other CMOS accesses in the kernel, as the EFI time
#11 lore raw

PATCH 6/14 efi: runtime-wrapper: Get rid of the rtc_lock spinlock

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

The rtc_lock spinlock aims to serialize access to the CMOS RTC between the UEFI firmware and the kernel drivers that use it directly. However, x86 is the only arch that performs such direct accesses, and that never uses the time related UEFI runtime services. Since no other UEFI enlightened architectures have a legcay CMOS RTC anyway, we can remove the rtc_lock spinlock entirely.

---
 drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +8 -24
index 311f415bff51..7b8b2f2702ca 100644
@@ -61,24 +61,14 @@
  */
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
-/*
- * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
- * the EFI specification requires that callers of the time related runtime
- * functions serialize with other CMOS accesses in the kernel, as the EFI time
- * functions may choose to also use the legacy CMOS RTC.
- */
-__weak DEFINE_SPINLOCK(rtc_lock);
-
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -87,11 +77,9 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -102,11 +90,9 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -115,11 +101,9 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
Signature · 1 line
2.6.2
#12 lore raw

[tip:efi/core] efi: Runtime-wrapper: Get rid of the rtc_lock spinlock

efi: Runtime-wrapper: Get rid of the rtc_lock spinlock
Commit-ID:  1bb6936473c07b5a7c8daced1000893b7145bb14
Gitweb:     http://git.kernel.org/tip/1bb6936473c07b5a7c8daced1000893b7145bb14
Author:     Ard Biesheuvel <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:00 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:31:05 +0100

efi: Runtime-wrapper: Get rid of the rtc_lock spinlock

The rtc_lock spinlock aims to serialize access to the CMOS RTC between the UEFI firmware and the kernel drivers that use it directly. However, x86 is the only arch that performs such direct accesses, and that never uses the time related UEFI runtime services. Since no other UEFI enlightened architectures have a legcay CMOS RTC anyway, we can remove the rtc_lock spinlock entirely.

---
 drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +8 -24
index 311f415..7b8b2f2 100644
@@ -61,24 +61,14 @@
  */
 static DEFINE_SPINLOCK(efi_runtime_lock);
 
-/*
- * As per commit ef68c8f87ed1 ("x86: Serialize EFI time accesses on rtc_lock"),
- * the EFI specification requires that callers of the time related runtime
- * functions serialize with other CMOS accesses in the kernel, as the EFI time
- * functions may choose to also use the legacy CMOS RTC.
- */
-__weak DEFINE_SPINLOCK(rtc_lock);
-
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -87,11 +77,9 @@ static efi_status_t virt_efi_set_time(efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -102,11 +90,9 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
@@ -115,11 +101,9 @@ static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&rtc_lock, flags);
-	spin_lock(&efi_runtime_lock);
+	spin_lock_irqsave(&efi_runtime_lock, flags);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock(&efi_runtime_lock);
-	spin_unlock_irqrestore(&rtc_lock, flags);
+	spin_unlock_irqrestore(&efi_runtime_lock, flags);
 	return status;
 }
 
#13 lore raw

PATCH 7/14 efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

In reply to Matt Fleming

From: Ard Biesheuvel <[email protected]>

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. The only reason we were disabling interrupts was to prevent recursive calls into the services on the same CPU, which will lead to deadlock. However, the only context where such invocations may occur legally is from efi-pstore via efivars, and that code has been updated to call a non-blocking alternative when invoked from a non-interruptible context.

So instead, update the ordinary, blocking UEFI Runtime Services wrappers to execute with interrupts enabled. This aims to prevent excessive interrupt latencies on uniprocessor platforms with slow variable stores.

---
 drivers/firmware/efi/runtime-wrappers.c | 73 ++++++++++++++-------------------
 1 file changed, 30 insertions(+), 43 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +30 -43
index 7b8b2f2702ca..aa3d9d0a27e6 100644
@@ -63,23 +63,21 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_time(efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -87,23 +85,21 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 					     efi_bool_t *pending,
 					     efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -113,13 +109,12 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -127,12 +122,13 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_variable, name_size, name, vendor);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -142,13 +138,12 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -157,15 +152,14 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 				  u32 attr, unsigned long data_size,
 				  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -175,16 +169,15 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -194,29 +187,27 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 					 u64 *remaining_space,
 					 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_high_mono_count, count);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -225,26 +216,23 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 }
 
 static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 					    unsigned long count,
 					    unsigned long sg_list)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(update_capsule, capsules, count, sg_list);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -253,16 +241,15 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 						u64 *max_size,
 						int *reset_type)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
 			       reset_type);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
Signature · 1 line
2.6.2
#14 lore raw

Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Quoted 10 lines · Matt Fleming

From: Ard Biesheuvel <[email protected]>

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. The only reason we were disabling interrupts was to prevent recursive calls into the services on the same CPU, which will lead to deadlock. However, the only context where such invocations may occur legally is from efi-pstore via efivars, and that code has been updated to call a non-blocking alternative when invoked from a non-interruptible context.

Quoted 3 lines

So instead, update the ordinary, blocking UEFI Runtime Services wrappers to execute with interrupts enabled. This aims to prevent excessive interrupt latencies on uniprocessor platforms with slow variable stores.

Well, those excessive latencies would affect SMP platforms as well, just that there are (usually) other CPUs free to do execution, right?

More fundamentally, this makes me nervous:

Quoted 2 lines

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. [...]

So what really matters is not what the spec says, but how Windows executes UEFI firmware code in practice.

If major versions of Windows calls UEFI firmware with interrupts disabled, then frankly I don't think we should interrupt them under Linux either, regardless of what the spec says ...

Random firmware code getting interrupted by the OS changes timings and might have other side effects the firmware code might not expect - so the question is, does Windows already de facto allow the IRQ preemption of firmware calls?

Also, this:

drivers/firmware/efi/runtime-wrappers.c static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, · line 129

Quoted 7 lines
drivers/firmware/efi/runtime-wrappers.c +3 -2
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	BUG_ON(in_irq());
+
+	spin_lock(&efi_runtime_lock);

... how does crashing the kernel help debuggability?

Please use WARN_ON_ONCE() - or in fact, this assert is probably not needed at all, as lockdep will warn about IRQ unsafe lock usage.

I'd add comments to the efi_runtime_lock definition site explaining that this is never taken from IRQ contexts.

Thanks,

Ingo

#15 lore raw

Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Quoted 13 lines · Ingo Molnar
Quoted 10 lines · Matt Fleming

From: Ard Biesheuvel <[email protected]>

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. The only reason we were disabling interrupts was to prevent recursive calls into the services on the same CPU, which will lead to deadlock. However, the only context where such invocations may occur legally is from efi-pstore via efivars, and that code has been updated to call a non-blocking alternative when invoked from a non-interruptible context.

Quoted 7 lines
Quoted 3 lines

So instead, update the ordinary, blocking UEFI Runtime Services wrappers to execute with interrupts enabled. This aims to prevent excessive interrupt latencies on uniprocessor platforms with slow variable stores.

Well, those excessive latencies would affect SMP platforms as well, just that there are (usually) other CPUs free to do execution, right?

Correct.

Quoted 8 lines

More fundamentally, this makes me nervous:

Quoted 2 lines

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. [...]

So what really matters is not what the spec says, but how Windows executes UEFI firmware code in practice.

Quoted 8 lines

If major versions of Windows calls UEFI firmware with interrupts disabled, then frankly I don't think we should interrupt them under Linux either, regardless of what the spec says ...

Random firmware code getting interrupted by the OS changes timings and might have other side effects the firmware code might not expect - so the question is, does Windows already de facto allow the IRQ preemption of firmware calls?

Good question. I will try to find out.

Quoted 10 lines

Also, this:

drivers/firmware/efi/runtime-wrappers.c static efi_status_t virt_efi_get_next_variable(unsigned long *name_size, · line 129

Quoted 7 lines
drivers/firmware/efi/runtime-wrappers.c +3 -2
-     unsigned long flags;
      efi_status_t status;
 
-     spin_lock_irqsave(&efi_runtime_lock, flags);
+     BUG_ON(in_irq());
+
+     spin_lock(&efi_runtime_lock);
Quoted 5 lines

... how does crashing the kernel help debuggability?

Please use WARN_ON_ONCE() - or in fact, this assert is probably not needed at all, as lockdep will warn about IRQ unsafe lock usage.

Actually, reading back the original thread, Matt had already identified this problem, and v2/v3 of this patch removed all of them but one, so thanks for spotting that.

Quoted 3 lines

I'd add comments to the efi_runtime_lock definition site explaining that this is never taken from IRQ contexts.

OK.

#16 lore raw

Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Quoted 12 lines · Ard Biesheuvel
Quoted 12 lines

More fundamentally, this makes me nervous:

Quoted 2 lines

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. [...]

So what really matters is not what the spec says, but how Windows executes UEFI firmware code in practice.

If major versions of Windows calls UEFI firmware with interrupts disabled, then frankly I don't think we should interrupt them under Linux either, regardless of what the spec says ...

Quoted 6 lines
Quoted 4 lines

Random firmware code getting interrupted by the OS changes timings and might have other side effects the firmware code might not expect - so the question is, does Windows already de facto allow the IRQ preemption of firmware calls?

Good question. I will try to find out.

Note that if there's a reasonable (but not 100%) case in favor of keeping irqs enabled, we can try your patch, with the possibility that we might have to revert it, should it cause problems.

In practice we probably already interrupt EFI services with NMI interrupts, which can be pretty heavy as well if they for example generate printks.

So I'm not against this change in a strong fashion - I'm just a bit cautious and it would be nice to know how Windows behaves here.

Thanks,

Ingo

#17 lore raw

Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Quoted 20 lines · Ingo Molnar
Quoted 17 lines · Ard Biesheuvel
Quoted 16 lines

More fundamentally, this makes me nervous:

Quoted 2 lines

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. [...]

So what really matters is not what the spec says, but how Windows executes UEFI firmware code in practice.

If major versions of Windows calls UEFI firmware with interrupts disabled, then frankly I don't think we should interrupt them under Linux either, regardless of what the spec says ...

Random firmware code getting interrupted by the OS changes timings and might have other side effects the firmware code might not expect - so the question is, does Windows already de facto allow the IRQ preemption of firmware calls?

Quoted 6 lines
Quoted 1 line

Good question. I will try to find out.

Note that if there's a reasonable (but not 100%) case in favor of keeping irqs enabled, we can try your patch, with the possibility that we might have to revert it, should it cause problems.

I think this might have been the reason Matt wanted this in -next early, but I will let him confirm whether that was the case.

Quoted 6 lines

In practice we probably already interrupt EFI services with NMI interrupts, which can be pretty heavy as well if they for example generate printks.

So I'm not against this change in a strong fashion - I'm just a bit cautious and it would be nice to know how Windows behaves here.

I am not sure how yet, but I am going to try and figure out what Windows does. I suppose hacking OVMF to record some IRQ mask information when RT services are being invoked should be sufficient, but I am going to need some help from someone that understands OVMF and x86 (Matt?)

#18 lore raw

Re: [PATCH 07/14] efi: runtime-wrappers: Run UEFI Runtime Services with interrupts enabled

Quoted 23 lines · Ard Biesheuvel
Quoted 22 lines · Ingo Molnar
Quoted 18 lines · Ard Biesheuvel
Quoted 8 lines

More fundamentally, this makes me nervous:

Quoted 2 lines

The UEFI spec allows Runtime Services to be invoked with interrupts enabled. [...]

So what really matters is not what the spec says, but how Windows executes UEFI firmware code in practice.

Quoted 8 lines

If major versions of Windows calls UEFI firmware with interrupts disabled, then frankly I don't think we should interrupt them under Linux either, regardless of what the spec says ...

Random firmware code getting interrupted by the OS changes timings and might have other side effects the firmware code might not expect - so the question is, does Windows already de facto allow the IRQ preemption of firmware calls?

Good question. I will try to find out.

Quoted 7 lines
Quoted 4 lines

Note that if there's a reasonable (but not 100%) case in favor of keeping irqs enabled, we can try your patch, with the possibility that we might have to revert it, should it cause problems.

I think this might have been the reason Matt wanted this in -next early, but I will let him confirm whether that was the case.

It was indeed. Additionally I didn't want the EFI material to miss the merge window again.

Quoted 7 lines
Quoted 6 lines

In practice we probably already interrupt EFI services with NMI interrupts, which can be pretty heavy as well if they for example generate printks.

So I'm not against this change in a strong fashion - I'm just a bit cautious and it would be nice to know how Windows behaves here.

Quoted 5 lines

I am not sure how yet, but I am going to try and figure out what Windows does. I suppose hacking OVMF to record some IRQ mask information when RT services are being invoked should be sufficient, but I am going to need some help from someone that understands OVMF and x86 (Matt?)

Sure, I can help out with that. Hit me up on IRC. I'm also looping in Sai who has done OVMF hacking for OS diagnostics in the past.

#19 lore raw

PATCH 1/1 efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

In reply to Ingo Molnar

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

@Sai: could you please confirm on-list as well? Thanks.

Below is an updated version of the patch, rebased onto current tip/efi/core, with the BUG_ON() removed that I left in inadvertently. I also added a mention in the commit log that Windows leaves interrupts enabled as well. As far as annotating the definition of efi_runtime_lock is concerned, the existing ~40 lines of documentation should be sufficient imo so I left that as is. Thanks.

--------8<----------------
The UEFI spec allows Runtime Services to be invoked with interrupts enabled. The only reason we were disabling interrupts was to prevent recursive calls into the services on the same CPU, which will lead to deadlock. However, the only context where such invocations may occur legally is from efi-pstore via efivars, and that code has been updated to call a non-blocking alternative when invoked from a non-interruptible context.

So instead, update the ordinary, blocking UEFI Runtime Services wrappers to execute with interrupts enabled. This aims to prevent excessive interrupt latencies on uniprocessor platforms with slow variable stores.

Note that other OSes such as Windows call UEFI Runtime Services with interrupts enabled as well.

---
 drivers/firmware/efi/runtime-wrappers.c | 71 ++++++++------------
 1 file changed, 28 insertions(+), 43 deletions(-)
drivers/firmware/efi/runtime-wrappers.c +28 -43
index 7b8b2f2702ca..de6953039af6 100644
@@ -63,23 +63,21 @@ static DEFINE_SPINLOCK(efi_runtime_lock);
 
 static efi_status_t virt_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_time, tm, tc);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_time(efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_time, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -87,23 +85,21 @@ static efi_status_t virt_efi_get_wakeup_time(efi_bool_t *enabled,
 					     efi_bool_t *pending,
 					     efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_wakeup_time, enabled, pending, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_set_wakeup_time(efi_bool_t enabled, efi_time_t *tm)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_wakeup_time, enabled, tm);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -113,13 +109,12 @@ static efi_status_t virt_efi_get_variable(efi_char16_t *name,
 					  unsigned long *data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -127,12 +122,11 @@ static efi_status_t virt_efi_get_next_variable(unsigned long *name_size,
 					       efi_char16_t *name,
 					       efi_guid_t *vendor)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_variable, name_size, name, vendor);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -142,13 +136,12 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name,
 					  unsigned long data_size,
 					  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -157,15 +150,14 @@ virt_efi_set_variable_nonblocking(efi_char16_t *name, efi_guid_t *vendor,
 				  u32 attr, unsigned long data_size,
 				  void *data)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(set_variable, name, vendor, attr, data_size,
 			       data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -175,16 +167,15 @@ static efi_status_t virt_efi_query_variable_info(u32 attr,
 						 u64 *remaining_space,
 						 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -194,29 +185,27 @@ virt_efi_query_variable_info_nonblocking(u32 attr,
 					 u64 *remaining_space,
 					 u64 *max_variable_size)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	if (!spin_trylock_irqsave(&efi_runtime_lock, flags))
+	if (!spin_trylock(&efi_runtime_lock))
 		return EFI_NOT_READY;
 
 	status = efi_call_virt(query_variable_info, attr, storage_space,
 			       remaining_space, max_variable_size);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
 static efi_status_t virt_efi_get_next_high_mono_count(u32 *count)
 {
-	unsigned long flags;
 	efi_status_t status;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(get_next_high_mono_count, count);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -225,26 +214,23 @@ static void virt_efi_reset_system(int reset_type,
 				  unsigned long data_size,
 				  efi_char16_t *data)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	__efi_call_virt(reset_system, reset_type, status, data_size, data);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 }
 
 static efi_status_t virt_efi_update_capsule(efi_capsule_header_t **capsules,
 					    unsigned long count,
 					    unsigned long sg_list)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(update_capsule, capsules, count, sg_list);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
@@ -253,16 +239,15 @@ static efi_status_t virt_efi_query_capsule_caps(efi_capsule_header_t **capsules,
 						u64 *max_size,
 						int *reset_type)
 {
-	unsigned long flags;
 	efi_status_t status;
 
 	if (efi.runtime_version < EFI_2_00_SYSTEM_TABLE_REVISION)
 		return EFI_UNSUPPORTED;
 
-	spin_lock_irqsave(&efi_runtime_lock, flags);
+	spin_lock(&efi_runtime_lock);
 	status = efi_call_virt(query_capsule_caps, capsules, count, max_size,
 			       reset_type);
-	spin_unlock_irqrestore(&efi_runtime_lock, flags);
+	spin_unlock(&efi_runtime_lock);
 	return status;
 }
 
Signature · 1 line
2.5.0
author Reviewed-by on v1 1/1
#20 lore raw

Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

Quoted 26 lines · Ard Biesheuvel

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

@Sai: could you please confirm on-list as well? Thanks.

Below is an updated version of the patch, rebased onto current tip/efi/core, with the BUG_ON() removed that I left in inadvertently. I also added a mention in the commit log that Windows leaves interrupts enabled as well. As far as annotating the definition of efi_runtime_lock is concerned, the existing ~40 lines of documentation should be sufficient imo so I left that as is. Thanks.

--------8<----------------
The UEFI spec allows Runtime Services to be invoked with interrupts enabled. The only reason we were disabling interrupts was to prevent recursive calls into the services on the same CPU, which will lead to deadlock. However, the only context where such invocations may occur legally is from efi-pstore via efivars, and that code has been updated to call a non-blocking alternative when invoked from a non-interruptible context.

So instead, update the ordinary, blocking UEFI Runtime Services wrappers to execute with interrupts enabled. This aims to prevent excessive interrupt latencies on uniprocessor platforms with slow variable stores.

Quoted 7 lines

Note that other OSes such as Windows call UEFI Runtime Services with interrupts enabled as well.

---
 drivers/firmware/efi/runtime-wrappers.c | 71 ++++++++------------
 1 file changed, 28 insertions(+), 43 deletions(-)

Ingo, if you want to pick up this patch directly you can add my,

Otherwise let me know and I'll send you a pull request.

#21 lore raw

Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

In reply to Ard Biesheuvel
Quoted 5 lines · Ard Biesheuvel

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

Could this use a mutex instead of a spinlock?

Can someone with a mixed mode setup read a variable in a loop and make sure it doesn't crash and burn? It should work fine, but explicit testing would be nice. (It's interesting mainly because doing a mixed mode call with interrupts on can result in a non-IST CPL0 to CPL0 exception delivery, which won't result in a stack switch. This could easily trigger a stack overflow, logic bug, microcode bug, or as-yet-unknown CPU "feature".

Hmm. We should also audit the mixed mode entry code to make sure that the high bits of RSP are explicitly clear before switching into compat mode. If I had to make a guess about how CPUs behave, I'd guess
pessimistically: Intel CPUs clear the high bits of RSP when switching into long mode due to interrupt delivery, and AMD CPUs leave them set just to mess with us.

Also, a WARN_ON(in_interrupt()) somewhere might be a good sanity check.

--Andy

#22 lore raw

Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

Quoted 9 lines · Andy Lutomirski
Quoted 5 lines · Ard Biesheuvel

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

Could this use a mutex instead of a spinlock?

When I first started working on this code, I proposed using a mutex, but at the time, we still had the efi-pstore case to worry about
http://article.gmane.org/gmane.linux.kernel.efi/4112

In the mean time, we have modified the efi-pstore code so it simply gives up when the EFI varstore is busy, and we also got rid of the NMI special case where locks are ignored. In summary, it sounds to me that moving to a mutex should be feasible, but I am only really familiar with the ARM side of the implementation, which is far less complex than the x86 side, so Matt should confirm.

@Matt?

Quoted 17 lines

Can someone with a mixed mode setup read a variable in a loop and make sure it doesn't crash and burn? It should work fine, but explicit testing would be nice. (It's interesting mainly because doing a mixed mode call with interrupts on can result in a non-IST CPL0 to CPL0 exception delivery, which won't result in a stack switch. This could easily trigger a stack overflow, logic bug, microcode bug, or as-yet-unknown CPU "feature".

Hmm. We should also audit the mixed mode entry code to make sure that the high bits of RSP are explicitly clear before switching into compat mode. If I had to make a guess about how CPUs behave, I'd guess
pessimistically: Intel CPUs clear the high bits of RSP when switching into long mode due to interrupt delivery, and AMD CPUs leave them set just to mess with us.

Also, a WARN_ON(in_interrupt()) somewhere might be a good sanity check.

#23 lore raw

Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

Quoted 15 lines · Ard Biesheuvel
Quoted 9 lines · Andy Lutomirski
Quoted 5 lines · Ard Biesheuvel

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

Could this use a mutex instead of a spinlock?

When I first started working on this code, I proposed using a mutex, but at the time, we still had the efi-pstore case to worry about
http://article.gmane.org/gmane.linux.kernel.efi/4112

Quoted 6 lines

In the mean time, we have modified the efi-pstore code so it simply gives up when the EFI varstore is busy, and we also got rid of the NMI special case where locks are ignored. In summary, it sounds to me that moving to a mutex should be feasible, but I am only really familiar with the ARM side of the implementation, which is far less complex than the x86 side, so Matt should confirm.

You cannot touch mutexes from interrupt context, not even to perform a mutex_trylock(), and that's the problem.

Since the efi-pstore code can run from interrupt context we need to use a locking primitive that works, even though we have all that non-blocking logic.

#24 lore raw

Re: [PATCH] efi: runtime-wrappers: run UEFI Runtime Services with interrupts enabled

In reply to Andy Lutomirski
Quoted 9 lines · Andy Lutomirski
Quoted 5 lines · Ard Biesheuvel

OK, since Sai has confirmed that Windows leaves interrupts enabled when calling the EFI variable store related runtime services, we should be able to do the same for Linux, or at least be slightly more confident that we won't have to back out this change later.

Could this use a mutex instead of a spinlock?

Quoted 7 lines

Can someone with a mixed mode setup read a variable in a loop and make sure it doesn't crash and burn? It should work fine, but explicit testing would be nice. (It's interesting mainly because doing a mixed mode call with interrupts on can result in a non-IST CPL0 to CPL0 exception delivery, which won't result in a stack switch. This could easily trigger a stack overflow, logic bug, microcode bug, or as-yet-unknown CPU "feature".

I don't have physical hardware for testing mixed mode anymore (that was returned to Intel when I left) but testing with Qemu didn't turn up any problems when running with interrupts enabled.

Quoted 6 lines

Hmm. We should also audit the mixed mode entry code to make sure that the high bits of RSP are explicitly clear before switching into compat mode. If I had to make a guess about how CPUs behave, I'd guess
pessimistically: Intel CPUs clear the high bits of RSP when switching into long mode due to interrupt delivery, and AMD CPUs leave them set just to mess with us.

Interesting thought. I'm not aware of anyone testing mixed mode with AMD CPUs, so that would be a good data point.

Quoted 1 line

Also, a WARN_ON(in_interrupt()) somewhere might be a good sanity check.

lockdep should catch this kind of stuff pretty quickly since we grab the spinlock in every code path.

#25 lore raw

PATCH 8/14 efivars: Use to_efivar_entry

In reply to Matt Fleming

From: Geliang Tang <[email protected]>

Use to_efivar_entry() instead of open-coding it.

---
 drivers/firmware/efi/efivars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/efivars.c +1 -1
index 756eca8c4cf8..c5b0d2bc1111 100644
@@ -386,7 +386,7 @@ static const struct sysfs_ops efivar_attr_ops = {
 
 static void efivar_release(struct kobject *kobj)
 {
-	struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
+	struct efivar_entry *var = to_efivar_entry(kobj);
 	kfree(var);
 }
 
Signature · 1 line
2.6.2
#26 lore raw

[tip:efi/core] efivars: Use to_efivar_entry

efivars: Use to_efivar_entry
Commit-ID:  9c09a342eb27902955ca939bd6ba29d875bd8b6b
Gitweb:     http://git.kernel.org/tip/9c09a342eb27902955ca939bd6ba29d875bd8b6b
Author:     Geliang Tang <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:02 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:41:19 +0100

efivars: Use to_efivar_entry

Use to_efivar_entry() instead of open-coding it.

---
 drivers/firmware/efi/efivars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/efivars.c +1 -1
index 756eca8..c5b0d2b 100644
@@ -386,7 +386,7 @@ static const struct sysfs_ops efivar_attr_ops = {
 
 static void efivar_release(struct kobject *kobj)
 {
-	struct efivar_entry *var = container_of(kobj, struct efivar_entry, kobj);
+	struct efivar_entry *var = to_efivar_entry(kobj);
 	kfree(var);
 }
 
#27 lore raw

PATCH 9/14 x86/efi-bgrt: Don't ignore the BGRT if the 'valid' bit is 0

In reply to Matt Fleming

From: Môshe van der Sterre <[email protected]>

Unintuitively, the BGRT graphic is apparently meant to be usable if the valid bit in not set. The valid bit only conveys uncertainty about the validity in relation to the screen state.

Windows 10 actually uses the BGRT image for its boot screen even if not 'valid', for example when the user triggered the boot menu. Because it is unclear if all firmwares will provide a usable graphic in this case, we now look at the BMP magic number as an additional check.

---
 arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/platform/efi/efi-bgrt.c +5 -5
index b0970661870a..a2433817c987 100644
@@ -57,11 +57,6 @@ void __init efi_bgrt_init(void)
 		       bgrt_tab->status);
 		return;
 	}
-	if (bgrt_tab->status != 1) {
-		pr_debug("Ignoring BGRT: invalid status %u (expected 1)\n",
-			 bgrt_tab->status);
-		return;
-	}
 	if (bgrt_tab->image_type != 0) {
 		pr_err("Ignoring BGRT: invalid image type %u (expected 0)\n",
 		       bgrt_tab->image_type);
@@ -80,6 +75,11 @@ void __init efi_bgrt_init(void)
 
 	memcpy(&bmp_header, image, sizeof(bmp_header));
 	memunmap(image);
+	if (bmp_header.id != 0x4d42) {
+		pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
+			bmp_header.id);
+		return;
+	}
 	bgrt_image_size = bmp_header.size;
 
 	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
Signature · 1 line
2.6.2
#28 lore raw

[tip:efi/core] x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0

x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0
Commit-ID:  66dbe99cfe30e113d2e571e68b9b6a1a8985a157
Gitweb:     http://git.kernel.org/tip/66dbe99cfe30e113d2e571e68b9b6a1a8985a157
Author:     Môshe van der Sterre <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:03 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:41:19 +0100

x86/efi/bgrt: Don't ignore the BGRT if the 'valid' bit is 0

Unintuitively, the BGRT graphic is apparently meant to be usable if the valid bit in not set. The valid bit only conveys uncertainty about the validity in relation to the screen state.

Windows 10 actually uses the BGRT image for its boot screen even if not 'valid', for example when the user triggered the boot menu. Because it is unclear if all firmwares will provide a usable graphic in this case, we now look at the BMP magic number as an additional check.

---
 arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
arch/x86/platform/efi/efi-bgrt.c +5 -5
index b097066..a243381 100644
@@ -57,11 +57,6 @@ void __init efi_bgrt_init(void)
 		       bgrt_tab->status);
 		return;
 	}
-	if (bgrt_tab->status != 1) {
-		pr_debug("Ignoring BGRT: invalid status %u (expected 1)\n",
-			 bgrt_tab->status);
-		return;
-	}
 	if (bgrt_tab->image_type != 0) {
 		pr_err("Ignoring BGRT: invalid image type %u (expected 0)\n",
 		       bgrt_tab->image_type);
@@ -80,6 +75,11 @@ void __init efi_bgrt_init(void)
 
 	memcpy(&bmp_header, image, sizeof(bmp_header));
 	memunmap(image);
+	if (bmp_header.id != 0x4d42) {
+		pr_err("Ignoring BGRT: Incorrect BMP magic number 0x%x (expected 0x4d42)\n",
+			bmp_header.id);
+		return;
+	}
 	bgrt_image_size = bmp_header.size;
 
 	bgrt_image = kmalloc(bgrt_image_size, GFP_KERNEL | __GFP_NOWARN);
#29 lore raw

PATCH 10/14 efi: Make checkpatch complain less about efi.h GUID additions

In reply to Matt Fleming

From: Peter Jones <[email protected]>

This reformats the GUID definitions in include/linux/efi.h so that if you add another one with the same style, checkpatch won't complain about it.

---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 21 deletions(-)
include/linux/efi.h +42 -21
index 09f1559e7525..f468f7c53236 100644
@@ -535,67 +535,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions
  */
 #define NULL_GUID \
-    EFI_GUID(  0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 )
+	EFI_GUID(0x00000000, 0x0000, 0x0000, \
+		 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define ACPI_20_TABLE_GUID    \
-    EFI_GUID(  0x8868e871, 0xe4f1, 0x11d3, 0xbc, 0x22, 0x0, 0x80, 0xc7, 0x3c, 0x88, 0x81 )
+	EFI_GUID(0x8868e871, 0xe4f1, 0x11d3, \
+		 0xbc, 0x22, 0x00, 0x80, 0xc7, 0x3c, 0x88, 0x81)
 
 #define SMBIOS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d31, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d31, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define SMBIOS3_TABLE_GUID    \
-    EFI_GUID(  0xf2fd1544, 0x9794, 0x4a2c, 0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94 )
+	EFI_GUID(0xf2fd1544, 0x9794, 0x4a2c, \
+		 0x99, 0x2e, 0xe5, 0xbb, 0xcf, 0x20, 0xe3, 0x94)
 
 #define SAL_SYSTEM_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d32, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d32, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
 #define HCDP_TABLE_GUID	\
-    EFI_GUID(  0xf951938d, 0x620b, 0x42ef, 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98 )
+	EFI_GUID(0xf951938d, 0x620b, 0x42ef, \
+		 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98)
 
 #define UGA_IO_PROTOCOL_GUID \
-    EFI_GUID(  0x61a4d49e, 0x6f68, 0x4f1b, 0xb9, 0x22, 0xa8, 0x6e, 0xed, 0xb, 0x7, 0xa2 )
+	EFI_GUID(0x61a4d49e, 0x6f68, 0x4f1b, \
+		 0xb9, 0x22, 0xa8, 0x6e, 0xed, 0x0b, 0x07, 0xa2)
 
 #define EFI_GLOBAL_VARIABLE_GUID \
-    EFI_GUID(  0x8be4df61, 0x93ca, 0x11d2, 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c )
+	EFI_GUID(0x8be4df61, 0x93ca, 0x11d2, \
+		 0xaa, 0x0d, 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
 
 #define UV_SYSTEM_TABLE_GUID \
-    EFI_GUID(  0x3b13a7d4, 0x633e, 0x11dd, 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93 )
+	EFI_GUID(0x3b13a7d4, 0x633e, 0x11dd, \
+		 0x93, 0xec, 0xda, 0x25, 0x56, 0xd8, 0x95, 0x93)
 
 #define LINUX_EFI_CRASH_GUID \
-    EFI_GUID(  0xcfc8fc79, 0xbe2e, 0x4ddc, 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0 )
+	EFI_GUID(0xcfc8fc79, 0xbe2e, 0x4ddc, \
+		 0x97, 0xf0, 0x9f, 0x98, 0xbf, 0xe2, 0x98, 0xa0)
 
 #define LOADED_IMAGE_PROTOCOL_GUID \
-    EFI_GUID(  0x5b1b31a1, 0x9562, 0x11d2, 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b )
+	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
+		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
 #define EFI_GRAPHICS_OUTPUT_PROTOCOL_GUID \
-    EFI_GUID(  0x9042a9de, 0x23dc, 0x4a38, 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a )
+	EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
+		 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
 
 #define EFI_UGA_PROTOCOL_GUID \
-    EFI_GUID(  0x982c298b, 0xf4fa, 0x41cb, 0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 0xb8, 0x39 )
+	EFI_GUID(0x982c298b, 0xf4fa, 0x41cb, \
+		 0xb8, 0x38, 0x77, 0xaa, 0x68, 0x8f, 0xb8, 0x39)
 
 #define EFI_PCI_IO_PROTOCOL_GUID \
-    EFI_GUID(  0x4cf5b200, 0x68b8, 0x4ca5, 0x9e, 0xec, 0xb2, 0x3e, 0x3f, 0x50, 0x2, 0x9a )
+	EFI_GUID(0x4cf5b200, 0x68b8, 0x4ca5, \
+		 0x9e, 0xec, 0xb2, 0x3e, 0x3f, 0x50, 0x02, 0x9a)
 
 #define EFI_FILE_INFO_ID \
-    EFI_GUID(  0x9576e92, 0x6d3f, 0x11d2, 0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b )
+	EFI_GUID(0x9576e92, 0x6d3f, 0x11d2, \
+		 0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
 #define EFI_SYSTEM_RESOURCE_TABLE_GUID \
-    EFI_GUID(  0xb122a263, 0x3661, 0x4f68, 0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80 )
+	EFI_GUID(0xb122a263, 0x3661, 0x4f68, \
+		 0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80)
 
 #define EFI_FILE_SYSTEM_GUID \
-    EFI_GUID(  0x964e5b22, 0x6459, 0x11d2, 0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b )
+	EFI_GUID(0x964e5b22, 0x6459, 0x11d2, \
+		 0x8e, 0x39, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
 #define DEVICE_TREE_GUID \
-    EFI_GUID(  0xb1b621d5, 0xf19c, 0x41a5, 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0 )
+	EFI_GUID(0xb1b621d5, 0xf19c, 0x41a5, \
+		 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0)
 
 #define EFI_PROPERTIES_TABLE_GUID \
-    EFI_GUID(  0x880aaca3, 0x4adc, 0x4a04, 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5 )
+	EFI_GUID(0x880aaca3, 0x4adc, 0x4a04, \
+		 0x90, 0x79, 0xb7, 0x47, 0x34, 0x08, 0x25, 0xe5)
 
 typedef struct {
 	efi_guid_t guid;
Signature · 1 line
2.6.2
#30 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

Quoted 19 lines · Matt Fleming

From: Peter Jones <[email protected]>

This reformats the GUID definitions in include/linux/efi.h so that if you add another one with the same style, checkpatch won't complain about it.

---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 21 deletions(-)
include/linux/efi.h +0 -0
index 09f1559e7525..f468f7c53236 100644
@@ -535,67 +535,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions

include/linux/efi.h void efi_native_runtime_setup(void); · line 543

Quoted 10 lines
include/linux/efi.h +4 -2
  */
 #define NULL_GUID \
-    EFI_GUID(  0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 )
+	EFI_GUID(0x00000000, 0x0000, 0x0000, \
+		 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)

So I really think this is a step backwards.

Checkpatch should be fixed/enhanced to allow targeted exemption. Something like:

	#define CHECKPATCH_IGNORE
	...
	#undef CHECKPATCH_IGNORE

... which checkpatch would parse and interpret accordingly.

Thanks,

Ingo

#31 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

Quoted 35 lines · Ingo Molnar
Quoted 19 lines · Matt Fleming

From: Peter Jones <[email protected]>

This reformats the GUID definitions in include/linux/efi.h so that if you add another one with the same style, checkpatch won't complain about it.

---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 21 deletions(-)
include/linux/efi.h +0 -0
index 09f1559e7525..f468f7c53236 100644
@@ -535,67 +535,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions

include/linux/efi.h void efi_native_runtime_setup(void); · line 543

Quoted 10 lines
include/linux/efi.h +4 -2
  */
 #define NULL_GUID \
-    EFI_GUID(  0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 )
+	EFI_GUID(0x00000000, 0x0000, 0x0000, \
+		 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)

So I really think this is a step backwards.

Quoted 8 lines

Checkpatch should be fixed/enhanced to allow targeted exemption. Something like:

	#define CHECKPATCH_IGNORE
	...
	#undef CHECKPATCH_IGNORE

... which checkpatch would parse and interpret accordingly.

Irrespective of which tool suggested this change, I think this patch is an improvement because the GUIDs now match the format from the UEFI spec, making checking for typos that much easier (yes, I've really had to do that in the past).

#32 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

Quoted 43 lines · Matt Fleming
Quoted 42 lines · Ingo Molnar
Quoted 19 lines · Matt Fleming

From: Peter Jones <[email protected]>

This reformats the GUID definitions in include/linux/efi.h so that if you add another one with the same style, checkpatch won't complain about it.

---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 21 deletions(-)
include/linux/efi.h +0 -0
index 09f1559e7525..f468f7c53236 100644
@@ -535,67 +535,88 @@ void efi_native_runtime_setup(void);
  *  EFI Configuration Table and GUID definitions

include/linux/efi.h void efi_native_runtime_setup(void); · line 543

Quoted 10 lines
include/linux/efi.h +4 -2
  */
 #define NULL_GUID \
-    EFI_GUID(  0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 )
+	EFI_GUID(0x00000000, 0x0000, 0x0000, \
+		 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
 
 #define MPS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )
+	EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)

So I really think this is a step backwards.

Checkpatch should be fixed/enhanced to allow targeted exemption. Something like:

	#define CHECKPATCH_IGNORE
	...
	#undef CHECKPATCH_IGNORE
Quoted 6 lines
Quoted 1 line

... which checkpatch would parse and interpret accordingly.

Irrespective of which tool suggested this change, I think this patch is an improvement because the GUIDs now match the format from the UEFI spec, making checking for typos that much easier (yes, I've really had to do that in the past).

Hm, so the GUIDs are line-broken in the same fashion in the spec, after the third parameter?

That's a strong reason indeed - and then the changelog and title should say that:
're-format GUID tables to follow the format of the UEFI spec'. That it also pacifies checkpatch is a side effect.

Thanks,

Ingo

#33 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

Quoted 3 lines · Ingo Molnar

Hm, so the GUIDs are line-broken in the same fashion in the spec, after the third parameter?

Yep, they are.

Quoted 3 lines

That's a strong reason indeed - and then the changelog and title should say that:
're-format GUID tables to follow the format of the UEFI spec'. That it also pacifies checkpatch is a side effect.

I think that's a fair change.

Peter could you take a look at updating the changelog in a v2? If not, I'll do it.

#34 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

Quoted 7 lines · Matt Fleming
Quoted 3 lines · Ingo Molnar

Hm, so the GUIDs are line-broken in the same fashion in the spec, after the third parameter?

Yep, they are.

Quoted 8 lines
Quoted 3 lines

That's a strong reason indeed - and then the changelog and title should say that:
're-format GUID tables to follow the format of the UEFI spec'. That it also pacifies checkpatch is a side effect.

I think that's a fair change.

Peter could you take a look at updating the changelog in a v2? If not, I'll do it.

Note that I applied most of your patches to tip:efi/core, which I've just pushed out. You might want to base v2 on that.

Thanks,

Ingo

#35 lore raw

Re: [PATCH 10/14] efi: Make checkpatch complain less about efi.h GUID additions

In reply to Ingo Molnar
Quoted 25 lines · Ingo Molnar
Quoted 23 lines · Matt Fleming

From: Peter Jones <[email protected]>

This reformats the GUID definitions in include/linux/efi.h so that if you add another one with the same style, checkpatch won't complain about it.

---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------

 1 file changed, 42 insertions(+), 21 deletions(-)
include/linux/efi.h +1 -1
index 09f1559e7525..f468f7c53236 100644
@@ -535,67 +535,88 @@ void efi_native_runtime_setup(void);
   *  EFI Configuration Table and GUID definitions
   */
  #define NULL_GUID \
-    EFI_GUID(  0x00000000, 0x0000, 0x0000, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 )
+	EFI_GUID(0x00000000, 0x0000, 0x0000, \
Quoted 8 lines

include/linux/efi.h

Quoted 6 lines
include/linux/efi.h +1 -0
+		 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)


 #define MPS_TABLE_GUID    \
-    EFI_GUID(  0xeb9d2d2f, 0x2d88, 0x11d3, 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d )

include/linux/efi.h +2 -0
+	EFI_GUID(0xeb9d2d2f, 0x2d88, 0x11d3, \
+		 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)

So I really think this is a step backwards.

Some people take checkpatch messages altogether too seriously.

from: https://lkml.org/lkml/2015/7/16/568
----------------------------------
The other thing that might help is for people to take the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such
----------------------------------

Quoted 8 lines

Checkpatch should be fixed/enhanced to allow targeted exemption. Something like:

	#define CHECKPATCH_IGNORE
	...
	#undef CHECKPATCH_IGNORE

... which checkpatch would parse and interpret accordingly.

A similar proposal: https://lkml.org/lkml/2016/1/30/175

checkpatch works on patches.
If the #define isn't in the patch scope the script won't know.

Perhaps it's simpler to add some facility to allow lines with known keywords to exceed $max_line_length similar to the format strings of logging functions.

#36 lore raw

PATCH 11/14 x86/efi: Show actual ending addresses in efi_print_memmap

In reply to Matt Fleming

From: Robert Elliott <[email protected]>

Adjust efi_print_memmap to print the real end address of each range, not 1 byte beyond. This matches other prints like those for SRAT and nosave memory.

While investigating grub persistent memory corruption issues, it was helpful to make this table match the ending address convention used by:
* the kernel's e820 table prints

	BIOS-e820: [mem 0x0000001680000000-0x0000001c7fffffff] reserved
* the kernel's nosave memory prints
	PM: Registered nosave memory: [mem 0x880000000-0xc7fffffff]
* the kernel's ACPI System Resource Affinity Table prints
	SRAT: Node 1 PXM 1 [mem 0x480000000-0x87fffffff]
* grub's lsmmap and lsefimmap commands
	reserved  0000001680000000-0000001c7fffffff 00600000     24GiB UC WC WT WB NV
* the UEFI shell's memmap command
	Reserved   000000007FC00000-000000007FFFFFFF 0000000000000400 0000000000000001

For example, if you grep all the various logs for c7fffffff, you won't find the kernel's line if it uses c80000000.

Also, change the closing ) to ] to match the opening [.

old:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c80000000) (16384MB)

new:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16384MB)
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi.c +2 -2
index bdd9477f937c..e80826e6f3a9 100644
@@ -235,10 +235,10 @@ void __init efi_print_memmap(void)
 		char buf[64];
 
 		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx) (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
 			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
+			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
 			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
 	}
 #endif  /*  EFI_DEBUG  */
Signature · 1 line
2.6.2
Reviewed-by on v1 11/14
#37 lore raw

Re: [PATCH 11/14] x86/efi: Show actual ending addresses in efi_print_memmap

Quoted 51 lines · Matt Fleming

From: Robert Elliott <[email protected]>

Adjust efi_print_memmap to print the real end address of each range, not 1 byte beyond. This matches other prints like those for SRAT and nosave memory.

While investigating grub persistent memory corruption issues, it was helpful to make this table match the ending address convention used by:
* the kernel's e820 table prints

	BIOS-e820: [mem 0x0000001680000000-0x0000001c7fffffff] reserved
* the kernel's nosave memory prints
	PM: Registered nosave memory: [mem 0x880000000-0xc7fffffff]
* the kernel's ACPI System Resource Affinity Table prints
	SRAT: Node 1 PXM 1 [mem 0x480000000-0x87fffffff]
* grub's lsmmap and lsefimmap commands
	reserved  0000001680000000-0000001c7fffffff 00600000     24GiB UC WC WT WB NV
* the UEFI shell's memmap command
	Reserved   000000007FC00000-000000007FFFFFFF 0000000000000400 0000000000000001

For example, if you grep all the various logs for c7fffffff, you won't find the kernel's line if it uses c80000000.

Also, change the closing ) to ] to match the opening [.

old:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c80000000) (16384MB)

new:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16384MB)
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi.c +0 -0
index bdd9477f937c..e80826e6f3a9 100644
@@ -235,10 +235,10 @@ void __init efi_print_memmap(void)
 		char buf[64];
 

arch/x86/platform/efi/efi.c void __init efi_print_memmap(void) · line 244

Quoted 10 lines
arch/x86/platform/efi/efi.c +2 -2
 		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx) (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
 			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
+			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
 			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
 	}
 #endif  /*  EFI_DEBUG  */

For this patch:

With regard to other arches:

- I think that "arch/arm64/kernel/efi.c" prints stuff like this already, in reserve_regions(). OK.

- However, I think "arch/ia64/kernel/efi.c" needs a similar patch. (Maybe your PULL includes such a patch already, but I'm not CC'd on it.) See in the function efi_init(), near efi_md_typeattr_format().

Thanks
Laszlo

#38 lore raw

[tip:efi/core] x86/efi: Show actual ending addresses in efi_print_memmap

In reply to Matt Fleming
x86/efi: Show actual ending addresses in efi_print_memmap
Commit-ID:  1e82b94790709fb2a22d16d53bb04d751fb3878d
Gitweb:     http://git.kernel.org/tip/1e82b94790709fb2a22d16d53bb04d751fb3878d
Author:     Robert Elliott <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:05 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:41:20 +0100

x86/efi: Show actual ending addresses in efi_print_memmap

Adjust efi_print_memmap to print the real end address of each range, not 1 byte beyond. This matches other prints like those for SRAT and nosave memory.

While investigating grub persistent memory corruption issues, it was helpful to make this table match the ending address convention used by:
* the kernel's e820 table prints

	BIOS-e820: [mem 0x0000001680000000-0x0000001c7fffffff] reserved
* the kernel's nosave memory prints
	PM: Registered nosave memory: [mem 0x880000000-0xc7fffffff]
* the kernel's ACPI System Resource Affinity Table prints
	SRAT: Node 1 PXM 1 [mem 0x480000000-0x87fffffff]
* grub's lsmmap and lsefimmap commands
	reserved  0000001680000000-0000001c7fffffff 00600000     24GiB UC WC WT WB NV
* the UEFI shell's memmap command
	Reserved   000000007FC00000-000000007FFFFFFF 0000000000000400 0000000000000001

For example, if you grep all the various logs for c7fffffff, you won't find the kernel's line if it uses c80000000.

Also, change the closing ) to ] to match the opening [.

old:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c80000000) (16384MB)

new:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16384MB)
Cc Andy Lutomirski <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Leif Lindholm <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/platform/efi/efi.c +2 -2
index bdd9477..e80826e 100644
@@ -235,10 +235,10 @@ void __init efi_print_memmap(void)
 		char buf[64];
 
 		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx) (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
 			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT),
+			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
 			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
 	}
 #endif  /*  EFI_DEBUG  */
#39 lore raw

PATCH 12/14 efi: Add NV memory attribute

In reply to Matt Fleming

From: Robert Elliott <[email protected]>

Add the NV memory attribute introduced in UEFI 2.5 and add a column for it in the types and attributes string used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [type=14 | | |NV| | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 drivers/firmware/efi/efi.c | 5 ++++-
 include/linux/efi.h        | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +4 -1
index 0cd8f039602e..9c8c0747c8cd 100644
@@ -580,13 +580,16 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 	if (attr & ~(EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT |
 		     EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_RO |
 		     EFI_MEMORY_WP | EFI_MEMORY_RP | EFI_MEMORY_XP |
+		     EFI_MEMORY_NV |
 		     EFI_MEMORY_RUNTIME | EFI_MEMORY_MORE_RELIABLE))
 		snprintf(pos, size, "|attr=0x%016llx]",
 			 (unsigned long long)attr);
 	else
-		snprintf(pos, size, "|%3s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
+		snprintf(pos, size,
+			 "|%3s|%2s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
 			 attr & EFI_MEMORY_RUNTIME ? "RUN" : "",
 			 attr & EFI_MEMORY_MORE_RELIABLE ? "MR" : "",
+			 attr & EFI_MEMORY_NV      ? "NV"  : "",
 			 attr & EFI_MEMORY_XP      ? "XP"  : "",
 			 attr & EFI_MEMORY_RP      ? "RP"  : "",
 			 attr & EFI_MEMORY_WP      ? "WP"  : "",
include/linux/efi.h +1 -0
index f468f7c53236..99b88c5a6770 100644
@@ -97,6 +97,7 @@ typedef	struct {
 #define EFI_MEMORY_WP		((u64)0x0000000000001000ULL)	/* write-protect */
 #define EFI_MEMORY_RP		((u64)0x0000000000002000ULL)	/* read-protect */
 #define EFI_MEMORY_XP		((u64)0x0000000000004000ULL)	/* execute-protect */
+#define EFI_MEMORY_NV		((u64)0x0000000000008000ULL)	/* non-volatile */
 #define EFI_MEMORY_MORE_RELIABLE \
 				((u64)0x0000000000010000ULL)	/* higher reliability */
 #define EFI_MEMORY_RO		((u64)0x0000000000020000ULL)	/* read-only */
Signature · 1 line
2.6.2
Reviewed-by on v1 12/14
#40 lore raw

Re: [PATCH 12/14] efi: Add NV memory attribute

Quoted 53 lines · Matt Fleming

From: Robert Elliott <[email protected]>

Add the NV memory attribute introduced in UEFI 2.5 and add a column for it in the types and attributes string used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [type=14 | | |NV| | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 drivers/firmware/efi/efi.c | 5 ++++-
 include/linux/efi.h        | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +4 -1
index 0cd8f039602e..9c8c0747c8cd 100644
@@ -580,13 +580,16 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 	if (attr & ~(EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT |
 		     EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_RO |
 		     EFI_MEMORY_WP | EFI_MEMORY_RP | EFI_MEMORY_XP |
+		     EFI_MEMORY_NV |
 		     EFI_MEMORY_RUNTIME | EFI_MEMORY_MORE_RELIABLE))
 		snprintf(pos, size, "|attr=0x%016llx]",
 			 (unsigned long long)attr);
 	else
-		snprintf(pos, size, "|%3s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
+		snprintf(pos, size,
+			 "|%3s|%2s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
 			 attr & EFI_MEMORY_RUNTIME ? "RUN" : "",
 			 attr & EFI_MEMORY_MORE_RELIABLE ? "MR" : "",
+			 attr & EFI_MEMORY_NV      ? "NV"  : "",
 			 attr & EFI_MEMORY_XP      ? "XP"  : "",
 			 attr & EFI_MEMORY_RP      ? "RP"  : "",
 			 attr & EFI_MEMORY_WP      ? "WP"  : "",
include/linux/efi.h +0 -0
index f468f7c53236..99b88c5a6770 100644

include/linux/efi.h typedef struct { · line 103

Quoted 7 lines
include/linux/efi.h +1 -0
@@ -97,6 +97,7 @@ typedef	struct {
 #define EFI_MEMORY_WP		((u64)0x0000000000001000ULL)	/* write-protect */
 #define EFI_MEMORY_RP		((u64)0x0000000000002000ULL)	/* read-protect */
 #define EFI_MEMORY_XP		((u64)0x0000000000004000ULL)	/* execute-protect */
+#define EFI_MEMORY_NV		((u64)0x0000000000008000ULL)	/* non-volatile */
 #define EFI_MEMORY_MORE_RELIABLE \
 				((u64)0x0000000000010000ULL)	/* higher reliability */
 #define EFI_MEMORY_RO		((u64)0x0000000000020000ULL)	/* read-only */
#41 lore raw

[tip:efi/core] efi: Add NV memory attribute

In reply to Matt Fleming
efi: Add NV memory attribute
Commit-ID:  c016ca08f89c6c78ed815f025262bdb87aba3f4c
Gitweb:     http://git.kernel.org/tip/c016ca08f89c6c78ed815f025262bdb87aba3f4c
Author:     Robert Elliott <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:06 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:41:20 +0100

efi: Add NV memory attribute

Add the NV memory attribute introduced in UEFI 2.5 and add a column for it in the types and attributes string used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [type=14 | | |NV| | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Andy Lutomirski <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Dan Williams <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ross Zwisler <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/firmware/efi/efi.c | 5 ++++-
 include/linux/efi.h        | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +4 -1
index 20451c2..f437048 100644
@@ -582,13 +582,16 @@ char * __init efi_md_typeattr_format(char *buf, size_t size,
 	if (attr & ~(EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT |
 		     EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_RO |
 		     EFI_MEMORY_WP | EFI_MEMORY_RP | EFI_MEMORY_XP |
+		     EFI_MEMORY_NV |
 		     EFI_MEMORY_RUNTIME | EFI_MEMORY_MORE_RELIABLE))
 		snprintf(pos, size, "|attr=0x%016llx]",
 			 (unsigned long long)attr);
 	else
-		snprintf(pos, size, "|%3s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
+		snprintf(pos, size,
+			 "|%3s|%2s|%2s|%2s|%2s|%2s|%2s|%3s|%2s|%2s|%2s|%2s]",
 			 attr & EFI_MEMORY_RUNTIME ? "RUN" : "",
 			 attr & EFI_MEMORY_MORE_RELIABLE ? "MR" : "",
+			 attr & EFI_MEMORY_NV      ? "NV"  : "",
 			 attr & EFI_MEMORY_XP      ? "XP"  : "",
 			 attr & EFI_MEMORY_RP      ? "RP"  : "",
 			 attr & EFI_MEMORY_WP      ? "WP"  : "",
include/linux/efi.h +1 -0
index 09f1559..3c6cbbd 100644
@@ -97,6 +97,7 @@ typedef	struct {
 #define EFI_MEMORY_WP		((u64)0x0000000000001000ULL)	/* write-protect */
 #define EFI_MEMORY_RP		((u64)0x0000000000002000ULL)	/* read-protect */
 #define EFI_MEMORY_XP		((u64)0x0000000000004000ULL)	/* execute-protect */
+#define EFI_MEMORY_NV		((u64)0x0000000000008000ULL)	/* non-volatile */
 #define EFI_MEMORY_MORE_RELIABLE \
 				((u64)0x0000000000010000ULL)	/* higher reliability */
 #define EFI_MEMORY_RO		((u64)0x0000000000020000ULL)	/* read-only */
#42 lore raw

PATCH 13/14 efi: Add Persistent Memory type name

In reply to Matt Fleming

From: Robert Elliott <[email protected]>

Add the "Persistent Memory" string for type 14 introduced in UEFI 2.5. This is used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [Persistent Memory | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +2 -1
index 9c8c0747c8cd..05a3d69250d4 100644
@@ -553,7 +553,8 @@ static __initdata char memory_type_name[][20] = {
 	"ACPI Memory NVS",
 	"Memory Mapped I/O",
 	"MMIO Port Space",
-	"PAL Code"
+	"PAL Code",
+	"Persistent Memory"
 };
 
 char * __init efi_md_typeattr_format(char *buf, size_t size,
Signature · 1 line
2.6.2
Reviewed-by on v1 13/14
#43 lore raw

Re: [PATCH 13/14] efi: Add Persistent Memory type name

Quoted 29 lines · Matt Fleming

From: Robert Elliott <[email protected]>

Add the "Persistent Memory" string for type 14 introduced in UEFI 2.5. This is used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [Persistent Memory | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +0 -0
index 9c8c0747c8cd..05a3d69250d4 100644

drivers/firmware/efi/efi.c static __initdata char memory_type_name[][20] = { · line 560

Quoted 9 lines
drivers/firmware/efi/efi.c +2 -1
@@ -553,7 +553,8 @@ static __initdata char memory_type_name[][20] = {
 	"ACPI Memory NVS",
 	"Memory Mapped I/O",
 	"MMIO Port Space",
-	"PAL Code"
+	"PAL Code",
+	"Persistent Memory"
 };
 
 char * __init efi_md_typeattr_format(char *buf, size_t size,

To minimize future churn, I suggest to add a trailing comma character:

"Persistent Memory",

With or without that changed:

#44 lore raw

[tip:efi/core] efi: Add Persistent Memory type name

In reply to Matt Fleming
efi: Add Persistent Memory type name
Commit-ID:  35575e0e8ba633fc8276509a21f89b599b4f9006
Gitweb:     http://git.kernel.org/tip/35575e0e8ba633fc8276509a21f89b599b4f9006
Author:     Robert Elliott <[email protected]>
AuthorDate: Mon, 1 Feb 2016 22:07:07 +0000
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 3 Feb 2016 11:41:20 +0100

efi: Add Persistent Memory type name

Add the "Persistent Memory" string for type 14 introduced in UEFI 2.5. This is used when printing the UEFI memory map.

old:
efi: mem61: [type=14 | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
efi: mem61: [Persistent Memory | | | | | | | |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

Cc Andy Lutomirski <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc Dan Williams <[email protected]>
Cc Denys Vlasenko <[email protected]>
Cc H. Peter Anvin <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Ross Zwisler <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/firmware/efi/efi.c +2 -1
index f437048..3a69ed5 100644
@@ -555,7 +555,8 @@ static __initdata char memory_type_name[][20] = {
 	"ACPI Memory NVS",
 	"Memory Mapped I/O",
 	"MMIO Port Space",
-	"PAL Code"
+	"PAL Code",
+	"Persistent Memory",
 };
 
 char * __init efi_md_typeattr_format(char *buf, size_t size,
#45 lore raw

PATCH 14/14 x86/efi: Print size in binary units in efi_print_memmap

In reply to Matt Fleming

From: Robert Elliott <[email protected]>

Print the size in the best-fit B, KiB, MiB, etc. units rather than always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI decimal units (KB, MB, etc.).

old:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:
    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)

Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
arch/x86/platform/efi/efi.c +18 -7
index e80826e6f3a9..2c457c5e8203 100644
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>
@@ -117,6 +118,17 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static char * __init efi_size_format(char *buf, size_t size, u64 bytes)
+{
+	static const char *const units_2[] = {
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+	};
+	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
+
+	snprintf(buf, size, "%llu %s", bytes >> (i * 10), units_2[i]);
+	return buf;
+}
+
 void __init efi_find_mirror(void)
 {
 	void *p;
@@ -225,21 +237,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
-		char buf[64];
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << PAGE_SHIFT;
+		char buf[64], buf2[64];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%s)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			efi_size_format(buf2, sizeof(buf2), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }
Signature · 1 line
2.6.2
#46 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

I'll take being CC'd as "please offer an opinion", so I'll offer one. :)

Quoted 40 lines · Matt Fleming

From: Robert Elliott <[email protected]>

Print the size in the best-fit B, KiB, MiB, etc. units rather than always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI decimal units (KB, MB, etc.).

old:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
arch/x86/platform/efi/efi.c +1 -0
index e80826e6f3a9..2c457c5e8203 100644
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>

arch/x86/platform/efi/efi.c void efi_get_time(struct timespec *now) · line 125

Quoted 8 lines
arch/x86/platform/efi/efi.c +5 -0
@@ -117,6 +118,17 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
+static char * __init efi_size_format(char *buf, size_t size, u64 bytes)
+{
+	static const char *const units_2[] = {
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+	};

Blech. Blech blech blech. As far as I'm concerned, "IEC binary units" rewrite history. I propose to just say "KB" & friends.

Not sure if I should refer kernel list subscribers to GNU utility manuals :), but "dd" gets it right:

N and BYTES may be followed by the following multiplicative
suffixes: c =1, w =2, b =512, kB =1000, K =1024, MB =1000*1000, M =1024*1024, xM =M GB =1000*1000*1000, G =1024*1024*1024, and so on for T, P, E, Z, Y.

Anyway, feel free to ignore this.

arch/x86/platform/efi/efi.c void efi_get_time(struct timespec *now) · line 130

Quoted 5 lines
arch/x86/platform/efi/efi.c +5 -0
+	unsigned long i = bytes ? __ffs64(bytes) / 10 : 0;
+
+	snprintf(buf, size, "%llu %s", bytes >> (i * 10), units_2[i]);
+	return buf;
+}

The calculation seems correct, and I agree "bytes" should have type "u64" -- this makes it clearer why we don't have to climb higher than offset 6 (count 7) in "units".

However, since we're printing the result of the right shift with the %llu conversion specifier, I believe I'd appreciate either an explicit ((unsigned long long)bytes) >> ... cast there, or a macro for the conversion specifier. (In userland I'd write "%"PRIu64, but I don't know if the kernel has anything like that.)

Quoted 23 lines
arch/x86/platform/efi/efi.c +4 -4
+
 void __init efi_find_mirror(void)
 {
 	void *p;
@@ -225,21 +237,20 @@ int __init efi_memblock_x86_reserve_range(void)
 void __init efi_print_memmap(void)
 {
 #ifdef EFI_DEBUG
-	efi_memory_desc_t *md;
 	void *p;
 	int i;
 
 	for (p = memmap.map, i = 0;
 	     p < memmap.map_end;
 	     p += memmap.desc_size, i++) {
-		char buf[64];
+		efi_memory_desc_t *md = p;
+		u64 size = md->num_pages << PAGE_SHIFT;
+		char buf[64], buf2[64];
 
-		md = p;
-		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%lluMB)\n",

arch/x86/platform/efi/efi.c int __init efi_memblock_x86_reserve_range(void) · line 256

Quoted 10 lines
arch/x86/platform/efi/efi.c +3 -3
+		pr_info("mem%02u: %s range=[0x%016llx-0x%016llx] (%s)\n",
 			i, efi_md_typeattr_format(buf, sizeof(buf), md),
-			md->phys_addr,
-			md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT) - 1,
-			(md->num_pages >> (20 - EFI_PAGE_SHIFT)));
+			md->phys_addr, md->phys_addr + size - 1,
+			efi_size_format(buf2, sizeof(buf2), size));
 	}
 #endif  /*  EFI_DEBUG  */
 }

Hm, apparently %llx is used to print u64 here as well. Did I write this code? :) Is %ll used interchangeably with 64-bits?

Also, I notice there's room for unification with ia64 code. In "arch/ia64/kernel/efi.c", the efi_init() function open-codes a similar suffix conversion -- notice it says KB and friends! :)

So maybe the efi_size_format() helper could go into "drivers/firmware/efi/efi.c", near its friend efi_md_typeattr_format().

Furthermore, the ia64 source has a macro efi_md_size(). Perhaps it can be moved to a common header and used here as well. Not too important.

... Ultimately, my only semi-important point here is that efi_size_format() should go into common EFI driver code, and be (optionally) used on ia64 as well.

Thanks
Laszlo

#47 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 46 lines · Laszlo Ersek

I'll take being CC'd as "please offer an opinion", so I'll offer one. :)

Quoted 43 lines · Matt Fleming

From: Robert Elliott <[email protected]>

Print the size in the best-fit B, KiB, MiB, etc. units rather than always MiB. This avoids rounding, which can be misleading.

Use proper IEC binary units (KiB, MiB, etc.) rather than misuse SI decimal units (KB, MB, etc.).

old:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff) (16384MB)

new:

    efi: mem61: [Persistent Memory  |   |  |  |  |  |  |   |WB|WT|WC|UC] range=[0x0000000880000000-0x0000000c7fffffff] (16 GiB)
Cc Thomas Gleixner <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Ard Biesheuvel <[email protected]>
Cc Taku Izumi <[email protected]>
Cc Laszlo Ersek <[email protected]>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)
arch/x86/platform/efi/efi.c +1 -0
index e80826e6f3a9..2c457c5e8203 100644
@@ -35,6 +35,7 @@
 #include <linux/efi.h>
 #include <linux/efi-bgrt.h>
 #include <linux/export.h>
+#include <linux/bitops.h>
 #include <linux/bootmem.h>
 #include <linux/slab.h>
 #include <linux/memblock.h>
@@ -117,6 +118,17 @@ void efi_get_time(struct timespec *now)
 	now->tv_nsec = 0;
 }
 
Quoted 8 lines

arch/x86/platform/efi/efi.c void efi_get_time(struct timespec *now) · line 125

Quoted 5 lines
arch/x86/platform/efi/efi.c +5 -0
+static char * __init efi_size_format(char *buf, size_t size, u64 bytes)
+{
+	static const char *const units_2[] = {
+		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"
+	};

Blech. Blech blech blech. As far as I'm concerned, "IEC binary units" rewrite history. I propose to just say "KB" & friends.

So I kind of agree. Memory is almost never measured in marketing bytes, we should simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does and ignore all the 'i' silliness that infests storage sizes ...

Thanks,

Ingo

#48 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 4 lines · Ingo Molnar

So I kind of agree. Memory is almost never measured in marketing bytes, we should simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does and ignore all the 'i' silliness that infests storage sizes ...

Thanks guys.

OK, this patch has caused enough headaches. Let's drop it from this series.

Robert, Andy, feel free to resubmit it after you've addressed everyone's concerns and we can discuss it in isolation.

#49 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 6 lines · Matt Fleming
Quoted 4 lines · Ingo Molnar

So I kind of agree. Memory is almost never measured in marketing bytes, we should simply output KB/MB/GB/TB/PB/EB like the rest of the memory management code does and ignore all the 'i' silliness that infests storage sizes ...

Quoted 7 lines

Thanks guys.

OK, this patch has caused enough headaches. Let's drop it from this series.

Robert, Andy, feel free to resubmit it after you've addressed everyone's concerns and we can discuss it in isolation.

Personally I gave up to promote standards. I have started with KB, MB, etc, but I like that eventually we have a distinct standard for binary vs. decimal units.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#50 lore raw

RE: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

In reply to Matt Fleming
Quoted 14 lines

-----Original Message-----
From: Matt Fleming [mailto:matt-mF/unelCI9GS6iBeEJttW/[email protected]]
Sent: Wednesday, February 3, 2016 5:28 AM
To: Ingo Molnar <[email protected]>
Cc: Laszlo Ersek <[email protected]>; H . Peter Anvin <[email protected]>; Thomas Gleixner <[email protected]>; [email protected]; linux- [email protected]; Elliott, Robert (Persistent Memory) <[email protected]>; Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>; Ard Biesheuvel <[email protected]>; Taku Izumi <[email protected]>; Linus Torvalds <torvalds@linux-
foundation.org>; Andrew Morton <akpm-de/[email protected]>
Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
efi_print_memmap

...

Quoted 5 lines

OK, this patch has caused enough headaches. Let's drop it from this series.

Robert, Andy, feel free to resubmit it after you've addressed everyone's concerns and we can discuss it in isolation.

We could just delete the size print altogether - better to print nothing than a silently rounded number. The end address already communicates the size - it's just not as readable.

The e820 table prints don't bother with a size print.

That would also shorten these extremely wide prints to 116 characters (131 if printk time is enabled).

[    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
vs.
[    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)

---
Robert Elliott, HPE Persistent Memory

#51 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 25 lines · Elliott, Robert (Persistent Memory)
Quoted 14 lines

-----Original Message-----
From: Matt Fleming [mailto:[email protected]]
Sent: Wednesday, February 3, 2016 5:28 AM
To: Ingo Molnar <[email protected]>
Cc: Laszlo Ersek <[email protected]>; H . Peter Anvin <[email protected]>; Thomas Gleixner <[email protected]>; [email protected]; linux- [email protected]; Elliott, Robert (Persistent Memory) <[email protected]>; Andy Shevchenko <[email protected]>; Ard Biesheuvel <[email protected]>; Taku Izumi <[email protected]>; Linus Torvalds <torvalds@linux- foundation.org>; Andrew Morton <[email protected]>
Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

...

Quoted 5 lines

OK, this patch has caused enough headaches. Let's drop it from this series.

Robert, Andy, feel free to resubmit it after you've addressed everyone's concerns and we can discuss it in isolation.

We could just delete the size print altogether - better to print nothing than a silently rounded number. The end address already communicates the size - it's just not as readable.

Quoted 8 lines

The e820 table prints don't bother with a size print.

That would also shorten these extremely wide prints to 116 characters (131 if printk time is enabled).

[    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
vs.
[    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)

So I find the latter a lot more readable - my terminals are wide enough ;-)

Humans are also rather bad at parsing 64-bit hexa address ranges at a glance, so the size display is very useful.

But the flags portion should be shortened via appropriately chosen single-character abbreviations for the flags. Anyone deeply intimate with the code will recognize the flags - others won't care one way or another.

plus there's no need to write out 'range='.

... and please keep the size and just use GB/TB for chrissake.

I.e. something like this would work for me:

Quoted 1 line
[    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"

Thanks,

Ingo

#52 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 46 lines · Ingo Molnar
Quoted 25 lines · Elliott, Robert (Persistent Memory)
Quoted 14 lines

-----Original Message-----
From: Matt Fleming [mailto:matt-mF/unelCI9GS6iBeEJttW/[email protected]]
Sent: Wednesday, February 3, 2016 5:28 AM
To: Ingo Molnar <[email protected]>
Cc: Laszlo Ersek <[email protected]>; H . Peter Anvin <[email protected]>; Thomas Gleixner <[email protected]>; [email protected]; linux- [email protected]; Elliott, Robert (Persistent Memory) <[email protected]>; Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]>; Ard Biesheuvel <[email protected]>; Taku Izumi <[email protected]>; Linus Torvalds <torvalds@linux-
foundation.org>; Andrew Morton <akpm-de/[email protected]>
Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
efi_print_memmap

...

Quoted 5 lines

OK, this patch has caused enough headaches. Let's drop it from this series.

Robert, Andy, feel free to resubmit it after you've addressed everyone's concerns and we can discuss it in isolation.

We could just delete the size print altogether - better to print nothing than a silently rounded number. The end address already communicates the size - it's just not as readable.

Quoted 8 lines

The e820 table prints don't bother with a size print.

That would also shorten these extremely wide prints to 116 characters (131 if printk time is enabled).

[    0.000000] BIOS-e820: [mem 0x0000001880000000-0x000000207fffffff] reserved
vs.
[    0.000000] efi: mem62: [Reserved           |   |  |NV|  |  |  |  |   |WB|WT|WC|UC] range=[0x0000001880000000-0x000000207fffffff] (32 GiB)

So I find the latter a lot more readable - my terminals are wide enough ;-)

Humans are also rather bad at parsing 64-bit hexa address ranges at a glance, so the size display is very useful.

But the flags portion should be shortened via appropriately chosen single-character abbreviations for the flags. Anyone deeply intimate with the code will recognize the flags - others won't care one way or another.

Quoted 7 lines

plus there's no need to write out 'range='.

... and please keep the size and just use GB/TB for chrissake.

I.e. something like this would work for me:

Quoted 1 line
[    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"

Sorry to disagree :), but I count myself somewhat intimate with UEFI (albeit more from the edk2 side), and while I can make sense of

|NV| | | | | |WB|WT|WC|UC]

I find

.N....BTCU

mostly undecipherable. :)

My original goal with this printout was to (a) provide a good impression of the entire UEFI memmap, at a glance, (b) provide sufficient detail per-entry, if necessary.

(I don't exactly recall why I was staring at the UEFI memmap dump at that time, maybe I was working on S3 in OVMF which took a lot of memmap massaging, or debugging some bug; either way my eyes were bleeding trying to decode the numeric attributes.)

My xterm, maximized, has 239 columns, which I think counts as pretty low for today's resolutions. It is nonetheless plenty wide for the current output. Given that we print this stuff only when debugging information is requested, I feel that the value of the current columnar output, in which I can follow a single attribute with my eye across all entries, should not be diminished, by compressing the columns.

I'm not a wide screen maniac; for example I insist on source code being wrapped at 79 characters, commit messages at 74, emails at 72 (except diagrams and log excerpts), and so on. But debug output is different.

My 2 cents, of course...

Thanks
Laszlo

#53 lore raw

Re: [PATCH 14/14] x86/efi: Print size in binary units in efi_print_memmap

Quoted 5 lines · Laszlo Ersek
Quoted 1 line
Quoted 1 line
[    0.000000] efi: mem62: 0x0000001880000000-0x000000207fffffff (  32 GB) .N....BTCU "Reserved"

Sorry to disagree :), but I count myself somewhat intimate with UEFI (albeit more from the edk2 side), and while I can make sense of

Quoted 7 lines

|NV| | | | | |WB|WT|WC|UC]

I find

.N....BTCU

mostly undecipherable. :)

Ok - the longer variant is fine to me as well.

Thanks,

Ingo