[GIT PULL 00/14] EFI changes for v4.6

[GIT PULL 00/14] EFI changes for v4.6

[Matt Fleming]

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(-)

[PATCH 01/14] efi: Expose non-blocking set_variable() wrapper to efivars

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index e9c458ba9353..0cd8f039602e 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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;
 
-- 
2.6.2

[PATCH 02/14] efi: Remove redundant efi_set_variable_nonblocking prototype

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 include/linux/efi.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 569b5a866bb1..8706e0aabedc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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;
 };
 
-- 
2.6.2

[PATCH 03/14] efi: runtime-wrappers: Add a nonblocking version of QueryVariableInfo

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
 include/linux/efi.h                     |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 228bbf910461..e9f2867f0d91 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -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;
 }
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 8706e0aabedc..ad1e177ba48e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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;
-- 
2.6.2

[PATCH 04/14] efi: Add nonblocking option to efi_query_variable_store()

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 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(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 6452070f3025..dc4f7b51abf9 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -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)
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb10517f..d2a49626a335 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -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;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ad1e177ba48e..09f1559e7525 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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;
 }
-- 
2.6.2

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

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/runtime-wrappers.c | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index e9f2867f0d91..311f415bff51 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -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
-- 
2.6.2

[PATCH 06/14] efi: runtime-wrapper: Get rid of the rtc_lock spinlock

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 311f415bff51..7b8b2f2702ca 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -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;
 }
 
-- 
2.6.2

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

[Matt Fleming]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/runtime-wrappers.c | 73 ++++++++++++++-------------------
 1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 7b8b2f2702ca..aa3d9d0a27e6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -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;
 }
 
-- 
2.6.2

[PATCH 08/14] efivars: Use to_efivar_entry

[Matt Fleming]

From: Geliang Tang <geliangtang@163.com>

Use to_efivar_entry() instead of open-coding it.

Signed-off-by: Geliang Tang <geliangtang@163.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efivars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efivars.c b/drivers/firmware/efi/efivars.c
index 756eca8c4cf8..c5b0d2bc1111 100644
--- a/drivers/firmware/efi/efivars.c
+++ b/drivers/firmware/efi/efivars.c
@@ -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);
 }
 
-- 
2.6.2

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

[Matt Fleming]

From: Môshe van der Sterre <me@moshe.nl>

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.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Môshe van der Sterre <me@moshe.nl>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/platform/efi/efi-bgrt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
index b0970661870a..a2433817c987 100644
--- a/arch/x86/platform/efi/efi-bgrt.c
+++ b/arch/x86/platform/efi/efi-bgrt.c
@@ -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);
-- 
2.6.2

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

[Matt Fleming]

From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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.

Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 09f1559e7525..f468f7c53236 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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;
-- 
2.6.2

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

[Matt Fleming]

From: Robert Elliott <elliott@hpe.com>

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)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/platform/efi/efi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index bdd9477f937c..e80826e6f3a9 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -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  */
-- 
2.6.2

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

[Matt Fleming]

From: Robert Elliott <elliott@hpe.com>

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)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efi.c | 5 ++++-
 include/linux/efi.h        | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 0cd8f039602e..9c8c0747c8cd 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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"  : "",
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f468f7c53236..99b88c5a6770 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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 */
-- 
2.6.2

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

[Matt Fleming]

From: Robert Elliott <elliott@hpe.com>

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)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 drivers/firmware/efi/efi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 9c8c0747c8cd..05a3d69250d4 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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,
-- 
2.6.2

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

[Matt Fleming]

From: Robert Elliott <elliott@hpe.com>

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)

Signed-off-by: Robert Elliott <elliott@hpe.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e80826e6f3a9..2c457c5e8203 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -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  */
 }
-- 
2.6.2

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

[Laszlo Ersek]

On 02/01/16 23:07, Matt Fleming wrote:
> From: Robert Elliott <elliott@hpe.com>
> 
> 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)
> 
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  arch/x86/platform/efi/efi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index bdd9477f937c..e80826e6f3a9 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -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  */
> 

For this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

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

[Laszlo Ersek]

On 02/01/16 23:07, Matt Fleming wrote:
> From: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org>
> 
> 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)
> 
> Signed-off-by: Robert Elliott <elliott-ZPxbGqLxI0U@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Taku Izumi <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> Cc: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> ---
>  drivers/firmware/efi/efi.c | 5 ++++-
>  include/linux/efi.h        | 1 +
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 0cd8f039602e..9c8c0747c8cd 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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"  : "",
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index f468f7c53236..99b88c5a6770 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -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 */
> 

Reviewed-by: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

[Laszlo Ersek]

On 02/01/16 23:07, Matt Fleming wrote:
> From: Robert Elliott <elliott@hpe.com>
> 
> 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)
> 
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  drivers/firmware/efi/efi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9c8c0747c8cd..05a3d69250d4 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

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

[Laszlo Ersek]

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

On 02/01/16 23:07, Matt Fleming wrote:
> From: Robert Elliott <elliott@hpe.com>
> 
> 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)
> 
> Signed-off-by: Robert Elliott <elliott@hpe.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index e80826e6f3a9..2c457c5e8203 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -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"
> +	};

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.

> +	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.)

> +
>  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  */
>  }
> 

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

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

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> 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.

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:

 > 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:

> -	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

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

[Ard Biesheuvel]

On 3 February 2016 at 10:43, Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
>
>> From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> 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.
>
> Well, those excessive latencies would affect SMP platforms as well, just that
> there are (usually) other CPUs free to do execution, right?
>

Correct.

> More fundamentally, this makes me nervous:
>
>  > 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?
>

Good question. I will try to find out.

> Also, this:
>
>> -     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.
>

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.

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

OK.

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

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> 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.
> 
> Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> ---
>  include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 09f1559e7525..f468f7c53236 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -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)

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

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

[Ingo Molnar]

* Laszlo Ersek <lersek@redhat.com> wrote:

> I'll take being CC'd as "please offer an opinion", so I'll offer one. :)
> 
> On 02/01/16 23:07, Matt Fleming wrote:
> > From: Robert Elliott <elliott@hpe.com>
> > 
> > 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)
> > 
> > Signed-off-by: Robert Elliott <elliott@hpe.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Taku Izumi <izumi.taku@jp.fujitsu.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> > ---
> >  arch/x86/platform/efi/efi.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> > index e80826e6f3a9..2c457c5e8203 100644
> > --- a/arch/x86/platform/efi/efi.c
> > +++ b/arch/x86/platform/efi/efi.c
> > @@ -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"
> > +	};
> 
> 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

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

[Matt Fleming]

On Wed, 03 Feb, at 11:33:35AM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > From: Peter Jones <pjones@redhat.com>
> > 
> > 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.
> > 
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> > ---
> >  include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 42 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 09f1559e7525..f468f7c53236 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -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)
> 
> 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.

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).

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

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> On Wed, 03 Feb, at 11:33:35AM, Ingo Molnar wrote:
> > 
> > * Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > 
> > > From: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > > ---
> > >  include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
> > >  1 file changed, 42 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 09f1559e7525..f468f7c53236 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -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)
> > 
> > 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.
> 
> 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

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

[Ingo Molnar]

* Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> > More fundamentally, this makes me nervous:
> >
> >  > 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?
> >
> 
> 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

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

[Joe Perches]

On Wed, 2016-02-03 at 11:33 +0100, Ingo Molnar wrote:
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > From: Peter Jones <pjones@redhat.com>
> > 
> > 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.
> > 
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> > ---
> >  include/linux/efi.h | 63 +++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 42 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 09f1559e7525..f468f7c53236 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -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)
> 
> 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
----------------------------------
> 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.

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

[Matt Fleming]

On Wed, 03 Feb, at 11:50:35AM, Ingo Molnar wrote:
> 
> Hm, so the GUIDs are line-broken in the same fashion in the spec, after the third 
> parameter?
 
Yep, they are.

> 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.

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

[Ingo Molnar]

* Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> On Wed, 03 Feb, at 11:50:35AM, Ingo Molnar wrote:
> > 
> > Hm, so the GUIDs are line-broken in the same fashion in the spec, after the third 
> > parameter?
>  
> Yep, they are.
> 
> > 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

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

[Matt Fleming]

On Wed, 03 Feb, at 11:40:45AM, Ingo Molnar wrote:
> 
> 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.

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

[Ard Biesheuvel]

On 3 February 2016 at 11:58, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > More fundamentally, this makes me nervous:
>> >
>> >  > 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?
>> >
>>
>> 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.

> 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?)

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

[Matt Fleming]

On Wed, 03 Feb, at 12:33:10PM, Ard Biesheuvel wrote:
> On 3 February 2016 at 11:58, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> >> > More fundamentally, this makes me nervous:
> >> >
> >> >  > 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?
> >> >
> >>
> >> 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.
 
It was indeed. Additionally I didn't want the EFI material to miss the
merge window again.

> > 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?)

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.

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

[Andy Shevchenko]

On Wed, Feb 3, 2016 at 1:28 PM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 03 Feb, at 11:40:45AM, Ingo Molnar wrote:
>>
>> 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.

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: Matt Fleming [mailto:matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org]
> Sent: Wednesday, February 3, 2016 5:28 AM
> To: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; H . Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>;
> Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Elliott, Robert (Persistent Memory)
> <elliott-ZPxbGqLxI0U@public.gmane.org>; Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>;
> Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; Taku Izumi
> <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>; Linus Torvalds <torvalds@linux-
> foundation.org>; Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
> efi_print_memmap
...
> 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

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

[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.

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

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/runtime-wrappers.c | 71 ++++++++------------
 1 file changed, 28 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 7b8b2f2702ca..de6953039af6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -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;
 }
 
-- 
2.5.0

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

[Matt Fleming]

On Thu, 04 Feb, at 02:58:47PM, Ard Biesheuvel wrote:
> 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.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  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,

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

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

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

[Andy Lutomirski]

On Feb 4, 2016 5:58 AM, "Ard Biesheuvel" <ard.biesheuvel@linaro.org> wrote:
>
> 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

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

[Ingo Molnar]

* Elliott, Robert (Persistent Memory) <elliott@hpe.com> wrote:

> 
> > -----Original Message-----
> > From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> > Sent: Wednesday, February 3, 2016 5:28 AM
> > To: Ingo Molnar <mingo@kernel.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>; H . Peter Anvin <hpa@zytor.com>;
> > Thomas Gleixner <tglx@linutronix.de>; linux-efi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Elliott, Robert (Persistent Memory)
> > <elliott@hpe.com>; Andy Shevchenko <andriy.shevchenko@linux.intel.com>;
> > Ard Biesheuvel <ard.biesheuvel@linaro.org>; Taku Izumi
> > <izumi.taku@jp.fujitsu.com>; Linus Torvalds <torvalds@linux-
> > foundation.org>; Andrew Morton <akpm@linux-foundation.org>
> > Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
> > efi_print_memmap
> ...
> > 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)

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:

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

Thanks,

	Ingo

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

[Laszlo Ersek]

On 02/09/16 13:20, Ingo Molnar wrote:
> 
> * Elliott, Robert (Persistent Memory) <elliott-ZPxbGqLxI0U@public.gmane.org> wrote:
> 
>>
>>> -----Original Message-----
>>> From: Matt Fleming [mailto:matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org]
>>> Sent: Wednesday, February 3, 2016 5:28 AM
>>> To: Ingo Molnar <mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; H . Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>;
>>> Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>; linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>>> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Elliott, Robert (Persistent Memory)
>>> <elliott-ZPxbGqLxI0U@public.gmane.org>; Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>;
>>> Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>; Taku Izumi
>>> <izumi.taku-+CUm20s59erQFUHtdCDX3A@public.gmane.org>; Linus Torvalds <torvalds@linux-
>>> foundation.org>; Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>>> Subject: Re: [PATCH 14/14] x86/efi: Print size in binary units in
>>> efi_print_memmap
>> ...
>>> 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)
> 
> 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:
> 
>> [    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

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

[Ingo Molnar]

* Laszlo Ersek <lersek-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> >> [    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. :)

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

Thanks,

	Ingo

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

[Ard Biesheuvel]

On 8 February 2016 at 20:37, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Feb 4, 2016 5:58 AM, "Ard Biesheuvel" <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>> 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?

> 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.
>

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

[Matt Fleming]

On Tue, 09 Feb, at 05:52:34PM, Ard Biesheuvel wrote:
> On 8 February 2016 at 20:37, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Feb 4, 2016 5:58 AM, "Ard Biesheuvel" <ard.biesheuvel@linaro.org> wrote:
> >>
> >> 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.

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. 

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

[Matt Fleming]

On Mon, 08 Feb, at 11:37:58AM, Andy Lutomirski wrote:
> On Feb 4, 2016 5:58 AM, "Ard Biesheuvel" <ard.biesheuvel@linaro.org> wrote:
> >
> > 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".
 
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.

> 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.

> 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.